MIME-Version: 1.0
References: 
 <CALj2ACWTQCYGHf=2pHQeX=8qfkMi-4JsXN0t_6W8+A-cW8wJpw@mail.gmail.com>
 <CAOxo6X+wXEu1QfTFTf-WiDs3jr8ERvgEOddHfUhAH_5eje-G2w@mail.gmail.com>
 <CAOxo6XJ03w=PWDnsnzDpGK+4Er7XZwFkhqiMbVWmt7VLHzidHw@mail.gmail.com>
 <CALj2ACUukW74XXb2n3FFAdRiisF06jSNDR8DegOpMevq_0yVoA@mail.gmail.com>
 <CAOxo6XJ+uN-kiCS9wVKm2LudZXEZOk6zzduF-uGc_4464uXs9w@mail.gmail.com>
 <CAOxo6X+ALOqqZ+a1r-=BTDaUB6LL0w7nnu=K_kvYbGsrkU2eyA@mail.gmail.com>
 <CALj2ACV4donjBLaxkJ1FO3Ybg73ut--9TozHTsNyBVCdX9opYA@mail.gmail.com>
 <CAOxo6XLhduEAn0yLh+BGOjpq6k0x9MsOY6PjDEoCh4VvdUxCiQ@mail.gmail.com>
 <CALj2ACWmgbDeCDMaZah0GRwQBCJMD01U4S69V6TE4W2jYmnLuQ@mail.gmail.com>
 <CAOxo6XJMcLyLb1D4oOV2FHfJvt1Z6DuediGUKKEHAEMvpPe2ow@mail.gmail.com>
 <Y5rAViBOVChfrsHN@paquier.xyz>
 <CAOxo6X+NCVAB3H5_TMSasgimf127KURoi=S0dTSNJUn8Z8+WNA@mail.gmail.com>
In-Reply-To: 
 <CAOxo6X+NCVAB3H5_TMSasgimf127KURoi=S0dTSNJUn8Z8+WNA@mail.gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 21 Dec 2022 17:17:01 +0530
Message-ID: 
 <CALj2ACUT+DB8PhF6ja8f-LP76cEappXrJuhzrKd81a90UmrnYA@mail.gmail.com>
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
To: David Christensen <david.christensen@crunchydata.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Justin Pryzby <pryzby@telsasoft.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALj2ACUT%2BDB8PhF6ja8f-LP76cEappXrJuhzrKd81a90UmrnYA%40mail.gmail.com>
Precedence: bulk

On Fri, Dec 16, 2022 at 4:47 AM David Christensen
<david.christensen@crunchydata.com> wrote:
>
> On Thu, Dec 15, 2022 at 12:36 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Dec 14, 2022 at 04:44:34PM -0600, David Christensen wrote:
> > > I can get one sent in tomorrow.
>
> This v10 should incorporate your feedback as well as Bharath's.

Thanks for the patch. Here're some minor comments:

1. +my $node =  PostgreSQL::Test::Cluster->new('primary');
Can the name be other than 'primary' because we don't create a standby
for this test? Something like - 'node_a' or 'node_extract_fpi' or some
other.

2. +$node->init(extra => ['-k'], allows_streaming => 1);
When enabled with allows_streaming, there are a bunch of things that
happen to the node while initializing, I don't think we need all of
them for this.

3. +$node->init(extra => ['-k'], allows_streaming => 1);
Can we use --data-checksums instead of -k for more readability?
Perhaps, a comment on why we need that option helps greatly.

4.
+        page = (Page) buf.data;
+
+        if (!XLogRecHasBlockRef(record, block_id))
+            continue;
+
+        if (!XLogRecHasBlockImage(record, block_id))
+            continue;
+
+        if (!RestoreBlockImage(record, block_id, page))
+            continue;
Can you shift  page = (Page) buf.data; just before the last if
condition RestoreBlockImage() so that it doesn't get executed for the
other two continue statements?

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com