Horton

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

Shlok Kyal <shlok.kyal.oss@gmail.com>

View thread
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
To: Nisha Moond <nisha.moond412@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-15T06:07:43Z
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

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'.

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.

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.

Thanks and Regards,
Shlok Kyal