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

On Fri, Mar 22, 2024 at 3:15 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Looking at v14-0002:

Thanks for reviewing. I agree that 0002 with last_inactive_at can go
independently and be of use on its own in addition to helping
implement inactive_timeout based invalidation.

> 1 ===
>
> @@ -691,6 +699,13 @@ ReplicationSlotRelease(void)
> ConditionVariableBroadcast(&slot->active_cv);
> }
>
> + if (slot->data.persistency == RS_PERSISTENT)
> + {
> + SpinLockAcquire(&slot->mutex);
> + slot->last_inactive_at = GetCurrentTimestamp();
> + SpinLockRelease(&slot->mutex);
> + }
>
> I'm not sure we should do system calls while we're holding a spinlock.
> Assign a variable before?

Can do that. Then, the last_inactive_at = current_timestamp + mutex
acquire time. But, that shouldn't be a problem than doing system calls
while holding the mutex. So, done that way.

> 2 ===
>
> Also, what about moving this here?
>
> "
> if (slot->data.persistency == RS_PERSISTENT)
> {
> /*
> * Mark persistent slot inactive. We're not freeing it, just
> * disconnecting, but wake up others that may be waiting for it.
> */
> SpinLockAcquire(&slot->mutex);
> slot->active_pid = 0;
> SpinLockRelease(&slot->mutex);
> ConditionVariableBroadcast(&slot->active_cv);
> }
> "
>
> That would avoid testing twice "slot->data.persistency == RS_PERSISTENT".

Ugh. Done that now.

> 3 ===
>
> @@ -2341,6 +2356,7 @@ RestoreSlotFromDisk(const char *name)
>
> slot->in_use = true;
> slot->active_pid = 0;
> + slot->last_inactive_at = 0;
>
> I think we should put GetCurrentTimestamp() here. It's done in v14-0006 but I
> think it's better to do it in 0002 (and not taking care of inactive_timeout).

Done.

> 4 ===
>
> Track last_inactive_at in pg_replication_slots
>
> doc/src/sgml/system-views.sgml | 11 +++++++++++
> src/backend/catalog/system_views.sql | 3 ++-
> src/backend/replication/slot.c | 16 ++++++++++++++++
> src/backend/replication/slotfuncs.c | 7 ++++++-
> src/include/catalog/pg_proc.dat | 6 +++---
> src/include/replication/slot.h | 3 +++
> src/test/regress/expected/rules.out | 5 +++--
> 7 files changed, 44 insertions(+), 7 deletions(-)
>
> Worth to add some tests too (or we postpone them in future commits because we're
> confident enough they will follow soon)?

Yes. Added some tests in a new TAP test file named
src/test/recovery/t/043_replslot_misc.pl. This new file can be used to
add miscellaneous replication tests in future as well. I couldn't find
a better place in existing test files - tried having the new tests for
physical slots in t/001_stream_rep.pl and I didn't find a right place
for logical slots.

> 5 ===
>
> Most of the fields that reflect a time (not duration) in the system views are
> xxxx_time, so I'm wondering if instead of "last_inactive_at" we should use
> something like "last_inactive_time"?

Yeah, I can see that. So, I changed it to last_inactive_time.

I agree with treating last_inactive_time as a separate property of the
slot having its own use in addition to helping implement
inactive_timeout based invalidation. I think it can go separately.

I tried to address the review comments received for this patch alone
and attached v15-0001. I'll post other patches soon.

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