From: David Rowley [mailto:dgrowleyml@gmail.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.
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.
