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
