Re: generate_series for timestamptz and time zone problem

On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch <przemyslaw(at)sztoch(dot)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 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.

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

> 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