Re: Review: Patch to compute Max LSN of Data Pages

On Thursday, July 04, 2013 11:19 PM Robert Haas wrote:

>+         fprintf(stderr, _("%s: .. file \"%s\" for seeking: %s\n"),
>+                 progname, filename, strerror(errno));

>Weird error message style - what's with the ".."?

>+         fprintf(stderr, _("%s: .. file \"%s\" length is more than
segment
>size: %d.\n"),
>+                 progname, filename, RELSEG_SIZE);

>Again.

Corrected.

>+             fprintf(stderr, _("%s: could not seek to next page
\"%s\": %s\n"),
>+                     progname, filename,
strerror(errno));

>I think this should be written as: could not seek to offset NUMBER in
>file "PATH"

>+             fprintf(stderr, _("%s: could not read file \"%s\":
%s\n"),
>+                     progname, filename,
strerror(errno));

>And this one as: could not read file "PATH" at offset NUMBER

>+         printf("File:%s Maximum LSN found is: %X/%X \nWAL segment
file
>name (fileid,seg): %X/%X\n",

>I think that we don't need to display the WAL segment file name for
>the per-file progress updates.  Instead, let's show the block number
>where that LSN was found, like this:

>Highest LSN for file "%s" is %X/%X in block %u.

>The overall output can stay as you have it.

Changed as per your suggestion.

>+     if (0 != result)

>Coding style.

>+ static int
>+ FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn)

>It seems that this function, and a few others, use -1 for a failure
>return, 0 for success, and all others undefined.  Although this is a
>defensible choice, I think it would be more PG-like to make the return
>value bool and use true for success and false for failure.

>+         if (S_ISREG(statbuf.st_mode))
>+             (void) FindMaxLSNinFile(pathbuf, maxlsn);
>+         else
>+             (void) FindMaxLSNinDir(pathbuf, maxlsn);

>I don't see why we're throwing away the return value here.  I would
>expect the function to have a "bool result = true" at the top and sent
>result = false if one of these functions returns false.  At the end,
>it returns result.

Fixed.

>+    This utility can only be run by the user who installed the server,
because
>+    it requires read/write access to the data directory.

>False.

Removed.

>+     LsnSearchPath = argv[optind];
>+
>+     if (LsnSearchPath == NULL)
>+         LsnSearchPath = getenv("PGDATA");

>I think you should write the code so that the tool loops over its
>input arguments; if none, it processes $PGDATA.  (Don't forget to
>update the syntax synopsis and documentation to match.)

Added the functionality of multiple file or directories parsing and printing
Max LSN for each input argument. 

>I think the documentation should say something about the intended uses
>of this tool, including cautions against using it for things to which
>it is not well-suited.  I guess the threshold question for this patch
>is whether those uses are enough to justify including the tool in the
>core distribution.

Added some use cases and notes regarding the tool. Please suggest if any
More information needs to be documented.

Thanks for the review, please find the updated patch attached in the mail.

Regards,
Hari babu.