decode timestamp without timezone as local DateTime and decode timestamp with timezone respecting the timezone defined in the connection

[insinfo]

decode timestamp without timezone as local DateTime and decode timestamp with timezone respecting the timezone defined in the connection
timeZone option in ConnectionSettings is now a TimeZoneSettings type instead of String

[isoos]

Thank you for looking into this, it looks like this won't be an easy feature to own/include in the package, due its sheer complexity.

The main thing is: the timezone database in its current form seems to be not maintainable: it feels like it is generated data, but also contains hand-written parts, and it has no automatic way to get updated from a canonical source. I think this part of the code should live in a separate package with a clean way to get updated.

There are tests that are failing due to the encode/decode changes. I am about to go offline for longer periods in the next 2 weeks, so I have only sporadic time to review, but I would expect the changes to be local to timestamp without timezone, and shouldn't affect other types.

I'd expect much more new tests for this (and one of the reasons to be cautious about a PR such like this), e.g. having different tests that set the postgres server's (and docker images's) timezone to different settings and also using different values for the client timezone. It is very likely that this is something we should implement in multiple steps: first create a test suite with different time zones that replicate the undesired behavior, then in a separate change update the timestamp handling, correcting the tests.

Nit, but the changes in their current form are breaking on so many ways that I'd just go for a 4.0.0 release version. Unless you could find a way to use the new ones only as an incremental setting.

Althogether, I think this is in a good direction, but these should be the next steps before this can be part of the postgres package:

• creating a test suite with different time zone settings revealing the current behaviour and marking what needs to be changed
• separating the timezone library out of this package
• implement the parsing change and test update

[insinfo]

I placed the timezone implementation as an external package pg_timezone, and modified the implementation so that it does not break the tests and is backwards compatible, that is, so that the decoding behavior of timestamp, timestamptz and date are UTC by default, so there is no need to make changes to the tests and it will not break any existing code base.

/// A class to configure time zone settings for decoding timestamps and dates.
class TimeZoneSettings {
  /// The default time zone value.
  /// 
  /// The [value] represents the name of the time zone location. Default is 'UTC'.
  String value = 'UTC';

  /// Creates a new instance of [TimeZoneSettings].
  /// 
  /// [value] is the name of the time zone location.
  /// 
  /// The optional named parameters:
  /// - [forceDecodeTimestamptzAsUTC]: if true, decodes timestamps with timezone (timestamptz) as UTC. If false, decodes them using the timezone defined in the connection.
  /// - [forceDecodeTimestampAsUTC]: if true, decodes timestamps without timezone (timestamp) as UTC. If false, decodes them as local datetime.
  /// - [forceDecodeDateAsUTC]: if true, decodes dates as UTC. If false, decodes them as local datetime.
  TimeZoneSettings(
    this.value, {
    this.forceDecodeTimestamptzAsUTC = true,
    this.forceDecodeTimestampAsUTC = true,
    this.forceDecodeDateAsUTC = true,
  });

  /// If true, decodes the timestamp with timezone (timestamptz) as UTC.
  /// If false, decodes the timestamp with timezone using the timezone defined in the connection.
  bool forceDecodeTimestamptzAsUTC = true;

  /// If true, decodes the timestamp without timezone (timestamp) as UTC.
  /// If false, decodes the timestamp without timezone as local datetime.
  bool forceDecodeTimestampAsUTC = true;

  /// If true, decodes the date as UTC.
  /// If false, decodes the date as local datetime.
  bool forceDecodeDateAsUTC = true;
}

[isoos]

Thanks for the updates so far, this is now getting into a code size which is reasonable to review. I've added a few more notes, and I'm planning to give this a try locally today or very soon.

I'll also take a look into the pg_timezone package, looks like great work there! As it seems, the client is getting bigger now, I should also think about how we manage all the dependencies if they happen to diverge later.

[isoos]

I'm wondering, without fully seeing the context, if this operation is always valid. E.g. can this be close to or larger than a full day? Or instead of substracting a half hour, are we adding 23.5 hours, just because of a strange combination?

[insinfo]

I believe that there will not be this problem because the timezone transitions of the past are more related to the daylight saving time of countries that by law no longer have daylight saving time.

[isoos]

Let's extract these two into a method that does the correction.

[insinfo]

yes definitely

[isoos]

Is there a reason for changing this to a switch statement? If all else being equal, let's not change this (or if this is needed, let's do this in a different PR).

[insinfo]

At first I thought I needed to change this but then I saw that it wouldn't be necessary, it turns out that I forgot to revert this change.

[isoos]

Let's not update the timeZone value like this, it is a bit unexpected here. The v3/connection.dart _handleMessage already stores it in PgConnectionImplementation._parameters and we should just read that value from there. I think the settings should be immutable (maybe rename TimeZoneSettings.value into defaultTimeZone), and use the server-provided timeZone if present, otherwise the fallback.

[insinfo]

I believe that in this case it should not be immutable because when you use the SQL command "set timezone TO 'GMT';" you are changing the connection configuration, and this has to be reflected in the connection instance, because if after the user executes the SQL command to change the timezone and he reads the timezone property he will want to see the current value, right? I don't know the driver code very well, especially version 3, so I don't know if the driver user can read the timezone of the current connection through some method or property, as I have been very busy I haven't had much time to look at this, I don't know exactly how this could be done in another way.