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.pg@gmail.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