Re: truncating timestamps on arbitrary intervals

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.

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

* Speaking of modulus, would it be clearer to express the
calculations like
	timestamp -= timestamp % interval;
(That's just a question, I'm not sure.)

* Code doesn't look to have thought carefully about what to do with
negative intervals, or BC timestamps.

* 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.

* 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
	}

* I'd use ERRCODE_INVALID_PARAMETER_VALUE for any case of disallowed
contents of the interval.

			regards, tom lane