Обсуждение: Clear logical slot's 'synced' flag on promotion of standby

Hi,

This is a spin-off thread from [1].

Currently, in the slot-sync worker, we have an error scenario [2]
where, during slot synchronization, if we detect a slot with the same
name and its synced flag is set to false, we emit an error. The
rationale is to avoid potentially overwriting a user-created slot.

But while analyzing [1], we observed that this error can lead to
inconsistent behavior during switchovers. On the first switchover, the
new standby logs an error: "Exiting from slot synchronization because
a slot with the same name already exists on the standby."   But during
a double switchover, this error does not occur.

Upon re-evaluating this, it seems more appropriate to clear the synced
flag after promotion, as the flag does not hold any meaning on the
primary. Doing so would ensure consistent behavior across all
switchovers, as the same error will be raised avoiding the risk of
overwriting user's slots.

A patch can be posted soon on the same idea.

[1]:
https://www.postgresql.org/message-id/CAJpy0uCZ-Z9Km-fGjXm9C90DoiF_EFe2SbCh9Aw7vnF-9K%2BJ_A%40mail.gmail.com

[2]:
                /* User-created slot with the same name exists, raise ERROR. */
                if (!synced)
                        ereport(ERROR,

errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                        errmsg("exiting from slot
synchronization because same"
                                                   " name slot \"%s\"
already exists on the standby",
                                                   remote_slot->name));

thanks
Shveta

On Tue, Sep 9, 2025 at 4:21 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Hi,
>
> This is a spin-off thread from [1].
>
> Currently, in the slot-sync worker, we have an error scenario [2]
> where, during slot synchronization, if we detect a slot with the same
> name and its synced flag is set to false, we emit an error. The
> rationale is to avoid potentially overwriting a user-created slot.
>
> But while analyzing [1], we observed that this error can lead to
> inconsistent behavior during switchovers. On the first switchover, the
> new standby logs an error: "Exiting from slot synchronization because
> a slot with the same name already exists on the standby."   But during
> a double switchover, this error does not occur.
>
> Upon re-evaluating this, it seems more appropriate to clear the synced
> flag after promotion, as the flag does not hold any meaning on the
> primary. Doing so would ensure consistent behavior across all
> switchovers, as the same error will be raised avoiding the risk of
> overwriting user's slots.
>
> A patch can be posted soon on the same idea.

Hi Shveta,

Here’s a patch that addresses this issue. It clears any “synced” flags
on logical replication slots when a standby is promoted. I’ve also
added handling for crashes; if the server crashes before the flags are
cleared, they are reset on restart.
The restart logic was a bit tricky, since I had to rely on the
database state to decide when the reset is needed. Documentation on
these states is sparse, but from my testing I found that
DB_IN_CRASH_RECOVERY occurs when a standby crashes during promotion.
That’s the state I use to trigger the flag reset on restart.

regards,
Ajin Cherian
Fujitsu Australia

Hi,

On Tue, Sep 9, 2025 at 12:53 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 4:21 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Hi,
> >
> > This is a spin-off thread from [1].
> >
> > Currently, in the slot-sync worker, we have an error scenario [2]
> > where, during slot synchronization, if we detect a slot with the same
> > name and its synced flag is set to false, we emit an error. The
> > rationale is to avoid potentially overwriting a user-created slot.
> >
> > But while analyzing [1], we observed that this error can lead to
> > inconsistent behavior during switchovers. On the first switchover, the
> > new standby logs an error: "Exiting from slot synchronization because
> > a slot with the same name already exists on the standby."   But during
> > a double switchover, this error does not occur.
> >
> > Upon re-evaluating this, it seems more appropriate to clear the synced
> > flag after promotion, as the flag does not hold any meaning on the
> > primary. Doing so would ensure consistent behavior across all
> > switchovers, as the same error will be raised avoiding the risk of
> > overwriting user's slots.
> >
> > A patch can be posted soon on the same idea.
>
> Hi Shveta,
>
> Here’s a patch that addresses this issue. It clears any “synced” flags
> on logical replication slots when a standby is promoted. I’ve also
> added handling for crashes; if the server crashes before the flags are
> cleared, they are reset on restart.
> The restart logic was a bit tricky, since I had to rely on the
> database state to decide when the reset is needed. Documentation on
> these states is sparse, but from my testing I found that
> DB_IN_CRASH_RECOVERY occurs when a standby crashes during promotion.
> That’s the state I use to trigger the flag reset on restart.
>

+ * required resources. Clear any leftover 'synced' flags on replication
+ * slots when in crash recovery on the primary. The DB_IN_CRASH_RECOVERY
+ * state check ensures that this code is only reached when a standby
+ * server crashes during promotion.
  */
  StartupReplicationSlots();
+ if (ControlFile->state == DB_IN_CRASH_RECOVERY)

I believe the primary server can also enter the DB_IN_CRASH_RECOVERY
state. For example, if the primary is already in crash recovery and
crashes again while in crash recovery, it will restart in the
DB_IN_CRASH_RECOVERY state, no?

--

With this change are we saying that on primary the synced flag must be
always false. Because the postgres doc on pg_replication_slots says:

"The value of this column has no meaning on the primary server; the
column value on the primary is default false for all slots but may (if
leftover from a promoted standby) also be true."

--
With Regards,
Ashutosh Sharma.

