Add support for AT LOCAL

On Mon, 2023-06-05 at 23:13 -0400, Vik Fearing wrote:

The Standard defines time zone conversion as follows:

<datetime factor> ::=
   <datetime primary> [ <time zone> ]

<time zone> ::=
   AT <time zone specifier>

<time zone specifier> ::=
     LOCAL
   | TIME ZONE <interval primary>

While looking at something else, I noticed we do not support AT LOCAL.
The local time zone is defined as that of *the session*, not the server,
which can make this quite interesting in views where the view will
automatically adjust to the session's time zone.

Patch against 3f1aaaa180 attached.

+1 on the idea; it should be faily trivial, if not very useful.

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed. Shouldn't the resolution happen at query
execution time?

Yours,
Laurenz Albe

On 6/6/23 03:56, Laurenz Albe wrote:

On Mon, 2023-06-05 at 23:13 -0400, Vik Fearing wrote:

The Standard defines time zone conversion as follows:

<datetime factor> ::=
   <datetime primary> [ <time zone> ]

<time zone> ::=
   AT <time zone specifier>

<time zone specifier> ::=
     LOCAL
   | TIME ZONE <interval primary>

While looking at something else, I noticed we do not support AT LOCAL.
The local time zone is defined as that of *the session*, not the server,
which can make this quite interesting in views where the view will
automatically adjust to the session's time zone.

Patch against 3f1aaaa180 attached.

+1 on the idea; it should be faily trivial, if not very useful.

Thanks.

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed. Shouldn't the resolution happen at query
execution time?

current_setting(text) is stable, and my tests show that it is calculated
at execution time.

postgres=# prepare x as values (now() at local);
PREPARE
postgres=# set timezone to 'UTC';
SET
postgres=# execute x;
column1
----------------------------
2023-06-06 08:23:02.088634
(1 row)

postgres=# set timezone to 'Asia/Pyongyang';
SET
postgres=# execute x;
column1
----------------------------
2023-06-06 17:23:14.837219
(1 row)

Am I missing something?
--
Vik Fearing

On Tue, 2023-06-06 at 04:24 -0400, Vik Fearing wrote:

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed.  Shouldn't the resolution happen at query
execution time?

current_setting(text) is stable, and my tests show that it is calculated
at execution time.

Ah, ok, then sorry for the noise. I misread the code then.

Yours,
Laurenz Albe

On 2023-Jun-06, Laurenz Albe wrote:

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed. Shouldn't the resolution happen at query
execution time?

Sounds like it -- consider the case where the timestamp value is a
partition key and one of the partition boundaries falls in between two
timezone offsets for some particular ts value; then you use a prepared
query to read from a view defined with AT LOCAL. Partition pruning
would need to compute partitions to read from at runtime, not plan time.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On 6/12/23 17:37, Alvaro Herrera wrote:

On 2023-Jun-06, Laurenz Albe wrote:

At a quick glance, it looks like you resolve "timezone" at the time
the query is parsed. Shouldn't the resolution happen at query
execution time?

Sounds like it -- consider the case where the timestamp value is a
partition key and one of the partition boundaries falls in between two
timezone offsets for some particular ts value; then you use a prepared
query to read from a view defined with AT LOCAL. Partition pruning
would need to compute partitions to read from at runtime, not plan time.

Can you show me an example of that happening with my patch?
--
Vik Fearing

On 6 Jun 2023, at 05:13, Vik Fearing <vik@postgresfriends.org> wrote:

Patch against 3f1aaaa180 attached.

This patch fails to compile, the declaration of variables in the switch block
needs to be scoped within a { } block. I've fixed this trivial error in the
attached v2 and also reflowed the comments which now no longer fit.

--
Daniel Gustafsson

On 7/3/23 15:42, Daniel Gustafsson wrote:

On 6 Jun 2023, at 05:13, Vik Fearing <vik@postgresfriends.org> wrote:

Patch against 3f1aaaa180 attached.

This patch fails to compile, the declaration of variables in the switch block
needs to be scoped within a { } block.

Interesting. It compiles for me.

I've fixed this trivial error in the
attached v2 and also reflowed the comments which now no longer fit.

Thank you.
--
Vik Fearing

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hello

I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine.

Thank you

Cary Huang
------------------
Highgo Software Canada
www.highgo.ca

On 9/22/23 23:46, cary huang wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hello

I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine.

Thank you

Thank you for reviewing!
--
Vik Fearing

On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:

On 9/22/23 23:46, cary huang wrote:

I think this feature can be a useful addition in dealing with time
zones. I have applied and tried out the patch, The feature works as
described and seems promising. The problem with compilation failure
was probably reported on CirrusCI when compiled on different
platforms. I have run the latest patch on my own Cirrus CI environment
and everything checked out fine.

