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

On Wed, Aug 14, 2024 at 9:20 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Some minor comments on the patch:

Thanks for reviewing.

> 1.
> + /*
> + * Release the lock if it's not yet to keep the cleanup path on
> + * error happy.
> + */
>
> I suggest rephrasing to: " "Release the lock if it hasn't been already to ensure smooth cleanup on error."

Changed.

> 2.
>
> elog(DEBUG1, "performing replication slot invalidation");
>
> Probably change it to "performing replication slot invalidation checks" as we might not actually invalidate any slot here.

Changed.

> 3.
>
> + ReplicationSlotsComputeRequiredXmin(false);
> + ReplicationSlotsComputeRequiredLSN();
> + }
>
> Is this calculation of resource limits really required here when the same is already done inside InvalidateObsoleteReplicationSlots()

Nice catch. Removed.

Please find the attached v42 patches.

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