On Mon, Sep 8, 2025 at 11:21 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Hi,
>
> This is a spin-off thread from [1].
>
> Currently, in the slot-sync worker, we have an error scenario [2]
> where, during slot synchronization, if we detect a slot with the same
> name and its synced flag is set to false, we emit an error. The
> rationale is to avoid potentially overwriting a user-created slot.
>
> But while analyzing [1], we observed that this error can lead to
> inconsistent behavior during switchovers. On the first switchover, the
> new standby logs an error: "Exiting from slot synchronization because
> a slot with the same name already exists on the standby."   But during
> a double switchover, this error does not occur.
>
> Upon re-evaluating this, it seems more appropriate to clear the synced
> flag after promotion, as the flag does not hold any meaning on the
> primary. Doing so would ensure consistent behavior across all
> switchovers, as the same error will be raised avoiding the risk of
> overwriting user's slots.

There is the following comment in FinishWalRecovery():

/*
 * Shutdown the slot sync worker to drop any temporary slots acquired by
 * it and to prevent it from keep trying to fetch the failover slots.
 *
 * We do not update the 'synced' column in 'pg_replication_slots' system
 * view from true to false here, as any failed update could leave 'synced'
 * column false for some slots. This could cause issues during slot sync
 * after restarting the server as a standby. While updating the 'synced'
 * column after switching to the new timeline is an option, it does not
 * simplify the handling for the 'synced' column. Therefore, we retain the
 * 'synced' column as true after promotion as it may provide useful
 * information about the slot origin.
 */
ShutDownSlotSync();

Does the patch address the above concerns?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Sep 9, 2025 at 2:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
>
> + * required resources. Clear any leftover 'synced' flags on replication
> + * slots when in crash recovery on the primary. The DB_IN_CRASH_RECOVERY
> + * state check ensures that this code is only reached when a standby
> + * server crashes during promotion.
>   */
>   StartupReplicationSlots();
> + if (ControlFile->state == DB_IN_CRASH_RECOVERY)
>
> I believe the primary server can also enter the DB_IN_CRASH_RECOVERY
> state. For example, if the primary is already in crash recovery and
> crashes again while in crash recovery, it will restart in the
> DB_IN_CRASH_RECOVERY state, no?
>

Yes, good point. I think we can differentiate the two cases based on
the timeline change. A regular primary won't have a timeline change,
whereas a promoted standby that failed during promotion will show a
timeline change immediately upon restart. Thoughts?

In the worst-case scenario, even if we end up running the Reset
function during a regular primary's crash recovery, it shouldn't cause
any harm. (That said, I'm not suggesting we shouldn't fix it).  What
concerns me more is the possibility of running it on a regular
standby, as it could disrupt slot synchronization. I attempted to
simulate a scenario where a regular standby ends up in
DB_IN_CRASH_RECOVERY after a crash, but I couldn't reproduce it. Do
you know of any situation where this could happen? The absence of
comments for these states makes it challenging to follow the flow.

> --
>
> With this change are we saying that on primary the synced flag must be
> always false. Because the postgres doc on pg_replication_slots says:
>
> "The value of this column has no meaning on the primary server; the
> column value on the primary is default false for all slots but may (if
> leftover from a promoted standby) also be true."
>

The doc needs change.

thanks
Shveta

On Wed, Sep 10, 2025 at 5:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Sep 8, 2025 at 11:21 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Hi,
> >
> > This is a spin-off thread from [1].
> >
> > Currently, in the slot-sync worker, we have an error scenario [2]
> > where, during slot synchronization, if we detect a slot with the same
> > name and its synced flag is set to false, we emit an error. The
> > rationale is to avoid potentially overwriting a user-created slot.
> >
> > But while analyzing [1], we observed that this error can lead to
> > inconsistent behavior during switchovers. On the first switchover, the
> > new standby logs an error: "Exiting from slot synchronization because
> > a slot with the same name already exists on the standby."   But during
> > a double switchover, this error does not occur.
> >
> > Upon re-evaluating this, it seems more appropriate to clear the synced
> > flag after promotion, as the flag does not hold any meaning on the
> > primary. Doing so would ensure consistent behavior across all
> > switchovers, as the same error will be raised avoiding the risk of
> > overwriting user's slots.
>
> There is the following comment in FinishWalRecovery():
>
> /*
>  * Shutdown the slot sync worker to drop any temporary slots acquired by
>  * it and to prevent it from keep trying to fetch the failover slots.
>  *
>  * We do not update the 'synced' column in 'pg_replication_slots' system
>  * view from true to false here, as any failed update could leave 'synced'
>  * column false for some slots. This could cause issues during slot sync
>  * after restarting the server as a standby. While updating the 'synced'
>  * column after switching to the new timeline is an option, it does not
>  * simplify the handling for the 'synced' column. Therefore, we retain the
>  * 'synced' column as true after promotion as it may provide useful
>  * information about the slot origin.
>  */
> ShutDownSlotSync();
>
> Does the patch address the above concerns?
>

Yes, the patch is attempting to address the above concern. it is
trying to Reset synced-column after switching to a new timeline. There
is an issue though as pointed out by Ashutosh in [1], which needs to
be addressed.

[1]: https://www.postgresql.org/message-id/CAE9k0P%3DWXRHXLGxkegFLj9tVLrY45%2BuTtdgv%2BPjt1mqyit4zZw%40mail.gmail.com

thanks
Shveta

Hi,

On Thu, Sep 11, 2025 at 9:17 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 2:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi,
> >
> >
> > + * required resources. Clear any leftover 'synced' flags on replication
> > + * slots when in crash recovery on the primary. The DB_IN_CRASH_RECOVERY
> > + * state check ensures that this code is only reached when a standby
> > + * server crashes during promotion.
> >   */
> >   StartupReplicationSlots();
> > + if (ControlFile->state == DB_IN_CRASH_RECOVERY)
> >
> > I believe the primary server can also enter the DB_IN_CRASH_RECOVERY
> > state. For example, if the primary is already in crash recovery and
> > crashes again while in crash recovery, it will restart in the
> > DB_IN_CRASH_RECOVERY state, no?
> >
>
> Yes, good point. I think we can differentiate the two cases based on
> the timeline change. A regular primary won't have a timeline change,
> whereas a promoted standby that failed during promotion will show a
> timeline change immediately upon restart. Thoughts?

