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.

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.

3) get rid of timestamp_str - use appendString* methods directly

See above, I'm afraid it is necessary for padding, because there are two formatted fields.

4) support just the "with milliseconds" using '%n' escape sequence

Ok.

All those changes are quite trivial. The only annoying bit is that both '%u' and '%e' are already used, so none of the obvious choices for 'Unix Epoch' are available. So I simply took (%m+1) which is %n.

It stands for "numeric" as well, so I think it is quite nice.

--
Fabien.


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

Reply via email to