Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session time zone to system context #6786

Closed
mrotteveel opened this issue May 3, 2021 · 23 comments
Closed

Add session time zone to system context #6786

mrotteveel opened this issue May 3, 2021 · 23 comments

Comments

@mrotteveel
Copy link
Member

Currently it is not possible to obtain the current session time zone other than through workarounds like substring(current_timestamp from 26).

I think we should add:

  • RDB$GET_CONTEXT('SYSTEM', 'SESSION_TIME_ZONE')
  • MON$TIME_ZONE in MON$ATTACHMENTS

Both should return the current session time zone (either named zone or offset in {+|-}HH:MM).

This would be similar to what is provided for the session idle timeout, which is accessible from the system context using RDB$GET_CONTEXT('SYSTEM', 'SESSION_IDLE_TIMEOUT') and from MON$IDLE_TIMEOUT in MON$ATTACHMENTS.

Desired situation:

set time zone 'America/New_York';
select RDB$GET_CONTEXT('SYSTEM', 'SESSION_TIME_ZONE') from rdb$database;
-- returns 'America/New_York'
select MON$TIME_ZONE from MON$ATTACHMENTS where MON$ATTACHMENT_ID = CURRENT_CONNECTION;
-- returns 'America/New_York'

and

set time zone '+01:00';
select RDB$GET_CONTEXT('SYSTEM', 'SESSION_TIME_ZONE') from rdb$database;
-- returns '+01:00'
select MON$TIME_ZONE from MON$ATTACHMENTS where MON$ATTACHMENT_ID = CURRENT_CONNECTION;
-- returns '+01:00'
@asfernandes
Copy link
Member

MON$TIME_ZONE in MON$ATTACHMENTS

This is overkill IMO as the information is already present in MON$TIMESTAMP.

We should better add extension to EXTRACT returning region string and let people use that EXTRACT extension with MON$TIMESTAMP.

@asfernandes
Copy link
Member

The standard EXTRACT uses TIMEZONE_HOUR and TIMEZONE_MINUTE while others commands uses TIME ZONE separated. What a bad thing.

@hvlad
Copy link
Member

hvlad commented May 3, 2021

MON$TIME_ZONE in MON$ATTACHMENTS

This is overkill IMO as the information is already present in MON$TIMESTAMP.

We should better add extension to EXTRACT returning region string and let people use that EXTRACT extension with MON$TIMESTAMP.

Agree

@aafemt
Copy link
Contributor

aafemt commented May 3, 2021

Isn't MON$TIMESTAMP connection time? Will it be changed after "SET TIME ZONE" issued?

@asfernandes asfernandes self-assigned this May 3, 2021
@mrotteveel
Copy link
Member Author

mrotteveel commented May 3, 2021

MON$TIME_ZONE in MON$ATTACHMENTS

This is overkill IMO as the information is already present in MON$TIMESTAMP.

We should better add extension to EXTRACT returning region string and let people use that EXTRACT extension with MON$TIMESTAMP.

Except it isn't available in MON$TIMESTAMP, the timestamp is always in the server time zone

select cast(mon$timestamp as varchar(50)) from mon$attachments where mon$attachment_id = current_connection;
-- Output: 2021-05-03 13:11:22.7720 Europe/Amsterdam
set time zone 'America/New_York';
select cast(mon$timestamp as varchar(50)) from mon$attachments where mon$attachment_id = current_connection;
-- Output: 2021-05-03 13:11:22.7720 Europe/Amsterdam

After connecting with isc_dpb_session_time_zone set to America/New_York:

select cast(mon$timestamp as varchar(50)) from mon$attachments where mon$attachment_id = current_connection;
-- Output: 2021-05-03 14:38:31.7910 Europe/Amsterdam

@mrotteveel
Copy link
Member Author

And to be clear, this isn't just about getting the time zone, it is also about providing a consistent API. That is, if session idle timeout is available from RDB$GET_CONTEXT and MON$ATTACHMENTS, then so should the session time zone.

@asfernandes
Copy link
Member

Except it doesn't, the timestamp is always in the server time zone

So I think we should change it to use isc_dpb_session_time_zone when it was set.

Isn't MON$TIMESTAMP connection time? Will it be changed after "SET TIME ZONE" issued?

It should not be changed, but should it be a real problem?

@mrotteveel
Copy link
Member Author

mrotteveel commented May 3, 2021

Except it doesn't, the timestamp is always in the server time zone

So I think we should change it to use isc_dpb_session_time_zone when it was set.

I don't see why it should. That column provides when the server received the connection. Having this report it in the time zone of the server is perfectly fine and logical. Changing it to reflect the current session time zone sounds like an abuse to avoid creating a logical API.

@asfernandes
Copy link
Member

And to be clear, this isn't just about getting the time zone, it is also about providing a consistent API. That is, if session idle timeout is available from RDB$GET_CONTEXT and MON$ATTACHMENTS, then so should the session time zone.

So you do not want to solve any real problem?

Then there is more work for you (how do one read set bindinds, etc).

@hvlad
Copy link
Member

hvlad commented May 3, 2021

EXTRACT allows to work with any timestamp, from any source. It is more general and I see no reason to not implement it.
I'm not objecting extending of system context, but in this case more generic solution exists and it is always preferred, imo.

