agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: JoongHyuk Shin <sjh910805@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Rui Zhao <zhaorui126@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Date: Fri, 25 Sep 2026 05:54:41 +0000
Message-ID: <arYModKZ2uLwUyES@bdtpg> (raw)
In-Reply-To: <CAJpy0uDASVicCkn-BJtecgpS081UXf4kG7UPicCB=cMzVJveMA@mail.gmail.com>
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>
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
view thread (33+ messages) latest in thread
Message-ID: <arYModKZ2uLwUyES@bdtpg>
Permalink: ../arYModKZ2uLwUyES@bdtpg/
Also on: postgresql.org/message-id/arYModKZ2uLwUyES@bdtpg
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com, shveta.malik@gmail.com, sjh910805@gmail.com, amit.kapila16@gmail.com, zhaorui126@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
In-Reply-To: <arYModKZ2uLwUyES@bdtpg>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox