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

On Thu, Mar 21, 2024 at 4:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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