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

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