Re: Fix 035_standby_logical_decoding.pl race conditions

Hi Kuroda-san and Amit,

On Fri, Mar 28, 2025 at 09:02:29AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Amit,
>
> >
> > Right, I think this is a better idea.

I like it too and the bonus point is that this injection point can be used
in more tests (more use cases).

A few comments:

==== About v2-0001-Stabilize

=== 1

s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?

=== 2 (Nit)

/* For testing slot invalidation due to the conflict */

Not sure "due to the conflict" is needed.

==== About PG17-v2-0001

=== 3

The commit message still mentions injection point.

=== 4

-# Note that pg_current_snapshot() is used to get the horizon. It does
-# not generate a Transaction/COMMIT WAL record, decreasing the risk of
-# seeing a xl_running_xacts that would advance an active replication slot's
-# catalog_xmin. Advancing the active replication slot's catalog_xmin
-# would break some tests that expect the active slot to conflict with
-# the catalog xmin horizon.

I'd be tempted to not remove this comment but reword it a bit instead. Something
like?

# Note that pg_current_snapshot() is used to get the horizon. It does
# not generate a Transaction/COMMIT WAL record, decreasing the risk of
# seeing a xl_running_xacts that would advance an active replication slot's
# catalog_xmin. Advancing the active replication slot's catalog_xmin
# would break some tests that expect the active slot to conflict with
# the catalog xmin horizon. We ensure that active replication slots are not
# created for tests that might produce this race condition though.

=== 5

The invalidation checks for active slots are kept for the wal_level case. Also
the active slots are still created to test that logical decoding on the standby
behaves correctly, when no conflict is expected and for the promotion.

The above makes sense to me.

=== 6 (Nit)

In drop_logical_slots(), s/needs_active_slot/drop_active_slot/?

=== 7 (Nit)

In check_slots_conflict_reason(), s/needs_active_slot/checks_active_slot/?

==== About PG16-v2-0001

Same as for PG17-v2-0001.

Regards,

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