On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch <przemys...@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 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