Thank you for reviewing!

+            | a_expr AT LOCAL                        %prec AT
+                {
+                    /* Use the value of the session's time zone */
+                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
+                                                list_make1(makeStringConst("TimeZone", -1)),
+                                                COERCE_SQL_SYNTAX,
+                                                -1);
+                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
+                                               list_make2(tz, $1),
+                                               COERCE_SQL_SYNTAX,
+                                               @2);

As the deparsing code introduced by this patch is showing, this leads
to a lot of extra complexity. And, actually, this can be quite
expensive as well with these two layers of functions. Note also that
in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
inlining. So here comes my question: why doesn't this stuff just use
one underlying function to do this job?
--
Michael

On 9/29/23 09:27, Michael Paquier wrote:

On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:

On 9/22/23 23:46, cary huang wrote:

I think this feature can be a useful addition in dealing with time
zones. I have applied and tried out the patch, The feature works as
described and seems promising. The problem with compilation failure
was probably reported on CirrusCI when compiled on different
platforms. I have run the latest patch on my own Cirrus CI environment
and everything checked out fine.

Thank you for reviewing!

+            | a_expr AT LOCAL                        %prec AT
+                {
+                    /* Use the value of the session's time zone */
+                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
+                                                list_make1(makeStringConst("TimeZone", -1)),
+                                                COERCE_SQL_SYNTAX,
+                                                -1);
+                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
+                                               list_make2(tz, $1),
+                                               COERCE_SQL_SYNTAX,
+                                               @2);

As the deparsing code introduced by this patch is showing, this leads
to a lot of extra complexity. And, actually, this can be quite
expensive as well with these two layers of functions. Note also that
in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
inlining. So here comes my question: why doesn't this stuff just use
one underlying function to do this job?

I guess I don't understand the question. Why do you think a single
function that repeats what these functions do would be preferable? I am
not sure how doing it a different way would be better.
--
Vik Fearing

On Tue, Oct 03, 2023 at 02:10:48AM +0200, Vik Fearing wrote:

On 9/29/23 09:27, Michael Paquier wrote:

As the deparsing code introduced by this patch is showing, this leads
to a lot of extra complexity. And, actually, this can be quite
expensive as well with these two layers of functions. Note also that
in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
inlining. So here comes my question: why doesn't this stuff just use
one underlying function to do this job?

I guess I don't understand the question. Why do you think a single function
that repeats what these functions do would be preferable? I am not sure how
doing it a different way would be better.

Leaving aside the ruleutils.c changes introduced by the patch that are
quite confusing, having one function in the executor stack is going to
be more efficient than two (aka less ExecInitFunc), and this syntax
could be used in SQL queries where the operations is repeated a lot.
This patch introduces two COERCE_SQL_SYNTAX, meaning that we would do
twice the ACL check, twice the function hook, etc, so this could lead
to significant differences. I think that we should be careful with
the approach taken, and do benchmarks to choose an efficient approach
from the start. See for example:
/messages/by-id/ZGHBGE45jKW8FEpe@paquier.xyz
--
Michael

On 9/29/23 09:27, Michael Paquier wrote:

On Sat, Sep 23, 2023 at 12:54:01AM +0200, Vik Fearing wrote:

On 9/22/23 23:46, cary huang wrote:

I think this feature can be a useful addition in dealing with time
zones. I have applied and tried out the patch, The feature works as
described and seems promising. The problem with compilation failure
was probably reported on CirrusCI when compiled on different
platforms. I have run the latest patch on my own Cirrus CI environment
and everything checked out fine.

Thank you for reviewing!

