Re: [HACKERS] Patch to implement pg_current_logfile() function

On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <kop@meme.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.