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