public inbox for [email protected]  

help / color / mirror / Atom feed

From: sho kato <[email protected]>
To: [email protected]
Cc: David Christensen <[email protected]>
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Date: Wed, 09 Nov 2022 02:33:10 +0000
Message-ID: <166796119059.1126.8001665249724628876.pgcf@coridan.postgresql.org> (raw)
In-Reply-To: <CAOxo6XLCwGGjLEK9--VK9SzOb4EFKiJuSrnOePSPfEyr-pLKPw@mail.gmail.com>
References: <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
	<CAOxo6XLCwGGjLEK9--VK9SzOb4EFKiJuSrnOePSPfEyr-pLKPw@mail.gmail.com>

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

view thread (57+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
  In-Reply-To: <166796119059.1126.8001665249724628876.pgcf@coridan.postgresql.org>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox