On Thu, Mar 21, 2024 at 4:25 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > This makes sense to me. Apart from this, few more comments on 0001.
Thanks for looking into it. > 1. > - "%s as caught_up, conflict_reason IS NOT NULL as invalid " > + "%s as caught_up, invalidation_reason IS NOT NULL as invalid " > 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'. Basically, the idea there is to not live_check for invalidated logical slots. It has nothing to do with conflicting. Up until now, conflict_reason is also reporting wal_removed (although wrongly including rows_removed, wal_level_insufficient, the two reasons for conflicts). So, I think invalidation_reason is right for invalid column. Also, I think we need to change conflicting to invalidation_reason for live_check. So, I've changed that to use invalidation_reason for both columns. > 2. > > Can the reasons 'rows_removed' and 'wal_level_insufficient' appear for > physical slots? No. They can only occur for logical slots, check InvalidatePossiblyObsoleteSlot, only the logical slots get invalidated. > If not, then it is not clear from above text. I've stated that "It is set only for logical slots." for rows_removed and wal_level_insufficient. Other reasons can occur for both slots. > 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. Yes. There the intention of the test case is to verify logical slots are reported as non conflicting. So, I changed them. Please find the v14-0001 patch for now. I'll post the other patches soon. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v14-0001-Track-invalidation_reason-in-pg_replication_slot.patch
Description: Binary data
