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

On Fri, Mar 15, 2024 at 10:15 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> > > > wal_level_insufficient it's the reason for conflict with recovery".
>
> +1 on maintaining both conflicting and invalidation_reason

Thanks.

> Thanks for the patch. JFYI, patch09 does not apply to HEAD, some
> recent commit caused the conflict.

Yep, the conflict is in src/test/recovery/meson.build and is because
of e6927270cd18d535b77cbe79c55c6584351524be.

> Some trivial comments on patch001 (yet to review other patches)

Thanks for looking into this.

> 1)
> info.c:
>
> - "%s as caught_up, conflict_reason IS NOT NULL as invalid "
> + "%s as caught_up, invalidation_reason IS NOT NULL as invalid "
>
> Can we revert back to 'conflicting as invalid' since it is a query for
> logical slots only.

I guess, no. There the intention is to check for invalid logical slots
not just for the conflicting ones. The logical slots can get
invalidated due to other reasons as well.

> 2)
> 040_standby_failover_slots_sync.pl:
>
> - q{SELECT conflict_reason IS NULL AND synced AND NOT temporary FROM
> pg_replication_slots WHERE slot_name = 'lsub1_slot';}
> + q{SELECT invalidation_reason IS NULL AND synced AND NOT temporary
> FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';}
>
> Here too, can we have 'NOT conflicting' instead of '
> invalidation_reason IS NULL' as it is a logical slot test.

I guess no. The tests are ensuring the slot on the standby isn't invalidated.

In general, one needs to use the 'conflicting' column from
pg_replication_slots when the intention is to look for reasons for
conflicts, otherwise use the 'invalidation_reason' column for
invalidations.

Please see the attached v10 patch set after resolving the merge
conflict and fixing an indentation warning in the TAP test file.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com