We already read the recovery signal files (standby.signal or
recovery.signal) at the start of StartupXLOG() via InitWalRecovery(),
which sets the StandbyModeRequested flag. Couldn’t we use this to
distinguish whether the server is a primary undergoing crash recovery
or a standby?

>
>
I attempted to
> simulate a scenario where a regular standby ends up in
> DB_IN_CRASH_RECOVERY after a crash, but I couldn't reproduce it. Do
> you know of any situation where this could happen? The absence of
> comments for these states makes it challenging to follow the flow.
>

The log message for "case DB_IN_CRASH_RECOVERY:" inside StartupXLOG
should indicate that the server has entered crash recovery, no? And..
If you still want the server to crash while in this state, you could
add your own PANIC or FATAL error message inside the startupxlog.

--
With Regards,
Ashutosh Sharma.

On Thu, Sep 11, 2025 at 9:17 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 2:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi,
> >
> >
> > + * required resources. Clear any leftover 'synced' flags on replication
> > + * slots when in crash recovery on the primary. The DB_IN_CRASH_RECOVERY
> > + * state check ensures that this code is only reached when a standby
> > + * server crashes during promotion.
> >   */
> >   StartupReplicationSlots();
> > + if (ControlFile->state == DB_IN_CRASH_RECOVERY)
> >
> > I believe the primary server can also enter the DB_IN_CRASH_RECOVERY
> > state. For example, if the primary is already in crash recovery and
> > crashes again while in crash recovery, it will restart in the
> > DB_IN_CRASH_RECOVERY state, no?
> >
>
> Yes, good point. I think we can differentiate the two cases based on
> the timeline change. A regular primary won't have a timeline change,
> whereas a promoted standby that failed during promotion will show a
> timeline change immediately upon restart. Thoughts?
>

Will there be any issues if we clear the sync status immediately after
the standby.signal file is removed from the standby server?

We could maybe introduce a temporary "promote.inprogress" marker file
on disk before removing standby.signal. The sequence would be:

1) Create promote.inprogress.
2) Unlink standby.signal
3) Clear the sync slot status.
4) Remove promote.inprogress.

This way, if the server crashes after standby.signal is removed but
before the sync status is cleared, the presence of promote.inprogress
would indicate that the standby was in the middle of promotion and
crashed before slot cleanup. On restart, we could use that marker to
detect the incomplete promotion and finish clearing the sync flags.

If the crash happens at a later stage, the server will no longer start
as a standby anyway, and by then the sync flags would already have
been reset.

This is just a thought and it may sound a bit naive. Let me know if I
am overlooking something.

--
With Regards,
Ashutosh Sharma.

On Thu, Sep 11, 2025 at 3:16 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
>
> We already read the recovery signal files (standby.signal or
> recovery.signal) at the start of StartupXLOG() via InitWalRecovery(),
> which sets the StandbyModeRequested flag. Couldn’t we use this to
> distinguish whether the server is a primary undergoing crash recovery
> or a standby?
>

The objective is not to distinguish between a primary and a standby
undergoing crash recovery, but to differentiate between a primary
undergoing crash recovery and a promoted standby (now the new primary)
during the immediate next startup—specifically in cases where the
promotion failed late in the process, such as during ResetSyncedSlots.
StandbyModeRequested will be false in both the cases and thus cannot
be used.

> >
> >
> I attempted to
> > simulate a scenario where a regular standby ends up in
> > DB_IN_CRASH_RECOVERY after a crash, but I couldn't reproduce it. Do
> > you know of any situation where this could happen? The absence of
> > comments for these states makes it challenging to follow the flow.
> >
>
> The log message for "case DB_IN_CRASH_RECOVERY:" inside StartupXLOG
> should indicate that the server has entered crash recovery, no? And..
> If you still want the server to crash while in this state, you could
> add your own PANIC or FATAL error message inside the startupxlog.
>

I think my query was not correctly understood. The intent was to know
if we can ever hit 'DB_IN_CRASH_RECOVERY' on a regular standby. But, I
think the answer is 'No'. We can hit it on primary (before , during,
or after promotion after a crash).

thanks
Shveta

On Thu, Sep 11, 2025 at 7:29 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Thu, Sep 11, 2025 at 9:17 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 2:19 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > >
> > > + * required resources. Clear any leftover 'synced' flags on replication
> > > + * slots when in crash recovery on the primary. The DB_IN_CRASH_RECOVERY
> > > + * state check ensures that this code is only reached when a standby
> > > + * server crashes during promotion.
> > >   */
> > >   StartupReplicationSlots();
> > > + if (ControlFile->state == DB_IN_CRASH_RECOVERY)
> > >
> > > I believe the primary server can also enter the DB_IN_CRASH_RECOVERY
> > > state. For example, if the primary is already in crash recovery and
> > > crashes again while in crash recovery, it will restart in the
> > > DB_IN_CRASH_RECOVERY state, no?
> > >
> >
> > Yes, good point. I think we can differentiate the two cases based on
> > the timeline change. A regular primary won't have a timeline change,
> > whereas a promoted standby that failed during promotion will show a
> > timeline change immediately upon restart. Thoughts?
> >
>
> Will there be any issues if we clear the sync status immediately after
> the standby.signal file is removed from the standby server?
>
> We could maybe introduce a temporary "promote.inprogress" marker file
> on disk before removing standby.signal. The sequence would be:
>
> 1) Create promote.inprogress.
> 2) Unlink standby.signal
> 3) Clear the sync slot status.
> 4) Remove promote.inprogress.
>
> This way, if the server crashes after standby.signal is removed but
> before the sync status is cleared, the presence of promote.inprogress
> would indicate that the standby was in the middle of promotion and
> crashed before slot cleanup. On restart, we could use that marker to
> detect the incomplete promotion and finish clearing the sync flags.
>
> If the crash happens at a later stage, the server will no longer start
> as a standby anyway, and by then the sync flags would already have
> been reset.
>
> This is just a thought and it may sound a bit naive. Let me know if I
> am overlooking something.
>

