REVIEW: Allow formatting in log_line_prefix

From: Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Cc: "dgrowleyml(at)gmail(dot)com" <dgrowleyml(at)gmail(dot)com>
Subject: REVIEW: Allow formatting in log_line_prefix
Date: 2013-09-18 14:18:43
Message-ID: A737B7A37273E048B164557ADEF4A58B17C220DA@ntex2010a.host.magwien.gv.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

This is a review for patch
CAApHDvpAgtyPZB2kwa0MmTkSaYG9+vasHYjmjFaTNgXr1aDC4Q(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".

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

- This comment:

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

should begin with lower case.

- 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".

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

Yours,
Laurenz Albe

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2013-09-18 14:22:51 where we are with dbuckets calculation?
Previous Message Alexander Korotkov 2013-09-18 14:11:03 Re: Fix picksplit with nan values