Re: Patch to implement pg_current_logfile() function

On January 15, 2017 11:47:51 PM CST, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>> I do have a question here regards code formatting.
>> The patch now contains:
>>
>> if (log_filepath == NULL)
>> {
>> /* Bad data. Avoid segfaults etc. and return NULL to caller.
>*/
>> break;
>> }
>>
>> I'm not sure how this fits in with PG coding style,
>> whether the {} should be removed or what. I've looked
>> around and can't find an example of an if with a single
>> line then block and a comment. Maybe this means that
>> I shouldn't be doing this, but if I put the comment above
>> the if then it will "run into" the comment block which
>> immediately precedes the above code which describes
>> a larger body of code. So perhaps someone should look
>> at this and tell me how to improve it.
>
>Including brackets in this case makes a more readable code. That's the
>pattern respected the most in PG code. See for example
>XLogInsertRecord():
> else
> {
> /*
> * This was an xlog-switch record, but the current insert location was
> * already exactly at the beginning of a segment, so there was no need
> * to do anything.
> */
> }
>
>> Attached also are 2 patches which abstract some hardcoded
>> constants into symbols. Whether to apply them is a matter
>> of style and left to the maintainers, which is why these
>> are separate patches.
>
>Making the strings csvlog, stderr and eventlog variables? Why not
>because the patch presented here uses them in new places. Now it is
>not like it is a huge maintenance burden to keep them as strings, so I
>would personally not bother much.
>
>> The first patch changes only code on the master
>> branch, the 2nd patch changes the new code.
>
>Thanks for keeping things separated.
>
>> I have not looked further at the patch or checked
>> to see that all changes previously recommended have been
>> made. Michael, if you're confident that everything is good
>> go ahead and move the patchs forward to the maintainers. Otherwise
>> please let me know and I'll see about finding time
>> for further review.
>
>OK. I have done a round of hands-on review on this patch, finishing
>with the attached. In the things I have done:
>- Elimination of useless noise diff
>- Fixes some typos (logile, log file log, etc.)
>- Adjusted docs.
>- Docs were overdoing in storage.sgml. Let's keep the description
>simple.
>- Having a paragraph at the beginning of "Error Reporting and Logging"
>in config.sgml does not look like a good idea to me. As the updates of
>current_logfiles is associated with log_destination I'd rather see
>this part added in the description of the GUC.
>- Missed a (void) in the definition of update_metainfo_datafile().
>- Moved update_metainfo_datafile() before the signal handler routines
>in syslogger.c for clarity.
>- The last "return" is useless in update_metainfo_datafile().
>- In pg_current_logfile(), it is useless to call PG_RETURN_NULL after
>emitting an ERROR message.
>- Two calls to FreeFile(fd) have been forgotten in
>pg_current_logfile(). In one case, errno needs to be saved beforehand
>to be sure that the correct error string is generated for the user.
>- A portion of 010_pg_basebackup.pl was not updated.
>- Incorrect header ordering in basebackup.c.
>- Decoding of CR and LF characters seem to work fine when
>log_file_name include some.
>
>With all those issues fixed, I finish with the attached, that I am
>fine to pass down to a committer. I still think that this should be
>only one function using a SRF with rows shaped as (type, path) for
>simplicity, but as I am visibly outnumbered I won't insist further.
>Also, I would rather see an ERROR returned to the user in case of bad
>data in current_logfiles, I did not change that either as that's the
>original intention of Gilles.
>--
>Michael

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back you pay forward."
-- Robert A. Heinlein