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