On Wed, Feb 26, 2020 at 11:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@2ndquadrant.com> writes:
> > [ v3-datetrunc_interval.patch ]
>
> A few thoughts:
>
> * In general, binning involves both an origin and a stride. When
> working with plain numbers it's almost always OK to set the origin
> to zero, but it's less clear to me whether that's all right for
> timestamps. Do we need another optional argument? Even if we
> don't, "zero" for tm_year is 1900, which is going to give results
> that surprise somebody.
Not sure.
A surprise I foresee in general might be: '1 week' is just '7 days',
and not aligned on "WOY". Since the function is passed an interval and
not text, we can't raise a warning. But date_trunc() already covers
that, so probably not a big deal.
> * I'm still not convinced that the code does the right thing for
> 1-based months or days. Shouldn't you need to subtract 1, then
> do the modulus, then add back 1?
Yes, brain fade on my part. Fixed in the attached v4.
> * Speaking of modulus, would it be clearer to express the
> calculations like
> timestamp -= timestamp % interval;
> (That's just a question, I'm not sure.)
Seems nicer, so done that way.
> * Code doesn't look to have thought carefully about what to do with
> negative intervals, or BC timestamps.
By accident, negative intervals currently behave the same as positive
ones. We could make negative intervals round up rather than truncate
(or vice versa). I don't know the best thing to do here.
As for BC, changed so it goes in the correct direction at least, and added test.
> * The comment
> * Justify all lower timestamp units and throw an error if any
> * of the lower interval units are non-zero.
> doesn't seem to have a lot to do with what the code after it actually
> does. Also, you need explicit /* FALLTHRU */-type comments in that
> switch, or pickier buildfarm members will complain.
Stale comment from an earlier version, fixed. Not sure if "justify" is
the right term, but "zero" is a bit misleading. Added fall thru's.
> * Seems like you could jam all the unit-related error checking into
> that switch's default: case, where it will cost nothing if the
> call is valid:
>
> switch (unit)
> {
> ...
> default:
> if (unit == 0)
> // complain about zero interval
> else
> // complain about interval with multiple components
> }
Done.
> * I'd use ERRCODE_INVALID_PARAMETER_VALUE for any case of disallowed
> contents of the interval.
Done.
Also removed the millisecond case, since it's impossible, or at least
not worth it, to distinguish from the microsecond case.
Note: I haven't done any additional docs changes in v4.
TODO: with timezone
possible TODO: origin parameter
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
