Gurjeet Singh wrote on 30.01.2023 08:18:
> On Sat, Nov 12, 2022 at 10:44 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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