The approach seems valid and should work, but introducing a new file
like promote.inprogress for this purpose might be excessive. We can
first try analyzing existing information to determine whether we can
distinguish between the two scenarios -- a primary in crash recovery
immediately after a promotion attempt versus a regular primary. If we
are unable to find any way, we can revisit the idea.

thanks
Shveta

On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> The approach seems valid and should work, but introducing a new file
> like promote.inprogress for this purpose might be excessive. We can
> first try analyzing existing information to determine whether we can
> distinguish between the two scenarios -- a primary in crash recovery
> immediately after a promotion attempt versus a regular primary. If we
> are unable to find any way, we can revisit the idea.
>

I needed a way to reset slots not only during promotion, but also
after a crash that occurs while slots are being reset, so there would
be a fallback mechanism to clear them again on startup. As Shveta
pointed out, it wasn’t trivial to tell apart a standby restarting
after crashing during promotion from a primary restarting after a
crash. So I decided to just reset slots every time primary (or a
standby after promotion) restarts.

Because this fallback logic will run on every primary restart, it was
important to minimize overhead added by the patch. After some
discussion, I placed the reset logic in RestoreSlotFromDisk(), which
is invoked by StartupReplicationSlots() whenever the server starts.
Because RestoreSlotFromDisk() already loops through all slots, this
adds minimum extra work; but also ensures the synced flag is cleared
when running on a primary.

The next challenge was finding a reliable flag to distinguish
primaries from standbys, since we really don’t want to reset the flag
on a standby. I tested StandbyMode, RecoveryInProgress(), and
InRecovery. But during restarts, both RecoveryInProgress() and
InRecovery are always true on both primary and standby. In all my
testing, StandbyMode was the only variable that consistently
differentiated between the two, which is what I used.

I have also changed the documentation and comments regarding 'synced'
flags not being reset on the primary.

regards,
Ajin Cherian
Fujitsu Australia

Hi Ajin,

On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > The approach seems valid and should work, but introducing a new file
> > like promote.inprogress for this purpose might be excessive. We can
> > first try analyzing existing information to determine whether we can
> > distinguish between the two scenarios -- a primary in crash recovery
> > immediately after a promotion attempt versus a regular primary. If we
> > are unable to find any way, we can revisit the idea.
> >
>
> I needed a way to reset slots not only during promotion, but also
> after a crash that occurs while slots are being reset, so there would
> be a fallback mechanism to clear them again on startup. As Shveta
> pointed out, it wasn’t trivial to tell apart a standby restarting
> after crashing during promotion from a primary restarting after a
> crash. So I decided to just reset slots every time primary (or a
> standby after promotion) restarts.
>
> Because this fallback logic will run on every primary restart, it was
> important to minimize overhead added by the patch. After some
> discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> is invoked by StartupReplicationSlots() whenever the server starts.
> Because RestoreSlotFromDisk() already loops through all slots, this
> adds minimum extra work; but also ensures the synced flag is cleared
> when running on a primary.
>
> The next challenge was finding a reliable flag to distinguish
> primaries from standbys, since we really don’t want to reset the flag
> on a standby. I tested StandbyMode, RecoveryInProgress(), and
> InRecovery. But during restarts, both RecoveryInProgress() and
> InRecovery are always true on both primary and standby. In all my
> testing, StandbyMode was the only variable that consistently
> differentiated between the two, which is what I used.
>

+/*
+ * ResetSyncedSlots()
+ *
+ * Reset all replication slots that have synced=true to synced=false.
+ */

I feel this is not correct, we are force resetting sync flag status
for all logical slots, not just the one that is set to true.

--

@@ -2664,6 +2715,10 @@ RestoreSlotFromDisk(const char *name)
  memcpy(&slot->data, &cp.slotdata,
     sizeof(ReplicationSlotPersistentData));

+ /* reset synced flag if this is a primary server */
+ if (!StandbyMode)
+ slot->data.synced = false;
+

I think you also need to ensure that you are only doing this for the
logical slots, it's currently just checking if the slot is in-use or
not.

--
With Regards,
Ashutosh Sharma.

On Thu, Sep 18, 2025 at 5:20 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Ajin,
>
> On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > The approach seems valid and should work, but introducing a new file
> > > like promote.inprogress for this purpose might be excessive. We can
> > > first try analyzing existing information to determine whether we can
> > > distinguish between the two scenarios -- a primary in crash recovery
> > > immediately after a promotion attempt versus a regular primary. If we
> > > are unable to find any way, we can revisit the idea.
> > >
> >
> > I needed a way to reset slots not only during promotion, but also
> > after a crash that occurs while slots are being reset, so there would
> > be a fallback mechanism to clear them again on startup. As Shveta
> > pointed out, it wasn’t trivial to tell apart a standby restarting
> > after crashing during promotion from a primary restarting after a
> > crash. So I decided to just reset slots every time primary (or a
> > standby after promotion) restarts.
> >
> > Because this fallback logic will run on every primary restart, it was
> > important to minimize overhead added by the patch. After some
> > discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> > is invoked by StartupReplicationSlots() whenever the server starts.
> > Because RestoreSlotFromDisk() already loops through all slots, this
> > adds minimum extra work; but also ensures the synced flag is cleared
> > when running on a primary.
> >
> > The next challenge was finding a reliable flag to distinguish
> > primaries from standbys, since we really don’t want to reset the flag
> > on a standby. I tested StandbyMode, RecoveryInProgress(), and
> > InRecovery. But during restarts, both RecoveryInProgress() and
> > InRecovery are always true on both primary and standby. In all my
> > testing, StandbyMode was the only variable that consistently
> > differentiated between the two, which is what I used.
> >
>
> +/*
> + * ResetSyncedSlots()
> + *
> + * Reset all replication slots that have synced=true to synced=false.
> + */
>
> I feel this is not correct, we are force resetting sync flag status
> for all logical slots, not just the one that is set to true.
>

