Thread: generate_series for timestamptz and time zone problem

generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval )
produces results depending on the timezone value set:

SET timezone = 'UTC';
SELECT ts, ts AT TIME ZONE 'UTC'
FROM generate_series('2022-03-26 00:00:00+01'::timestamptz, '2022-03-30 00:00:00+01'::timestamptz, '1 day') AS ts;

SET timezone = 'Europe/Warsaw';
SELECT ts, ts AT TIME ZONE 'UTC'
FROM generate_series('2022-03-26 00:00:00+01'::timestamptz, '2022-03-30 00:00:00+01'::timestamptz, '1 day') AS ts;

Sometimes this is a very big problem.

The fourth argument with the time zone will be very useful:
generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval  [, zone text] )

=?UTF-8?Q?Przemys=c5=82aw_Sztoch?= <przemyslaw@sztoch.pl> writes:
> |generate_series| ( /|start|/ |timestamp with time zone|, /|stop|/ 
> |timestamp with time zone|, /|step|/ |interval| )
> produces results depending on the timezone value set:

That's intentional.  If you don't want it, maybe you should be using
generate_series on timestamp without time zone?

            regards, tom lane

Tom Lane wrote on 31.05.2022 22:54:

Przemysław Sztoch <przemyslaw@sztoch.pl> writes:

|generate_series| ( /|start|/ |timestamp with time zone|, /|stop|/ 
|timestamp with time zone|, /|step|/ |interval| )
produces results depending on the timezone value set:

That's intentional.  If you don't want it, maybe you should be using
generate_series on timestamp without time zone?
			regards, tom lane

1. Of course it is intentional.  And usually everything works as it should.

But with multi-zone applications, using timestamptz generates a lot of trouble.
It would be appropriate to supplement a few functions with the possibility of specifying a zone (of course, for timestamptz variants):
- generate_series
- date_bin (additionally  with support for months and years)
- timestamptz_plus_interval (the key issue is adding months and years, "+" operator only does this in the local zone)

Not everything can be solved by converting the time between timestamptz and timestamp (e.g. using the timezone function).
Daylight saving time reveals additional problems that are not visible at first glance.

Just if DST did not exist, a simple conversion (AT TIME ZONE '...') would have been enough.
Unfortunately, DST is popular and, additionally, countries modify their time zones from time to time.

2. Because I lack the necessary experience, I want to introduce changes in parts.
There is patch for first function timestamptz_plus_interval.

I don't know how to properly correct pg_proc.dat and add a variant of this function with 3 arguments now.

Please comment on the patch and provide tips for pg_proc.
If it works for me, I will improve generate_series.

Dear colleagues,
Please let me know what is the convention (procedure) of adding new functions to pg_proc. Specifically how oid is allocated.
This will allow me to continue working on the patch.

I have to extend the timestamptz_pl_interval function, which is in fact an addition operator. But an additional parameter is needed to specify the timezone.
Therefore, should I add a second function timestamptz_pl_interval with three arguments, or should a function with a different name be added so that it does not get confused with operator functions (which only have two arguments)?
What is the proposed name for such a function (add(timestamptz, interval, timezone), date_add(timestamptz, interval, timezone), ...)?

Przemysław Sztoch wrote on 01.06.2022 16:45:

Tom Lane wrote on 31.05.2022 22:54:

Przemysław Sztoch <przemyslaw@sztoch.pl> writes:

|generate_series| ( /|start|/ |timestamp with time zone|, /|stop|/ 
|timestamp with time zone|, /|step|/ |interval| )
produces results depending on the timezone value set:

That's intentional.  If you don't want it, maybe you should be using
generate_series on timestamp without time zone?
			regards, tom lane

1. Of course it is intentional.  And usually everything works as it should.

But with multi-zone applications, using timestamptz generates a lot of trouble.
It would be appropriate to supplement a few functions with the possibility of specifying a zone (of course, for timestamptz variants):
- generate_series
- date_bin (additionally  with support for months and years)
- timestamptz_plus_interval (the key issue is adding months and years, "+" operator only does this in the local zone)

Not everything can be solved by converting the time between timestamptz and timestamp (e.g. using the timezone function).
Daylight saving time reveals additional problems that are not visible at first glance.

