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