On Wed, Dec 21, 2022 at 5:47 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Fri, Dec 16, 2022 at 4:47 AM David Christensen > <david.christen...@crunchydata.com> wrote: > > > > On Thu, Dec 15, 2022 at 12:36 AM Michael Paquier <mich...@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.
Sure, no issues. > 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. I think the "allows_streaming" was required to ensure the WAL files were preserved properly, and was the approach we ended up taking rather than trying to fail the archive_command or other approaches I'd taken earlier. I'd rather keep this if we can, unless you can propose a different approach that would continue to work in the same way. > 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. Yeah, can spell out; don't recall exactly why we needed it offhand, but will confirm or remove if insignificant. > 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? Sure; since it was just setting a pointer value I didn't consider it to be a hotspot for optimization. Best, David