+            | a_expr AT LOCAL                        %prec AT
+                {
+                    /* Use the value of the session's time zone */
+                    FuncCall *tz = makeFuncCall(SystemFuncName("current_setting"),
+                                                list_make1(makeStringConst("TimeZone", -1)),
+                                                COERCE_SQL_SYNTAX,
+                                                -1);
+                    $$ = (Node *) makeFuncCall(SystemFuncName("timezone"),
+                                               list_make2(tz, $1),
+                                               COERCE_SQL_SYNTAX,
+                                               @2);

As the deparsing code introduced by this patch is showing, this leads
to a lot of extra complexity. And, actually, this can be quite
expensive as well with these two layers of functions. Note also that
in comparison to SQLValueFunctions, COERCE_SQL_SYNTAX does less
inlining. So here comes my question: why doesn't this stuff just use
one underlying function to do this job?

Okay. Here is a v3 using that approach.
--
Vik Fearing

On Wed, Oct 04, 2023 at 03:49:03PM +0100, Vik Fearing wrote:

Okay. Here is a v3 using that approach.

You have not posted any numbers to show if there's a difference in
performance, so I have run a simple test:
PREPARE test AS SELECT TIMESTAMP '1978-07-07 19:38' AT LOCAL;
DO $$ BEGIN
FOR i IN 1..1000000 LOOP
EXECUTE 'EXECUTE test';
END LOOP;
END $$;

On a medium-ish benchmark machine I have (16 vCPUs, 32GB of memory,
-O2, no asserts), this DO block takes in average 4.3s to run with v2,
versus 3.6s with v3. So yes, that's faster.

I haven't yet finished my review of the patch, still looking at it.
--
Michael

On 10/6/23 07:05, Michael Paquier wrote:

I haven't yet finished my review of the patch, still looking at it.

I realized that my regression tests are not exercising what I originally
intended them to after this change. They are supposed to show that
calling the function explicitly or using AT LOCAL is correctly
reproduced by ruleutils.c.

The attached v4 changes the regression tests (and nothing else).
--
Vik Fearing

On Sat, Oct 07, 2023 at 02:35:06AM +0100, Vik Fearing wrote:

I realized that my regression tests are not exercising what I originally
intended them to after this change. They are supposed to show that calling
the function explicitly or using AT LOCAL is correctly reproduced by
ruleutils.c.

Yes, I don't really see the point in adding more tests for the
deparsing of AT TIME ZONE in this context. I would not expect one to
call directly timezone() with the grammar in place, but I have no
objections either to do that in the view for the regression tests as
you are suggesting in v4. The minimal set of changes to test is to
make sure that both paths (ts->tstz and tstz->tz) are exercised, and
that's what you do.

Anyway, upon review, I have a few issues with this patch. First is
the documentation that I find too light:
- There is no description for AT LOCAL and what kind of result one
gets back depending on the input given, while AT TIME ZONE has more
details about the arguments that can be used and the results
obtained.
- The function timezone(zone, timestamp) is mentioned, and I think
that we should do the same with timezone(timestamp) for AT LOCAL.

Another thing that I have been surprised with is the following, which
is inconsistent with AT TIME ZONE because we are lacking one system
function:
=# select time with time zone '05:34:17-05' at local;
ERROR: 42883: function pg_catalog.timezone(time with time zone) does not exist

I think that we should include that to have a full set of operations
supported, similar to AT TIME ZONE (see \df+ timezone). It looks like
this would need one extra timetz_at_local(), which would require a bit
more refactoring in date.c so as an equivalent of timetz_zone() could
feed on the current session's TimeZone instead. I guess that you
could just have an timetz_zone_internal() that optionally takes a
timezone provided by the user or gets the current session's Timezone
(session_timezone). Am I missing something?

I am attaching a v5 that addresses the documentation bits, could you
look at the business with date.c?
--
Michael

On 10/10/23 05:34, Michael Paquier wrote:

I am attaching a v5 that addresses the documentation bits, could you
look at the business with date.c?

Here is a v6 which hopefully addresses all of your concerns.
--
Vik Fearing

On Fri, Oct 13, 2023 at 02:20:59AM +0200, Vik Fearing wrote:

On 10/10/23 05:34, Michael Paquier wrote:

I am attaching a v5 that addresses the documentation bits, could you
look at the business with date.c?

Here is a v6

Thanks for the new version.

which hopefully addresses all of your concerns.

Mostly ;)

The first thing I did was to extract the doc bits about timezone(zone,
time) for AT TIME ZONE from v6 and applied it independently.

I have then looked at the rest and it looked mostly OK to me,
including the extra description you have added for the fifth example
in the docs. I have tweaked a few things: the regression tests to
make the views a bit more appealing to the eye, an indentation to not
have koel complain and did a catalog bump. Then applied it.
--
Michael

On 10/13/23 05:07, Michael Paquier wrote:

On Fri, Oct 13, 2023 at 02:20:59AM +0200, Vik Fearing wrote:

On 10/10/23 05:34, Michael Paquier wrote:

I am attaching a v5 that addresses the documentation bits, could you
look at the business with date.c?

Here is a v6

Thanks for the new version.

which hopefully addresses all of your concerns.

Mostly ;)

The first thing I did was to extract the doc bits about timezone(zone,
time) for AT TIME ZONE from v6 and applied it independently.

I have then looked at the rest and it looked mostly OK to me,
including the extra description you have added for the fifth example
in the docs. I have tweaked a few things: the regression tests to
make the views a bit more appealing to the eye, an indentation to not
have koel complain and did a catalog bump. Then applied it.

Thank you, Michael君!
--
Vik Fearing