You may ignore this, it's actually resetting only the synced slots.
Sorry for the noise.

--
With Regards,
Ashutosh Sharma.

On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > The approach seems valid and should work, but introducing a new file
> > like promote.inprogress for this purpose might be excessive. We can
> > first try analyzing existing information to determine whether we can
> > distinguish between the two scenarios -- a primary in crash recovery
> > immediately after a promotion attempt versus a regular primary. If we
> > are unable to find any way, we can revisit the idea.
> >
>
> I needed a way to reset slots not only during promotion, but also
> after a crash that occurs while slots are being reset, so there would
> be a fallback mechanism to clear them again on startup. As Shveta
> pointed out, it wasn’t trivial to tell apart a standby restarting
> after crashing during promotion from a primary restarting after a
> crash. So I decided to just reset slots every time primary (or a
> standby after promotion) restarts.
>
> Because this fallback logic will run on every primary restart, it was
> important to minimize overhead added by the patch. After some
> discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> is invoked by StartupReplicationSlots() whenever the server starts.
> Because RestoreSlotFromDisk() already loops through all slots, this
> adds minimum extra work; but also ensures the synced flag is cleared
> when running on a primary.

+1 for the idea.  I would like to know what others think here.

> The next challenge was finding a reliable flag to distinguish
> primaries from standbys, since we really don’t want to reset the flag
> on a standby. I tested StandbyMode, RecoveryInProgress(), and
> InRecovery. But during restarts, both RecoveryInProgress() and
> InRecovery are always true on both primary and standby. In all my
> testing, StandbyMode was the only variable that consistently
> differentiated between the two, which is what I used.
>
> I have also changed the documentation and comments regarding 'synced'
> flags not being reset on the primary.
>

Please find a few comments:

1)
+ * Reset all replication slots that have synced=true to synced=false.

Can we please change it to:
Reset the synced flag to false for all replication slots where it is
currently true.

2)
I was wondering that since we reset the sync flag everytime we load
slots from disk , then do we even need ResetSyncedSlots() during
promotion? But I guess we still need it because even after promotion
(if not restarted), existing backend sessions stay alive and it makes
sense if they too see 'synced' as false after promotion. Is it worth
adding this in comments atop ResetSyncedSlots() call during promotion?

3)
+ if (!StandbyMode)
+ slot->data.synced = false;

a)
Do we need to mark the slot as dirty so it gets saved to disk on the
next chance?

I think ReplicationSlotSave can be skipped, as it may not be
appropriate in the restore flow. But marking the slot dirty is
important to avoid resetting the sync flag again on the next startup.
A crash between marking it dirty and persisting it would still require
a reset, but that seems acceptable. Thoughts?

b)
Also if we are marking it dirty, it makes sense to set synced to false
only after checking if synced is true already.

thanks
Shveta

On Thu, Sep 18, 2025 at 5:20 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Ajin,
>
> On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > The approach seems valid and should work, but introducing a new file
> > > like promote.inprogress for this purpose might be excessive. We can
> > > first try analyzing existing information to determine whether we can
> > > distinguish between the two scenarios -- a primary in crash recovery
> > > immediately after a promotion attempt versus a regular primary. If we
> > > are unable to find any way, we can revisit the idea.
> > >
> >
> > I needed a way to reset slots not only during promotion, but also
> > after a crash that occurs while slots are being reset, so there would
> > be a fallback mechanism to clear them again on startup. As Shveta
> > pointed out, it wasn’t trivial to tell apart a standby restarting
> > after crashing during promotion from a primary restarting after a
> > crash. So I decided to just reset slots every time primary (or a
> > standby after promotion) restarts.
> >
> > Because this fallback logic will run on every primary restart, it was
> > important to minimize overhead added by the patch. After some
> > discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> > is invoked by StartupReplicationSlots() whenever the server starts.
> > Because RestoreSlotFromDisk() already loops through all slots, this
> > adds minimum extra work; but also ensures the synced flag is cleared
> > when running on a primary.
> >
> > The next challenge was finding a reliable flag to distinguish
> > primaries from standbys, since we really don’t want to reset the flag
> > on a standby. I tested StandbyMode, RecoveryInProgress(), and
> > InRecovery. But during restarts, both RecoveryInProgress() and
> > InRecovery are always true on both primary and standby. In all my
> > testing, StandbyMode was the only variable that consistently
> > differentiated between the two, which is what I used.
> >
>
> +/*
> + * ResetSyncedSlots()
> + *
> + * Reset all replication slots that have synced=true to synced=false.
> + */
>
> I feel this is not correct, we are force resetting sync flag status
> for all logical slots, not just the one that is set to true.
>
> --
>
> @@ -2664,6 +2715,10 @@ RestoreSlotFromDisk(const char *name)
>   memcpy(&slot->data, &cp.slotdata,
>      sizeof(ReplicationSlotPersistentData));
>
> + /* reset synced flag if this is a primary server */
> + if (!StandbyMode)
> + slot->data.synced = false;
> +
>
> I think you also need to ensure that you are only doing this for the
> logical slots, it's currently just checking if the slot is in-use or
> not.
>

