Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed

Hello,

I tested this patch on Linux and there is no problem.
Also, I reviewed this patch and commented below.

@@ -439,6 +447,107 @@ XLogRecordHasFPW(XLogReaderState *record)
+ if (fork >= 0 && fork <= MAX_FORKNUM)
+ {
+ if (fork)
+ sprintf(forkname, "_%s", forkNames[fork]);
+ else
+ forkname[0] = 0;
+ }
+ else
+ pg_fatal("found invalid fork number: %u", fork);

Should we add the comment if the main fork is saved without "_main" suffix for code readability?

@@ -679,6 +788,9 @@ usage(void)
" (default: 1 or the value used in STARTSEG)\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -w, --fullpage only show records with a full page write\n"));
+ printf(_(" -W, --save-fpi=path save full page images to given path as\n"
+ " LSN.T.D.R.B_F\n"));
+ printf(_(" -X, --fixup-fpi=path like --save-fpi but apply LSN fixups to saved page\n"));
printf(_(" -x, --xid=XID only show records with transaction ID XID\n"));
printf(_(" -z, --stats[=record] show statistics instead of records\n"
" (optionally, show per-record statistics)\n"));

Since lower-case options are displayed at the top, should we switch the order of -x and -X?

@@ -972,6 +1093,25 @@ main(int argc, char **argv)
}
}

+ int dir_status = pg_check_dir(config.save_fpw_path);
+
+ if (dir_status < 0)
+ {
+ pg_log_error("could not access output directory: %s", config.save_fpw_path);
+ goto bad_argument;
+ }

Should we output %s enclosed with \"?

Regards,
Sho Kato