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

This looks better.

+         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.

+             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.

+     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.

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

False.

+     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.)

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company