Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL - Mailing list pgsql-hackers

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