From: | John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | David Fetter <david(at)fetter(dot)org>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: truncating timestamps on arbitrary intervals |
Date: | 2020-02-28 08:42:34 |
Message-ID: | CACPNZCvU9hDwWSiFcT-rvSf_gtt0xWBi8pO9JT4orfm57C8+pg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Feb 26, 2020 at 11:36 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> John Naylor <john(dot)naylor(at)2ndquadrant(dot)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
Attachment | Content-Type | Size |
---|---|---|
v4-datetrunc_interval.patch | application/octet-stream | 9.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2020-02-28 09:02:40 | Re: base backup client as auxiliary backend process |
Previous Message | Kyotaro Horiguchi | 2020-02-28 08:28:06 | Re: Make mesage at end-of-recovery less scary. |