On Mon, Jun 24, 2024 at 4:01 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:
> Hi,
>
> On Mon, Jun 17, 2024 at 5:55 PM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.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
