Re: Fix 035_standby_logical_decoding.pl race conditions - Mailing list pgsql-hackers

On Wed, Apr 02, 2025 at 12:13:52PM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Amit, Bertrand,
> 
> > The other idea to simplify the changes for backbranches:
> > sub reactive_slots_change_hfs_and_wait_for_xmins
> > {
> > ...
> > +  my ($slot_prefix, $hsf, $invalidated, $needs_active_slot) = @_;
> > 
> >   # create the logical slots
> > -  create_logical_slots($node_standby, $slot_prefix);
> > +  create_logical_slots($node_standby, $slot_prefix, $needs_active_slot);
> > 
> > ...
> > +  if ($needs_active_slot)
> > +  {
> > +    $handle =
> > +      make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
> > +  }
> > 
> > What if this function doesn't take input parameter needs_active_slot
> > and rather removes the call to make_slot_active? We will call
> > make_slot_active only at the required places. This should make the
> > changes much less because after that, we don't need to make changes
> > related to drop and create. Sure, in some cases, we will test two
> > inactive slots instead of one, but I guess that would be the price to
> > keep the tests simple and more like HEAD.
> 
> Actually, I could not decide which one is better, so let me share both drafts.

Thanks!

> V5-PG17-1 uses the previous approach, and v5-PG17-2 uses new proposed one.
> Bertrand, which one do you like?

I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
keep the slots active and only avoid doing the checks for them (they are invalidated
that's fine, they are not that's fine too).

Also I think that we should change this part:

"
 # Verify that invalidated logical slots do not lead to retaining WAL.
@@ -602,7 +610,7 @@ check_slots_conflict_reason('vacuum_full_', 'rows_removed');
 my $restart_lsn = $node_standby->safe_psql(
        'postgres',
        "SELECT restart_lsn FROM pg_replication_slots
-               WHERE slot_name = 'vacuum_full_activeslot' AND conflicting;"
+               WHERE slot_name = 'vacuum_full_inactiveslot' AND conflicting;"
 );

" to be on the safe side of thing.

What do you think of the attached (to apply on top of v5-PG17-2)?

Regards,

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