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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
