Re: Clear logical slot's 'synced' flag on promotion of standby

On Tue, Sep 23, 2025 at 7:54 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Tue, Sep 23, 2025 at 1:11 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Attaching v3 with the above code changes.
> >
>
> Thanks for the patch. Please find a few comments:
>
> 1)
> + if (!StandbyMode && slot->data.synced)
> + {
> + ReplicationSlotAcquire(NameStr(slot->data.name), false, true);
> + slot->data.synced = false;
> + ReplicationSlotMarkDirty();
> + ReplicationSlotRelease();
> + }
>
> Do we really need to acquire in this startup flow? Can we directly
> set the 'just_dirtied' and 'dirty' as true without calling
> ReplicationSlotMarkDirty(). IMO, there is no possibility of another
> session connection at this point right as we have not started up
> completely, and thus it should be okay to directly mark it as dirty
> without acquiring slot or taking locks.
>
I've modified it accordingly.
> 2)
> + * If this was a promotion, first reset any slots that had been marked as
> + * synced during standby mode. Although slots that are marked as synced
> + * are reset on a restart of the primary, we need to do it in the promotion
> + * path as it could be some time before the next restart.
>
> Shall we rephrase to:
> If this was a promotion, first reset the synced flag for any logical
> slots if it's set. Although the synced flag for logical slots is reset
> on every primary restart, we also need to handle it during promotion
> since existing backend sessions remain active even after promotion,
> and a restart may not happen for some time.
>
Changed.
> 3)
> + ereport(LOG,
> + (errmsg("reset synced flag for replication slot \"%s\"",
> + NameStr(s->data.name))));
>
> a) Shall we change it to DEBUG1?
> b) Shall the msg be:
> synced flag reset for replication slot \"%s\" during promotion
>
Changed to DEBUG1
> 4)
> Regarding:
> -# Confirm the synced slot 'lsub1_slot' is retained on the new primary
> +# Confirm the synced slot 'lsub1_slot' is reset on the new primary
> is( $standby1->safe_psql(
> 'postgres',
> q{SELECT count(*) = 2 FROM pg_replication_slots WHERE slot_name IN
> ('lsub1_slot', 'snap_test_slot') AND synced AND NOT temporary;}
> ),
> - 't',
> - 'synced slot retained on the new primary');
> + 'f',
> + 'synced slot reset on the new primary');
>
> I think the original test was trying to confirm that both the logical
> slots are retained after promotion. See test-title:
> # Promote the standby1 to primary. Confirm that:
> # a) the slot 'lsub1_slot' and 'snap_test_slot' are retained on the new primary
>
> But with this change, it will not be able to verify that. Shall we
> modify the test to say 'Confirm that the slots 'lsub1_slot' and
> 'snap_test_slot' are retained on the new primary and synced flag is
> cleared' and change the query to have 'NOT synced'. Thoughts?
>
Changed.

Attaching v4 which addresses all the above comments.

regards,
Ajin Cherian
Fujitsu Australia