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

On Thu, Mar 21, 2024 at 2:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Mar 21, 2024 at 12:40 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > v13-0001 looks good to me. The only Nit (that I've mentioned up-thread) is that
> > in the pg_replication_slots view, the invalidation_reason is "far away" from the
> > conflicting field. I understand that one could query the fields individually but
> > when describing the view or reading the doc, it seems more appropriate to see
> > them closer. Also as "failover" and "synced" are also new in version 17, there
> > is no risk to break order by "17,18" kind of queries (which are the failover
> > and sync positions).
>
> Hm, yeah, I can change that in the next version of the patches. Thanks.
>

This makes sense to me. Apart from this, few more comments on 0001.
1.
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -676,13 +676,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 conflicting THEN FALSE "

I think here at both places we need to change 'conflict_reason' to
'conflicting'.

2.
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>invalidation_reason</structfield> <type>text</type>
+      </para>
+      <para>
+       The reason for the slot's invalidation. It is set for both logical and
+       physical slots. <literal>NULL</literal> if the slot is not invalidated.
+       Possible values are:
+       <itemizedlist spacing="compact">
+        <listitem>
+         <para>
+          <literal>wal_removed</literal> means that the required WAL has been
+          removed.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>rows_removed</literal> means that the required rows have
+          been removed.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>wal_level_insufficient</literal> means that the
+          primary doesn't have a <xref linkend="guc-wal-level"/> sufficient to
+          perform logical decoding.
+         </para>

Can the reasons 'rows_removed' and 'wal_level_insufficient' appear for
physical slots? If not, then it is not clear from above text.

3.
-# Verify slots are reported as non conflicting in pg_replication_slots
+# Verify slots are reported as valid in pg_replication_slots
 is( $node_standby->safe_psql(
  'postgres',
  q[select bool_or(conflicting) from
-   (select conflict_reason is not NULL as conflicting
-    from pg_replication_slots WHERE slot_type = 'logical')]),
+   (select conflicting from pg_replication_slots
+ where slot_type = 'logical')]),
  'f',
- 'Logical slots are reported as non conflicting');
+ 'Logical slots are reported as valid');

I don't think we need to change the comment or success message in this test.

-- 
With Regards,
Amit Kapila.