Re: generate_series for timestamptz and time zone problem

Przemysław Sztoch <przemyslaw@sztoch.pl>

From: Przemysław Sztoch <przemyslaw@sztoch.pl>
To: Gurjeet Singh <gurjeet@singh.im>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2023-01-30T12:21:01Z
Lists: pgsql-hackers
Gurjeet Singh wrote on 30.01.2023 08:18:
> On Sat, Nov 12, 2022 at 10:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, what it shouldn't be is part of this patch.  It's worth
>> pushing it separately to have a record of that decision.  I've
>> now done that, so you'll need to rebase to remove that delta.
>>
>> I looked over the v5 patch very briefly, and have two main
>> complaints:
>>
>> * There's no documentation additions.  You can't add a user-visible
>> function without adding an appropriate entry to func.sgml.
>>
>> * I'm pretty unimpressed with the whole truncate-to-interval thing
>> and would recommend you drop it.  I don't think it's adding much
>> useful functionality beyond what we can already do with the existing
>> date_trunc variants; and the definition seems excessively messy
>> (too many restrictions and special cases).
> Please see attached v6 of the patch.
>
> The changes since v5 are:
> .) Rebased and resolved conflicts caused by commit 533e02e92.
> .) Removed code and tests related to new date_trunc() functions, as
> suggested by Tom.
> .) Added 3 more variants to accompany with date_add(tstz, interval, zone).
>      date_add(tstz, interval)
>      date_subtract(tstz, interval)
>      date_subtract(tstz, interval, zone)
>
> .) Eliminate duplication of code; use common function to implement
> generate_series_timestamptz[_at_zone]() functions.
> .) Fixed bug where in one of the new code paths,
> generate_series_timestamptz_with_zone(),  did not perform
> TIMESTAMP_NOT_FINITE() check.
> .) Replaced some DirectFunctionCall?() with direct calls to the
> relevant *_internal() function; should be better for performance.
> .) Added documentation all 5 functions (2 date_add(), 2
> date_subtract(), 1 overloaded version of generate_series()).
Other work distracted me from this patch.
I looked at your update v6 and it looks ok.
For me the date_trunc function is important and I still have some corner 
cases. Now I will continue working with data_trunc in a separate patch.
> I'm not sure of the convention around authorship. But since this was
> not an insignificant amount of work, would this patch be considered as
> co-authored by Przemyslaw and myself? Should I add myself to Authors
> field in the Commitfest app?
I see no obstacles for us to be co-authors.
> Hi Przemyslaw,
>      I started working on this patch based on Tom's review a few days
> ago, since you hadn't responded in a while, and I presumed you're not
> working on this anymore. I should've consulted with/notified you of my
> intent before starting to work on it, to avoid duplication of work.
> Sorry if this submission obviates any work you have in progress.
> Please feel free to provide your feedback on the v6 of the patch.
I propose to get the approval of the current truncated version of the 
patch. As I wrote above, I will continue work on date_trunc later and as 
a separate patch.
-- 
Przemysław Sztoch | Mobile +48 509 99 00 66

Commits

  1. Add functions to do timestamptz arithmetic in a non-default timezone.

  2. Refactor datetime functions' timezone lookup code to reduce duplication.

  3. Fix volatility marking of timestamptz_trunc_zone.