I think a better approach would be to reset synced only if it is
marked as synced. Adding a LogicalSlot check wouldn't be incorrect,
but IMO, it may not be necessary.

thanks
Shveta

On Fri, Sep 19, 2025 at 3:04 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Sep 18, 2025 at 5:20 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi Ajin,
> >
> > On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > The approach seems valid and should work, but introducing a new file
> > > > like promote.inprogress for this purpose might be excessive. We can
> > > > first try analyzing existing information to determine whether we can
> > > > distinguish between the two scenarios -- a primary in crash recovery
> > > > immediately after a promotion attempt versus a regular primary. If we
> > > > are unable to find any way, we can revisit the idea.
> > > >
> > >
> > > I needed a way to reset slots not only during promotion, but also
> > > after a crash that occurs while slots are being reset, so there would
> > > be a fallback mechanism to clear them again on startup. As Shveta
> > > pointed out, it wasn’t trivial to tell apart a standby restarting
> > > after crashing during promotion from a primary restarting after a
> > > crash. So I decided to just reset slots every time primary (or a
> > > standby after promotion) restarts.
> > >
> > > Because this fallback logic will run on every primary restart, it was
> > > important to minimize overhead added by the patch. After some
> > > discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> > > is invoked by StartupReplicationSlots() whenever the server starts.
> > > Because RestoreSlotFromDisk() already loops through all slots, this
> > > adds minimum extra work; but also ensures the synced flag is cleared
> > > when running on a primary.
> > >
> > > The next challenge was finding a reliable flag to distinguish
> > > primaries from standbys, since we really don’t want to reset the flag
> > > on a standby. I tested StandbyMode, RecoveryInProgress(), and
> > > InRecovery. But during restarts, both RecoveryInProgress() and
> > > InRecovery are always true on both primary and standby. In all my
> > > testing, StandbyMode was the only variable that consistently
> > > differentiated between the two, which is what I used.
> > >
> >
> > +/*
> > + * ResetSyncedSlots()
> > + *
> > + * Reset all replication slots that have synced=true to synced=false.
> > + */
> >
> > I feel this is not correct, we are force resetting sync flag status
> > for all logical slots, not just the one that is set to true.
> >
> > --
> >
> > @@ -2664,6 +2715,10 @@ RestoreSlotFromDisk(const char *name)
> >   memcpy(&slot->data, &cp.slotdata,
> >      sizeof(ReplicationSlotPersistentData));
> >
> > + /* reset synced flag if this is a primary server */
> > + if (!StandbyMode)
> > + slot->data.synced = false;
> > +
> >
> > I think you also need to ensure that you are only doing this for the
> > logical slots, it's currently just checking if the slot is in-use or
> > not.
> >
>
> I think a better approach would be to reset synced only if it is
> marked as synced. Adding a LogicalSlot check wouldn't be incorrect,
> but IMO, it may not be necessary.
>

Thinking further on this, I believe it’s fine even if the slot is
forcefully set to false on the primary. The slot type or its sync
status doesn’t really matter in this case.

--
With Regards,
Ashutosh Sharma.

On Fri, Sep 19, 2025 at 7:29 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Fri, Sep 19, 2025 at 3:04 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Thu, Sep 18, 2025 at 5:20 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > Hi Ajin,
> > >
> > > On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > > >
> > > > > The approach seems valid and should work, but introducing a new file
> > > > > like promote.inprogress for this purpose might be excessive. We can
> > > > > first try analyzing existing information to determine whether we can
> > > > > distinguish between the two scenarios -- a primary in crash recovery
> > > > > immediately after a promotion attempt versus a regular primary. If we
> > > > > are unable to find any way, we can revisit the idea.
> > > > >
> > > >
> > > > I needed a way to reset slots not only during promotion, but also
> > > > after a crash that occurs while slots are being reset, so there would
> > > > be a fallback mechanism to clear them again on startup. As Shveta
> > > > pointed out, it wasn’t trivial to tell apart a standby restarting
> > > > after crashing during promotion from a primary restarting after a
> > > > crash. So I decided to just reset slots every time primary (or a
> > > > standby after promotion) restarts.
> > > >
> > > > Because this fallback logic will run on every primary restart, it was
> > > > important to minimize overhead added by the patch. After some
> > > > discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> > > > is invoked by StartupReplicationSlots() whenever the server starts.
> > > > Because RestoreSlotFromDisk() already loops through all slots, this
> > > > adds minimum extra work; but also ensures the synced flag is cleared
> > > > when running on a primary.
> > > >
> > > > The next challenge was finding a reliable flag to distinguish
> > > > primaries from standbys, since we really don’t want to reset the flag
> > > > on a standby. I tested StandbyMode, RecoveryInProgress(), and
> > > > InRecovery. But during restarts, both RecoveryInProgress() and
> > > > InRecovery are always true on both primary and standby. In all my
> > > > testing, StandbyMode was the only variable that consistently
> > > > differentiated between the two, which is what I used.
> > > >
> > >
> > > +/*
> > > + * ResetSyncedSlots()
> > > + *
> > > + * Reset all replication slots that have synced=true to synced=false.
> > > + */
> > >
> > > I feel this is not correct, we are force resetting sync flag status
> > > for all logical slots, not just the one that is set to true.
> > >
> > > --
> > >
> > > @@ -2664,6 +2715,10 @@ RestoreSlotFromDisk(const char *name)
> > >   memcpy(&slot->data, &cp.slotdata,
> > >      sizeof(ReplicationSlotPersistentData));
> > >
> > > + /* reset synced flag if this is a primary server */
> > > + if (!StandbyMode)
> > > + slot->data.synced = false;
> > > +
> > >
> > > I think you also need to ensure that you are only doing this for the
> > > logical slots, it's currently just checking if the slot is in-use or
> > > not.
> > >
> >
> > I think a better approach would be to reset synced only if it is
> > marked as synced. Adding a LogicalSlot check wouldn't be incorrect,
> > but IMO, it may not be necessary.
> >
>
> Thinking further on this, I believe it’s fine even if the slot is
> forcefully set to false on the primary. The slot type or its sync
> status doesn’t really matter in this case.
>

