Horton

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

Nisha Moond <nisha.moond412@gmail.com>

View thread
From: Nisha Moond <nisha.moond412@gmail.com>
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Ajin Cherian <itsajin@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nathan Bossart <nathandbossart@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-01-16T07:05:16Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add a test for commit ac0e33136a using the injection point.

  2. Invalidate inactive replication slots.

  3. Fix incorrect slot type in BuildTupleHashTableExt

  4. Allow synced slots to have their inactive_since.

  5. Change last_inactive_time to inactive_since in pg_replication_slots.

  6. Track last_inactive_time in pg_replication_slots.

  7. Track invalidation_reason in pg_replication_slots.

  8. Add option force_initdb to PostgreSQL::Test::Cluster:init()

  9. Add a failover option to subscriptions.

  10. Allow setting failover property in the replication command.

  11. Allow to enable failover property for replication slots via SQL API.

  12. Track conflict_reason in pg_replication_slots.

  13. Log messages for replication slot acquisition and release.

  14. Remove vacuum_defer_cleanup_age

  15. Fix corruption due to vacuum_defer_cleanup_age underflowing 64bit xids

  16. meson: Add initial version of meson based build system

Attachments

On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Hi Nisha,
> > >
> > > Here are some minor review comments for patch v58-0002.
> > >
> >
> > Thank you for your feedback! Please find the v59 patch set addressing
> > all the comments.
> > Note: There are no new changes in patch-0001.
> >
>
> Hi Nisha,
> I reviewed the v59-0001 patch. I have few comments:
>
> 1.I think we should update the comment for function
> 'InvalidatePossiblyObsoleteSlot'
> Currently the comment is like:
>
> /*
>  * Helper for InvalidateObsoleteReplicationSlots
>  *
>  * Acquires the given slot and mark it invalid, if necessary and possible.
>  *
>  * Returns whether ReplicationSlotControlLock was released in the interim (and
>  * in that case we're not holding the lock at return, otherwise we are).
>  *
>  * Sets *invalidated true if the slot was invalidated. (Untouched otherwise.)
>  *
>  * This is inherently racy, because we release the LWLock
>  * for syscalls, so caller must restart if we return true.
>  */
>
> I think we should add a comment for the case 'when slot is already ours'.

Done.

> 2. Similarly we should update comment here:
>         /*
>          * Check if the slot needs to be invalidated. If it needs to be
>          * invalidated, and is not currently acquired, acquire it and mark it
>          * as having been invalidated.  We do this with the spinlock held to
>          * avoid race conditions -- for example the restart_lsn could move
>          * forward, or the slot could be dropped.
>          */
>         SpinLockAcquire(&s->mutex);
>
> Before we release the lock, we are marking the slot as invalidated for
> the case when the slot is already acquired by our process. So we
> should update it in comment.
>

Clarified the comments as per the mentioned case.

> 3. I think we should also update the following 'if condition':
>
> if (active_pid != 0)
>         {
>             /*
>              * Prepare the sleep on the slot's condition variable before
>              * releasing the lock, to close a possible race condition if the
>              * slot is released before the sleep below.
>              */
> We should not enter the if condition for the case when the slot was
> already acquired by our process.
>

Thank you for pointing that out. I've included the fix and also
reorganized this section of the code in patch-0001 to improve the
readability of the logic.

Attached the v60 patch set addressing above comments and all other
comments at [1].

[1] https://www.postgresql.org/message-id/CALDaNm2r969ZZPDaAZQEtxcfL-sGUW8AGdbdwC8AcMn1V8w%2Bhw%40mail.gmail.com

--
Thanks,
Nisha