Re: Add support for AT LOCAL

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Vik Fearing <vik(at)postgresfriends(dot)org>
Cc: cary huang <hcary328(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Add support for AT LOCAL
Date: 2023-10-10 04:34:32
Message-ID: ZSTUWON7DOfo-2ES@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v5-0001-Add-support-for-AT-LOCAL-with-timestamps.patch text/x-diff 13.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2023-10-10 04:36:01 Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
Previous Message Andres Freund 2023-10-10 04:14:15 Re: Lowering the default wal_blocksize to 4K