I think we need to mark the slots dirty once we set its synced-flag to
false. In such a case, we should only mark those which actually needed
synced flag resetting. IMO, we need a 'synced' flag check here.

thanks
Shveta

On Mon, Sep 22, 2025 at 8:58 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Sep 19, 2025 at 7:29 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Fri, Sep 19, 2025 at 3:04 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Thu, Sep 18, 2025 at 5:20 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > > >
> > > > Hi Ajin,
> > > >
> > > > On Thu, Sep 18, 2025 at 4:16 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > > >
> > > > > On Fri, Sep 12, 2025 at 1:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > > > >
> > > > > > The approach seems valid and should work, but introducing a new file
> > > > > > like promote.inprogress for this purpose might be excessive. We can
> > > > > > first try analyzing existing information to determine whether we can
> > > > > > distinguish between the two scenarios -- a primary in crash recovery
> > > > > > immediately after a promotion attempt versus a regular primary. If we
> > > > > > are unable to find any way, we can revisit the idea.
> > > > > >
> > > > >
> > > > > I needed a way to reset slots not only during promotion, but also
> > > > > after a crash that occurs while slots are being reset, so there would
> > > > > be a fallback mechanism to clear them again on startup. As Shveta
> > > > > pointed out, it wasn’t trivial to tell apart a standby restarting
> > > > > after crashing during promotion from a primary restarting after a
> > > > > crash. So I decided to just reset slots every time primary (or a
> > > > > standby after promotion) restarts.
> > > > >
> > > > > Because this fallback logic will run on every primary restart, it was
> > > > > important to minimize overhead added by the patch. After some
> > > > > discussion, I placed the reset logic in RestoreSlotFromDisk(), which
> > > > > is invoked by StartupReplicationSlots() whenever the server starts.
> > > > > Because RestoreSlotFromDisk() already loops through all slots, this
> > > > > adds minimum extra work; but also ensures the synced flag is cleared
> > > > > when running on a primary.
> > > > >
> > > > > The next challenge was finding a reliable flag to distinguish
> > > > > primaries from standbys, since we really don’t want to reset the flag
> > > > > on a standby. I tested StandbyMode, RecoveryInProgress(), and
> > > > > InRecovery. But during restarts, both RecoveryInProgress() and
> > > > > InRecovery are always true on both primary and standby. In all my
> > > > > testing, StandbyMode was the only variable that consistently
> > > > > differentiated between the two, which is what I used.
> > > > >
> > > >
> > > > +/*
> > > > + * ResetSyncedSlots()
> > > > + *
> > > > + * Reset all replication slots that have synced=true to synced=false.
> > > > + */
> > > >
> > > > I feel this is not correct, we are force resetting sync flag status
> > > > for all logical slots, not just the one that is set to true.
> > > >
> > > > --
> > > >
> > > > @@ -2664,6 +2715,10 @@ RestoreSlotFromDisk(const char *name)
> > > >   memcpy(&slot->data, &cp.slotdata,
> > > >      sizeof(ReplicationSlotPersistentData));
> > > >
> > > > + /* reset synced flag if this is a primary server */
> > > > + if (!StandbyMode)
> > > > + slot->data.synced = false;
> > > > +
> > > >
> > > > I think you also need to ensure that you are only doing this for the
> > > > logical slots, it's currently just checking if the slot is in-use or
> > > > not.
> > > >
> > >
> > > I think a better approach would be to reset synced only if it is
> > > marked as synced. Adding a LogicalSlot check wouldn't be incorrect,
> > > but IMO, it may not be necessary.
> > >
> >
> > Thinking further on this, I believe it’s fine even if the slot is
> > forcefully set to false on the primary. The slot type or its sync
> > status doesn’t really matter in this case.
> >
>
> I think we need to mark the slots dirty once we set its synced-flag to
> false. In such a case, we should only mark those which actually needed
> synced flag resetting. IMO, we need a 'synced' flag check here.
>

Fair enough, point taken.

--
With Regards,
Ashutosh Sharma.

On Fri, Sep 19, 2025 at 7:26 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Please find a few comments:
>
> 1)
> + * Reset all replication slots that have synced=true to synced=false.
>
> Can we please change it to:
> Reset the synced flag to false for all replication slots where it is
> currently true.
>

Changed.

> 2)
> I was wondering that since we reset the sync flag everytime we load
> slots from disk , then do we even need ResetSyncedSlots() during
> promotion? But I guess we still need it because even after promotion
> (if not restarted), existing backend sessions stay alive and it makes
> sense if they too see 'synced' as false after promotion. Is it worth
> adding this in comments atop ResetSyncedSlots() call during promotion?
>

Yes, on promotion a server could run for a long time without ever
being restarted. Do we want slots to remain in the synced state until
the next restart? I think not. I think it's best to have the logic
both in the promotion path and in the restart path.
Added a comment in xlog.c for this.

> 3)
> + if (!StandbyMode)
> + slot->data.synced = false;
>
> a)
> Do we need to mark the slot as dirty so it gets saved to disk on the
> next chance?
>
> I think ReplicationSlotSave can be skipped, as it may not be
> appropriate in the restore flow. But marking the slot dirty is
> important to avoid resetting the sync flag again on the next startup.
> A crash between marking it dirty and persisting it would still require
> a reset, but that seems acceptable. Thoughts?
>
> b)
> Also if we are marking it dirty, it makes sense to set synced to false
> only after checking if synced is true already.