Just if DST did not exist, a simple conversion (AT TIME ZONE '...') would have been enough.
Unfortunately, DST is popular and, additionally, countries modify their time zones from time to time.

2. Because I lack the necessary experience, I want to introduce changes in parts.
There is patch for first function timestamptz_plus_interval.

I don't know how to properly correct pg_proc.dat and add a variant of this function with 3 arguments now.

Please comment on the patch and provide tips for pg_proc.
If it works for me, I will improve generate_series.

--
Przemysław Sztoch | Mobile +48 509 99 00 66

=?UTF-8?Q?Przemys=c5=82aw_Sztoch?= <przemyslaw@sztoch.pl> writes:
> Please let me know what is the convention (procedure) of adding new
> functions to pg_proc. Specifically how oid is allocated.

See
https://www.postgresql.org/docs/devel/system-catalog-initial-data.html#SYSTEM-CATALOG-OID-ASSIGNMENT
(you should probably read that whole chapter for context).

> Therefore, should I add a second function timestamptz_pl_interval with
> three arguments, or should a function with a different name be added so
> that it does not get confused with operator functions (which only have
> two arguments)?

That's where you get into beauty-is-in-the-eye-of-the-beholder
territory.  There's some value in naming related functions alike,
but on the other hand I doubt timestamptz_pl_interval would have
been named so verbosely if anyone expected it to be called by
name rather than via an operator.  Coming up with good names is
part of the work of preparing a patch like this.

            regards, tom lane

Tom Lane wrote on 14.06.2022 15:43:

Przemysław Sztoch <przemyslaw@sztoch.pl> writes:

Please let me know what is the convention (procedure) of adding new 
functions to pg_proc. Specifically how oid is allocated.

See
https://www.postgresql.org/docs/devel/system-catalog-initial-data.html#SYSTEM-CATALOG-OID-ASSIGNMENT
(you should probably read that whole chapter for context).

Thx.

There is another patch.
It works, but one thing is wrongly done because I lack knowledge.

Where I'm using DirectFunctionCall3 I need to pass the timezone name, but I'm using cstring_to_text and I'm pretty sure there's a memory leak here. But I need help to fix this.
I don't know how best to store the timezone in the generate_series context. Please, help.

Przemysław Sztoch wrote on 14.06.2022 21:46:

Tom Lane wrote on 14.06.2022 15:43:

Przemysław Sztoch <przemyslaw@sztoch.pl> writes:

Please let me know what is the convention (procedure) of adding new 
functions to pg_proc. Specifically how oid is allocated.

See
https://www.postgresql.org/docs/devel/system-catalog-initial-data.html#SYSTEM-CATALOG-OID-ASSIGNMENT
(you should probably read that whole chapter for context).

Thx.

There is another patch.
It works, but one thing is wrongly done because I lack knowledge.

Where I'm using DirectFunctionCall3 I need to pass the timezone name, but I'm using cstring_to_text and I'm pretty sure there's a memory leak here. But I need help to fix this.
I don't know how best to store the timezone in the generate_series context. Please, help.

Please give me feedback on how to properly store the timezone name in the function context structure. I can't finish my work without it.

Additionally, I added a new variant of the date_trunc function that takes intervals as an argument.
It enables functionality similar to date_bin, but supports monthly, quarterly, annual, etc. periods.
In addition, it is resistant to the problems of different time zones and daylight saving time (DST).

--

On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch <przemyslaw@sztoch.pl> wrote:
> There is another patch.
> It works, but one thing is wrongly done because I lack knowledge.

Thank you for continuing to work on it despite this being your first
time contributing, and despite the difficulties. I'll try to help as
much as I can.

> Where I'm using DirectFunctionCall3 I need to pass the timezone name, but I'm using cstring_to_text and I'm pretty
surethere's a memory leak here. But I need help to fix this. 
> I don't know how best to store the timezone in the generate_series context. Please, help.

In Postgres code we generally don't worry about memory leaks (a few
caveats apply). The MemoryContext infrastructure (see aset.c) enables
us to be fast and loose with memory allocations. A good way to know if
you should be worried about your allocations, is to look in the
neighboring code, and see what does it do with the memory it
allocates.

I think your use of cstring_to_text() is safe.

> Please give me feedback on how to properly store the timezone name in the function context structure. I can't finish
mywork without it. 

The way I see it, I don't think you need to store the tz-name in the
function context structure, like you're currently doing. I think you
can remove the additional member from the
generate_series_timestamptz_fctx struct, and refactor your code in
generate_series_timestamptz() to work without it.; you seem to  be
using the tzname member almost as a boolean flag, because the actual
value you pass to DFCall3() can be calculated without first storing
anything in the struct.

> Additionally, I added a new variant of the date_trunc function that takes intervals as an argument.
> It enables functionality similar to date_bin, but supports monthly, quarterly, annual, etc. periods.
> In addition, it is resistant to the problems of different time zones and daylight saving time (DST).

This addition is beyond the original scope (add TZ param), so I think
it would be considered a separate change/feature. But for now, we can
keep it in.

Although not necessary, it'd be nice to have changes that can be
presented as single units, be a patch of their own. If  you're
proficient with Git, can you please maintain each SQL-callable
function as a separate commit in your branch,  and use `git
format-patch` to generate a series for submission.

Can you please explain why you chose to remove the provolatile
attribute from the existing entry of date_trunc in pg_proc.dat.

It seems like you've picked/reused code from neighboring functions
(e.g. from timestamptz_trunc_zone()). Can you please see if you can
turn such code into a function, and  call the function, instead of
copying it.

Also, according to the comment at the top of pg_proc.dat,

 # Once upon a time these entries were ordered by OID.  Lately it's often
 # been the custom to insert new entries adjacent to related older entries.

So instead of adding your entries at the bottom of the file, please
each entry closer to an existing entry that's relevant to it.

I'm glad that you're following the advice on the patch-submission wiki
page [1]. When submitting a patch for committers' consideration,
though, the submission needs to cross quite a few hurdles. So have
prepared a markdown doc [2]. Let's fill in as much detail there as
possible, before we mark it 'Ready for Committer' in the CF app.

[1]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
[2]: https://wiki.postgresql.org/wiki/Patch_Reviews

Best regards,
Gurjeet
http://Gurje.et

Gurjeet Singh wrote on 01.07.2022 06:35:

On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch <przemyslaw@sztoch.pl> wrote:

Please give me feedback on how to properly store the timezone name in the function context structure. I can't finish my work without it.

The way I see it, I don't think you need to store the tz-name in the
function context structure, like you're currently doing. I think you
can remove the additional member from the
generate_series_timestamptz_fctx struct, and refactor your code in
generate_series_timestamptz() to work without it.; you seem to  be
using the tzname member almost as a boolean flag, because the actual
value you pass to DFCall3() can be calculated without first storing
anything in the struct.

Do I understand correctly that functions that return SET are executed multiple times?
Is access to arguments available all the time?
I thought PG_GETARG_ could only be used when SRF_IS_FIRSTCALL () is true - was I right or wrong?

Can you please explain why you chose to remove the provolatile
attribute from the existing entry of date_trunc in pg_proc.dat.

I believe it was a mistake in PG code.
All timestamptz functions must be STABLE as they depend on the current: SHOW timezone.
If new functions are created that pass the zone as a parameter, they become IMMUTABLE.
FIrst date_trunc function implementaion was without time zone parameter and someone who
added second variant (with timezone as parameter) copied the definition without removing the STABLE flag.

It seems like you've picked/reused code from neighboring functions
(e.g. from timestamptz_trunc_zone()). Can you please see if you can
turn such code into a function, and  call the function, instead of
copying it.

Ok. Changed.

Also, according to the comment at the top of pg_proc.dat,
 # Once upon a time these entries were ordered by OID.  Lately it's often # been the custom to insert new entries adjacent to related older entries.

So instead of adding your entries at the bottom of the file, please
each entry closer to an existing entry that's relevant to it.

Ok. Changed.

Some regression tests has been added.

