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

On Mon, Apr 25, 2022 at 1:11 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

Yeah, though I am tending towards what another user had suggested and
just accepting an existing directory rather than requiring it to be
empty, so thinking I might just skip this one. (Will review the file
you've pointed out to see if there is a relevant function though.)

> + {"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.

Makes sense.

> + 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.

Can make a mention; you thinking just a comment in the code is
sufficient, or want there to be user-visible docs as well?

> + /* 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.

Makes sense.

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

This was to make the page as extracted equivalent to the effect of
applying the WAL record block on replay (the LSN and checksum both);
since recovery calls this function to mark the LSN where the page came
from this is why I did this as well. (I do see perhaps a case for
--raw output that doesn't munge the page whatsoever, just made
comparisons easier.)

> git diff --check complains a bit.

Can look into this.

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

Yeah, makes sense, will include some in the next version.

David