This is a review for patch caaphdvpagtypzb2kwa0mmtksayg9+vashyjmjfatngxr1ad...@mail.gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
