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

On Thu, Mar 21, 2024 at 2:44 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Mar 21, 2024 at 12:40 PM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)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.