public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Christensen <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Date: Mon, 25 Apr 2022 10:11:10 -0500
Message-ID: <CAOxo6X+zVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
<CAEze2Wi_WsKqmCZV0e85eS64TdtgcdANDXTH+xUUD54om2i5hw@mail.gmail.com>
<CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
<[email protected]>
On Mon, Apr 25, 2022 at 1:11 AM Michael Paquier <[email protected]> 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
view thread (6+ 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: <CAOxo6X+zVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ@mail.gmail.com>
* 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