On January 15, 2017 11:47:51 PM CST, Michael Paquier <michael.paqu...@gmail.com> wrote: >On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <k...@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. >-- >Michael
Karl <k...@meme.com> Free Software: "You don't pay back you pay forward." -- Robert A. Heinlein