Re: Introduce XID age and inactive timeout based replication slot invalidation

Hi,

On Wed, Mar 06, 2024 at 02:46:57PM +0530, Bharath Rupireddy wrote:
> On Wed, Mar 6, 2024 at 2:42 PM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > Yeah, I'm okay with one column.
>
> Thanks. v8-0001 is how it looks. Please see the v8 patch set with this change.

Thanks!

A few comments:

1 ===

+ The reason for the slot's invalidation. <literal>NULL</literal> if the
+ slot is currently actively being used.

s/currently actively being used/not invalidated/ ? (I mean it could be valid
and not being used).

2 ===

+ the slot is marked as invalidated. In case of logical slots, it
+ represents the reason for the logical slot's conflict with recovery.

s/the reason for the logical slot's conflict with recovery./the recovery conflict reason./ ?

3 ===

@@ -667,13 +667,13 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check)
* removed.
*/
res = executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase, failover, "
- "%s as caught_up, conflict_reason IS NOT NULL as invalid "
+ "%s as caught_up, invalidation_reason IS NOT NULL as invalid "
"FROM pg_catalog.pg_replication_slots "
"WHERE slot_type = 'logical' AND "
"database = current_database() AND "
"temporary IS FALSE;",
live_check ? "FALSE" :
- "(CASE WHEN conflict_reason IS NOT NULL THEN FALSE "
+ "(CASE WHEN invalidation_reason IS NOT NULL THEN FALSE "

Yeah that's fine because there is logical slot filtering here.

4 ===

-GetSlotInvalidationCause(const char *conflict_reason)
+GetSlotInvalidationCause(const char *invalidation_reason)

Should we change the comment "Maps a conflict reason" above this function?

5 ===

-# Check conflict_reason is NULL for physical slot
+# Check invalidation_reason is NULL for physical slot
$res = $node_primary->safe_psql(
'postgres', qq[
- SELECT conflict_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';]
+ SELECT invalidation_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';]
);

I don't think this test is needed anymore: it does not make that much sense since
it's done after the primary database initialization and startup.

6 ===

@@ -680,7 +680,7 @@ ok( $node_standby->poll_query_until(
is( $node_standby->safe_psql(
'postgres',
q[select bool_or(conflicting) from
- (select conflict_reason is not NULL as conflicting
+ (select invalidation_reason is not NULL as conflicting
from pg_replication_slots WHERE slot_type = 'logical')]),
'f',
'Logical slots are reported as non conflicting');

What about?

"
# Verify slots are reported as valid in pg_replication_slots
is( $node_standby->safe_psql(
'postgres',
q[select bool_or(invalidated) from
(select invalidation_reason is not NULL as invalidated
from pg_replication_slots WHERE slot_type = 'logical')]),
'f',
'Logical slots are reported as valid');
"

Regards,

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