On Fri, Mar 22, 2024 at 7:15 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote:
> On Fri, Mar 22, 2024 at 12:39 PM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > > > > Please find the v14-0001 patch for now. > > > > Thanks! > > > > > LGTM. Let's wait for Bertrand to see if he has more comments on 0001 > > > and then I'll push it. > > > > LGTM too. > > Thanks. Here I'm implementing the following: > > 0001 Track invalidation_reason in pg_replication_slots > 0002 Track last_inactive_at in pg_replication_slots > 0003 Allow setting inactive_timeout for replication slots via SQL API > 0004 Introduce new SQL funtion pg_alter_replication_slot > 0005 Allow setting inactive_timeout in the replication command > 0006 Add inactive_timeout based replication slot invalidation > > 1. Keep it last_inactive_at as a shared memory variable, but always > set it at restart if the slot's inactive_timeout has non-zero value > and reset it as soon as someone acquires that slot so that if the slot > doesn't get acquired till inactive_timeout, checkpointer will > invalidate the slot. > 2. Ensure with pg_alter_replication_slot one could "only" alter the > timeout property for the time being, if not that could lead to the > subscription inconsistency. > 3. Have some notes in the CREATE and ALTER SUBSCRIPTION docs about > using an existing slot to leverage inactive_timeout feature. > 4. last_inactive_at should also be set to the current time during slot > creation because if one creates a slot and does nothing with it then > it's the time it starts to be inactive. > 5. We don't set last_inactive_at to GetCurrentTimestamp() for failover > slots. > 6. Leave the patch that added support for inactive_timeout in > subscriptions. > > Please see the attached v14 patch set. No change in the attached > v14-0001 from the previous patch. > > > Some comments: 1. In patch 0005: In ReplicationSlotAlter(): + lock_acquired = false; if (MyReplicationSlot->data.failover != failover) { SpinLockAcquire(&MyReplicationSlot->mutex); + lock_acquired = true; MyReplicationSlot->data.failover = failover; + } + + if (MyReplicationSlot->data.inactive_timeout != inactive_timeout) + { + if (!lock_acquired) + { + SpinLockAcquire(&MyReplicationSlot->mutex); + lock_acquired = true; + } + + MyReplicationSlot->data.inactive_timeout = inactive_timeout; + } + + if (lock_acquired) + { SpinLockRelease(&MyReplicationSlot->mutex); Can't you make it shorter like below: lock_acquired = false; if (MyReplicationSlot->data.failover != failover || MyReplicationSlot->data.inactive_timeout != inactive_timeout) { SpinLockAcquire(&MyReplicationSlot->mutex); lock_acquired = true; } if (MyReplicationSlot->data.failover != failover) { MyReplicationSlot->data.failover = failover; } if (MyReplicationSlot->data.inactive_timeout != inactive_timeout) { MyReplicationSlot->data.inactive_timeout = inactive_timeout; } if (lock_acquired) { SpinLockRelease(&MyReplicationSlot->mutex); ReplicationSlotMarkDirty(); ReplicationSlotSave(); } 2. In patch 0005: why change walrcv_alter_slot option? it doesn't seem to be used anywhere, any use case for it? If required, would the intention be to add this as a Create Subscription option? regards, Ajin Cherian Fujitsu Australia