Date: Fri, 25 Sep 2026 05:54:41 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: JoongHyuk Shin <sjh910805@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Rui Zhao <zhaorui126@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Message-ID: <arYModKZ2uLwUyES@bdtpg>
References: 
 <CAJpy0uDAyMbD61iJE-2qRMCtdQ4Qtuq+hy=mqDszFapgCHXaJQ@mail.gmail.com>
 <arNsgodAHkkxbxvX@bdtpg>
 <CAJpy0uBQgPf3R04RZp5BxfgABeDOyZdwm44hSOWA4JJraSo-_A@mail.gmail.com>
 <arOP6DEhgKcsNS5b@bdtpg>
 <CAJpy0uAiqT-__cV-MWNCQTXpAjcVDNh53+L2-DzZnYH8O5K1Vg@mail.gmail.com>
 <arP4wN5HW8h0Sr90@bdtpg>
 <CAJpy0uCeW9Kf1Ofgn=ojiG-FEpsf=KVgpF-289A_j9xX0G5AyA@mail.gmail.com>
 <CAJpy0uBDGfq8sermY3CE9M_LeBx+fAWsiKQLSZkpGeUOzEgcwg@mail.gmail.com>
 <arTwLinbxvVkEqu2@bdtpg>
 <CAJpy0uDASVicCkn-BJtecgpS081UXf4kG7UPicCB=cMzVJveMA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAJpy0uDASVicCkn-BJtecgpS081UXf4kG7UPicCB=cMzVJveMA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/arYModKZ2uLwUyES%40bdtpg>
Precedence: bulk

Hi,

On Fri, Sep 25, 2026 at 09:38:27AM +0530, shveta malik wrote:
> On Thu, Sep 24, 2026 at 3:11 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> 
> Thanks for addressing comments. A few concerns on 001:

Thanks for looking at it!

> 1)
> 
> In SaveSlotToPath(), should we add an 'Assert(cp.slotdata.restart_lsn
> == InvalidXLogRecPtr)' at the end for the 'if (clear_restart_lsn)'
> case?
> 
>                 slot->data.invalidated = invalidation_cause;
>                 if (clear_restart_lsn)
> +               {
> +                       Assert(cp.slotdata.restart_lsn == InvalidXLogRecPtr);
>                         slot->data.restart_lsn = InvalidXLogRecPtr;
> +               }
> 
> While slot->last_saved_restart_lsn correctly inherits
> cp.slotdata.restart_lsn on the next line, adding this Assert
> guarantees that the removed logic from
> InvalidatePossiblyObsoleteSlot() was successfully compensated for in
> the on-disk struct before we propagate it to shared memory. It is not
> mandatory, but it would be good to have.

I’m not sure this assertion adds much, since cp.slotdata.restart_lsn is explicitly
cleared above and is not modified afterward.

> 2)
> + Assert(update_inactive_since || slot->data.persistency == RS_PERSISTENT);
> 
> In ReplicationSlotReleaseInternal(), I didn’t quite understand the
> reasoning behind above Assert.  Does this mean that when the caller
> passes update_inactive_since=true, the slot can even be temporary,
> whereas if we are not updating inactive_since, the slot must be
> persistent?

Yes. In fact, with update_inactive_since=true it can also be ephemeral, since
ReplicationSlotRelease() uses that value for the ordinary release path.

This is not specific to slotsync. The false case is introduced by 0001 and is
only used to preserve inactive_since when rolling back ownership of an inactive
persistent slot.

Maybe the following comment would make that clearer?

"
  /*
   * Skipping the inactive_since update is only needed when undoing the
   * internal acquisition of an inactive persistent slot after an ERROR.
   */
"

> 3)
> Another doubt I have is that with above Assert, when
> update_inactive_since is TRUE, we are even allowing RS_EPHEMERAL
> slots. However, ReplicationSlotPersistInvalidation() explicitly
> disallows them in patch002 with:
> 
> Assert(slot->data.persistency != RS_EPHEMERAL);
> 
> Both checks are not in sync.

I think they apply to different scopes. ReplicationSlotReleaseInternal() is the
general release implementation, so update_inactive_since=true imposes no
persistency restriction. In particular, an ephemeral slot is dropped by that
path.

ReplicationSlotPersistInvalidation() has a narrower contract and is only
intended for persistent or temporary slots. That said, maybe its Assert could
express all the supported combinations more clearly?

"
  Assert(slot->data.persistency == RS_PERSISTENT ||
         (slot->data.persistency == RS_TEMPORARY &&
          update_inactive_since));
"

Regards,

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