Changed this accordingly. Marking it as dirty also required acquiring
the slot and releasing it afterwards. I've also not checked for
logical slots, as if the synced flag is set, it should only be for
logical slots.

Attaching v3 with the above code changes.

regards,
Ajin Cherian
Fujitsu Australia

On Tue, Sep 23, 2025 at 1:11 PM Ajin Cherian <itsajin@gmail.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.

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.

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

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?

thanks
Shveta

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

I think this can stay as a LOG message, it only runs once at startup
and applies just to logical slots, so it won’t be noisy. I’d also
avoid mentioning “during promotion,” since the flag might accidentally
be set on the primary and then reset later during startup, making that
description inaccurate.

--
With Regards,
Ashutosh Sharma.

On Tue, Sep 23, 2025 at 11:11 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> > 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
> >
>
> I think this can stay as a LOG message, it only runs once at startup
> and applies just to logical slots, so it won’t be noisy. I’d also
> avoid mentioning “during promotion,” since the flag might accidentally
> be set on the primary and then reset later during startup, making that
> description inaccurate.

Oops! that was actually leftover debug logs that I used in my testing
but I am happy to leave it as DEBUG1 or LOG1 as necessary.

regards,
Ajin Cherian
Fujitsu Australia

On Tue, Sep 23, 2025 at 6:41 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> > 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
> >
>
> I think this can stay as a LOG message, it only runs once at startup
> and applies just to logical slots, so it won’t be noisy.

It does not run at startup, it runs during promotion. Having said
that, if there are a lot many slots, and we have one message per slot,
overall logs can still be more. I somehow find DEBUG better here. But
we can leave it as LOG and can revisit later when others review.

> I’d also
> avoid mentioning “during promotion,” since the flag might accidentally
> be set on the primary and then reset later during startup, making that
> description inaccurate.

This function is called only during promotion (see check 'if
(promoted)') and thus the suggested message ( “during promotion")
seems better to me.

Ajin, we can add comments atop ResetSyncedSlots() that this function
is invoked/used only during promotion, making the usage more clear.

thanks
Shveta

On Wed, Sep 24, 2025 at 9:42 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Sep 23, 2025 at 6:41 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > > 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
> > >
> >
> > I think this can stay as a LOG message, it only runs once at startup
> > and applies just to logical slots, so it won’t be noisy.
>
> It does not run at startup, it runs during promotion. Having said
> that, if there are a lot many slots, and we have one message per slot,
> overall logs can still be more. I somehow find DEBUG better here. But
> we can leave it as LOG and can revisit later when others review.
>
> > I’d also
> > avoid mentioning “during promotion,” since the flag might accidentally
> > be set on the primary and then reset later during startup, making that
> > description inaccurate.
>
> This function is called only during promotion (see check 'if
> (promoted)') and thus the suggested message ( “during promotion")
> seems better to me.
>

ResetSyncedSlots might be called during promotion, but
RestoreSlotFromDisk (which runs during standard PostgreSQL startup)
performs the same functionality. This creates a scenario where the
sync flag could be reset during either promotion or regular startup. I
think we should either remove it, or ensure it is present in both
places for consistency.

--
With Regards,
Ashutosh Sharma.

On Tue, Sep 23, 2025 at 7:54 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Sep 23, 2025 at 1:11 PM Ajin Cherian <itsajin@gmail.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

On Wed, Sep 24, 2025 at 10:18 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Wed, Sep 24, 2025 at 9:42 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Sep 23, 2025 at 6:41 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > >
> > > > 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
> > > >
> > >
> > > I think this can stay as a LOG message, it only runs once at startup
> > > and applies just to logical slots, so it won’t be noisy.
> >
> > It does not run at startup, it runs during promotion. Having said
> > that, if there are a lot many slots, and we have one message per slot,
> > overall logs can still be more. I somehow find DEBUG better here. But
> > we can leave it as LOG and can revisit later when others review.
> >
> > > I’d also
> > > avoid mentioning “during promotion,” since the flag might accidentally
> > > be set on the primary and then reset later during startup, making that
> > > description inaccurate.
> >
> > This function is called only during promotion (see check 'if
> > (promoted)') and thus the suggested message ( “during promotion")
> > seems better to me.
> >
>
> ResetSyncedSlots might be called during promotion, but
> RestoreSlotFromDisk (which runs during standard PostgreSQL startup)
> performs the same functionality. This creates a scenario where the
> sync flag could be reset during either promotion or regular startup. I
> think we should either remove it, or ensure it is present in both
> places for consistency.
>

Sorry I missed this email somehow. Yes, I agree. We can have DEBUG1 at
both places. But let's see what others think on this.

thanks
Shveta

On Fri, Sep 26, 2025 at 3:26 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> Attaching v4 which addresses all the above comments.
>

Few trivial comments:

1)
 # Confirm the synced slot 'lsub1_slot' is retained 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;}
+ q{SELECT count(*) = 2 FROM pg_replication_slots WHERE slot_name IN
('lsub1_slot', 'snap_test_slot') AND NOT synced AND NOT temporary;}
+
  ),
  't',
  'synced slot retained on the new primary');

a)
It is not fault of this patch, but I see comment and query not
matching. We shall have  both the names 'lsub1_slot', 'snap_test_slot'
in comment.

b) Also it will be good to mention the expectation from synced flag in
the comment. How about:

Confirm the synced slots 'lsub1_slot' and 'snap_test_slot' are
retained on the new primary and 'synced' flag is cleared on promotion.

2)
As Ashutosh suggested, even in RestoreSlotFromDisk(), we can have
DEBUG1 msg: "synced flag reset for replication slot \"%s\""

thanks
Shveta