MIME-Version: 1.0
References: <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
 <CAEze2Wi_WsKqmCZV0e85eS64TdtgcdANDXTH+xUUD54om2i5hw@mail.gmail.com>
 <CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com> <YmY7eMgqycsWExQt@paquier.xyz>
In-Reply-To: <YmY7eMgqycsWExQt@paquier.xyz>
From: David Christensen <david.christensen@crunchydata.com>
Date: Mon, 25 Apr 2022 10:11:10 -0500
Message-ID: <CAOxo6X+zVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ@mail.gmail.com>
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
To: Michael Paquier <michael@paquier.xyz>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAOxo6X%2BzVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ%40mail.gmail.com>
Precedence: bulk

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