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

On Sat, Apr 23, 2022 at 01:43:36PM -0500, David Christensen wrote:
> Hi Matthias, great point.  Enclosed is a revised version of the patch
> that adds the fork identifier to the end if it's a non-main fork.

Like Alvaro, I have seen cases where this would have been really
handy.  So +1 from me, as well, to have more tooling like what you are
proposing.  Fine for me to use one file for each block with a name
like what you are suggesting for each one of them. 

+       /* we accept an empty existing directory */
+       if (stat(config.save_fpw_path, &st) == 0 && S_ISDIR(st.st_mode))
+       {
I don't think that there is any need to rely on a new logic if there
is already some code in place able to do the same work.  See
verify_dir_is_empty_or_create() in pg_basebackup.c, as one example,
that relies on pg_check_dir().  I think that you'd better rely at
least on what pgcheckdir.c offers.

+       {"raw-fpi", required_argument, NULL, 'W'},
I think that we'd better rename this option.  "fpi", that is not used
much in the user-facing docs, is additionally not adapted when we have
an other option called -w/--fullpage.  I can think of
--save-fullpage.

+                   PageSetLSN(page, record->ReadRecPtr);
+                   /* if checksum field is non-zero then we have checksums enabled,
+                    * so recalculate the checksum with new LSN (yes, this is a hack)
+                    */
Yeah, that looks like a hack, but putting in place a page on a cluster
that has checksums enabled would be more annoying with
zero_damaged_pages enabled if we don't do that, so that's fine by me
as-is.  Perhaps you should mention that FPWs don't have their
pd_checksum updated when written.

+       /* we will now extract the fullpage image from the XLogRecord and save
+        * it to a calculated filename */
The format of this comment is incorrect.

+            The LSN of the record with this block, formatted
+            as %08x-%08X instead of the
+            conventional %X/%X due to filesystem naming
+            limits
The last part of the sentence about %X/%X could just be removed.  That
could be confusing, at worse.

+                   PageSetLSN(page, record->ReadRecPtr);
Why is pd_lsn set?

git diff --check complains a bit.

This stuff should include some tests.  With --end, the tests can
be cheap.
--
Michael