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.

+ <entry>The LSN of the record with this block, formatted
+ as <literal>%08x-%08X</literal> instead of the
+ conventional <literal>%X/%X</literal> due to filesystem naming
+ limits</entry>
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