As for:

MON$TIMESTAMP, the timestamp is always in the server time zone

use current_timestamp, not mon$timestamp

@mrotteveel
Copy link
Member Author

mrotteveel commented May 3, 2021

And to be clear, this isn't just about getting the time zone, it is also about providing a consistent API. That is, if session idle timeout is available from RDB$GET_CONTEXT and MON$ATTACHMENTS, then so should the session time zone.

So you do not want to solve any real problem?

I'm currently updating the Language Reference, and I foresee and expect that people will want to know this information.

Then there is more work for you (how do one read set bindinds, etc).

Well, yes, I argue those should also be exposed as well. Personally, I'd be fine with exposing this only through RDB$GET_CONTEXT, but on the other hand the case of the session idle timeout being exposed through both RDB$GET_CONTEXT and MON$ATTACHMENTS argues that the rest should too if only for API consistency and to make this information available to adminstrators about other connections.

@mrotteveel
Copy link
Member Author

mrotteveel commented May 3, 2021

MON$TIMESTAMP, the timestamp is always in the server time zone

use current_timestamp, not mon$timestamp

And I repeat, that is a workaround at best. It also doesn't allow an administrator to see this information in MON$ATTACHMENTS for a different connection (which might help troubleshooting, etc).

@asfernandes
Copy link
Member

I don't see why it should. That column provides when the server received the connection. Having this report it in the time zone of the server is perfectly fine and logical.

I don't agree with you. The server receives a connection and can store that client time zone in the field.

It's not documented in any place nor was the intention to hide that.

Moreover, in MON$STATEMENTS it puts the time zone from the session, so it is more consistent to use same logic in MON$ATTACHMENTS.

@asfernandes
Copy link
Member

About RDB$GET_CONTEXT('SYSTEM', 'SESSION_TIME_ZONE'), I'm not sure it should be SESION_TIME_ZONE or CURRENT_TIMEZONE (like CURRENT_USER, CURRENT_ROLE).

@mrotteveel
Copy link
Member Author

I don't see why it should. That column provides when the server received the connection. Having this report it in the time zone of the server is perfectly fine and logical.

I don't agree with you. The server receives a connection and can store that client time zone in the field.

It's not documented in any place nor was the intention to hide that.

Moreover, in MON$STATEMENTS it puts the time zone from the session, so it is more consistent to use same logic in MON$ATTACHMENTS.

Even if you do that, this would - I assume - still be the initial time zone of the session, not the current time zone of the session, unless you plan to rebase MON$TIMESTAMP each time the session time zone of an attachment changes.

@mrotteveel
Copy link
Member Author

About RDB$GET_CONTEXT('SYSTEM', 'SESSION_TIME_ZONE'), I'm not sure it should be SESION_TIME_ZONE or CURRENT_TIMEZONE (like CURRENT_USER, CURRENT_ROLE).

That is fine with me, although I would argue that SESSION_TIME_ZONE makes more sense and would be more consistent because 1) it is initially configured through isc_dpb_session_time_zone, 2) we call it session time zone in a lot of other places, and we don't have a context variable CURRENT_TIMEZONE (as in select CURRENT_TIMEZONE from rdb$database) (the existence of those context variables is the primary reason the RDB$GET_CONTEXT items CURRENT_USER and CURRENT_ROLE are called that).

@asfernandes
Copy link
Member

Even if you do that, this would - I assume - still be the initial time zone of the session, not the current time zone of the session, unless you plan to rebase MON$TIMESTAMP each time the session time zone of an attachment changes.

Sure, the initial (or original) time zone. I created #6787 for that.

@mrotteveel
Copy link
Member Author

Even if you do that, this would - I assume - still be the initial time zone of the session, not the current time zone of the session, unless you plan to rebase MON$TIMESTAMP each time the session time zone of an attachment changes.

Sure, the initial (or original) time zone. I created #6787 for that.

Ok, but to be explicit: that wouldn't match what I request, namely a way to obtain the current session time zone.

@asfernandes asfernandes linked a pull request May 3, 2021 that will close this issue
@asfernandes
Copy link
Member

Ok, but to be explicit: that wouldn't match what I request, namely a way to obtain the current session time zone.

Ok. I may see the value of it for debug purposes (not API consistency).

Let's see how/when #6789 can be merged and also when an ODS change to MON could be done.

@AlexPeshkoff
Copy link
Member

AlexPeshkoff commented May 3, 2021 via email

@dyemanov
Copy link
Member

dyemanov commented May 4, 2021

I'm not sure it should be SESION_TIME_ZONE or CURRENT_TIMEZONE (like CURRENT_USER, CURRENT_ROLE).

The SQL spec has both CURRENT_USER and SESSION_USER and our CURRENT_USER actually behaves as standard's SESSION_USER. So I suppose one day we'll include SESSION_USER too and thus SESSION_TIMEZONE would be pretty consistent.

@asfernandes asfernandes changed the title Add session time zone to system context and MON$ATTACHMENTS Add session time zone to system context May 4, 2021
@asfernandes
Copy link
Member

I have changed title to match what was been committed in v4.0.

@asfernandes
Copy link
Member

New pull request created with the other part of this original issue.

#6794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants