Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 7/02/2016 4:14 am, "Tom Lane"  wrote:
>
> David Rowley  writes:
> [ timestamp_out_speedup_2015-11-05.patch ]
>
> Pushed with a bunch of cosmetic tweaks.

Great. Thanks for pushing this.

David

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Tom Lane]

David Rowley  writes:
[ timestamp_out_speedup_2015-11-05.patch ]

Pushed with a bunch of cosmetic tweaks.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 29 November 2015 at 14:00, Peter Geoghegan  wrote:

> On Sun, Nov 22, 2015 at 2:20 AM, David Rowley
>  wrote:
> > 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.
>
> That's fair. I defer to the judgement of the committer here.
>
> > Is there anything else you see that's pending before it can be marked as
> > ready for committer?
>
> Can't think of any reason not to. It's been marked "ready for committer".
>
>
Many thanks for reviewing this Peter.

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Peter Geoghegan]

On Sun, Nov 22, 2015 at 2:20 AM, David Rowley
 wrote:
> 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.

That's fair. I defer to the judgement of the committer here.

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

Can't think of any reason not to. It's been marked "ready for committer".

Thanks

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 12 November 2015 at 13:44, Peter Geoghegan  wrote:

> On Wed, Nov 4, 2015 at 6:30 PM, David Rowley
>  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
> > 10, 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/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Peter Geoghegan]

On Wed, Nov 4, 2015 at 6:30 PM, David Rowley
 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
> 10, 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Peter Geoghegan]

On Fri, Sep 11, 2015 at 9:00 AM, David Rowley
 wrote:
> Attached is also timestamp_delta.patch which changes pg_ltostr() to use the
> INT_MIN special case that pg_ltoa also uses. I didn't make a similar change
> to pg_ltostr_zeropad() as I'd need to add even more special code to prepend
> the correct number of zero before the 2147483648. This zero padding reason
> was why I originally came up with the alternative way to handle the negative
> numbers. I had just thought that it was best to keep both functions as
> similar as possible.

I've started looking at this -- "timestamp_out_speedup_2015-09-12.patch".

> I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> AppendSeconds(). The only way I can think to handle this is to just make
> fsec_t unconditionally an int64 (since with float datetimes the precision is
> 10 decimal digits after the dec point, this does not fit in int32). I'd go
> off and do this, but this means I need to write an int64 version of
> pg_ltostr(). Should I do it this way?

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?

In general, years are 1900-based in Postgres:

struct pg_tm
{
 ...
int tm_mday;
int tm_mon; /* origin 0, not 1 */
int tm_year;/* relative to 1900 */
 ...
};

While it's not your patch's fault, it is not noted by EncodeDateTime()
and EncodeDateOnly() and others that there pg_tm structs are not
1900-based. This is in contrast to multiple functions within
formatting.c, nabstime.c, and timestamp.c (some of which are clients
or clients of clients for this new code). I think that the rule has
been undermined so much that it doesn't make sense to indicate
exceptions directly, though. I think pg_tm should have comments saying
that there are many cases where tm_year is not relative to 1900.

I have a few minor nitpicks about the patch.

No need for "negative number" comment here -- just use
"Assert(precision >= 0)" code:

+ * Note 'precision' must not be a negative number.
+ * Note callers should assume cp will not be NUL terminated.
  */
-static void
+static char *
 AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
 {
+#ifdef HAVE_INT64_TIMESTAMP
+

I would just use a period/full stop here:

+ * Note str is not NUL terminated, callers are responsible for NUL terminating
+ * str themselves.

Don't think you need so many "Note:" specifiers here:

+ * Note: Callers should ensure that 'padding' is above zero.
+ * Note: This function is optimized for the case where the number is not too
+ *  big to fit inside of the specified padding.
+ * Note: Caller must ensure that 'str' points to enough memory to hold the
+result (at least 12 bytes, counting a leading sign and trailing NUL,
+or padding + 1 bytes, whichever is larger).
+ */
+char *
+pg_ltostr_zeropad(char *str, int32 value, int32 padding)

Not so sure about this, within the new pg_ltostr_zeropad() function:

+   /*
+* Handle negative numbers in a special way. We can't just append a '-'
+* prefix and reverse the sign as on two's complement machines negative
+* numbers can be 1 further from 0 than positive numbers, we do it this way
+* so we properly handle the smallest possible value.
+*/
+   if (num < 0)
+   {
  ...
+   }
+   else
+   {
  ...
+   }

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.

More generally, maybe routines like EncodeDateTime() ought now to use
the Abs() macro.

Those are all of my nitpicks. :-)

> I also did some tests on server grade hardware, and the performance increase
> is looking a bit better.
>
> create table ts (ts timestamp not null);
> insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
> 00:00:00', '1 sec'::interval);
> vacuum freeze ts;

On my laptop, I saw a 2.4X - 2.6X speedup for this case. That seems
pretty nice to me.

Will make another pass over this soon.

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 5 November 2015 at 13:20, Peter Geoghegan  wrote:

> On Fri, Sep 11, 2015 at 9:00 AM, David Rowley
>  wrote:
> > Attached is also timestamp_delta.patch which changes pg_ltostr() to use
> the
> > INT_MIN special case that pg_ltoa also uses. I didn't make a similar
> change
> > to pg_ltostr_zeropad() as I'd need to add even more special code to
> prepend
> > the correct number of zero before the 2147483648. This zero padding
> reason
> > was why I originally came up with the alternative way to handle the
> negative
> > numbers. I had just thought that it was best to keep both functions as
> > similar as possible.
>
> I've started looking at this -- "timestamp_out_speedup_2015-09-12.patch".
>
> > I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> > AppendSeconds(). The only way I can think to handle this is to just make
> > fsec_t unconditionally an int64 (since with float datetimes the
> precision is
> > 10 decimal digits after the dec point, this does not fit in int32). I'd
> go
> > off and do this, but this means I need to write an int64 version of
> > pg_ltostr(). Should I do it this way?
>
> 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
10, which is based on the MAX_TIME_PRECISION setting as defined
when HAVE_INT64_TIMESTAMP is undefined.



> I have a few minor nitpicks about the patch.
>
> No need for "negative number" comment here -- just use
> "Assert(precision >= 0)" code:
>
> + * Note 'precision' must not be a negative number.
> + * Note callers should assume cp will not be NUL terminated.
>   */
> -static void
> +static char *
>  AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool
> fillzeros)
>  {
> +#ifdef HAVE_INT64_TIMESTAMP
> +
>
> 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." ?



> Don't think you need so many "Note:" specifiers here:
>
> + * Note: Callers should ensure that 'padding' is above zero.
> + * Note: This function is optimized for the case where the number is not
> too
> + *  big to fit inside of the specified padding.
> + * Note: Caller must ensure that 'str' points to enough memory to hold the
> +result (at least 12 bytes, counting a leading sign and trailing
> NUL,
> +or padding + 1 bytes, whichever is larger).
> + */
> +char *
> +pg_ltostr_zeropad(char *str, int32 value, int32 padding)
>
>
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.


> Not so sure about this, within the new pg_ltostr_zeropad() function:
>
> +   /*
> +* Handle negative numbers in a special way. We can't just append a '-'
> +* prefix and reverse the sign as on two's complement machines negative
> +* numbers can be 1 further from 0 than positive numbers, we do it
> this way
> +* so we properly handle the smallest possible value.
> +*/
> +   if (num < 0)
> +   {
>   ...
> +   }
> +   else
> +   {
>   ...
> +   }
>
> 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.


> More generally, maybe routines like 

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-09-14 12:03:31 -0500, Jim Nasby wrote:
> On 9/13/15 2:43 AM, David Rowley wrote:
> >Are you worried about this because I've not focused on optimising float
> >timestamps as much as int64 timestamps?  Are there many people compiling
> >with float timestamps in the real world?
> 
> My $0.02: the default was changed some 5 years ago so FP time is probably
> pretty rare now. I don't think it's worth a bunch of extra work to speed
> them up.

Agreed. That's not why I'm arguing for it. It's about having kind of duplicate
code that's not exercised at al. by any common setup.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Jim Nasby]

On 9/13/15 2:43 AM, David Rowley wrote:

Are you worried about this because I've not focused on optimising float
timestamps as much as int64 timestamps?  Are there many people compiling
with float timestamps in the real world?


My $0.02: the default was changed some 5 years ago so FP time is 
probably pretty rare now. I don't think it's worth a bunch of extra work 
to speed them up.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 15 September 2015 at 05:52, Alvaro Herrera 
wrote:

> Jim Nasby wrote:
> > On 9/13/15 2:43 AM, David Rowley wrote:
> > >Are you worried about this because I've not focused on optimising float
> > >timestamps as much as int64 timestamps?  Are there many people compiling
> > >with float timestamps in the real world?
> >
> > My $0.02: the default was changed some 5 years ago so FP time is probably
> > pretty rare now.
>
> The default was FP for 8.3 and was changed before 8.4.  Anybody who was
> running with the default back then and who has pg_upgraded all the way
> to current releases is still using floating-point date/times.
>
> > I don't think it's worth a bunch of extra work to speed them up.
>
> Not sure about that.
>
>
It's not like nothing is improved in float timestamps with this patch, it's
only appending the non-zero fractional seconds that I've left alone. Every
other portion of the timestamp has been improved.

I made a quick test as a demo. This is compiled with float timestamps.

create table ts (ts timestamp not null);
insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
00:00:00', '1 sec'::interval);
vacuum freeze ts;
create table ts2 (ts timestamp not null);
insert into ts2 select generate_series('2010-01-01 00:00:00', '2010-01-01
00:00:31.536001', '1 microsecond'::interval);
vacuum freeze ts2;

Master:
copy ts to '/dev/null'; -- Test 1
Time: 18252.128 ms
Time: 18230.650 ms
Time: 18247.622 ms

copy ts2 to '/dev/null'; -- Test 2
Time: 30928.999 ms
Time: 30973.576 ms
Time: 30935.489 ms

Patched

copy ts to '/dev/null'; -- Test 1 (247%)
Time: 7378.243 ms
Time: 7383.486 ms
Time: 7385.675 ms

copy ts2 to '/dev/null'; -- Test 2 (142%)
Time: 21761.047 ms
Time: 21757.026 ms
Time: 21759.621 ms

The patched difference between ts and ts2 can be accounted for by the fact
that I didn't find a better way to do:

  if (fillzeros)
  sprintf(cp, "%0*.*f", precision + 3, precision, fabs(sec + fsec));
  else
  sprintf(cp, "%.*f", precision, fabs(sec + fsec));

A fast path exists when the fractional seconds is zero, which is why
there's such a variation between the 2 tests.

I did, however spend some time a few weekends ago writing a function to
improve this, which is more aimed at improving poor performance of
float4out and float8out. It's quite likely if I can get finished one day,
it can help improve this case too.

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Alvaro Herrera]

David Rowley wrote:

> It's not like nothing is improved in float timestamps with this patch, it's
> only appending the non-zero fractional seconds that I've left alone. Every
> other portion of the timestamp has been improved.

OK, sounds good enough to me.

> I did, however spend some time a few weekends ago writing a function to
> improve this, which is more aimed at improving poor performance of
> float4out and float8out. It's quite likely if I can get finished one day,
> it can help improve this case too.

Even better, prolly not a blocker for this patch.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Alvaro Herrera]

Jim Nasby wrote:
> On 9/13/15 2:43 AM, David Rowley wrote:
> >Are you worried about this because I've not focused on optimising float
> >timestamps as much as int64 timestamps?  Are there many people compiling
> >with float timestamps in the real world?
> 
> My $0.02: the default was changed some 5 years ago so FP time is probably
> pretty rare now.

The default was FP for 8.3 and was changed before 8.4.  Anybody who was
running with the default back then and who has pg_upgraded all the way
to current releases is still using floating-point date/times.

> I don't think it's worth a bunch of extra work to speed them up.

Not sure about that.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 12 September 2015 at 04:24, Andres Freund  wrote:

> On 2015-09-12 04:00:26 +1200, David Rowley wrote:
> > I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> > AppendSeconds(). The only way I can think to handle this is to just
> > make fsec_t unconditionally an int64 (since with float datetimes the
> > precision is 10 decimal digits after the dec point, this does not fit in
> > int32). I'd go off and do this, but this means I need to write an int64
> > version of pg_ltostr(). Should I do it this way?
>
> I don't have time to look into this in detail right now. But isn't that
> the wrong view?
>
>
I understand you have a lot on. I've attempted to explain my reasoning
below.


> The precision with float timestamps is still microseconds which you can
> easily express in an integer. That the value can be higher is just
> because fsec_t for floating points also includes seconds, right? It
> shouldn't be too hard to separate those?
>
>
I don't think it's possible to claim that "The precision with float
timestamps is still microseconds". I'd say it's more limited to double
itself.

As a demo (compiled with float datetime)

postgres=# select '11:59:59.02 PM'::time;
time
-
 23:59:59.02
(1 row)

Which is 23:59:59 and 0.0002 microseconds, or 0.2 nano seconds, or 200 pico
seconds.

This formatted this way by AppendSeconds as MAX_TIME_PRECISION is defined
as 10 when compiled with HAVE_INT64_TIMESTAMP undefined.

On the other end of the spectrum:

postgres=# select '99-01-01 11:59:59.0005'::timestamp;
   timestamp
---
 99-01-01 11:59:59
(1 row)

There was no precision left for the fractional seconds here. So I'd say
claims of microsecond precision to be incorrect in many regards.

I could change AppendSeconds() to only format to 6 decimal places, but that
would be a behavioral change that I'm certainly not going to argue for.
Quite possibly this extra precision with the time type is about the only
reason to bother keeping the float time code at all, else what's it good
for?

There's just not enough bits in an int32 to do 0.99 * 100.0. Which
is why I asked about int64. It all just seems more hassle than it's worth
to get rid of a small special case in AppendSeconds()

The other problem with changing timestamp2tm() to output microseconds is
that this would require changes in all functions which call timestamp2tm(),
and likely many functions which call those. Many of these would break 3rd
party code. I found myself in contrib code when looking at changing it.
(Specifically a DecodeDateTime() call in adminpack.c)

I do agree that it's quite ugly that I've only kept TrimTrailingZeros() in
float timestamp mode, but at least it is static.

What's the best way to proceed with this:
1. Change AppendSeconds() to always format to 6 decimal places? (Breaks
applications which rely on more precision with TIME)
2. Change AppendSeconds() to multiply f_sec by 100.0 (this requires
an int64 version of pg_ltostr, but still requires a special case in
AppendSeconds())
3. Just keep things as they are with my proposed patch.
4. ?

Are you worried about this because I've not focused on optimising float
timestamps as much as int64 timestamps?  Are there many people compiling
with float timestamps in the real world?

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 3 September 2015 at 05:10, Andres Freund  wrote:

