Re: Patch to implement pg_current_logfile() function

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