FW: REVIEW: Allow formatting in log_line_prefix

(Forgot to copy list)

From: David Rowley [mailto:dgrowleyml(at)gmail(dot)com]
Sent: 19 September 2013 22:35
To: Albe Laurenz
Subject: Re: REVIEW: Allow formatting in log_line_prefix

Hi Laurenz,

Thanks for the review.

Please see my comments below and the updated patch attached.

On Thu, Sep 19, 2013 at 2:18 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at
<mailto:laurenz(dot)albe(at)wien(dot)gv(dot)at> > wrote:

This is a review for patch
CAApHDvpAgtyPZB2kwa0MmTkSaYG9+vasHYjmjFaTNgXr1aDC4Q(at)mail(dot)gmail(dot)com
<mailto:CAApHDvpAgtyPZB2kwa0MmTkSaYG9%2BvasHYjmjFaTNgXr1aDC4Q(at)mail(dot)gmail(dot)com
>

The patch is readable, applies fine and builds without warnings.

It contains sufficient documentation.

It works as it should, no crashes or errors.

It is well written, in fact it improves the readability of
the log_line_prefix function in backend/utils/error/elog.c.

I think that this is a useful feature as it can help to make
stderr logging more readable for humans.

I have a few gripes with the English:

- In the documentation patch:

+ numeric literal after the % and before the option. A negative
+ padding will cause the status information to be padded on the
+ right with spaces to give it a minimum width. Whereas a positive
+ padding will pad on the left. Padding can be useful keep log
+ files more human readable.

I think that there should be a comma, not a period, before "whereas".

I've made the change you request here and also made a change to my last
sentence, which hopefully is an improvement.

- The description for the function process_log_prefix_padding
should probably mention that it returns NULL if it encounters
bad syntax.

I've added a comment before the function for this.

- This comment:

+ /* Format is invalid if it ends with the padding number */

should begin with lower case.

Fixed, but not quite sure of the reason for lack of caps at the moment.

- In this comment:

+ /*
+ * Process any formatting which may exist after the '%'.
+ * Note that this moves p passed the padding number
+ * if it exists.
+ */

It should be "past", not "passed".

Fixed.

If these details get fixed, I'll mark the patch ready for committer.

Yours,
Laurenz Albe