On 08/22/2015 09:54 PM, Fabien COELHO wrote:
>
> Hello Tomas,
>
> Review of v2:
>
>> attached is a v2 of the patch, reworked based on the comments.
>
> The patch applies cleanly to head, it compiles, I tested it and it
> mostly work as expected, see below.
>
>> 1) fix the docs (explicitly say that it's a Unix epoch)
>
> I would add the word "numeric" in front of timestamp both in the doc and
> in the postgresql.conf.sample, as it justifies the chosen %n.
I think we're already using 'unix epoch' in the docs without explicitly
stating that it's a numeric value, so I don't think we should use it
here as it'd be inconsistent.
>
>> 2) handle 'padding' properly
>
> I tried that without success. ISTM that what is padded is the empty
> string, and the timestamp is just printed on its own without padding
> afterwards.
>
> I think that it should use a string buffer and then used the padding on
> the string, as case 'c' or 't' for instance.
Hmmm, I'm not entirely sure how exactly the padding is supposed to work
(IIRC I've never used it), and I thought it behaved correctly. But maybe
not - I think the safest thing is copy what 't' does, so I've done that
in attached v3 of the patch.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
