Re: WIP: Make timestamptz_out less slow.

On 12 November 2015 at 13:44, Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> On Wed, Nov 4, 2015 at 6:30 PM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> >> Have you thought about *just* having an int64 pg_ltostr()? Are you
> >> aware of an appreciable overhead from having int32 callers just rely
> >> on a promotion to int64?
> >
> > I'd not thought of that. It would certainly add unnecessary overhead on
> > 32bit machines.
> > To be honest, I don't really like the idea of making fsec_t an int64,
> there
> > are just to many places around the code base that need to be updated.
> Plus
> > with float datetimes, we'd need to multiple the fractional seconds by
> > 1000000000, which is based on the MAX_TIME_PRECISION setting as defined
> when
> > HAVE_INT64_TIMESTAMP is undefined.
>
> OK.
>
> >> I would just use a period/full stop here:
> >>
> >> + * Note str is not NUL terminated, callers are responsible for NUL
> >> terminating
> >> + * str themselves.
> >>
> >
> > Do you mean change the comment to "Note str is not NUL terminated.
> Callers
> > are responsible for NUL terminating str themselves." ?
>
> Yes.
>
> > Yes, that's a bit ugly. How about just Assert(padding > 0) just like
> above?
> > That gets rid of one Note:
> > The optimized part is probably not that important anyway. I just
> mentioned
> > it to try and reduce the changes of someone using it when the padding is
> > always too small.
>
> Cool.
>
> >> Maybe it's better to just special case INT_MIN and the do an Abs()?
> >> Actually, would any likely caller of pg_ltostr_zeropad() actually care
> >> if INT_MIN was a case that you cannot handle, and assert against? You
> >> could perhaps reasonably make it the caller's problem. Haven't made up
> >> my mind if your timestamp_delta.patch is better than that; cannot
> >> immediately see a problem with putting it on the caller.
> >>
> >
> > I can make that case work the same as pg_ltoa() for pg_ltostr(), that's
> not
> > a problem, I did that in the delta patch. With pg_ltostr_zeropad() I'd
> need
> > to add some corner case code to prepend the correct number of zeros onto
> > -2147483648. This is likely not going to look very nice, and also it's
> > probably going to end up about the same number of lines of code to
> what's in
> > the patch already. If you think it's better for performance, then I've
> > already done tests to show that it's not better. I really don't think
> it'll
> > look very nice either. I quite like my negative number handling, since
> it's
> > actually code that would be exercised 50% of the time ran than 1/ 2^32 of
> > the time, assuming all numbers have an equal chance of being passed.
>
> Fair enough.
>
>
Many thanks for spending time on this Peter.

> >> More generally, maybe routines like EncodeDateTime() ought now to use
> >> the Abs() macro.
> >
> > You might be right about that. Then we can just have pg_ltostr() do
> unsigned
> > only. The reason I didn't do that is because I was trying to minimise
> what I
> > was changing in the patch, and I thought it was less likely to make it
> if I
> > started adding calls to Abs() around the code base.
>
> I am very familiar with being conflicted about changing things beyond
> what is actually strictly necessary. It's still something I deal with
> a lot. One case that I think I have right in recommending commenting
> on is the need to centrally document that there are many exceptions to
> the 1900-based behavior of struct pg_tm. As I said, we should not
> continue to inconsistently locally note the exceptions, even if your
> patch doesn't make it any worse.
>
>
Just to confirm, you mean this comment?

int tm_year; /* relative to 1900 */

Please let me know if you disagree, but I'm not sure it's the business of
this patch to fix that. If it's wrong now, then it was wrong before my
patch, so it should be a separate patch which fixes it.

At this stage I don't quite know what the fix should be, weather it's doing
tm->tm_year -= 1900; in timestamp2tm() after the j2date() call, or if it's
just removing the misleading comment.

I also don't quite understand why we bother having it relative to 1900 and
not just base it on 0.

Is there anything else you see that's pending before it can be marked as
ready for committer?

--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Training & Services