> > +/*
> > + * pg_int2str
> > + *   Converts 'value' into a decimal string representation of
> the number.
> > + *
> > + * Caller must ensure that 'str' points to enough memory to hold the
> result
> > + * (at least 12 bytes, counting a leading sign and trailing NUL).
> > + * Return value is a pointer to the new NUL terminated end of string.
> > + */
> > +char *
> > +pg_int2str(char *str, int32 value)
> > +{
> > + char *start;
> > + char *end;
> > +
> > + /*
> > +  * Handle negative numbers in a special way. We can't just append
> a '-'
> > +  * prefix and reverse the sign as on two's complement machines
> negative
> > +  * numbers can be 1 further from 0 than positive numbers, we do it
> this way
> > +  * so we properly handle the smallest possible value.
> > +  */
> > + if (value < 0)
> > + {
>
> We could alternatively handle this by special-casing INT_MIN, probably
> resulting in a bit less duplicated code.
>

Hi Andres,

I've made a few updates to the patch to cleanup a few badly written
comments, and also to fix a missing Abs() call.
I've also renamed the pg_int2str to become pg_ltostr() to bring it more
inline with pg_ltoa.

Attached is also timestamp_delta.patch which changes pg_ltostr() to use the
INT_MIN special case that pg_ltoa also uses. I didn't make a similar change
to pg_ltostr_zeropad() as I'd need to add even more special code to prepend
the correct number of zero before the 2147483648. This zero padding reason
was why I originally came up with the alternative way to handle the
negative numbers. I had just thought that it was best to keep both
functions as similar as possible.

I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
AppendSeconds(). The only way I can think to handle this is to just
make fsec_t unconditionally an int64 (since with float datetimes the
precision is 10 decimal digits after the dec point, this does not fit in
int32). I'd go off and do this, but this means I need to write an int64
version of pg_ltostr(). Should I do it this way?

I also did some tests on server grade hardware, and the performance
increase is looking a bit better.

create table ts (ts timestamp not null);
insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
00:00:00', '1 sec'::interval);
vacuum freeze ts;

Master

tpch=# copy ts to '/dev/null';
COPY 31536001
Time: 17071.255 ms

Patched
tpch=# copy ts to '/dev/null';
COPY 31536001
Time: 6402.835 ms

The times don't seem to vary any depending on the attached
timestamp_delta.patch

Regards

David Rowley


timestamp_delta.patch
Description: Binary data


timestamp_out_speedup_2015-09-12.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-09-12 04:00:26 +1200, David Rowley wrote:
> I've not done anything yet to remove the #ifdef HAVE_INT64_TIMESTAMP from
> AppendSeconds(). The only way I can think to handle this is to just
> make fsec_t unconditionally an int64 (since with float datetimes the
> precision is 10 decimal digits after the dec point, this does not fit in
> int32). I'd go off and do this, but this means I need to write an int64
> version of pg_ltostr(). Should I do it this way?

I don't have time to look into this in detail right now. But isn't that
the wrong view?

The precision with float timestamps is still microseconds which you can
easily express in an integer. That the value can be higher is just
because fsec_t for floating points also includes seconds, right? It
shouldn't be too hard to separate those?

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 3 September 2015 at 22:17, Andres Freund  wrote:

> On 2015-09-03 16:28:40 +1200, David Rowley wrote:
> > > Wouldn't it be better to just normalize fsec to an integer in that
> case?
> > > Afaics that's the only remaining reason for the alternative path?
> > >
> > > A special case would need to exist somewhere, unless we just modified
> > timestamp2tm() to multiply fsec by 1 million when HAVE_INT64_TIMESTAMP is
> > not defined, but that changes the function signature.
>
> Sure, but that's a one line #ifdef?
>

I had a look at this but I found that the precision is 10 with double
timestamps per:

#define MAX_TIME_PRECISION 10
#define TIME_PREC_INV 100.0

So if I do something like:

int fseconds = fsec * TIME_PREC_INV;

In AppendSeconds(), it overflows fseconds.

I could of course make fseconds an int64, but then I'll need to include a
64bit version of pg_int2str(). That does not seem like an improvement.

I'm also starting to not like the name pg_int2str(), It may cause confusion
with 2 bytes, instead of convert "2".

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-09-03 16:28:40 +1200, David Rowley wrote:
> > Wouldn't it be better to just normalize fsec to an integer in that case?
> > Afaics that's the only remaining reason for the alternative path?
> >
> > A special case would need to exist somewhere, unless we just modified
> timestamp2tm() to multiply fsec by 1 million when HAVE_INT64_TIMESTAMP is
> not defined, but that changes the function signature.

Sure, but that's a one line #ifdef?

> > We could alternatively handle this by special-casing INT_MIN, probably
> > resulting in a bit less duplicated code.
> >
> 
> Those special cases seem to do some weird stuff to the performance
> characteristics of those functions. I find my method to handle negative
> numbers to produce much faster code.

That's a bit odd.

> I experimented with finding the fastest way to convert an int to a string
> at the weekend, I did happen to be testing with int64's but likely int32
> will be the same.
> 
> This is the time taken to convert 30 into "30" 2 billion times.
> 
> The closest implementation I'm using in the patch is pg_int64tostr() one.
> The pg_int64tostr_memcpy() only performs better with bigger numbers /
> longer strings.
> 
> david@ubuntu:~/C$ gcc5.2 tostring.c -o tostring -O2
> david@ubuntu:~/C$ ./tostring
> pg_int64tostr_memcpy in 13.653252 seconds
> pg_int64tostr in 2.042616 seconds
> pg_int64tostr_new in 2.056688 seconds
> pg_lltoa in 13.604653 seconds
> 
> david@ubuntu:~/C$ clang tostring.c -o tostring_clang -O2
> david@ubuntu:~/C$ ./tostring_clang
> pg_int64tostr_memcpy in 0.04 seconds
> pg_int64tostr in 2.063335 seconds
> pg_int64tostr_new in 2.102068 seconds
> pg_lltoa in 3.424630 seconds

Are you sure this isn't an optimization artifact where the actual work
is optimized away? Doing 2 billion conversions in 2s kinda implies that
the string conversion is done in ~4 cycles. That seems unrealistically
fast, even for a pipelined cpu.


Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 3 September 2015 at 22:17, Andres Freund  wrote:

> On 2015-09-03 16:28:40 +1200, David Rowley wrote:
> > I experimented with finding the fastest way to convert an int to a string
> > at the weekend, I did happen to be testing with int64's but likely int32
> > will be the same.
> >
> > This is the time taken to convert 30 into "30" 2 billion times.
> >
> > The closest implementation I'm using in the patch is pg_int64tostr() one.
> > The pg_int64tostr_memcpy() only performs better with bigger numbers /
> > longer strings.
> >
> > david@ubuntu:~/C$ gcc5.2 tostring.c -o tostring -O2
> > david@ubuntu:~/C$ ./tostring
> > pg_int64tostr_memcpy in 13.653252 seconds
> > pg_int64tostr in 2.042616 seconds
> > pg_int64tostr_new in 2.056688 seconds
> > pg_lltoa in 13.604653 seconds
> >
> > david@ubuntu:~/C$ clang tostring.c -o tostring_clang -O2
> > david@ubuntu:~/C$ ./tostring_clang
> > pg_int64tostr_memcpy in 0.04 seconds
> > pg_int64tostr in 2.063335 seconds
> > pg_int64tostr_new in 2.102068 seconds
> > pg_lltoa in 3.424630 seconds
>
> Are you sure this isn't an optimization artifact where the actual work
> is optimized away? Doing 2 billion conversions in 2s kinda implies that
> the string conversion is done in ~4 cycles. That seems unrealistically
> fast, even for a pipelined cpu.
>

I think you're right.
If I change the NUM_TO_STRING in my tostring.c to
 #define NUM_TO_PRINT i%100

I just wanted a way to keep the strings 2 digits long, so that it matches
what will happen when building timestamps.

It looks a bit different, but it's still a bit faster than pg_lltoa()

david@ubuntu:~/C$ ./tostring
pg_int64tostr_memcpy in 16.557578 seconds
pg_int64tostr in 8.070706 seconds
pg_int64tostr_new in 7.597320 seconds
pg_lltoa in 12.315339 seconds

david@ubuntu:~/C$ ./tostring_clang
pg_int64tostr_memcpy in 19.698807 seconds
pg_int64tostr in 12.800270 seconds
pg_int64tostr_new in 14.174052 seconds
pg_lltoa in 14.427803 seconds

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-08-09 12:47:53 +1200, David Rowley wrote:
> I took a bit of weekend time to finish this one off. Patch attached.
> 
> A quick test shows a pretty good performance increase:
> 
> create table ts (ts timestamp not null);
> insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
> 00:00:00', '1 sec'::interval);
> vacuum freeze ts;
> 
> Master:
> david=# copy ts to 'l:/ts.sql';
> COPY 31536001
> Time: 20444.468 ms
> 
> Patched:
> david=# copy ts to 'l:/ts.sql';
> COPY 31536001
> Time: 10947.097 ms

Yes, that's pretty cool.

> There's probably a bit more to squeeze out of this.
> 1. EncodeDateTime() could return the length of the string to allow callers
> to use pnstrdup() instead of pstrdup(), which would save the strlen() call.
> 2. Have pg_int2str_zeropad() and pg_int2str() skip appending the NUL char
> and leave this up to the calling function.
> 3. Make something to replace the sprintf(str, " %.*s", MAXTZLEN, tzn); call.
> 
> Perhaps 1 and 3 are worth looking into, but I think 2 is likely too error
> prone for the small gain we'd get from it.

I'm inclined to first get the majority of the optimization - as in
somethign similar to the current patch. If we then feel a need to
optimize further we can do that. Otherwise we might end up not getting
the 95% performance improvement in 9.6 because we're playing with the
remaining 5 ;)


> Also I was not too sure on if pg_int2str() was too similar to pg_ltoa().
> Perhaps pg_ltoa() should just be modified to return the end of string?

I don't think the benefits are worth breaking pg_ltoa interface.

>  /*
> - * Append sections and fractional seconds (if any) at *cp.
> + * Append seconds and fractional seconds (if any) at *cp.
>   * precision is the max number of fraction digits, fillzeros says to
>   * pad to two integral-seconds digits.
>   * Note that any sign is stripped from the input seconds values.
> + * Note 'precision' must not be a negative number.
>   */
> -static void
> +static char *
>  AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool fillzeros)
>  {
> +#ifdef HAVE_INT64_TIMESTAMP

Wouldn't it be better to just normalize fsec to an integer in that case?
Afaics that's the only remaining reason for the alternative path?

> +/*
> + * pg_int2str
> + *   Converts 'value' into a decimal string representation of the 
> number.
> + *
> + * Caller must ensure that 'str' points to enough memory to hold the result
> + * (at least 12 bytes, counting a leading sign and trailing NUL).
> + * Return value is a pointer to the new NUL terminated end of string.
> + */
> +char *
> +pg_int2str(char *str, int32 value)
> +{
> + char *start;
> + char *end;
> +
> + /*
> +  * Handle negative numbers in a special way. We can't just append a '-'
> +  * prefix and reverse the sign as on two's complement machines negative
> +  * numbers can be 1 further from 0 than positive numbers, we do it this 
> way
> +  * so we properly handle the smallest possible value.
> +  */
> + if (value < 0)
> + {

We could alternatively handle this by special-casing INT_MIN, probably
resulting in a bit less duplicated code.
>  
>  /*
>   *   Per-opclass comparison functions for new btrees.  These are
> diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
> index e107d41..1e2dd62 100644
> --- a/src/tools/msvc/build.pl
> +++ b/src/tools/msvc/build.pl
> @@ -61,7 +61,7 @@ elsif ($buildwhat)
>  }
>  else
>  {
> - system("msbuild pgsql.sln /verbosity:detailed /p:Configuration=$bconf");
> + system("msbuild pgsql.sln /verbosity:quiet /p:Configuration=$bconf");
>  }

Uh? I assume that's an acciental change?

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 3 September 2015 at 05:10, Andres Freund  wrote:

> On 2015-08-09 12:47:53 +1200, David Rowley wrote:
> > I took a bit of weekend time to finish this one off. Patch attached.
> >
> > A quick test shows a pretty good performance increase:
> >
> > create table ts (ts timestamp not null);
> > insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
> > 00:00:00', '1 sec'::interval);
> > vacuum freeze ts;
> >
> > Master:
> > david=# copy ts to 'l:/ts.sql';
> > COPY 31536001
> > Time: 20444.468 ms
> >
> > Patched:
> > david=# copy ts to 'l:/ts.sql';
> > COPY 31536001
> > Time: 10947.097 ms
>
> Yes, that's pretty cool.
>
> > There's probably a bit more to squeeze out of this.
> > 1. EncodeDateTime() could return the length of the string to allow
> callers
> > to use pnstrdup() instead of pstrdup(), which would save the strlen()
> call.
> > 2. Have pg_int2str_zeropad() and pg_int2str() skip appending the NUL char
> > and leave this up to the calling function.
> > 3. Make something to replace the sprintf(str, " %.*s", MAXTZLEN, tzn);
> call.
> >
> > Perhaps 1 and 3 are worth looking into, but I think 2 is likely too error
> > prone for the small gain we'd get from it.
>

I see the logic there, but a few weekends ago I modified the patch to
implement number 2. This is perhaps one optimization that would be hard to
change later since, if we suddenly change pg_int2str() to not append the
NUL, then it might break any 3rd party code that's started using them. The
more I think about it the more I think allowing those to skip appending the
NUL is ok. They're very useful functions for incrementally building strings.

Attached is a patch to implement this, which performs as follows
postgres=# copy ts to 'l:/ts.sql';
COPY 31536001
Time: 10665.297 ms

However, this version of the patch does change many existing functions in
datetime.c so that they no longer append the NUL char... The good news is
that these are all static functions, so nothing else should be using them.

I'm happy to leave 1 and 3 for another rainy weekend one day.


> > Also I was not too sure on if pg_int2str() was too similar to pg_ltoa().
> > Perhaps pg_ltoa() should just be modified to return the end of string?
>
> I don't think the benefits are worth breaking pg_ltoa interface.
>
>
Ok let's leave pg_ltoa() alone


> >  /*
> > - * Append sections and fractional seconds (if any) at *cp.
> > + * Append seconds and fractional seconds (if any) at *cp.
> >   * precision is the max number of fraction digits, fillzeros says to
> >   * pad to two integral-seconds digits.
> >   * Note that any sign is stripped from the input seconds values.
> > + * Note 'precision' must not be a negative number.
> >   */
> > -static void
> > +static char *
> >  AppendSeconds(char *cp, int sec, fsec_t fsec, int precision, bool
> fillzeros)
> >  {
> > +#ifdef HAVE_INT64_TIMESTAMP
>
> Wouldn't it be better to just normalize fsec to an integer in that case?
> Afaics that's the only remaining reason for the alternative path?
>
> A special case would need to exist somewhere, unless we just modified
timestamp2tm() to multiply fsec by 1 million when HAVE_INT64_TIMESTAMP is
not defined, but that changes the function signature.


> > +/*
> > + * pg_int2str
> > + *   Converts 'value' into a decimal string representation of
> the number.
> > + *
> > + * Caller must ensure that 'str' points to enough memory to hold the
> result
> > + * (at least 12 bytes, counting a leading sign and trailing NUL).
> > + * Return value is a pointer to the new NUL terminated end of string.
> > + */
> > +char *
> > +pg_int2str(char *str, int32 value)
> > +{
> > + char *start;
> > + char *end;
> > +
> > + /*
> > +  * Handle negative numbers in a special way. We can't just append
> a '-'
> > +  * prefix and reverse the sign as on two's complement machines
> negative
> > +  * numbers can be 1 further from 0 than positive numbers, we do it
> this way
> > +  * so we properly handle the smallest possible value.
> > +  */
> > + if (value < 0)
> > + {
>
> We could alternatively handle this by special-casing INT_MIN, probably
> resulting in a bit less duplicated code.
>

Those special cases seem to do some weird stuff to the performance
characteristics of those functions. I find my method to handle negative
numbers to produce much faster code.

I experimented with finding the fastest way to convert an int to a string
at the weekend, I did happen to be testing with int64's but likely int32
will be the same.

This is the time taken to convert 30 into "30" 2 billion times.

The closest implementation I'm using in the patch is pg_int64tostr() one.
The pg_int64tostr_memcpy() only performs better with bigger numbers /
longer strings.

david@ubuntu:~/C$ gcc5.2 tostring.c -o tostring -O2
david@ubuntu:~/C$ ./tostring
pg_int64tostr_memcpy in 13.653252 seconds
pg_int64tostr in 2.042616 seconds
pg_int64tostr_new in 2.056688 seconds
pg_lltoa in 

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 29 July 2015 at 03:25, Andres Freund and...@anarazel.de wrote:

 On 2015-07-29 03:10:41 +1200, David Rowley wrote:
  timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
  timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
  timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000
 
  timestamp_out_old is master's version, the timestamp_out_af() is yours,
 and
  timestamp_out() is my one. times are in seconds to perform 100 million
  calls.

 That looks good.

  So it appears your version is a bit faster than mine, but we're both
 about
  20 times faster than the current one.
  Also mine needs fixed up as the fractional part is not padded the same as
  yours, but I doubt that'll affect the performance by much.

 Worthwhile to finish that bit and try ;)

  My view: It's probably not worth going quite as far as you've gone for a
  handful of nanoseconds per call, but perhaps something along the lines of
  mine can be fixed up.

 Yes, I agreee that your's is probably going to be fast enough.


I took a bit of weekend time to finish this one off. Patch attached.

A quick test shows a pretty good performance increase:

create table ts (ts timestamp not null);
insert into ts select generate_series('2010-01-01 00:00:00', '2011-01-01
00:00:00', '1 sec'::interval);
vacuum freeze ts;

Master:
david=# copy ts to 'l:/ts.sql';
COPY 31536001
Time: 20444.468 ms

Patched:
david=# copy ts to 'l:/ts.sql';
COPY 31536001
Time: 10947.097 ms

There's probably a bit more to squeeze out of this.
1. EncodeDateTime() could return the length of the string to allow callers
to use pnstrdup() instead of pstrdup(), which would save the strlen() call.
2. Have pg_int2str_zeropad() and pg_int2str() skip appending the NUL char
and leave this up to the calling function.
3. Make something to replace the sprintf(str,  %.*s, MAXTZLEN, tzn); call.

Perhaps 1 and 3 are worth looking into, but I think 2 is likely too error
prone for the small gain we'd get from it.

Also I was not too sure on if pg_int2str() was too similar to pg_ltoa().
Perhaps pg_ltoa() should just be modified to return the end of string?

Thoughts?

Regards

David Rowley

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


timestamp_out_speedup_2015-08-09.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 5 August 2015 at 12:51, David Rowley david.row...@2ndquadrant.com
wrote:

 On 29 July 2015 at 03:25, Andres Freund and...@anarazel.de wrote:

 On 2015-07-29 03:10:41 +1200, David Rowley wrote:
  timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
  timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
  timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000
 
  timestamp_out_old is master's version, the timestamp_out_af() is yours,
 and
  timestamp_out() is my one. times are in seconds to perform 100 million
  calls.

 That looks good.

  So it appears your version is a bit faster than mine, but we're both
 about
  20 times faster than the current one.
  Also mine needs fixed up as the fractional part is not padded the same
 as
  yours, but I doubt that'll affect the performance by much.

 Worthwhile to finish that bit and try ;)

  My view: It's probably not worth going quite as far as you've gone for a
  handful of nanoseconds per call, but perhaps something along the lines
 of
  mine can be fixed up.

 Yes, I agreee that your's is probably going to be fast enough.

  Have you thought about what to do when HAVE_INT64_TIMESTAMP is not
 defined?

 I don't think it's actually important. The only difference vs float
 timestamps is that in the latter case we set fsecs to zero BC.

 Unless we want to slow down the common case it seems not unlikely that
 we're going to end up with a separate slow path anyway. E.g. neither
 your version nor mine handles 5 digit years (which is why I fell back to
 the slow path in that case in my patch).


 It occurred to me that handling the 5 digit year is quite a simple change
 to my code:

 We simply just need to check if there was any 'num' left after consuming
 the given space. If there's any left then just use pg_uint2str().
 This keeps things very fast for the likely most common case where the year
 is 4 digits long.

 I've not thought about negative years. The whole function should perhaps
 take signed ints instead of unsigned.


I've made a few changes to this to get the fractional seconds part working
as it should.

It also now works fine with 5 digit years.

It's still in the form of the test program, but it should be simple enough
to pull out what's required from that and put into Postgres.

I've also changed my version of AppendSeconds() so that it returns a
pointer to the new end of string. This should be useful as I see some
strlen() calls to get the new end of string. It'll easy to remove those now
which will further increase performance.

timestamp_out() is the proposed new version
timestamp_out_old() is master's version
timestamp_out_af() is your version

Performance is as follows:

With Clang
david@ubuntu:~/C$ clang timestamp_out.c -o timestamp_out -O2
david@ubuntu:~/C$ ./timestamp_out
timestamp_out() = 2015-07-29 02:24:33.034 in 0.313686
timestamp_out_old() = 2015-07-29 02:24:33.034 in 5.048472
timestamp_out_af() = 2015-07-29 02:24:33.034 in 0.198147

With gcc
david@ubuntu:~/C$ gcc timestamp_out.c -o timestamp_out -O2
david@ubuntu:~/C$ ./timestamp_out
timestamp_out() = 2015-07-29 02:24:33.034 in 0.405795
timestamp_out_old() = 2015-07-29 02:24:33.034 in 4.678918
timestamp_out_af() = 2015-07-29 02:24:33.034 in 0.270557

Regards

David Rowley
--
 David Rowley   http://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
#include stdio.h
#include time.h
#include string.h
#include stdlib.h

struct pg_tm
{
	int			tm_sec;
	int			tm_min;
	int			tm_hour;
	int			tm_mday;
	int			tm_mon;			/* origin 0, not 1 */
	int			tm_year;		/* relative to 1900 */
	int			tm_wday;
	int			tm_yday;
	int			tm_isdst;
	long int	tm_gmtoff;
	const char *tm_zone;
};

static char *pg_uint2str(char *str, unsigned int value);
static char *pg_uint2str_padding(char *str, unsigned int value, unsigned int padding);

static char *
AppendSeconds2(char *cp, int sec, unsigned int fsec, int precision, char fillzeros)
{
	
	if (fillzeros)
		cp = pg_uint2str_padding(cp, abs(sec), 2);
	else
		cp = pg_uint2str(cp, abs(sec));
	
	if (fsec != 0)
	{
		unsigned int value = fsec;
		char *end = cp[precision + 1];
		int gotnonzero = 0;

		*cp++ = '.';
	
		/*
		 * Append the fractional seconds part. Note that we don't want any
		 * trailing zeros here, so since we're building the number in reverse
		 * we'll skip appending any zeros, unless we've seen a non-zero.
		 */
		while (precision--)
		{
			int		remainder;
			int		oldval = value;

			value /= 10;
			remainder = oldval - value * 10;
			
			/* check if we got a non-zero */
			if (remainder)
gotnonzero = 1;
		
			if (gotnonzero)
cp[precision] = '0' + remainder;
			else
end = cp[precision];
		}
		
		/*
		 * if we have a non-zero value then precision must have not been enough
		 * to print the number, we'd better have another go. There won't be any
		 * zero padding, so we can just use pg_uint2str()
		 */
		if (value  0)
			return pg_uint2str(cp, fsec);

		*end = '\0';
		
		

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 29 July 2015 at 03:25, Andres Freund and...@anarazel.de wrote:

 On 2015-07-29 03:10:41 +1200, David Rowley wrote:
  timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
  timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
  timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000
 
  timestamp_out_old is master's version, the timestamp_out_af() is yours,
 and
  timestamp_out() is my one. times are in seconds to perform 100 million
  calls.

 That looks good.

  So it appears your version is a bit faster than mine, but we're both
 about
  20 times faster than the current one.
  Also mine needs fixed up as the fractional part is not padded the same as
  yours, but I doubt that'll affect the performance by much.

 Worthwhile to finish that bit and try ;)

  My view: It's probably not worth going quite as far as you've gone for a
  handful of nanoseconds per call, but perhaps something along the lines of
  mine can be fixed up.

 Yes, I agreee that your's is probably going to be fast enough.

  Have you thought about what to do when HAVE_INT64_TIMESTAMP is not
 defined?

 I don't think it's actually important. The only difference vs float
 timestamps is that in the latter case we set fsecs to zero BC.

 Unless we want to slow down the common case it seems not unlikely that
 we're going to end up with a separate slow path anyway. E.g. neither
 your version nor mine handles 5 digit years (which is why I fell back to
 the slow path in that case in my patch).


It occurred to me that handling the 5 digit year is quite a simple change
to my code:

static char *
pg_uint2str_padding(char *str, unsigned int value, unsigned int padding)
{
char   *start = str;
char   *end = str[padding];
unsigned int num = value;
//Assert(padding  0);
*end = '\0';
while (padding--)
{
str[padding] = num % 10 + '0';
num /= 10;
}
/*
* if value was too big for the specified padding then rebuild the whole
* number again without padding. Conveniently pg_uint2str() does exactly
* this.
*/
if (num  0)
return pg_uint2str(str, value);

return end;
}

We simply just need to check if there was any 'num' left after consuming
the given space. If there's any left then just use pg_uint2str().
This keeps things very fast for the likely most common case where the year
is 4 digits long.

I've not thought about negative years. The whole function should perhaps
take signed ints instead of unsigned.

Regards

David Rowley

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

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-07-29 09:37:26 +1200, David Rowley wrote:
 On 29 July 2015 at 03:25, Andres Freund and...@anarazel.de wrote:
 
  On 2015-07-29 03:10:41 +1200, David Rowley wrote:
   Have you thought about what to do when HAVE_INT64_TIMESTAMP is not
  defined?
 
  I don't think it's actually important. The only difference vs float
  timestamps is that in the latter case we set fsecs to zero BC.
 
 
  I was also thinking that the % 10 won't work when fsec_t is double.
 
 typedef double fsec_t

It seems quite possible to move that bit to timestamp2tm and do it
without a dependency on HAVE_INT64_TIMESTAMP. As long as it doesn't slow
down the int timestamp case I'm happy to simplify code in this area.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-07-28 10:59:15 +1200, David Rowley wrote:
 It won't be quite as fast as what you've written, but I think it will be
 much neater and more likely to be used in other places if we invent a
 function like pg_ltoa() which returns a pointer to the new end of string.
 
 Also if we're specifying padding with zeros then we can skip the reverse
 part that's in pg_ltoa(), (normally needed since the numeric string is
 build in reverse)
 
 The code could then be written as:
 
 str = pg_int2str_pad(str, year, 4);
 *str++ = '-';
 str = pg_int2str_pad(str, tm-tm_mon, 2);
 *str++ = '-';
 str = pg_int2str_pad(str, tm-tm_mday, 2);
 
 etc
 
 I've used this method before and found it to be about 10 times faster than
 snprintf(), but I was reversing the string, so quite likely it be more than
 10 times.

Yes, that might be worthwhile to try. Certainly would look less
ugly. Willing to give it a try?

 I'm interested to see how much you're really gaining by manually unrolling
 the part that builds the fractional part of the second.

It seems to help a fair amount, but this really was more a quick POC
than something serious. My theory why it helps is that each loop
iteration is independent of the previous one and thus can take full
advantage of the pipeline.

Regards,

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 28 July 2015 at 19:10, Andres Freund and...@anarazel.de wrote:

 On 2015-07-28 10:59:15 +1200, David Rowley wrote:
  It won't be quite as fast as what you've written, but I think it will be
  much neater and more likely to be used in other places if we invent a
  function like pg_ltoa() which returns a pointer to the new end of string.
 
  Also if we're specifying padding with zeros then we can skip the reverse
  part that's in pg_ltoa(), (normally needed since the numeric string is
  build in reverse)
 
  The code could then be written as:
 
  str = pg_int2str_pad(str, year, 4);
  *str++ = '-';
  str = pg_int2str_pad(str, tm-tm_mon, 2);
  *str++ = '-';
  str = pg_int2str_pad(str, tm-tm_mday, 2);
 
  etc
 
  I've used this method before and found it to be about 10 times faster
 than
  snprintf(), but I was reversing the string, so quite likely it be more
 than
  10 times.

 Yes, that might be worthwhile to try. Certainly would look less
 ugly. Willing to give it a try?


I had a quick try at this and ended up just writing a small test program to
see what's faster.

Please excuse the mess of the file, I just hacked it together as quickly as
I could with the sole intention of just to get an idea of which is faster
and by how much.

For me the output is as follows:

timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000

timestamp_out_old is master's version, the timestamp_out_af() is yours, and
timestamp_out() is my one. times are in seconds to perform 100 million
calls.

So it appears your version is a bit faster than mine, but we're both about
20 times faster than the current one.
Also mine needs fixed up as the fractional part is not padded the same as
yours, but I doubt that'll affect the performance by much.

My view: It's probably not worth going quite as far as you've gone for a
handful of nanoseconds per call, but perhaps something along the lines of
mine can be fixed up.

Have you thought about what to do when HAVE_INT64_TIMESTAMP is not defined?

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
#include stdio.h
#include time.h

struct pg_tm
{
	int			tm_sec;
	int			tm_min;
	int			tm_hour;
	int			tm_mday;
	int			tm_mon;			/* origin 0, not 1 */
	int			tm_year;		/* relative to 1900 */
	int			tm_wday;
	int			tm_yday;
	int			tm_isdst;
	long int	tm_gmtoff;
	const char *tm_zone;
};


char *
pg_uint2str_padding(char *str, unsigned int value, unsigned int padding)
{
	char	   *start = str;
	char	   *end = str[padding];
	//Assert(padding  0);
	
	*end = '\0';
	
	while (padding--)
	{
		str[padding] = value % 10 + '0';
		value /= 10;
	}
	
	return end;
}

char *
pg_uint2str(char *str, unsigned int value)
{
	char *start = str;
	char *end;
	/* Compute the result string backwards. */
	do
	{
		int		remainder;
		int		oldval = value;

		value /= 10;
		remainder = oldval - value * 10;
		*str++ = '0' + remainder;
	} while (value != 0);


	/* Add trailing NUL byte, and back up 'str' to the last character. */
	*str-- = '\0';
	end = str;
	
	/* Reverse string. */
	while (start  str)
	{
		char		swap = *start;

		*start++ = *str;
		*str-- = swap;
	}
	return end;
}


char *
timestamp_out_af(char *buffer, struct pg_tm *tm, unsigned int fsec)
{
	char *str = buffer;

	*str++ = (tm-tm_year / 1000) + '0';
	*str++ = (tm-tm_year / 100) % 10 + '0';
	*str++ = (tm-tm_year / 10) % 10 + '0';
	*str++ = tm-tm_year % 10 + '0';
	*str++ = '-';
	*str++ = (tm-tm_mon / 10) + '0';
	*str++ = tm-tm_mon % 10 + '0';
	*str++ = '-';
	*str++ = (tm-tm_mday / 10) + '0';
	*str++ = tm-tm_mday % 10 + '0';
	*str++ = ' ';
	*str++ = (tm-tm_hour / 10) + '0';
	*str++ = tm-tm_hour % 10 + '0';
	*str++ = ':';
	*str++ = (tm-tm_min / 10) + '0';
	*str++ = tm-tm_min % 10 + '0';
	*str++ = ':';
	*str++ = (tm-tm_sec / 10) + '0';
	*str++ = tm-tm_sec % 10 + '0';

	/*
	 * Yes, this is darned ugly and would look nicer in a loop,
	 * but some versions of gcc can't convert the divisions into
	 * more efficient instructions unless manually unrolled.
	 */
	if (fsec != 0)
	{
		int fseca = abs(fsec);

		*str++ = '.';

		if (fseca % 100 != 0)
		{
			*str++ = (fseca / 10) + '0';

			if (fseca % 10 != 0)
			{
*str++ = ((fseca / 1) % 10) + '0';

if (fseca % 1 != 0)
{
	*str++ = ((fseca / 1000) % 10) + '0';

	if (fseca % 1000 != 0)
	{
		*str++ = ((fseca / 100) % 10) + '0';

		if (fseca % 100 != 0)
		{
			*str++ = ((fseca / 10) % 10) + '0';

			if (fseca % 10 != 0)
			{
*str++ = (fseca % 10) + '0';
			}
		}
	}
}
			}
		}
	}
	
	return buffer;
}

char *
timestamp_out(char *buffer, struct pg_tm *tm, unsigned int fsec)
{
	char *str = buffer;
	
	str = pg_uint2str_padding(str, tm-tm_year, 4);
	*str++ = '-';
	str = pg_uint2str_padding(str, tm-tm_mon, 2);
	*str++ = '-';
	

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-07-29 03:10:41 +1200, David Rowley wrote:
 timestamp_out() = 2015-07-29 02:24:33.34 in 3.506000
 timestamp_out_old() = 2015-07-29 02:24:33.034 in 64.518000
 timestamp_out_af() = 2015-07-29 02:24:33.034 in 2.981000

 timestamp_out_old is master's version, the timestamp_out_af() is yours, and
 timestamp_out() is my one. times are in seconds to perform 100 million
 calls.

That looks good.

 So it appears your version is a bit faster than mine, but we're both about
 20 times faster than the current one.
 Also mine needs fixed up as the fractional part is not padded the same as
 yours, but I doubt that'll affect the performance by much.

Worthwhile to finish that bit and try ;)

 My view: It's probably not worth going quite as far as you've gone for a
 handful of nanoseconds per call, but perhaps something along the lines of
 mine can be fixed up.

Yes, I agreee that your's is probably going to be fast enough.

 Have you thought about what to do when HAVE_INT64_TIMESTAMP is not defined?

I don't think it's actually important. The only difference vs float
timestamps is that in the latter case we set fsecs to zero BC.

Unless we want to slow down the common case it seems not unlikely that
we're going to end up with a separate slow path anyway. E.g. neither
your version nor mine handles 5 digit years (which is why I fell back to
the slow path in that case in my patch).

Regards,

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 29 July 2015 at 03:25, Andres Freund and...@anarazel.de wrote:

 On 2015-07-29 03:10:41 +1200, David Rowley wrote:
  Have you thought about what to do when HAVE_INT64_TIMESTAMP is not
 defined?

 I don't think it's actually important. The only difference vs float
 timestamps is that in the latter case we set fsecs to zero BC.


 I was also thinking that the % 10 won't work when fsec_t is double.

typedef double fsec_t

Regards

David Rowley

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

[HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

Hi,

I recently once more noticed that timestamptz_out is really, really
slow. To quantify that, I created a bunch of similar sized tables:

CREATE TABLE tbl_timestamp AS SELECT NOW() FROM generate_series(1, 10) a, 
generate_series(1, 100) b;
CREATE TABLE tbl_int8 AS SELECT 1::bigint FROM generate_series(1,  10) a, 
generate_series(1, 100) b;
CREATE TABLE tbl_bytea AS SELECT '   '::bytea FROM generate_series(1, 10) 
a, generate_series(1, 100) b;

These all end up being 346MB large.

COPY tbl_bytea TO '/dev/null';
Time: 1173.484 ms
COPY tbl_int8 TO '/dev/null';
Time: 1030.756 ms
COPY tbl_timestamp TO '/dev/null';
Time: 6598.030

(all best of three)

Yes, timestamp outputs more data as a whole, but being 5 times as slow
is still pretty darn insane. To make sure that's not the cause, here's
another table:

CREATE TABLE tbl_timestamptext AS SELECT NOW()::text FROM generate_series(1, 
10) a, generate_series(1, 100) b;
COPY tbl_timestamptext TO '/dev/null';
Time: 1449.554 ms

So it's really just the timestamp code.


Profiling it shows:
  Overhead  Command Shared Object Symbol
  -   38.33%  postgres_stock  libc-2.19.so  [.] vfprintf
 - 97.92% vfprintf
  _IO_vsprintf
- sprintf
   + 70.25% EncodeDateTime
   + 29.75% AppendSeconds.constprop.10
 + 1.11% _IO_default_xsputn
  -8.22%  postgres_stock  libc-2.19.so  [.] _IO_default_xsputn
 - 99.43% _IO_default_xsputn
- 90.09% vfprintf
 _IO_vsprintf
   - sprintf
  + 74.15% EncodeDateTime
  + 25.85% AppendSeconds.constprop.10
+ 9.72% _IO_padn
 + 0.57% vfprintf
  +   7.76%  postgres_stock  postgres_stock[.] CopyOneRowTo   

So nearly all the time is spent somewhere inside the sprintf calls. Not
nice.

The only thing I could come up to make the sprintfs cheaper was to
combine them into one and remove some of the width specifiers that
aren't needed. That doesn't buy us very much.

I then proceeded to replace the sprintf call with hand-rolled
conversions. And wow, the benefit is far bigger than I'd assumed:
postgres[7236][1]=# COPY tbl_timestamp TO '/dev/null';
Time: 2430.521 ms

So, by hand-rolling the ISO conversion in EncodeDateTime() we got a
~250% performance improvement. I'd say that's worthwhile.

The attached patch shows what I did. While there's some polishing
possible, as a whole, it's pretty ugly. But I think timestamp data is so
common that it's worth the effort.

Does anybody have a fundamentally nicer idea than the attached to
improvide this?

Greetings,

Andres Freund
From 1d7b6110f8864ee00c1fe4f54d8937347ade7d80 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Mon, 27 Jul 2015 23:09:33 +0200
Subject: [PATCH] WIP: faster timestamp[tz]_out

---
 src/backend/utils/adt/datetime.c| 108 
 src/test/regress/expected/horology.out  |  24 ---
 src/test/regress/expected/timestamp.out |  62 +++---
 src/test/regress/sql/timestamp.sql  |   1 +
 4 files changed, 164 insertions(+), 31 deletions(-)

diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 2a44b6e..4c13f01 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3968,7 +3968,115 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char
 
 	switch (style)
 	{
+#ifdef HAVE_INT64_TIMESTAMP
+		case USE_ISO_DATES:
+			/*
+			 * Fastpath for most common format and range. Not using sprintf
+			 * provides significant performance benefits, and timestamp data
+			 * is pretty common. All sane use cases dealing with large amounts
+			 * of data use iso timestamps, so we can focus on optimizing
+			 * those, and keep the rest of the code maintainable.
+			 */
+			if (tm-tm_year  0  tm-tm_year  1)
+			{
+int year  = (tm-tm_year  0) ? tm-tm_year : -(tm-tm_year - 1);
+
+*str++ = (year / 1000) + '0';
+*str++ = (year / 100) % 10 + '0';
+*str++ = (year / 10) % 10 + '0';
+*str++ = year % 10 + '0';
+*str++ = '-';
+*str++ = (tm-tm_mon / 10) + '0';
+*str++ = tm-tm_mon % 10 + '0';
+*str++ = '-';
+*str++ = (tm-tm_mday / 10) + '0';
+*str++ = tm-tm_mday % 10 + '0';
+*str++ = ' ';
+*str++ = (tm-tm_hour / 10) + '0';
+*str++ = tm-tm_hour % 10 + '0';
+*str++ = ':';
+*str++ = (tm-tm_min / 10) + '0';
+*str++ = tm-tm_min % 10 + '0';
+*str++ = ':';
+*str++ = (tm-tm_sec / 10) + '0';
+*str++ = tm-tm_sec % 10 + '0';
+
+/*
+ * Yes, this is darned ugly and would look nicer in a loop,
+ * but some versions of gcc can't convert the divisions into
+ * more efficient instructions unless manually unrolled.
+ */
+if (fsec != 0)
+{
+	int fseca = abs(fsec);
+
+	*str++ = '.';
+
+	if (fseca % 100 != 0)
+	{
+		*str++ = (fseca / 10) + '0';
+
+		if (fseca % 10 != 0)
+		{
+			*str++ = ((fseca / 

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Andres Freund]

On 2015-07-27 17:31:41 -0400, Tom Lane wrote:
 Andres Freund and...@anarazel.de writes:
  So nearly all the time is spent somewhere inside the sprintf calls. Not
  nice.
 
 What happens if you force use of port/snprintf.c instead of glibc's
 version?

Good question.

Even worse. 15900.014 ms.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Tom Lane]

Andres Freund and...@anarazel.de writes:
 So nearly all the time is spent somewhere inside the sprintf calls. Not
 nice.

What happens if you force use of port/snprintf.c instead of glibc's
version?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[Tom Lane]

Andres Freund and...@anarazel.de writes:
 On 2015-07-27 17:31:41 -0400, Tom Lane wrote:
 What happens if you force use of port/snprintf.c instead of glibc's
 version?

 Even worse. 15900.014 ms.

Interesting.  So as a separate optimization problem, we might consider
try to put snprintf.c at least on a par with glibc.  I'm kind of
surprised by this result really, since snprintf.c lacks a lot of the
bells and whistles that are in glibc.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Make timestamptz_out less slow.

[David Rowley]

On 28 July 2015 at 09:17, Andres Freund and...@anarazel.de wrote:

 Hi,

 I recently once more noticed that timestamptz_out is really, really
 slow. To quantify that, I created a bunch of similar sized tables:

 CREATE TABLE tbl_timestamp AS SELECT NOW() FROM generate_series(1, 10)
 a, generate_series(1, 100) b;
 CREATE TABLE tbl_int8 AS SELECT 1::bigint FROM generate_series(1,  10)
 a, generate_series(1, 100) b;
 CREATE TABLE tbl_bytea AS SELECT '   '::bytea FROM generate_series(1,
 10) a, generate_series(1, 100) b;

 These all end up being 346MB large.

 COPY tbl_bytea TO '/dev/null';
 Time: 1173.484 ms
 COPY tbl_int8 TO '/dev/null';
 Time: 1030.756 ms
 COPY tbl_timestamp TO '/dev/null';
 Time: 6598.030

 (all best of three)

 Yes, timestamp outputs more data as a whole, but being 5 times as slow
 is still pretty darn insane. To make sure that's not the cause, here's
 another table:

 CREATE TABLE tbl_timestamptext AS SELECT NOW()::text FROM
 generate_series(1, 10) a, generate_series(1, 100) b;
 COPY tbl_timestamptext TO '/dev/null';
 Time: 1449.554 ms

 So it's really just the timestamp code.


 Profiling it shows:
   Overhead  Command Shared Object Symbol
   -   38.33%  postgres_stock  libc-2.19.so  [.] vfprintf
  - 97.92% vfprintf
   _IO_vsprintf
 - sprintf
+ 70.25% EncodeDateTime
+ 29.75% AppendSeconds.constprop.10
  + 1.11% _IO_default_xsputn
   -8.22%  postgres_stock  libc-2.19.so  [.] _IO_default_xsputn
  - 99.43% _IO_default_xsputn
 - 90.09% vfprintf
  _IO_vsprintf
- sprintf
   + 74.15% EncodeDateTime
   + 25.85% AppendSeconds.constprop.10
 + 9.72% _IO_padn
  + 0.57% vfprintf
   +   7.76%  postgres_stock  postgres_stock[.] CopyOneRowTo

 So nearly all the time is spent somewhere inside the sprintf calls. Not
 nice.

 The only thing I could come up to make the sprintfs cheaper was to
 combine them into one and remove some of the width specifiers that
 aren't needed. That doesn't buy us very much.

 I then proceeded to replace the sprintf call with hand-rolled
 conversions. And wow, the benefit is far bigger than I'd assumed:
 postgres[7236][1]=# COPY tbl_timestamp TO '/dev/null';
 Time: 2430.521 ms

 So, by hand-rolling the ISO conversion in EncodeDateTime() we got a
 ~250% performance improvement. I'd say that's worthwhile.

 The attached patch shows what I did. While there's some polishing
 possible, as a whole, it's pretty ugly. But I think timestamp data is so
 common that it's worth the effort.

 Does anybody have a fundamentally nicer idea than the attached to
 improvide this?


It won't be quite as fast as what you've written, but I think it will be
much neater and more likely to be used in other places if we invent a
function like pg_ltoa() which returns a pointer to the new end of string.

Also if we're specifying padding with zeros then we can skip the reverse
part that's in pg_ltoa(), (normally needed since the numeric string is
build in reverse)

The code could then be written as:

str = pg_int2str_pad(str, year, 4);
*str++ = '-';
str = pg_int2str_pad(str, tm-tm_mon, 2);
*str++ = '-';
str = pg_int2str_pad(str, tm-tm_mday, 2);

etc

I've used this method before and found it to be about 10 times faster than
snprintf(), but I was reversing the string, so quite likely it be more than
10 times.

I'm interested to see how much you're really gaining by manually unrolling
the part that builds the fractional part of the second.

We could just build that part with: (untested)

if (fsec != 0)
{
int fseca = abs(fsec);
while (fseca % 10 == 0  fseca  0)
fseca /= 10;
 *str++ = '.';
str = pg_int2str(str, fseca);
}

Regards

David Rowley

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