Re: WIP: Make timestamptz_out less slow. - Mailing list pgsql-hackers

On Wed, Nov 4, 2015 at 6:30 PM, David Rowley
<david.rowley@2ndquadrant.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.

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

-- 
Peter Geoghegan