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

On Fri, Mar 22, 2024 at 3:15 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.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