I have problem with this:
-- Considering only built-in procs (prolang = 12), look for multiple uses
-- of the same internal function (ie, matching prosrc fields).  It's OK to
-- have several entries with different pronames for the same internal function,
-- but conflicts in the number of arguments and other critical items should
-- be complained of.  (We don't check data types here; see next query.)
-- Note: ignore aggregate functions here, since they all point to the same
-- dummy built-in function.
SELECT p1.oid, p1.proname, p2.oid, p2.proname (...):
 oid  |         proname         | oid  |     proname
------+-------------------------+------+-----------------
 1189 | timestamptz_pl_interval | 8800 | date_add
  939 | generate_series         | 8801 | generate_series
(2 rows)

=?UTF-8?Q?Przemys=c5=82aw_Sztoch?= <przemyslaw@sztoch.pl> writes:
> I have problem with this:
> -- Considering only built-in procs (prolang = 12), look for multiple uses
> -- of the same internal function (ie, matching prosrc fields).  It's OK to
> -- have several entries with different pronames for the same internal 
> function,
> -- but conflicts in the number of arguments and other critical items should
> -- be complained of.  (We don't check data types here; see next query.)

It's telling you you're violating project style.  Don't make multiple
pg_proc entries point at the same C function and then use PG_NARGS
to disambiguate; instead point at two separate functions.  The functions
can share code at the next level down, if they want.  (Just looking
at the patch, though, I wonder if sharing code is really beneficial
in this case.  It seems quite messy, and I wouldn't be surprised
if it hurts performance in the existing case.)

You also need to expend some more effort on refactoring code, to
eliminate silliness like looking up the timezone name each time
through the SRF.  That's got to be pretty awful performance-wise.

            regards, tom lane

Tom Lane wrote on 04.07.2022 00:31:

Przemysław Sztoch <przemyslaw@sztoch.pl> writes:

I have problem with this:
-- Considering only built-in procs (prolang = 12), look for multiple uses
-- of the same internal function (ie, matching prosrc fields).  It's OK to
-- have several entries with different pronames for the same internal 
function,
-- but conflicts in the number of arguments and other critical items should
-- be complained of.  (We don't check data types here; see next query.)

It's telling you you're violating project style.  Don't make multiple
pg_proc entries point at the same C function and then use PG_NARGS
to disambiguate; instead point at two separate functions.  The functions
can share code at the next level down, if they want.  (Just looking
at the patch, though, I wonder if sharing code is really beneficial
in this case.  It seems quite messy, and I wouldn't be surprised
if it hurts performance in the existing case.)

You also need to expend some more effort on refactoring code, to
eliminate silliness like looking up the timezone name each time
through the SRF.  That's got to be pretty awful performance-wise.
			regards, tom lane

Thx. Code is refactored. It is better, now.
 

Przemysław Sztoch wrote on 01.07.2022 15:43:

Gurjeet Singh wrote on 01.07.2022 06:35:

On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch <przemyslaw@sztoch.pl> wrote:

Please give me feedback on how to properly store the timezone name in the function context structure. I can't finish my work without it.

The way I see it, I don't think you need to store the tz-name in the
function context structure, like you're currently doing. I think you
can remove the additional member from the
generate_series_timestamptz_fctx struct, and refactor your code in
generate_series_timestamptz() to work without it.; you seem to  be
using the tzname member almost as a boolean flag, because the actual
value you pass to DFCall3() can be calculated without first storing
anything in the struct.

Do I understand correctly that functions that return SET are executed multiple times?
Is access to arguments available all the time?
I thought PG_GETARG_ could only be used when SRF_IS_FIRSTCALL () is true - was I right or wrong?

Dear Gurjeet,
I thought a bit after riding the bikes and the code repaired itself. :-)
Thanks for the clarification. Please check if patch v5 is satisfactory for you.

Can you please explain why you chose to remove the provolatile
attribute from the existing entry of date_trunc in pg_proc.dat.

I believe it was a mistake in PG code.
All timestamptz functions must be STABLE as they depend on the current: SHOW timezone.
If new functions are created that pass the zone as a parameter, they become IMMUTABLE.
FIrst date_trunc function implementaion was without time zone parameter and someone who
added second variant (with timezone as parameter) copied the definition without removing the STABLE flag.

Have I convinced everyone that this change is right? I assume I'm right and the mistake will be fatal.

=?UTF-8?Q?Przemys=c5=82aw_Sztoch?= <przemyslaw@sztoch.pl> writes:
> Gurjeet Singh wrote on 01.07.2022 06:35:
>> Can you please explain why you chose to remove the provolatile
>> attribute from the existing entry of date_trunc in pg_proc.dat.

> I believe it was a mistake in PG code.
> All timestamptz functions must be STABLE as they depend on the current: 
> SHOW timezone.
> If new functions are created that pass the zone as a parameter, they 
> become IMMUTABLE.
> FIrst date_trunc function implementaion was without time zone parameter 
> and someone who
> added second variant (with timezone as parameter) copied the definition 
> without removing the STABLE flag.

Yeah, I think you are right, and the someone was me :-( (see 600b04d6b).

I think what I was thinking is that timezone definitions do change
fairly often and maybe we shouldn't risk treating them as immutable.
However, we've not taken that into account in other volatility
markings; for example the timezone() functions that underly AT TIME
ZONE are marked immutable, which is surely wrong if you are worried
about zone definitions changing.  Given how long that's stood without
complaint, I think marking timestamptz_trunc_zone as immutable
should be fine.

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).

            regards, tom lane

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()).

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?

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.

Best regards,
Gurjeet
http://Gurje.et

Gurjeet Singh <gurjeet@singh.im> writes:
> On Sat, Nov 12, 2022 at 10:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I looked over the v5 patch very briefly, and have two main
>> complaints:
>> ...

> Please see attached v6 of the patch.

Thanks for updating that!

> 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?

While I'm not promising to commit this, if I were doing so I would
cite both of you as authors.  So feel free to change the CF entry.

            regards, tom lane

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.

Gurjeet Singh <gurjeet@singh.im> writes:
> [ generate_series_with_timezone.v6.patch ]

The cfbot isn't terribly happy with this.  It looks like UBSan
is detecting some undefined behavior.  Possibly an uninitialized
variable?

            regards, tom lane

On Mon, Jan 30, 2023 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Gurjeet Singh <gurjeet@singh.im> writes:
> > [ generate_series_with_timezone.v6.patch ]
>
> The cfbot isn't terribly happy with this.  It looks like UBSan
> is detecting some undefined behavior.  Possibly an uninitialized
> variable?

It was the classical case of out-of-bounds access. I was trying to
access 4th argument, even in the case where the 3-argument variant of
generate_series() was called.

Please see attached v7 of the patch. It now checks PG_NARGS() before
accessing the optional parameter.

This mistake would've been caught early if there were assertions
preventing access beyond the number of arguments passed to the
function. I'll send the assert_enough_args.patch, that adds these
checks, in a separate thread to avoid potentially confusing cfbot.

Best regards,
Gurjeet
http://Gurje.et

On 31/01/2023 08:50, Gurjeet Singh wrote:
> On Mon, Jan 30, 2023 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Gurjeet Singh <gurjeet@singh.im> writes:
>>> [ generate_series_with_timezone.v6.patch ]
>> The cfbot isn't terribly happy with this.  It looks like UBSan
>> is detecting some undefined behavior.  Possibly an uninitialized
>> variable?
> It was the classical case of out-of-bounds access. I was trying to
> access 4th argument, even in the case where the 3-argument variant of
> generate_series() was called.
>
> Please see attached v7 of the patch. It now checks PG_NARGS() before
> accessing the optional parameter.
>
> This mistake would've been caught early if there were assertions
> preventing access beyond the number of arguments passed to the
> function. I'll send the assert_enough_args.patch, that adds these
> checks, in a separate thread to avoid potentially confusing cfbot.

Tested this patch on current head.
The patch applies, with a few offsets.

Functionality wise it works as documented, also tried with 
"America/New_York" and "Europe/Berlin" as time zone.
The included tests cover both an entire year (including a new year), and 
also a DST switch (date_add() for 2021-10-31 in Europe/Warsaw, which is 
the date the country switches to standard time).

Minor nitpick: the texts use both "time zone" and "timezone".


Regards,

-- 
                Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Pushed v7 after making a bunch of cosmetic changes.  One gripe
I had was that rearranging the logic in timestamptz_pl_interval[_internal]
made it nearly impossible to see what functional changes you'd made
there, while not really buying anything in return.  I undid that to
make the diff readable.

I did not push the fmgr.h changes.  Maybe that is worthwhile (although
I'd vote against it), but it certainly does not belong in a localized
feature patch.

            regards, tom lane