On Wed, Sep 23, 2026 at 9:35 PM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Wed, Sep 23, 2026 at 04:07:02PM +0530, shveta malik wrote:
> > Can we create the helper function for this logic as it is not core to
> > the sync-function. Please use the attached patch if you agree, and
> > feel free to change comments as you see apt.
>
> I was initially a bit skeptical about adding a helper with only one caller,
> as it
> seemed to just move the PG_TRY() block elsewhere without reducing duplication.
>
> On second thought, I agree it makes sense here, as it keeps the lock
> acquisition
> and required ERROR cleanup ordering together. There are also similar "one
> caller"
> helpers wrapping PG_TRY() blocks, such as start_table_sync() and
> start_sequence_sync().
>
> +static void
> +persist_slot_invalidation(ReplicationSlot *slot,
> ReplicationSlotInvalidationCause cause)
> +{
>
> I changed it slightly to derive slot from MyReplicationSlot, ensuring that
> the I/O
> lock belongs to the same slot that ReplicationSlotPersistInvalidation() and
> and ReplicationSlotRelease() are using. This is also consistent with other
> operations
> on the currently acquired slot.
Yes, that makes sense.
> Please find v5 attached.
Code changes looks good to me. Regarding the test, I have one trivial
comment: I think we should cover all the cases:
a) Failed sync: Memory is NOT updated.
b) Successful sync: Memory IS updated.
c) Server restart: Memory is STILL updated (because it was recovered
from disk).
I think b) is not covered. It will be good to check the memory state
before standby-stop here:
$standby->safe_psql('postgres', 'SELECT pg_sync_replication_slots()');
$standby->stop('immediate');
$standby->start;
thanks
Shveta
