public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: David Christensen <[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 15:11:04 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
References: <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
<CAEze2Wi_WsKqmCZV0e85eS64TdtgcdANDXTH+xUUD54om2i5hw@mail.gmail.com>
<CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (59+ 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: <[email protected]>
* 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