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

On Mon, Jun 24, 2024 at 4:01 PM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> Hi,
>
> On Mon, Jun 17, 2024 at 5:55 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Please find the attached v41 patches implementing the idea of vacuum
> doing the invalidation.
>
> Thoughts?
>
>
>

Some minor comments on the patch:
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."

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.

3.

In CheckPointReplicationSlots()

+ invalidated =
InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT,
+ 0,
+ InvalidOid,
+ InvalidTransactionId);
+
+ if (invalidated)
+ {
+ /*
+ * If any slots have been invalidated, recalculate the resource
+ * limits.
+ */
+ ReplicationSlotsComputeRequiredXmin(false);
+ ReplicationSlotsComputeRequiredLSN();
+ }

Is this calculation of resource limits really required here when the same
is already done inside InvalidateObsoleteReplicationSlots()

regards,
Ajin Cherian
Fujitsu Australia