Обсуждение: Re: Improve pg_sync_replication_slots() to wait for primary to advance
On Tue, Jun 24, 2025 at 4:11 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Hello,
>
> Creating this thread for a POC based on discussions in thread [1].
> Hou-san had created this patch, and I just cleaned up some documents,
> did some testing and now sharing the patch here.
>
> In this patch, the pg_sync_replication_slots() API now waits
> indefinitely for the remote slot to catch up. We could later add a
> timeout parameter to control maximum wait time if this approach seems
> acceptable. If there are more ideas on improving this patch, let me
> know.
+1 on the idea.
I believe the timeout option may not be necessary here, since the API
can be manually canceled if needed. Otherwise, the recommended
approach is to let it complete. But I would like to know what others
think here.
Few comments:
1)
When the API is waiting for the primary to advance, standby fails to
handle promotion requests. Promotion fails:
./pg_ctl -D ../../standbydb/ promote -w
waiting for server to promote.................stopped waiting
pg_ctl: server did not promote in time
See the logs at [1]
2)
Also when the API is waiting for a long time, it just dumps the
'waiting for remote_slot..' LOG only once. Do you think it makes sense
to log it at a regular interval until the wait is over? See logs at
[1]. It dumped the log once in 3minutes.
3)
+ /*
+ * It is possible to get null value for restart_lsn if the slot is
+ * invalidated on the primary server, so handle accordingly.
+ */
+ if (new_invalidated || XLogRecPtrIsInvalid(new_restart_lsn))
+ {
+ /*
+ * The slot won't be persisted by the caller; it will be cleaned up
+ * at the end of synchronization.
+ */
+ ereport(WARNING,
+ errmsg("aborting initial sync for slot \"%s\"",
+ remote_slot->name),
+ errdetail("This slot was invalidated on the primary server."));
Which case are we referring to here where null restart_lsn would mean
invalidation? Can you please point me to such code where it happens or
a test-case which does that. I tried a few invalidation cases, but did
not hit it.
[1]:
Log file:
2025-07-02 14:38:09.851 IST [153187] LOG: waiting for remote slot
"failover_slot" LSN (0/3003F60) and catalog xmin (754) to pass local
slot LSN (0/3003F60) and catalog xmin (767)
2025-07-02 14:38:09.851 IST [153187] STATEMENT: SELECT
pg_sync_replication_slots();
2025-07-02 14:41:36.200 IST [153164] LOG: received promote request
thanks
Shveta
Please find few more comments:
1)
In pg_sync_replication_slots() doc, we have this:
"Note that this function is primarily intended for testing and
debugging purposes and should be used with caution. Additionally, this
function cannot be executed if ...."
We can get rid of this info as well and change to:
"Note that this function cannot be executed if...."
2)
We got rid of NOTE in logicaldecoding.sgml, but now the page does not
mention pg_sync_replication_slots() at all. We need to bring back the
change removed by [1] (or something on similar line) which is this:
- <command>CREATE SUBSCRIPTION</command> during slot creation, and
then calling
- <link linkend="pg-sync-replication-slots">
- <function>pg_sync_replication_slots</function></link>
- on the standby. By setting <link linkend="guc-sync-replication-slots">
+ <command>CREATE SUBSCRIPTION</command> during slot creation.
+ Additionally, enabling <link linkend="guc-sync-replication-slots">
+ <varname>sync_replication_slots</varname></link> on the standby
+ is required. By enabling <link linkend="guc-sync-replication-slots">
3)
wait_for_primary_slot_catchup():
+ /*
+ * It is possible to get null values for confirmed_lsn and
+ * catalog_xmin if on the primary server the slot is just created with
+ * a valid restart_lsn and slot-sync worker has fetched the slot
+ * before the primary server could set valid confirmed_lsn and
+ * catalog_xmin.
+ */
Do we need this special handling? We already have one such handling in
synchronize_slots(). please see:
/*
* If restart_lsn, confirmed_lsn or catalog_xmin is
invalid but the
* slot is valid, that means we have fetched the
remote_slot in its
* RS_EPHEMERAL state. In such a case, don't sync it;
we can always
* sync it in the next sync cycle when the remote_slot
is persisted
* and has valid lsn(s) and xmin values.
*/
if ((XLogRecPtrIsInvalid(remote_slot->restart_lsn) ||
XLogRecPtrIsInvalid(remote_slot->confirmed_lsn) ||
!TransactionIdIsValid(remote_slot->catalog_xmin)) &&
remote_slot->invalidated == RS_INVAL_NONE)
pfree(remote_slot);
Due to the above check in synchronize_slots(), we will not reach
wait_for_primary_slot_catchup() when any of confirmed_lsn or
catalog_xmin is not initialized.
[1]: https://www.postgresql.org/message-id/CAJpy0uAD_La2vi%2BB%2BiSBbCYTMayMstvbF9ndrAJysL9t5fHtbQ%40mail.gmail.com
thanks
Shveta
On Wed, Jul 2, 2025 at 7:56 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Few comments:
>
> 1)
> When the API is waiting for the primary to advance, standby fails to
> handle promotion requests. Promotion fails:
> ./pg_ctl -D ../../standbydb/ promote -w
> waiting for server to promote.................stopped waiting
> pg_ctl: server did not promote in time
>
> See the logs at [1]
>
I've modified this to handle promotion request and stop slot
synchronization if standby is promoted.
> 2)
> Also when the API is waiting for a long time, it just dumps the
> 'waiting for remote_slot..' LOG only once. Do you think it makes sense
> to log it at a regular interval until the wait is over? See logs at
> [1]. It dumped the log once in 3minutes.
>
I've modified it to log once every 10 seconds.
> 3)
> + /*
> + * It is possible to get null value for restart_lsn if the slot is
> + * invalidated on the primary server, so handle accordingly.
> + */
>
> + if (new_invalidated || XLogRecPtrIsInvalid(new_restart_lsn))
> + {
> + /*
> + * The slot won't be persisted by the caller; it will be cleaned up
> + * at the end of synchronization.
> + */
> + ereport(WARNING,
> + errmsg("aborting initial sync for slot \"%s\"",
> + remote_slot->name),
> + errdetail("This slot was invalidated on the primary server."));
>
> Which case are we referring to here where null restart_lsn would mean
> invalidation? Can you please point me to such code where it happens or
> a test-case which does that. I tried a few invalidation cases, but did
> not hit it.
>
I've removed all this code as the checks for null restart_lsn and
other parameters are handled in earlier functions and we won't get
here if these had null, I've added asserts to check that it is not
null.
On Wed, Jul 9, 2025 at 2:53 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Please find few more comments:
>
> 1)
> In pg_sync_replication_slots() doc, we have this:
>
> "Note that this function is primarily intended for testing and
> debugging purposes and should be used with caution. Additionally, this
> function cannot be executed if ...."
>
> We can get rid of this info as well and change to:
>
> "Note that this function cannot be executed if...."
Modified as requested.
>
> 2)
> We got rid of NOTE in logicaldecoding.sgml, but now the page does not
> mention pg_sync_replication_slots() at all. We need to bring back the
> change removed by [1] (or something on similar line) which is this:
>
> - <command>CREATE SUBSCRIPTION</command> during slot creation, and
> then calling
> - <link linkend="pg-sync-replication-slots">
> - <function>pg_sync_replication_slots</function></link>
> - on the standby. By setting <link linkend="guc-sync-replication-slots">
> + <command>CREATE SUBSCRIPTION</command> during slot creation.
> + Additionally, enabling <link linkend="guc-sync-replication-slots">
> + <varname>sync_replication_slots</varname></link> on the standby
> + is required. By enabling <link linkend="guc-sync-replication-slots">
>
I've added that back.
> 3)
> wait_for_primary_slot_catchup():
> + /*
> + * It is possible to get null values for confirmed_lsn and
> + * catalog_xmin if on the primary server the slot is just created with
> + * a valid restart_lsn and slot-sync worker has fetched the slot
> + * before the primary server could set valid confirmed_lsn and
> + * catalog_xmin.
> + */
>
> Do we need this special handling? We already have one such handling in
> synchronize_slots(). please see:
> /*
> * If restart_lsn, confirmed_lsn or catalog_xmin is
> invalid but the
> * slot is valid, that means we have fetched the
> remote_slot in its
> * RS_EPHEMERAL state. In such a case, don't sync it;
> we can always
> * sync it in the next sync cycle when the remote_slot
> is persisted
> * and has valid lsn(s) and xmin values.
> */
> if ((XLogRecPtrIsInvalid(remote_slot->restart_lsn) ||
> XLogRecPtrIsInvalid(remote_slot->confirmed_lsn) ||
> !TransactionIdIsValid(remote_slot->catalog_xmin)) &&
> remote_slot->invalidated == RS_INVAL_NONE)
> pfree(remote_slot);
>
>
> Due to the above check in synchronize_slots(), we will not reach
> wait_for_primary_slot_catchup() when any of confirmed_lsn or
> catalog_xmin is not initialized.
>
Yes, you are correct. I've removed all that logic.
The modified patch (v2) is attached.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Wed, Jul 16, 2025 at 3:00 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Jul 2, 2025 at 7:56 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Few comments:
> >
> > 1)
> > When the API is waiting for the primary to advance, standby fails to
> > handle promotion requests. Promotion fails:
> > ./pg_ctl -D ../../standbydb/ promote -w
> > waiting for server to promote.................stopped waiting
> > pg_ctl: server did not promote in time
> >
> > See the logs at [1]
> >
>
> I've modified this to handle promotion request and stop slot
> synchronization if standby is promoted.
>
>
> > 2)
> > Also when the API is waiting for a long time, it just dumps the
> > 'waiting for remote_slot..' LOG only once. Do you think it makes sense
> > to log it at a regular interval until the wait is over? See logs at
> > [1]. It dumped the log once in 3minutes.
> >
>
> I've modified it to log once every 10 seconds.
>
> > 3)
> > + /*
> > + * It is possible to get null value for restart_lsn if the slot is
> > + * invalidated on the primary server, so handle accordingly.
> > + */
> >
> > + if (new_invalidated || XLogRecPtrIsInvalid(new_restart_lsn))
> > + {
> > + /*
> > + * The slot won't be persisted by the caller; it will be cleaned up
> > + * at the end of synchronization.
> > + */
> > + ereport(WARNING,
> > + errmsg("aborting initial sync for slot \"%s\"",
> > + remote_slot->name),
> > + errdetail("This slot was invalidated on the primary server."));
> >
> > Which case are we referring to here where null restart_lsn would mean
> > invalidation? Can you please point me to such code where it happens or
> > a test-case which does that. I tried a few invalidation cases, but did
> > not hit it.
> >
>
> I've removed all this code as the checks for null restart_lsn and
> other parameters are handled in earlier functions and we won't get
> here if these had null, I've added asserts to check that it is not
> null.
>
>
> On Wed, Jul 9, 2025 at 2:53 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Please find few more comments:
> >
> > 1)
> > In pg_sync_replication_slots() doc, we have this:
> >
> > "Note that this function is primarily intended for testing and
> > debugging purposes and should be used with caution. Additionally, this
> > function cannot be executed if ...."
> >
> > We can get rid of this info as well and change to:
> >
> > "Note that this function cannot be executed if...."
>
> Modified as requested.
>
>
> >
> > 2)
> > We got rid of NOTE in logicaldecoding.sgml, but now the page does not
> > mention pg_sync_replication_slots() at all. We need to bring back the
> > change removed by [1] (or something on similar line) which is this:
> >
> > - <command>CREATE SUBSCRIPTION</command> during slot creation, and
> > then calling
> > - <link linkend="pg-sync-replication-slots">
> > - <function>pg_sync_replication_slots</function></link>
> > - on the standby. By setting <link linkend="guc-sync-replication-slots">
> > + <command>CREATE SUBSCRIPTION</command> during slot creation.
> > + Additionally, enabling <link linkend="guc-sync-replication-slots">
> > + <varname>sync_replication_slots</varname></link> on the standby
> > + is required. By enabling <link linkend="guc-sync-replication-slots">
> >
>
> I've added that back.
>
>
> > 3)
> > wait_for_primary_slot_catchup():
> > + /*
> > + * It is possible to get null values for confirmed_lsn and
> > + * catalog_xmin if on the primary server the slot is just created with
> > + * a valid restart_lsn and slot-sync worker has fetched the slot
> > + * before the primary server could set valid confirmed_lsn and
> > + * catalog_xmin.
> > + */
> >
> > Do we need this special handling? We already have one such handling in
> > synchronize_slots(). please see:
> > /*
> > * If restart_lsn, confirmed_lsn or catalog_xmin is
> > invalid but the
> > * slot is valid, that means we have fetched the
> > remote_slot in its
> > * RS_EPHEMERAL state. In such a case, don't sync it;
> > we can always
> > * sync it in the next sync cycle when the remote_slot
> > is persisted
> > * and has valid lsn(s) and xmin values.
> > */
> > if ((XLogRecPtrIsInvalid(remote_slot->restart_lsn) ||
> > XLogRecPtrIsInvalid(remote_slot->confirmed_lsn) ||
> > !TransactionIdIsValid(remote_slot->catalog_xmin)) &&
> > remote_slot->invalidated == RS_INVAL_NONE)
> > pfree(remote_slot);
> >
> >
> > Due to the above check in synchronize_slots(), we will not reach
> > wait_for_primary_slot_catchup() when any of confirmed_lsn or
> > catalog_xmin is not initialized.
> >
>
> Yes, you are correct. I've removed all that logic.
>
> The modified patch (v2) is attached.
>
I am not able to apply the patch to the latest head or even to a week
back version. Can you please check and rebase?
thanks
Shveta
> I am not able to apply the patch to the latest head or even to a week > back version. Can you please check and rebase? > > thanks > Shveta Rebased. Regards, Ajin Cherian Fujitsu Australia.
Вложения
On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > I am not able to apply the patch to the latest head or even to a week
> > back version. Can you please check and rebase?
> >
> > thanks
> > Shveta
>
> Rebased.
>
Thanks. Please find a few comments:
1)
/* Any slot with NULL in these fields should not have made it this far */
It is good to get rid of the case where we had checks for NULL
confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
state), as that has already been checked by synchronize_slots() and
such a slot will not even reach wait_for_primary_slot_catchup(). But a
slot can still be invalidated on primary anytime, and thus during this
wait, we should check for primary's invalidation as we were doing in
v1.
2)
+ * If in SQL API synchronization, and we've been promoted, then no point
extra space before promoted.
3)
+ if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
checked at the beginning of this function.
4)
+ ereport(WARNING,
+ errmsg("aborting sync for slot \"%s\"",
+ remote_slot->name),
+ errdetail("Promotion occurred before this slot was fully"
+ " synchronized."));
+ pfree(cmd.data);
+
+ return false;
a) Please add an error-code.
b) Shall we change msg to
errmsg("aborting sync for slot \"%s\"",
remote_slot->name),
errhint("%s cannot be executed once promotion is
triggered.",
"pg_sync_replication_slots()")));
5)
Instead of using PromoteIsTriggered, shall we rely on
'SlotSyncCtx->stopSignaled' as we do when we start this API.
6)
In logicaldecoding.sgml, we can get rid of "Additionally, enabling
sync_replication_slots on the standby is required" to make it same as
what we had prior to the patch I pointed earlier.
Or better we can refine it to below. Thoughts?
The logical replication slots on the primary can be enabled for
synchronization to the hot standby by using the failover parameter of
pg_create_logical_replication_slot, or by using the failover option of
CREATE SUBSCRIPTION during slot creation. After that, synchronization
can be performed either manually by calling pg_sync_replication_slots
on the standby, or automatically by enabling sync_replication_slots on
the standby. When sync_replication_slots is enabled, the failover
slots are periodically synchronized by the slot sync worker. For the
synchronization to work, .....
thanks
Shveta
On Thu, Jul 17, 2025 at 9:34 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > I am not able to apply the patch to the latest head or even to a week
> > > back version. Can you please check and rebase?
> > >
> > > thanks
> > > Shveta
> >
> > Rebased.
> >
>
> Thanks. Please find a few comments:
>
>
> 1)
> /* Any slot with NULL in these fields should not have made it this far */
>
> It is good to get rid of the case where we had checks for NULL
> confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> state), as that has already been checked by synchronize_slots() and
> such a slot will not even reach wait_for_primary_slot_catchup(). But a
> slot can still be invalidated on primary anytime, and thus during this
> wait, we should check for primary's invalidation as we were doing in
> v1.
>
> 2)
> + * If in SQL API synchronization, and we've been promoted, then no point
>
> extra space before promoted.
>
> 3)
>
> + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
>
> We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> checked at the beginning of this function.
>
> 4)
> + ereport(WARNING,
> + errmsg("aborting sync for slot \"%s\"",
> + remote_slot->name),
> + errdetail("Promotion occurred before this slot was fully"
> + " synchronized."));
> + pfree(cmd.data);
> +
> + return false;
>
> a) Please add an error-code.
>
> b) Shall we change msg to
>
> errmsg("aborting sync for slot \"%s\"",
> remote_slot->name),
> errhint("%s cannot be executed once promotion is
> triggered.",
>
> "pg_sync_replication_slots()")));
>
>
>
> 5)
> Instead of using PromoteIsTriggered, shall we rely on
> 'SlotSyncCtx->stopSignaled' as we do when we start this API.
>
> 6)
> In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> sync_replication_slots on the standby is required" to make it same as
> what we had prior to the patch I pointed earlier.
>
> Or better we can refine it to below. Thoughts?
>
> The logical replication slots on the primary can be enabled for
> synchronization to the hot standby by using the failover parameter of
> pg_create_logical_replication_slot, or by using the failover option of
> CREATE SUBSCRIPTION during slot creation. After that, synchronization
> can be performed either manually by calling pg_sync_replication_slots
> on the standby, or automatically by enabling sync_replication_slots on
> the standby. When sync_replication_slots is enabled, the failover
> slots are periodically synchronized by the slot sync worker. For the
> synchronization to work, .....
I am wondering if we should provide an optional parameter to
pg_sync_replication_slots(), to control whether to wait for the slot
to be synced or just return with ERROR as it is doing now, default can
be wait.
--
Regards,
Dilip Kumar
Google
On Fri, Jul 18, 2025 at 10:14 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jul 17, 2025 at 9:34 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > > I am not able to apply the patch to the latest head or even to a week
> > > > back version. Can you please check and rebase?
> > > >
> > > > thanks
> > > > Shveta
> > >
> > > Rebased.
> > >
> >
> > Thanks. Please find a few comments:
> >
> >
> > 1)
> > /* Any slot with NULL in these fields should not have made it this far */
> >
> > It is good to get rid of the case where we had checks for NULL
> > confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> > state), as that has already been checked by synchronize_slots() and
> > such a slot will not even reach wait_for_primary_slot_catchup(). But a
> > slot can still be invalidated on primary anytime, and thus during this
> > wait, we should check for primary's invalidation as we were doing in
> > v1.
> >
> > 2)
> > + * If in SQL API synchronization, and we've been promoted, then no point
> >
> > extra space before promoted.
> >
> > 3)
> >
> > + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
> >
> > We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> > checked at the beginning of this function.
> >
> > 4)
> > + ereport(WARNING,
> > + errmsg("aborting sync for slot \"%s\"",
> > + remote_slot->name),
> > + errdetail("Promotion occurred before this slot was fully"
> > + " synchronized."));
> > + pfree(cmd.data);
> > +
> > + return false;
> >
> > a) Please add an error-code.
> >
> > b) Shall we change msg to
> >
> > errmsg("aborting sync for slot \"%s\"",
> > remote_slot->name),
> > errhint("%s cannot be executed once promotion is
> > triggered.",
> >
> > "pg_sync_replication_slots()")));
> >
> >
> >
> > 5)
> > Instead of using PromoteIsTriggered, shall we rely on
> > 'SlotSyncCtx->stopSignaled' as we do when we start this API.
> >
> > 6)
> > In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> > sync_replication_slots on the standby is required" to make it same as
> > what we had prior to the patch I pointed earlier.
> >
> > Or better we can refine it to below. Thoughts?
> >
> > The logical replication slots on the primary can be enabled for
> > synchronization to the hot standby by using the failover parameter of
> > pg_create_logical_replication_slot, or by using the failover option of
> > CREATE SUBSCRIPTION during slot creation. After that, synchronization
> > can be performed either manually by calling pg_sync_replication_slots
> > on the standby, or automatically by enabling sync_replication_slots on
> > the standby. When sync_replication_slots is enabled, the failover
> > slots are periodically synchronized by the slot sync worker. For the
> > synchronization to work, .....
>
> I am wondering if we should provide an optional parameter to
> pg_sync_replication_slots(), to control whether to wait for the slot
> to be synced or just return with ERROR as it is doing now, default can
> be wait.
>
Do you mean specifically in case of promotion or in general, as in do
not wait for primary to catch-up (anytime) and exit and drop the
temporary slot while exiting?
thanks
Shveta
On Fri, Jul 18, 2025 at 10:45 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Jul 18, 2025 at 10:14 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Jul 17, 2025 at 9:34 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > > I am not able to apply the patch to the latest head or even to a week
> > > > > back version. Can you please check and rebase?
> > > > >
> > > > > thanks
> > > > > Shveta
> > > >
> > > > Rebased.
> > > >
> > >
> > > Thanks. Please find a few comments:
> > >
> > >
> > > 1)
> > > /* Any slot with NULL in these fields should not have made it this far */
> > >
> > > It is good to get rid of the case where we had checks for NULL
> > > confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> > > state), as that has already been checked by synchronize_slots() and
> > > such a slot will not even reach wait_for_primary_slot_catchup(). But a
> > > slot can still be invalidated on primary anytime, and thus during this
> > > wait, we should check for primary's invalidation as we were doing in
> > > v1.
> > >
> > > 2)
> > > + * If in SQL API synchronization, and we've been promoted, then no point
> > >
> > > extra space before promoted.
> > >
> > > 3)
> > >
> > > + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
> > >
> > > We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> > > checked at the beginning of this function.
> > >
> > > 4)
> > > + ereport(WARNING,
> > > + errmsg("aborting sync for slot \"%s\"",
> > > + remote_slot->name),
> > > + errdetail("Promotion occurred before this slot was fully"
> > > + " synchronized."));
> > > + pfree(cmd.data);
> > > +
> > > + return false;
> > >
> > > a) Please add an error-code.
> > >
> > > b) Shall we change msg to
> > >
> > > errmsg("aborting sync for slot \"%s\"",
> > > remote_slot->name),
> > > errhint("%s cannot be executed once promotion is
> > > triggered.",
> > >
> > > "pg_sync_replication_slots()")));
> > >
> > >
> > >
> > > 5)
> > > Instead of using PromoteIsTriggered, shall we rely on
> > > 'SlotSyncCtx->stopSignaled' as we do when we start this API.
> > >
> > > 6)
> > > In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> > > sync_replication_slots on the standby is required" to make it same as
> > > what we had prior to the patch I pointed earlier.
> > >
> > > Or better we can refine it to below. Thoughts?
> > >
> > > The logical replication slots on the primary can be enabled for
> > > synchronization to the hot standby by using the failover parameter of
> > > pg_create_logical_replication_slot, or by using the failover option of
> > > CREATE SUBSCRIPTION during slot creation. After that, synchronization
> > > can be performed either manually by calling pg_sync_replication_slots
> > > on the standby, or automatically by enabling sync_replication_slots on
> > > the standby. When sync_replication_slots is enabled, the failover
> > > slots are periodically synchronized by the slot sync worker. For the
> > > synchronization to work, .....
> >
> > I am wondering if we should provide an optional parameter to
> > pg_sync_replication_slots(), to control whether to wait for the slot
> > to be synced or just return with ERROR as it is doing now, default can
> > be wait.
> >
>
> Do you mean specifically in case of promotion or in general, as in do
> not wait for primary to catch-up (anytime) and exit and drop the
> temporary slot while exiting?
I am specifically pointing to the exposed function
pg_sync_replication_slots() which was earlier non-blocking and was
giving an error if the primary slot for not catch-up, so we have
improved the functionality in this thread by making it wait for
primary to catch-up instead of just throwing an error. But my
question was since this is a user facing function so shall we keep the
old behavior intact with some optional parameters so that if the user
chooses not to wait they have options to do that?
--
Regards,
Dilip Kumar
Google
On Fri, Jul 18, 2025 at 10:52 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Jul 18, 2025 at 10:45 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Jul 18, 2025 at 10:14 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, Jul 17, 2025 at 9:34 AM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > > >
> > > > > > I am not able to apply the patch to the latest head or even to a week
> > > > > > back version. Can you please check and rebase?
> > > > > >
> > > > > > thanks
> > > > > > Shveta
> > > > >
> > > > > Rebased.
> > > > >
> > > >
> > > > Thanks. Please find a few comments:
> > > >
> > > >
> > > > 1)
> > > > /* Any slot with NULL in these fields should not have made it this far */
> > > >
> > > > It is good to get rid of the case where we had checks for NULL
> > > > confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> > > > state), as that has already been checked by synchronize_slots() and
> > > > such a slot will not even reach wait_for_primary_slot_catchup(). But a
> > > > slot can still be invalidated on primary anytime, and thus during this
> > > > wait, we should check for primary's invalidation as we were doing in
> > > > v1.
> > > >
> > > > 2)
> > > > + * If in SQL API synchronization, and we've been promoted, then no point
> > > >
> > > > extra space before promoted.
> > > >
> > > > 3)
> > > >
> > > > + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
> > > >
> > > > We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> > > > checked at the beginning of this function.
> > > >
> > > > 4)
> > > > + ereport(WARNING,
> > > > + errmsg("aborting sync for slot \"%s\"",
> > > > + remote_slot->name),
> > > > + errdetail("Promotion occurred before this slot was fully"
> > > > + " synchronized."));
> > > > + pfree(cmd.data);
> > > > +
> > > > + return false;
> > > >
> > > > a) Please add an error-code.
> > > >
> > > > b) Shall we change msg to
> > > >
> > > > errmsg("aborting sync for slot \"%s\"",
> > > > remote_slot->name),
> > > > errhint("%s cannot be executed once promotion is
> > > > triggered.",
> > > >
> > > > "pg_sync_replication_slots()")));
> > > >
> > > >
> > > >
> > > > 5)
> > > > Instead of using PromoteIsTriggered, shall we rely on
> > > > 'SlotSyncCtx->stopSignaled' as we do when we start this API.
> > > >
> > > > 6)
> > > > In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> > > > sync_replication_slots on the standby is required" to make it same as
> > > > what we had prior to the patch I pointed earlier.
> > > >
> > > > Or better we can refine it to below. Thoughts?
> > > >
> > > > The logical replication slots on the primary can be enabled for
> > > > synchronization to the hot standby by using the failover parameter of
> > > > pg_create_logical_replication_slot, or by using the failover option of
> > > > CREATE SUBSCRIPTION during slot creation. After that, synchronization
> > > > can be performed either manually by calling pg_sync_replication_slots
> > > > on the standby, or automatically by enabling sync_replication_slots on
> > > > the standby. When sync_replication_slots is enabled, the failover
> > > > slots are periodically synchronized by the slot sync worker. For the
> > > > synchronization to work, .....
> > >
> > > I am wondering if we should provide an optional parameter to
> > > pg_sync_replication_slots(), to control whether to wait for the slot
> > > to be synced or just return with ERROR as it is doing now, default can
> > > be wait.
> > >
> >
> > Do you mean specifically in case of promotion or in general, as in do
> > not wait for primary to catch-up (anytime) and exit and drop the
> > temporary slot while exiting?
>
> I am specifically pointing to the exposed function
> pg_sync_replication_slots() which was earlier non-blocking and was
> giving an error if the primary slot for not catch-up, so we have
> improved the functionality in this thread by making it wait for
> primary to catch-up instead of just throwing an error. But my
> question was since this is a user facing function so shall we keep the
> old behavior intact with some optional parameters so that if the user
> chooses not to wait they have options to do that?
>
Okay. I see your point. Yes, it was non-blocking earlier but it was
not giving ERROR, it was just dumping in logilfe that primary is
behind and thus slot-sync could not be done.
If we continue using the non-blocking mode, there’s a risk that the
API may never successfully sync the slots. This is because it
eventually drops the temporary slot on exit, and when it tries to
create a new one later on subsequent call, it’s likely that the new
slot will again be ahead of the primary. This may happen if we have
continuous ongoing writes on the primary and the logical slot is not
being consumed at the same pace.
My preference would be to avoid including such an option as it is
confusing. With such an option in place, users may think that
slot-sync is completed while that may not be the case. But if it's
necessary for backward compatibility, it should be okay to provide it
as a non-default option as you suggested. Would like to know what
others think of this.
thanks
Shveta
On Fri, Jul 18, 2025 at 11:25 AM shveta malik <shveta.malik@gmail.com> wrote: > > Okay. I see your point. Yes, it was non-blocking earlier but it was > not giving ERROR, it was just dumping in logilfe that primary is > behind and thus slot-sync could not be done. > > If we continue using the non-blocking mode, there’s a risk that the > API may never successfully sync the slots. This is because it > eventually drops the temporary slot on exit, and when it tries to > create a new one later on subsequent call, it’s likely that the new > slot will again be ahead of the primary. This may happen if we have > continuous ongoing writes on the primary and the logical slot is not > being consumed at the same pace. > > My preference would be to avoid including such an option as it is > confusing. With such an option in place, users may think that > slot-sync is completed while that may not be the case. Fair enough But if it's > necessary for backward compatibility, it should be okay to provide it > as a non-default option as you suggested. Would like to know what > others think of this. I think we don't need to maintain backward compatibility here as it was not behaving sanely before, so I am fine with providing the behaviour we are doing with the patch. -- Regards, Dilip Kumar Google
On Fri, Jul 18, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Jul 18, 2025 at 11:25 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > Okay. I see your point. Yes, it was non-blocking earlier but it was > > not giving ERROR, it was just dumping in logilfe that primary is > > behind and thus slot-sync could not be done. > > > > If we continue using the non-blocking mode, there’s a risk that the > > API may never successfully sync the slots. This is because it > > eventually drops the temporary slot on exit, and when it tries to > > create a new one later on subsequent call, it’s likely that the new > > slot will again be ahead of the primary. This may happen if we have > > continuous ongoing writes on the primary and the logical slot is not > > being consumed at the same pace. > > > > My preference would be to avoid including such an option as it is > > confusing. With such an option in place, users may think that > > slot-sync is completed while that may not be the case. > > Fair enough > I think if we want we may return bool and return false when sync is not complete say due to promotion or other reason like timeout. However, at this stage it is not very clear whether it will be useful to provide additional timeout parameter. But we can consider retruning true/false depending on whether we are successful in syncing the slots or not. -- With Regards, Amit Kapila.
On Sat, Jul 19, 2025 at 5:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Jul 18, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Jul 18, 2025 at 11:25 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > Okay. I see your point. Yes, it was non-blocking earlier but it was > > > not giving ERROR, it was just dumping in logilfe that primary is > > > behind and thus slot-sync could not be done. > > > > > > If we continue using the non-blocking mode, there’s a risk that the > > > API may never successfully sync the slots. This is because it > > > eventually drops the temporary slot on exit, and when it tries to > > > create a new one later on subsequent call, it’s likely that the new > > > slot will again be ahead of the primary. This may happen if we have > > > continuous ongoing writes on the primary and the logical slot is not > > > being consumed at the same pace. > > > > > > My preference would be to avoid including such an option as it is > > > confusing. With such an option in place, users may think that > > > slot-sync is completed while that may not be the case. > > > > Fair enough > > > > I think if we want we may return bool and return false when sync is > not complete say due to promotion or other reason like timeout. > However, at this stage it is not very clear whether it will be useful > to provide additional timeout parameter. But we can consider retruning > true/false depending on whether we are successful in syncing the slots > or not. I am not very sure if in the current scenario, such a return-value will have any value addition. Since this function will be waiting indefinitely until all the slots are synced, it is supposed to return true in such normal scenarios. If it is interrupted by promotion or user cancels it manually, then it is supposed to return false. But in those cases, a more helpful approach would be to log a clear WARNING or ERROR message like "sync interrupted by promotion" (or similar reasons), rather than relying on a return value. In future, if we plan to add a timeout-parameter, then this return value makes more sense as in normal scenarios as well, as it can easily return false if the timeout value is short or the number of slots are huge or are stuck waiting on primary. Additionally, if we do return a value, there may be an expectation that the API should also provide details on the list of slots that couldn't be synced. That could introduce unnecessary complexity at this stage. We can avoid it for now and consider adding such enhancements later if we receive relevant customer feedback. Please note that our recommended approach for syncing slots still remains the 'slot sync worker' method. thanks Shveta
On Mon, Jul 21, 2025 at 10:08 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Sat, Jul 19, 2025 at 5:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Jul 18, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Fri, Jul 18, 2025 at 11:25 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > Okay. I see your point. Yes, it was non-blocking earlier but it was > > > > not giving ERROR, it was just dumping in logilfe that primary is > > > > behind and thus slot-sync could not be done. > > > > > > > > If we continue using the non-blocking mode, there’s a risk that the > > > > API may never successfully sync the slots. This is because it > > > > eventually drops the temporary slot on exit, and when it tries to > > > > create a new one later on subsequent call, it’s likely that the new > > > > slot will again be ahead of the primary. This may happen if we have > > > > continuous ongoing writes on the primary and the logical slot is not > > > > being consumed at the same pace. > > > > > > > > My preference would be to avoid including such an option as it is > > > > confusing. With such an option in place, users may think that > > > > slot-sync is completed while that may not be the case. > > > > > > Fair enough > > > > > > > I think if we want we may return bool and return false when sync is > > not complete say due to promotion or other reason like timeout. > > However, at this stage it is not very clear whether it will be useful > > to provide additional timeout parameter. But we can consider retruning > > true/false depending on whether we are successful in syncing the slots > > or not. > > I am not very sure if in the current scenario, such a return-value > will have any value addition. Since this function will be waiting > indefinitely until all the slots are synced, it is supposed to return > true in such normal scenarios. If it is interrupted by promotion or > user cancels it manually, then it is supposed to return false. But in > those cases, a more helpful approach would be to log a clear WARNING > or ERROR message like "sync interrupted by promotion" (or similar > reasons), rather than relying on a return value. In future, if we plan > to add a timeout-parameter, then this return value makes more sense as > in normal scenarios as well, as it can easily return false if the > timeout value is short or the number of slots are huge or are stuck > waiting on primary. > > Additionally, if we do return a value, there may be an expectation > that the API should also provide details on the list of slots that > couldn't be synced. That could introduce unnecessary complexity at > this stage. We can avoid it for now and consider adding such > enhancements later if we receive relevant customer feedback. > makes sense. > Please > note that our recommended approach for syncing slots still remains the > 'slot sync worker' method. > Right. -- With Regards, Amit Kapila.
On Mon, Jul 21, 2025 at 10:08 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Sat, Jul 19, 2025 at 5:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Jul 18, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Fri, Jul 18, 2025 at 11:25 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > Okay. I see your point. Yes, it was non-blocking earlier but it was > > > > not giving ERROR, it was just dumping in logilfe that primary is > > > > behind and thus slot-sync could not be done. > > > > > > > > If we continue using the non-blocking mode, there’s a risk that the > > > > API may never successfully sync the slots. This is because it > > > > eventually drops the temporary slot on exit, and when it tries to > > > > create a new one later on subsequent call, it’s likely that the new > > > > slot will again be ahead of the primary. This may happen if we have > > > > continuous ongoing writes on the primary and the logical slot is not > > > > being consumed at the same pace. > > > > > > > > My preference would be to avoid including such an option as it is > > > > confusing. With such an option in place, users may think that > > > > slot-sync is completed while that may not be the case. > > > > > > Fair enough > > > > > > > I think if we want we may return bool and return false when sync is > > not complete say due to promotion or other reason like timeout. > > However, at this stage it is not very clear whether it will be useful > > to provide additional timeout parameter. But we can consider retruning > > true/false depending on whether we are successful in syncing the slots > > or not. > > I am not very sure if in the current scenario, such a return-value > will have any value addition. Since this function will be waiting > indefinitely until all the slots are synced, it is supposed to return > true in such normal scenarios. If it is interrupted by promotion or > user cancels it manually, then it is supposed to return false. But in > those cases, a more helpful approach would be to log a clear WARNING > or ERROR message like "sync interrupted by promotion" (or similar > reasons), rather than relying on a return value. In future, if we plan > to add a timeout-parameter, then this return value makes more sense as > in normal scenarios as well, as it can easily return false if the > timeout value is short or the number of slots are huge or are stuck > waiting on primary. > Additionally, if we do return a value, there may be an expectation > that the API should also provide details on the list of slots that > couldn't be synced. That could introduce unnecessary complexity at > this stage. We can avoid it for now and consider adding such > enhancements later if we receive relevant customer feedback. Please > note that our recommended approach for syncing slots still remains the > 'slot sync worker' method. +1 -- Regards, Dilip Kumar Google
On Thu, Jul 17, 2025 at 2:04 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > I am not able to apply the patch to the latest head or even to a week
> > > back version. Can you please check and rebase?
> > >
> > > thanks
> > > Shveta
> >
> > Rebased.
> >
>
> Thanks. Please find a few comments:
>
>
> 1)
> /* Any slot with NULL in these fields should not have made it this far */
>
> It is good to get rid of the case where we had checks for NULL
> confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> state), as that has already been checked by synchronize_slots() and
> such a slot will not even reach wait_for_primary_slot_catchup(). But a
> slot can still be invalidated on primary anytime, and thus during this
> wait, we should check for primary's invalidation as we were doing in
> v1.
>
I've added back the check for invalidated slots.
> 2)
> + * If in SQL API synchronization, and we've been promoted, then no point
>
> extra space before promoted.
Fixed.
>
> 3)
>
> + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
>
> We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> checked at the beginning of this function.
>
Fixed.
> 4)
> + ereport(WARNING,
> + errmsg("aborting sync for slot \"%s\"",
> + remote_slot->name),
> + errdetail("Promotion occurred before this slot was fully"
> + " synchronized."));
> + pfree(cmd.data);
> +
> + return false;
>
> a) Please add an error-code.
>
> b) Shall we change msg to
>
> errmsg("aborting sync for slot \"%s\"",
> remote_slot->name),
> errhint("%s cannot be executed once promotion is
> triggered.",
>
> "pg_sync_replication_slots()")));
>
>
Since there is already an error return in the start if promotion is
triggered, I've kept the same error code and message here as well for
consistency.
>
> 5)
> Instead of using PromoteIsTriggered, shall we rely on
> 'SlotSyncCtx->stopSignaled' as we do when we start this API.
>
Fixed.
> 6)
> In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> sync_replication_slots on the standby is required" to make it same as
> what we had prior to the patch I pointed earlier.
>
> Or better we can refine it to below. Thoughts?
>
> The logical replication slots on the primary can be enabled for
> synchronization to the hot standby by using the failover parameter of
> pg_create_logical_replication_slot, or by using the failover option of
> CREATE SUBSCRIPTION during slot creation. After that, synchronization
> can be performed either manually by calling pg_sync_replication_slots
> on the standby, or automatically by enabling sync_replication_slots on
> the standby. When sync_replication_slots is enabled, the failover
> slots are periodically synchronized by the slot sync worker. For the
> synchronization to work, .....
>
Updated as above.
Patch v3 attached.
Regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Thu, Jul 31, 2025 at 3:11 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> Patch v3 attached.
>
Thanks for the patch. I tested it, please find a few comments:
1)
it hits an assert
(slotsync_reread_config()-->Assert(sync_replication_slots)) when API
is trying to sync and is in wait loop while in another session, I
enable sync_replication_slots using:
ALTER SYSTEM SET sync_replication_slots = 'on';
SELECT pg_reload_conf();
Assert:
025-08-01 10:55:43.637 IST [118576] STATEMENT: SELECT
pg_sync_replication_slots();
2025-08-01 10:55:51.730 IST [118563] LOG: received SIGHUP, reloading
configuration files
2025-08-01 10:55:51.731 IST [118563] LOG: parameter
"sync_replication_slots" changed to "on"
TRAP: failed Assert("sync_replication_slots"), File: "slotsync.c",
Line: 1334, PID: 118576
postgres: shveta postgres [local]
SELECT(ExceptionalCondition+0xbb)[0x61df0160e090]
postgres: shveta postgres [local] SELECT(+0x6520dc)[0x61df0133a0dc]
2025-08-01 10:55:51.739 IST [118666] ERROR: cannot synchronize
replication slots concurrently
postgres: shveta postgres [local] SELECT(+0x6522b2)[0x61df0133a2b2]
postgres: shveta postgres [local] SELECT(+0x650664)[0x61df01338664]
postgres: shveta postgres [local] SELECT(+0x650cf8)[0x61df01338cf8]
postgres: shveta postgres [local] SELECT(+0x6513ea)[0x61df013393ea]
postgres: shveta postgres [local] SELECT(+0x6519df)[0x61df013399df]
postgres: shveta postgres [local]
SELECT(SyncReplicationSlots+0xbb)[0x61df0133af60]
postgres: shveta postgres [local]
SELECT(pg_sync_replication_slots+0x1b1)[0x61df01357e52]
2)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot synchronize replication slots when"
+ " standby promotion is ongoing")));
I think better error message will be:
"exiting from slot synchronization as promotion is triggered"
This will be better suited in log file as well after below wait statements:
LOG: continuing to wait for remote slot "failover_slot" LSN
(0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060)
and catalog xmin (757)
STATEMENT: SELECT pg_sync_replication_slots();
3)
API dumps this when it is waiting for primary:
----
LOG: could not synchronize replication slot "failover_slot2"
DETAIL: Synchronization could lead to data loss, because the remote
slot needs WAL at LSN 0/03066E70 and catalog xmin 755, but the standby
has LSN 0/03066E70 and catalog xmin 770.
STATEMENT: SELECT pg_sync_replication_slots();
LOG: waiting for remote slot "failover_slot2" LSN (0/3066E70) and
catalog xmin (755) to pass local slot LSN (0/3066E70) and catalog xmin
(770)
STATEMENT: SELECT pg_sync_replication_slots();
LOG: continuing to wait for remote slot "failover_slot2" LSN
(0/3066E70) and catalog xmin (755) to pass local slot LSN (0/3066E70)
and catalog xmin (770)
STATEMENT: SELECT pg_sync_replication_slots();
----
Unsure if we shall still dump 'could not synchronize..' when it is
going to retry until it succeeds? The concerned log gives a feeling
that we are done trying and could not synchronize it. What do you
think?
thanks
Shveta
On Fri, Aug 1, 2025 at 12:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Jul 31, 2025 at 3:11 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > Patch v3 attached.
> >
>
> Thanks for the patch. I tested it, please find a few comments:
>
>
> 1)
> it hits an assert
> (slotsync_reread_config()-->Assert(sync_replication_slots)) when API
> is trying to sync and is in wait loop while in another session, I
> enable sync_replication_slots using:
>
> ALTER SYSTEM SET sync_replication_slots = 'on';
> SELECT pg_reload_conf();
>
> Assert:
> 025-08-01 10:55:43.637 IST [118576] STATEMENT: SELECT
> pg_sync_replication_slots();
> 2025-08-01 10:55:51.730 IST [118563] LOG: received SIGHUP, reloading
> configuration files
> 2025-08-01 10:55:51.731 IST [118563] LOG: parameter
> "sync_replication_slots" changed to "on"
> TRAP: failed Assert("sync_replication_slots"), File: "slotsync.c",
> Line: 1334, PID: 118576
> postgres: shveta postgres [local]
> SELECT(ExceptionalCondition+0xbb)[0x61df0160e090]
> postgres: shveta postgres [local] SELECT(+0x6520dc)[0x61df0133a0dc]
> 2025-08-01 10:55:51.739 IST [118666] ERROR: cannot synchronize
> replication slots concurrently
> postgres: shveta postgres [local] SELECT(+0x6522b2)[0x61df0133a2b2]
> postgres: shveta postgres [local] SELECT(+0x650664)[0x61df01338664]
> postgres: shveta postgres [local] SELECT(+0x650cf8)[0x61df01338cf8]
> postgres: shveta postgres [local] SELECT(+0x6513ea)[0x61df013393ea]
> postgres: shveta postgres [local] SELECT(+0x6519df)[0x61df013399df]
> postgres: shveta postgres [local]
> SELECT(SyncReplicationSlots+0xbb)[0x61df0133af60]
> postgres: shveta postgres [local]
> SELECT(pg_sync_replication_slots+0x1b1)[0x61df01357e52]
>
> 2)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot synchronize replication slots when"
> + " standby promotion is ongoing")));
>
> I think better error message will be:
> "exiting from slot synchronization as promotion is triggered"
>
> This will be better suited in log file as well after below wait statements:
> LOG: continuing to wait for remote slot "failover_slot" LSN
> (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060)
> and catalog xmin (757)
> STATEMENT: SELECT pg_sync_replication_slots();
>
> 3)
> API dumps this when it is waiting for primary:
>
> ----
> LOG: could not synchronize replication slot "failover_slot2"
> DETAIL: Synchronization could lead to data loss, because the remote
> slot needs WAL at LSN 0/03066E70 and catalog xmin 755, but the standby
> has LSN 0/03066E70 and catalog xmin 770.
> STATEMENT: SELECT pg_sync_replication_slots();
> LOG: waiting for remote slot "failover_slot2" LSN (0/3066E70) and
> catalog xmin (755) to pass local slot LSN (0/3066E70) and catalog xmin
> (770)
> STATEMENT: SELECT pg_sync_replication_slots();
> LOG: continuing to wait for remote slot "failover_slot2" LSN
> (0/3066E70) and catalog xmin (755) to pass local slot LSN (0/3066E70)
> and catalog xmin (770)
> STATEMENT: SELECT pg_sync_replication_slots();
> ----
>
> Unsure if we shall still dump 'could not synchronize..' when it is
> going to retry until it succeeds? The concerned log gives a feeling
> that we are done trying and could not synchronize it. What do you
> think?
>
A few more comments:
4)
+ if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ {
+ ereport(WARNING,
+ errmsg("aborting initial sync for slot \"%s\"",
+ remote_slot->name),
+ errdetail("This slot was not found on the primary server."));
+
+ pfree(cmd.data);
+ walrcv_clear_result(res);
+
+ return false;
+ }
We may have 'aborting sync for slot', can remove 'initial'.
5)
I tried a test where there were 4 slots on the publisher, where one
was getting used while the others were not. Initiated
pg_sync_replication_slots on standby. Forced unused slots to be
invalidated by setting idle_replication_slot_timeout=60 on primary,
due to which API finished but gave a warning:
postgres=# SELECT pg_sync_replication_slots();
WARNING: aborting initial sync for slot "failover_slot"
DETAIL: This slot was invalidated on the primary server.
WARNING: aborting initial sync for slot "failover_slot2"
DETAIL: This slot was invalidated on the primary server.
WARNING: aborting initial sync for slot "failover_slot3"
DETAIL: This slot was invalidated on the primary server.
pg_sync_replication_slots
---------------------------
(1 row)
Do we need these warnings here? I think we can have it as a LOG rather
than having it on console. Thoughts?
If we inclined towards WARNING here, will ti be better to have it as
a single line:
WARNING: aborting sync for slot "failover_slot" as the slot was
invalidated on primary
WARNING: aborting sync for slot "failover_slot1" as the slot was
invalidated on primary
WARNING: aborting sync for slot "failover_slot2" as the slot was
invalidated on primary
6)
- * We do not drop the slot because the restart_lsn can be ahead of the
- * current location when recreating the slot in the next cycle. It may
- * take more time to create such a slot. Therefore, we keep this slot
- * and attempt the synchronization in the next cycle.
+ * If we're in the slotsync worker, we retain the slot and retry in the
+ * next cycle. The restart_lsn might advance by then, allowing the slot
+ * to be created successfully later.
*/
I like the previous command better as it was conveying the side-effect
of dropping the slot herer. Can we try to incorporate the previous
comment here and make it specific to slotsync workers?
thanks
Shveta
On Fri, Aug 1, 2025 at 2:50 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> 5)
> I tried a test where there were 4 slots on the publisher, where one
> was getting used while the others were not. Initiated
> pg_sync_replication_slots on standby. Forced unused slots to be
> invalidated by setting idle_replication_slot_timeout=60 on primary,
> due to which API finished but gave a warning:
>
> postgres=# SELECT pg_sync_replication_slots();
> WARNING: aborting initial sync for slot "failover_slot"
> DETAIL: This slot was invalidated on the primary server.
> WARNING: aborting initial sync for slot "failover_slot2"
> DETAIL: This slot was invalidated on the primary server.
> WARNING: aborting initial sync for slot "failover_slot3"
> DETAIL: This slot was invalidated on the primary server.
> pg_sync_replication_slots
> ---------------------------
>
> (1 row)
>
> Do we need these warnings here? I think we can have it as a LOG rather
> than having it on console. Thoughts?
>
What is the behaviour of a slotsync worker in the same case? I don't
see any such WARNING messages in the code of slotsync worker, so why
do we want a different behaviour here?
Few other comments:
======================
1.
update_and_persist_local_synced_slot()
{
...
+ /*
+ * For SQL API synchronization, we wait for the remote slot to catch up
+ * here, since we can't assume the SQL API will be called again soon.
+ * We will retry the sync once the slot catches up.
+ *
+ * Note: This will return false if a promotion is triggered on the
+ * standby while waiting, in which case we stop syncing and drop the
+ * temporary slot.
+ */
+ if (!wait_for_primary_slot_catchup(wrconn, remote_slot))
+ return false;
Why is the wait added at this level? Shouldn't it be at API level aka
in SyncReplicationSlots() or pg_sync_replication_slots() similar to
what we do in ReplSlotSyncWorkerMain() for slotsync workers?
2.
REPLICATION_SLOTSYNC_MAIN "Waiting in main loop of slot sync worker."
...
+REPLICATION_SLOTSYNC_PRIMARY_CATCHUP "Waiting for the primary to catch-up."
Can't we reuse existing waitevent REPLICATION_SLOTSYNC_MAIN? We may
want to change the description. Is there a specific reason to add a
new wait_event for this API?
--
With Regards,
Amit Kapila.
On Mon, Aug 4, 2025 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Aug 1, 2025 at 2:50 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > 5) > > I tried a test where there were 4 slots on the publisher, where one > > was getting used while the others were not. Initiated > > pg_sync_replication_slots on standby. Forced unused slots to be > > invalidated by setting idle_replication_slot_timeout=60 on primary, > > due to which API finished but gave a warning: > > > > postgres=# SELECT pg_sync_replication_slots(); > > WARNING: aborting initial sync for slot "failover_slot" > > DETAIL: This slot was invalidated on the primary server. > > WARNING: aborting initial sync for slot "failover_slot2" > > DETAIL: This slot was invalidated on the primary server. > > WARNING: aborting initial sync for slot "failover_slot3" > > DETAIL: This slot was invalidated on the primary server. > > pg_sync_replication_slots > > --------------------------- > > > > (1 row) > > > > Do we need these warnings here? I think we can have it as a LOG rather > > than having it on console. Thoughts? > > > > What is the behaviour of a slotsync worker in the same case? I don't > see any such WARNING messages in the code of slotsync worker, so why > do we want a different behaviour here? > We don’t have continuous waiting in the slot-sync worker if the remote slot is behind the local slot. But if during the first sync cycle the remote slot is behind, we keep the local slot as a temporary slot. In the next sync cycle, if we find the remote slot is invalidated, we mark the local slot as invalidated too, keeping it in this temporary state. There are no LOG or WARNING messages in this case. When the slot-sync worker stops or shuts down (like during promotion), it cleans up this temporary slot. Now, for the API behavior: if the remote slot is behind the local slot, the API enters a wait loop and logs: LOG: waiting for remote slot "failover_slot" LSN (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060) and catalog xmin (770) If it keeps waiting, every 10 seconds it logs: LOG: continuing to wait for remote slot "failover_slot" LSN (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060) and catalog xmin (770) If the remote slot becomes invalidated during this wait, currently it logs a WARNING and moves to syncing the next slot: WARNING: aborting initial sync for slot "failover_slot" as the slot was invalidated on primary I think this WARNING is too strong. We could change it to a LOG message instead, mark the local slot as invalidated, exit the wait loop, and move on to syncing the next slot. Even though this LOG is not there in slotsync worker case, I think it makes more sense in API case due to continuous LOGs which suggested that API was waiting to sync this slot in wait-loop and thus we shall conclude it either by saying wait-over (like we do in successful sync case) or we can say 'LOG: aborting wait as the remote slot was invalidated' instead of above WARNING message. What do you suggest? thanks Shveta
On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Aug 4, 2025 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Aug 1, 2025 at 2:50 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > 5) > > > I tried a test where there were 4 slots on the publisher, where one > > > was getting used while the others were not. Initiated > > > pg_sync_replication_slots on standby. Forced unused slots to be > > > invalidated by setting idle_replication_slot_timeout=60 on primary, > > > due to which API finished but gave a warning: > > > > > > postgres=# SELECT pg_sync_replication_slots(); > > > WARNING: aborting initial sync for slot "failover_slot" > > > DETAIL: This slot was invalidated on the primary server. > > > WARNING: aborting initial sync for slot "failover_slot2" > > > DETAIL: This slot was invalidated on the primary server. > > > WARNING: aborting initial sync for slot "failover_slot3" > > > DETAIL: This slot was invalidated on the primary server. > > > pg_sync_replication_slots > > > --------------------------- > > > > > > (1 row) > > > > > > Do we need these warnings here? I think we can have it as a LOG rather > > > than having it on console. Thoughts? > > > > > > > What is the behaviour of a slotsync worker in the same case? I don't > > see any such WARNING messages in the code of slotsync worker, so why > > do we want a different behaviour here? > > > > We don’t have continuous waiting in the slot-sync worker if the remote > slot is behind the local slot. But if during the first sync cycle the > remote slot is behind, we keep the local slot as a temporary slot. In > the next sync cycle, if we find the remote slot is invalidated, we > mark the local slot as invalidated too, keeping it in this temporary > state. There are no LOG or WARNING messages in this case. When the > slot-sync worker stops or shuts down (like during promotion), it > cleans up this temporary slot. > > Now, for the API behavior: if the remote slot is behind the local > slot, the API enters a wait loop and logs: > > LOG: waiting for remote slot "failover_slot" LSN (0/3000060) and > catalog xmin (755) to pass local slot LSN (0/3000060) and catalog xmin > (770) > > If it keeps waiting, every 10 seconds it logs: > LOG: continuing to wait for remote slot "failover_slot" LSN > (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060) > and catalog xmin (770) > > If the remote slot becomes invalidated during this wait, currently it > logs a WARNING and moves to syncing the next slot: > WARNING: aborting initial sync for slot "failover_slot" as the slot > was invalidated on primary > > I think this WARNING is too strong. We could change it to a LOG > message instead, mark the local slot as invalidated, exit the wait > loop, and move on to syncing the next slot. > > Even though this LOG is not there in slotsync worker case, I think it > makes more sense in API case due to continuous LOGs which suggested > that API was waiting to sync this slot in wait-loop and thus we shall > conclude it either by saying wait-over (like we do in successful sync > case) or we can say 'LOG: aborting wait as the remote slot was > invalidated' instead of above WARNING message. What do you suggest? > I also think LOG is a better choice for this because there is nothing we can expect users to do even after seeing this. I feel this is more of an info for users. -- With Regards, Amit Kapila.
On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Aug 4, 2025 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Aug 1, 2025 at 2:50 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > 5)
> > > > I tried a test where there were 4 slots on the publisher, where one
> > > > was getting used while the others were not. Initiated
> > > > pg_sync_replication_slots on standby. Forced unused slots to be
> > > > invalidated by setting idle_replication_slot_timeout=60 on primary,
> > > > due to which API finished but gave a warning:
> > > >
> > > > postgres=# SELECT pg_sync_replication_slots();
> > > > WARNING: aborting initial sync for slot "failover_slot"
> > > > DETAIL: This slot was invalidated on the primary server.
> > > > WARNING: aborting initial sync for slot "failover_slot2"
> > > > DETAIL: This slot was invalidated on the primary server.
> > > > WARNING: aborting initial sync for slot "failover_slot3"
> > > > DETAIL: This slot was invalidated on the primary server.
> > > > pg_sync_replication_slots
> > > > ---------------------------
> > > >
> > > > (1 row)
> > > >
> > > > Do we need these warnings here? I think we can have it as a LOG rather
> > > > than having it on console. Thoughts?
> > > >
> > >
> > > What is the behaviour of a slotsync worker in the same case? I don't
> > > see any such WARNING messages in the code of slotsync worker, so why
> > > do we want a different behaviour here?
> > >
> >
> > We don’t have continuous waiting in the slot-sync worker if the remote
> > slot is behind the local slot. But if during the first sync cycle the
> > remote slot is behind, we keep the local slot as a temporary slot. In
> > the next sync cycle, if we find the remote slot is invalidated, we
> > mark the local slot as invalidated too, keeping it in this temporary
> > state. There are no LOG or WARNING messages in this case. When the
> > slot-sync worker stops or shuts down (like during promotion), it
> > cleans up this temporary slot.
> >
> > Now, for the API behavior: if the remote slot is behind the local
> > slot, the API enters a wait loop and logs:
> >
> > LOG: waiting for remote slot "failover_slot" LSN (0/3000060) and
> > catalog xmin (755) to pass local slot LSN (0/3000060) and catalog xmin
> > (770)
> >
> > If it keeps waiting, every 10 seconds it logs:
> > LOG: continuing to wait for remote slot "failover_slot" LSN
> > (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060)
> > and catalog xmin (770)
> >
> > If the remote slot becomes invalidated during this wait, currently it
> > logs a WARNING and moves to syncing the next slot:
> > WARNING: aborting initial sync for slot "failover_slot" as the slot
> > was invalidated on primary
> >
> > I think this WARNING is too strong. We could change it to a LOG
> > message instead, mark the local slot as invalidated, exit the wait
> > loop, and move on to syncing the next slot.
> >
> > Even though this LOG is not there in slotsync worker case, I think it
> > makes more sense in API case due to continuous LOGs which suggested
> > that API was waiting to sync this slot in wait-loop and thus we shall
> > conclude it either by saying wait-over (like we do in successful sync
> > case) or we can say 'LOG: aborting wait as the remote slot was
> > invalidated' instead of above WARNING message. What do you suggest?
> >
>
> I also think LOG is a better choice for this because there is nothing
> we can expect users to do even after seeing this. I feel this is more
> of an info for users.
>
Yes, it is more of an info for users.
> 1.
> update_and_persist_local_synced_slot()
> {
> ...
> + /*
> + * For SQL API synchronization, we wait for the remote slot to catch up
> + * here, since we can't assume the SQL API will be called again soon.
> + * We will retry the sync once the slot catches up.
> + *
> + * Note: This will return false if a promotion is triggered on the
> + * standby while waiting, in which case we stop syncing and drop the
> + * temporary slot.
> + */
> + if (!wait_for_primary_slot_catchup(wrconn, remote_slot))
> + return false;
>
> Why is the wait added at this level? Shouldn't it be at API level aka
> in SyncReplicationSlots() or pg_sync_replication_slots() similar to
> what we do in ReplSlotSyncWorkerMain() for slotsync workers?
The initial goal was to perform a single sync cycle for all slots. The
logic was simple, if any slot couldn’t be synced because its remote
slot was lagging, we would wait for the remote slot to catch up, and
only then move on to the next slot.
But if we consider moving wait logic to SyncReplicationSlots(), we
will necessarily have to go with the logic that it will attempt to
sync all slots in the first sync cycle, skipping those where remote
slots are lagging. It will then continue with multiple sync cycles
until all slots are successfully synced. But if new remote slots are
added in the meantime, they will be picked up in the next cycle, and
the API then has to wait on those as well and this cycle may go on for
longer.
If we want to avoid continuously syncing newly added slots in later
cycles and instead focus only on the ones that failed to sync during
the first attempt, one approach is to maintain a list of failed slots
from the initial cycle and only retry those in subsequent attempts.
But this will add complexity to the implementation.
IMO, attempting multiple sync cycles essentially makes the API behave
more like slotsyncworker, which might not be desirable. I feel that
performing just one sync cycle in the API is more in line with the
expected behavior. And for that, the current implementation of
wait-logic seems simpler. But let me know if you think otherwise or I
have not understood your point clearly. I am open to more
approaches/ideas here.
thanks
Shveta
On Tue, Aug 5, 2025 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > If we want to avoid continuously syncing newly added slots in later > cycles and instead focus only on the ones that failed to sync during > the first attempt, one approach is to maintain a list of failed slots > from the initial cycle and only retry those in subsequent attempts. > But this will add complexity to the implementation. > There will be some additional code for this but overall it improves the code in the lower level functions. We may want to use the existing remote_slot list for this purpose. The current proposed change in low-level functions appears to be difficult to maintain, especially the change proposed in update_and_persist_local_synced_slot(). If we can find a better way to achieve the same then we can consider the current approach as well. -- With Regards, Amit Kapila.
On Fri, Aug 1, 2025 at 4:32 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Thanks for the patch. I tested it, please find a few comments:
>
>
> 1)
> it hits an assert
> (slotsync_reread_config()-->Assert(sync_replication_slots)) when API
> is trying to sync and is in wait loop while in another session, I
> enable sync_replication_slots using:
>
> ALTER SYSTEM SET sync_replication_slots = 'on';
> SELECT pg_reload_conf();
>
> Assert:
> 025-08-01 10:55:43.637 IST [118576] STATEMENT: SELECT
> pg_sync_replication_slots();
> 2025-08-01 10:55:51.730 IST [118563] LOG: received SIGHUP, reloading
> configuration files
> 2025-08-01 10:55:51.731 IST [118563] LOG: parameter
> "sync_replication_slots" changed to "on"
> TRAP: failed Assert("sync_replication_slots"), File: "slotsync.c",
> Line: 1334, PID: 118576
> postgres: shveta postgres [local]
> SELECT(ExceptionalCondition+0xbb)[0x61df0160e090]
> postgres: shveta postgres [local] SELECT(+0x6520dc)[0x61df0133a0dc]
> 2025-08-01 10:55:51.739 IST [118666] ERROR: cannot synchronize
> replication slots concurrently
> postgres: shveta postgres [local] SELECT(+0x6522b2)[0x61df0133a2b2]
> postgres: shveta postgres [local] SELECT(+0x650664)[0x61df01338664]
> postgres: shveta postgres [local] SELECT(+0x650cf8)[0x61df01338cf8]
> postgres: shveta postgres [local] SELECT(+0x6513ea)[0x61df013393ea]
> postgres: shveta postgres [local] SELECT(+0x6519df)[0x61df013399df]
> postgres: shveta postgres [local]
> SELECT(SyncReplicationSlots+0xbb)[0x61df0133af60]
> postgres: shveta postgres [local]
> SELECT(pg_sync_replication_slots+0x1b1)[0x61df01357e52]
>
Fixed.
> 2)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot synchronize replication slots when"
> + " standby promotion is ongoing")));
>
> I think better error message will be:
> "exiting from slot synchronization as promotion is triggered"
>
> This will be better suited in log file as well after below wait statements:
> LOG: continuing to wait for remote slot "failover_slot" LSN
> (0/3000060) and catalog xmin (755) to pass local slot LSN (0/3000060)
> and catalog xmin (757)
> STATEMENT: SELECT pg_sync_replication_slots();
>
Fixed.
> 3)
> API dumps this when it is waiting for primary:
>
> ----
> LOG: could not synchronize replication slot "failover_slot2"
> DETAIL: Synchronization could lead to data loss, because the remote
> slot needs WAL at LSN 0/03066E70 and catalog xmin 755, but the standby
> has LSN 0/03066E70 and catalog xmin 770.
> STATEMENT: SELECT pg_sync_replication_slots();
> LOG: waiting for remote slot "failover_slot2" LSN (0/3066E70) and
> catalog xmin (755) to pass local slot LSN (0/3066E70) and catalog xmin
> (770)
> STATEMENT: SELECT pg_sync_replication_slots();
> LOG: continuing to wait for remote slot "failover_slot2" LSN
> (0/3066E70) and catalog xmin (755) to pass local slot LSN (0/3066E70)
> and catalog xmin (770)
> STATEMENT: SELECT pg_sync_replication_slots();
> ----
>
> Unsure if we shall still dump 'could not synchronize..' when it is
> going to retry until it succeeds? The concerned log gives a feeling
> that we are done trying and could not synchronize it. What do you
> think?
>
I've modified the log to now say, "initial sync of replication slot
\"%s\" failed; will keep retrying"
On Fri, Aug 1, 2025 at 7:20 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> A few more comments:
>
> 4)
>
> + if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
> + {
> + ereport(WARNING,
> + errmsg("aborting initial sync for slot \"%s\"",
> + remote_slot->name),
> + errdetail("This slot was not found on the primary server."));
> +
> + pfree(cmd.data);
> + walrcv_clear_result(res);
> +
> + return false;
> + }
>
> We may have 'aborting sync for slot', can remove 'initial'.
Fixed.
>
> 5)
> I tried a test where there were 4 slots on the publisher, where one
> was getting used while the others were not. Initiated
> pg_sync_replication_slots on standby. Forced unused slots to be
> invalidated by setting idle_replication_slot_timeout=60 on primary,
> due to which API finished but gave a warning:
>
> postgres=# SELECT pg_sync_replication_slots();
> WARNING: aborting initial sync for slot "failover_slot"
> DETAIL: This slot was invalidated on the primary server.
> WARNING: aborting initial sync for slot "failover_slot2"
> DETAIL: This slot was invalidated on the primary server.
> WARNING: aborting initial sync for slot "failover_slot3"
> DETAIL: This slot was invalidated on the primary server.
> pg_sync_replication_slots
> ---------------------------
>
> (1 row)
>
> Do we need these warnings here? I think we can have it as a LOG rather
> than having it on console. Thoughts?
>
> If we inclined towards WARNING here, will ti be better to have it as
> a single line:
>
> WARNING: aborting sync for slot "failover_slot" as the slot was
> invalidated on primary
> WARNING: aborting sync for slot "failover_slot1" as the slot was
> invalidated on primary
> WARNING: aborting sync for slot "failover_slot2" as the slot was
> invalidated on primary
>
I've changed it to LOG now.
>
> 6)
> - * We do not drop the slot because the restart_lsn can be ahead of the
> - * current location when recreating the slot in the next cycle. It may
> - * take more time to create such a slot. Therefore, we keep this slot
> - * and attempt the synchronization in the next cycle.
> + * If we're in the slotsync worker, we retain the slot and retry in the
> + * next cycle. The restart_lsn might advance by then, allowing the slot
> + * to be created successfully later.
> */
>
> I like the previous command better as it was conveying the side-effect
> of dropping the slot herer. Can we try to incorporate the previous
> comment here and make it specific to slotsync workers?
>
Reverted to the previous version.
Attaching patch v4 which addresses these comments.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Tue, Aug 5, 2025 at 4:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 5, 2025 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > If we want to avoid continuously syncing newly added slots in later > > cycles and instead focus only on the ones that failed to sync during > > the first attempt, one approach is to maintain a list of failed slots > > from the initial cycle and only retry those in subsequent attempts. > > But this will add complexity to the implementation. > > > > There will be some additional code for this but overall it improves > the code in the lower level functions. We may want to use the existing > remote_slot list for this purpose. > > The current proposed change in low-level functions appears to be > difficult to maintain, especially the change proposed in > update_and_persist_local_synced_slot(). If we can find a better way to > achieve the same then we can consider the current approach as well. > Next patch, I'll work on addressing this comment. I'll need to restructure the code to make this happen. regards, Ajin Cherian Fujitsu Australia
On Wed, Aug 6, 2025 at 7:35 AM Ajin Cherian <itsajin@gmail.com> wrote: > > On Tue, Aug 5, 2025 at 4:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Aug 5, 2025 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > If we want to avoid continuously syncing newly added slots in later > > > cycles and instead focus only on the ones that failed to sync during > > > the first attempt, one approach is to maintain a list of failed slots > > > from the initial cycle and only retry those in subsequent attempts. > > > But this will add complexity to the implementation. > > > > > > > There will be some additional code for this but overall it improves > > the code in the lower level functions. We may want to use the existing > > remote_slot list for this purpose. > > > > The current proposed change in low-level functions appears to be > > difficult to maintain, especially the change proposed in > > update_and_persist_local_synced_slot(). If we can find a better way to > > achieve the same then we can consider the current approach as well. > > > > Next patch, I'll work on addressing this comment. I'll need to > restructure the code to make this happen. > Okay, thanks Ajin. I will resume review after this comment is addressed as I am assuming that the new logic will get rid of most of the current wait logic and thus it makes sense to review it after it is addressed. thanks Shveta
On Wed, Aug 6, 2025 at 8:48 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, Aug 6, 2025 at 7:35 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > On Tue, Aug 5, 2025 at 4:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Tue, Aug 5, 2025 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > > > On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > If we want to avoid continuously syncing newly added slots in later > > > > cycles and instead focus only on the ones that failed to sync during > > > > the first attempt, one approach is to maintain a list of failed slots > > > > from the initial cycle and only retry those in subsequent attempts. > > > > But this will add complexity to the implementation. > > > > > > > > > > There will be some additional code for this but overall it improves > > > the code in the lower level functions. We may want to use the existing > > > remote_slot list for this purpose. > > > > > > The current proposed change in low-level functions appears to be > > > difficult to maintain, especially the change proposed in > > > update_and_persist_local_synced_slot(). If we can find a better way to > > > achieve the same then we can consider the current approach as well. > > > > > > > Next patch, I'll work on addressing this comment. I'll need to > > restructure the code to make this happen. > > > > Okay, thanks Ajin. I will resume review after this comment is > addressed as I am assuming that the new logic will get rid of most of > the current wait logic and thus it makes sense to review it after it > is addressed. There's also a minor merge conflict because func.sgml is not split into multiple files. -- Best Wishes, Ashutosh Bapat
On Tue, Aug 5, 2025 at 4:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 5, 2025 at 9:28 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Mon, Aug 4, 2025 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Mon, Aug 4, 2025 at 12:19 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > If we want to avoid continuously syncing newly added slots in later > > cycles and instead focus only on the ones that failed to sync during > > the first attempt, one approach is to maintain a list of failed slots > > from the initial cycle and only retry those in subsequent attempts. > > But this will add complexity to the implementation. > > > > There will be some additional code for this but overall it improves > the code in the lower level functions. We may want to use the existing > remote_slot list for this purpose. > > The current proposed change in low-level functions appears to be > difficult to maintain, especially the change proposed in > update_and_persist_local_synced_slot(). If we can find a better way to > achieve the same then we can consider the current approach as well. Right. I've reworked the design to have the wait at a much lower level. I've also used a single WAIT EVENT - REPLICATION_SLOTSYNC_PRIMARY_CATCHUP for both the slotsync worker and the sync API. regards, Ajin Cherian Fujitsu Australia
Вложения
On Fri, Aug 8, 2025 at 11:22 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > > There's also a minor merge conflict because func.sgml is not split > into multiple files. > Yes, I fixed this. regards, Ajin Cherian Fujitsu Australia
On Mon, Aug 11, 2025 at 1:37 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Fri, Aug 8, 2025 at 11:22 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > > > > > There's also a minor merge conflict because func.sgml is not split > > into multiple files. > > > > Yes, I fixed this. > Thanks for the patch. Please find a few comments: 1) We can merge refresh_remote_slots and fetch_remote_slots by passing an argument of remote_list. If no remote_list passed, fetch all failover slots, else extend the query and fetch only the listed ones. 2) We can get rid of 'sync_iterations' and the logic within, as I think there is no need to distinguish between slotsync and API in terms of logs. 3) sync_start_pending is not needed to be passed to update_and_persist_local_synced_slot(), as the output of this function is good enough to tell whether slot is persisted or not. 4) Also how about having sync-pending in SlotSyncCtxStruct. It can be set unconditionally by both slotsync and API, but will be used by API. I think it can simplify the code. 5) We can get rid of 'pending_sync_start_slots', as it is not being used anywhere. 6) Also we can mention in comments as to why we are using the old remote_slots list in refresh_remote_slots() during subsequent cycles of API rather than using only the pending-slot list. thanks Shveta
On Wed, Aug 13, 2025 at 2:47 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Aug 11, 2025 at 1:37 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > On Fri, Aug 8, 2025 at 11:22 PM Ashutosh Bapat > > <ashutosh.bapat.oss@gmail.com> wrote: > > > > > > > > > There's also a minor merge conflict because func.sgml is not split > > > into multiple files. > > > > > > > Yes, I fixed this. > > > > Thanks for the patch. Please find a few comments: > > 1) > We can merge refresh_remote_slots and fetch_remote_slots by passing an > argument of remote_list. If no remote_list passed, fetch all failover > slots, else extend the query and fetch only the listed ones. > Done. > 2) > We can get rid of 'sync_iterations' and the logic within, as I think > there is no need to distinguish between slotsync and API in terms of > logs. > Done. > 3) > sync_start_pending is not needed to be passed to > update_and_persist_local_synced_slot(), as the output of this function > is good enough to tell whether slot is persisted or not. > > 4) > Also how about having sync-pending in SlotSyncCtxStruct. It can be set > unconditionally by both slotsync and API, but will be used by API. I > think it can simplify the code. > Done. > 5) > We can get rid of 'pending_sync_start_slots', as it is not being used anywhere. > Fixed. > 6) > Also we can mention in comments as to why we are using the old > remote_slots list in refresh_remote_slots() during subsequent cycles > of API rather than using only the pending-slot list. Done. Patch v6 attached. regards, Ajin Cherian Fujitsu Australia
Вложения
On Thu, Aug 14, 2025 at 7:28 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> Patch v6 attached.
>
Thanks Ajin. Please find my comments:
1)
SyncReplicationSlots:
+ remote_slots = fetch_or_refresh_remote_slots(wrconn, NIL);
+
+ /* Retry until all slots are sync ready atleast */
+ for (;;)
+ {
+ bool some_slot_updated = false;
+
+ /*
+ * Refresh the remote slot data. We keep using the original slot
+ * list, even if some slots are already sync ready, so that all
+ * slots are updated with the latest status from the primary.
+ */
+ remote_slots = fetch_or_refresh_remote_slots(wrconn, remote_slots);
When the API begins, it seems we are fetching remote_list twice
before we even sync it once. We can get rid of
'fetch_or_refresh_remote_slots' from outside the loop and retain the
inside one. At first call, remote_slots will be NIL and thus it will
fetch all slots and in subsequent calls, it will be populated one.
2)
SyncReplicationSlots:
+ /*
+ * The syscache access in fetch_or_refresh_remote_slots() needs a
+ * transaction env.
+ */
+ if (!IsTransactionState()) {
+ StartTransactionCommand();
+ started_tx = true;
+ }
+ if (started_tx)
+ CommitTransactionCommand();
Shall we move these 2 inside fetch_or_refresh_remote_slots() (both
worker and APi flow) similar to how validate_remote_info() also has it
inside?
3)
SyncReplicationSlots:
+ /* Done if all slots are atleast sync ready */
+ if (!SlotSyncCtx->slot_not_persisted)
+ break;
+ else
+ {
+ /* wait for 2 seconds before retrying */
+ wait_for_slot_activity(some_slot_updated, true);
No need to have 'else' block here. The code can be put without having
'else' because 'if' when true, breaks from the loop.
4)
'fetch_or_refresh_remote_slots' can be renamed to 'fetch_remote_slots'
simply and then a comment can define an extra argument. Because
ultimately we are re-fetching some/all slots in both cases.
5)
In the case of API, wait_for_slot_activity() does not change its wait
time based on 'some_slot_updated'. I think we can pull 'WaitLatch,
ResetLatch' in API-function itself and lets not change worker's
wait_for_slot_activity().
6)
fetch_or_refresh_remote_slots:
+ {
+ if (is_refresh)
+ {
+ ereport(WARNING,
+ errmsg("could not fetch updated failover logical slots info"
+ " from the primary server: %s",
+ res->err));
+ pfree(query.data);
+ return remote_slot_list; /* Return original list on refresh failure */
+ }
+ else
+ {
+ ereport(ERROR,
+ errmsg("could not fetch failover logical slots info from the primary
server: %s",
+ res->err));
+ }
+ }
I think there is no need for different behaviour here for worker and
API. Since worker errors-out here, we can make API also error-out.
7)
+fetch_or_refresh_remote_slots(WalReceiverConn *wrconn, List *remote_slot_list)
We can name the argument as 'target_slot_list' and replace the name
'updated_slot_list' with 'remote_slot_list'.
8)
+ /* If refreshing, free the original list structures */
+ if (is_refresh)
+ {
+ foreach(lc, remote_slot_list)
+ {
+ RemoteSlot *old_slot = (RemoteSlot *) lfirst(lc);
+ pfree(old_slot);
+ }
+ list_free(remote_slot_list);
+ }
We can get rid of 'is_refresh' and can simply check if
'target_slot_list != NIL', free it. We can use list_free_deep instead
of freeing each element. Having said that, it looks slightly odd to
free the list in this function, I will think more here. Meanwhile, we
can do this.
9)
-update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
+update_and_persist_local_synced_slot(WalReceiverConn * wrconn,
+ RemoteSlot * remote_slot, Oid remote_dbid)
We can get rid of wrconn as we are not using it. Same with wrconn
argument for synchronize_one_slot()
10)
+ /* used by pg_sync_replication_slots() API only */
+ bool slot_not_persisted;
We can move comment outside structure. We can first define it and then
say the above line.
11)
+ SlotSyncCtx->slot_not_persisted = false;
This may overwrite the 'slot_not_persisted' set for the previous slot
and ultimately make it 'false' at the end of cycle even though we had
few not-persisted slots in the beginning of cycle. Should it be:
SlotSyncCtx->slot_not_persisted |= false;
12)
Shall we rename this to : slot_persistence_pending (based on many
other modules using similar names: detach_pending, send_pending,
callback_pending)?
13)
- errmsg("could not synchronize replication slot \"%s\"",
- remote_slot->name),
- errdetail("Synchronization could lead to data loss, because the
remote slot needs WAL at LSN %X/%08X and catalog xmin %u, but the
standby has LSN %X/%08X and catalog xmin %u.",
- LSN_FORMAT_ARGS(remote_slot->restart_lsn),
- remote_slot->catalog_xmin,
- LSN_FORMAT_ARGS(slot->data.restart_lsn),
- slot->data.catalog_xmin));
+ errmsg("Replication slot \"%s\" is not sync ready; will keep retrying",
+ remote_slot->name),
+ errdetail("Attempting Synchronization could lead to data loss,
because the remote slot needs WAL at LSN %X/%08X and catalog xmin %u,
but the standby has LSN %X/%08X and catalog xmin %u.",
+ LSN_FORMAT_ARGS(remote_slot->restart_lsn),
+ remote_slot->catalog_xmin,
+ LSN_FORMAT_ARGS(slot->data.restart_lsn),
+ slot->data.catalog_xmin));
We can retain the same message as it was put after a lot of
discussion. We can attempt to change if others comment. The idea is
since a worker dumps it in each subsequent cycle (if such a situation
arises), on the same basis now the API can also do so because it is
also performing multiple cycles now. Earlier I had suggested changing
it for API based on messages 'continuing to wait..' which are no
longer there now.
thanks
Shveta
On Thu, Aug 14, 2025 at 12:14 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> 8)
> + /* If refreshing, free the original list structures */
> + if (is_refresh)
> + {
> + foreach(lc, remote_slot_list)
> + {
> + RemoteSlot *old_slot = (RemoteSlot *) lfirst(lc);
> + pfree(old_slot);
> + }
> + list_free(remote_slot_list);
> + }
>
> We can get rid of 'is_refresh' and can simply check if
> 'target_slot_list != NIL', free it. We can use list_free_deep instead
> of freeing each element. Having said that, it looks slightly odd to
> free the list in this function, I will think more here. Meanwhile, we
> can do this.
>
+1. The function prologue doesn't mention that the original list is
deep freed. So a caller may try to access it after this call, which
will lead to a crash. As a safe programming practice we should let the
caller free the original list if it is not needed anymore OR modify
the input list in-place and return it for the convenience of the
caller like all list_* interfaces. At least we should document this
behavior in the function prologue. You could also use foreach_ptr
instead of foreach.
> 13)
> - errmsg("could not synchronize replication slot \"%s\"",
> - remote_slot->name),
> - errdetail("Synchronization could lead to data loss, because the
> remote slot needs WAL at LSN %X/%08X and catalog xmin %u, but the
> standby has LSN %X/%08X and catalog xmin %u.",
> - LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> - remote_slot->catalog_xmin,
> - LSN_FORMAT_ARGS(slot->data.restart_lsn),
> - slot->data.catalog_xmin));
> + errmsg("Replication slot \"%s\" is not sync ready; will keep retrying",
> + remote_slot->name),
> + errdetail("Attempting Synchronization could lead to data loss,
> because the remote slot needs WAL at LSN %X/%08X and catalog xmin %u,
> but the standby has LSN %X/%08X and catalog xmin %u.",
> + LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> + remote_slot->catalog_xmin,
> + LSN_FORMAT_ARGS(slot->data.restart_lsn),
> + slot->data.catalog_xmin));
>
> We can retain the same message as it was put after a lot of
> discussion. We can attempt to change if others comment. The idea is
> since a worker dumps it in each subsequent cycle (if such a situation
> arises), on the same basis now the API can also do so because it is
> also performing multiple cycles now. Earlier I had suggested changing
> it for API based on messages 'continuing to wait..' which are no
> longer there now.
Also we usually don't use capital letters at the start of the error
message. Any reason this is different?
Some more
+ * When called from pg_sync_replication_slots, use a fixed 2
+ * second wait time.
Function prologue doesn't mention this. Probably the prologue should
contain only the first sentence there. Rest of the prologues just
repeat comments in the function. The function is small enough that a
reader could read the details from the function instead of the
prologue.
+ wait_time = SLOTSYNC_API_NAPTIME_MS;
+ } else {
} else and { should be on separate lines.
--
Best Wishes,
Ashutosh Bapat
On Thu, Aug 14, 2025 at 4:44 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Aug 14, 2025 at 7:28 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > Patch v6 attached.
> >
>
> Thanks Ajin. Please find my comments:
>
> 1)
> SyncReplicationSlots:
> + remote_slots = fetch_or_refresh_remote_slots(wrconn, NIL);
> +
> + /* Retry until all slots are sync ready atleast */
> + for (;;)
> + {
> + bool some_slot_updated = false;
> +
> + /*
> + * Refresh the remote slot data. We keep using the original slot
> + * list, even if some slots are already sync ready, so that all
> + * slots are updated with the latest status from the primary.
> + */
> + remote_slots = fetch_or_refresh_remote_slots(wrconn, remote_slots);
>
> When the API begins, it seems we are fetching remote_list twice
> before we even sync it once. We can get rid of
> 'fetch_or_refresh_remote_slots' from outside the loop and retain the
> inside one. At first call, remote_slots will be NIL and thus it will
> fetch all slots and in subsequent calls, it will be populated one.
>
Fixed.
>
> 2)
> SyncReplicationSlots:
> + /*
> + * The syscache access in fetch_or_refresh_remote_slots() needs a
> + * transaction env.
> + */
> + if (!IsTransactionState()) {
> + StartTransactionCommand();
> + started_tx = true;
> + }
>
> + if (started_tx)
> + CommitTransactionCommand();
>
> Shall we move these 2 inside fetch_or_refresh_remote_slots() (both
> worker and APi flow) similar to how validate_remote_info() also has it
> inside?
>
I tried this but it doesn't work because when the transaction is
committed, the memory allocation for the remote slots are also freed.
So, this needs to be on the outside.
> 3)
> SyncReplicationSlots:
> + /* Done if all slots are atleast sync ready */
> + if (!SlotSyncCtx->slot_not_persisted)
> + break;
> + else
> + {
> + /* wait for 2 seconds before retrying */
> + wait_for_slot_activity(some_slot_updated, true);
>
> No need to have 'else' block here. The code can be put without having
> 'else' because 'if' when true, breaks from the loop.
>
Fixed.
>
> 4)
> 'fetch_or_refresh_remote_slots' can be renamed to 'fetch_remote_slots'
> simply and then a comment can define an extra argument. Because
> ultimately we are re-fetching some/all slots in both cases.
>
Done.
> 5)
> In the case of API, wait_for_slot_activity() does not change its wait
> time based on 'some_slot_updated'. I think we can pull 'WaitLatch,
> ResetLatch' in API-function itself and lets not change worker's
> wait_for_slot_activity().
>
Done.
> 6)
> fetch_or_refresh_remote_slots:
> + {
> + if (is_refresh)
> + {
> + ereport(WARNING,
> + errmsg("could not fetch updated failover logical slots info"
> + " from the primary server: %s",
> + res->err));
> + pfree(query.data);
> + return remote_slot_list; /* Return original list on refresh failure */
> + }
> + else
> + {
> + ereport(ERROR,
> + errmsg("could not fetch failover logical slots info from the primary
> server: %s",
> + res->err));
> + }
> + }
>
> I think there is no need for different behaviour here for worker and
> API. Since worker errors-out here, we can make API also error-out.
>
Fixed.
> 7)
> +fetch_or_refresh_remote_slots(WalReceiverConn *wrconn, List *remote_slot_list)
>
> We can name the argument as 'target_slot_list' and replace the name
> 'updated_slot_list' with 'remote_slot_list'.
>
Fixed.
>
> 8)
> + /* If refreshing, free the original list structures */
> + if (is_refresh)
> + {
> + foreach(lc, remote_slot_list)
> + {
> + RemoteSlot *old_slot = (RemoteSlot *) lfirst(lc);
> + pfree(old_slot);
> + }
> + list_free(remote_slot_list);
> + }
>
> We can get rid of 'is_refresh' and can simply check if
> 'target_slot_list != NIL', free it. We can use list_free_deep instead
> of freeing each element. Having said that, it looks slightly odd to
> free the list in this function, I will think more here. Meanwhile, we
> can do this.
>
Fixed.
>
> 9)
> -update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
> +update_and_persist_local_synced_slot(WalReceiverConn * wrconn,
> + RemoteSlot * remote_slot, Oid remote_dbid)
>
> We can get rid of wrconn as we are not using it. Same with wrconn
> argument for synchronize_one_slot()
>
Done.
> 10)
> + /* used by pg_sync_replication_slots() API only */
> + bool slot_not_persisted;
>
> We can move comment outside structure. We can first define it and then
> say the above line.
>
Done.
>
> 11)
> + SlotSyncCtx->slot_not_persisted = false;
>
> This may overwrite the 'slot_not_persisted' set for the previous slot
> and ultimately make it 'false' at the end of cycle even though we had
> few not-persisted slots in the beginning of cycle. Should it be:
>
> SlotSyncCtx->slot_not_persisted |= false;
>
Fixed.
> 12)
> Shall we rename this to : slot_persistence_pending (based on many
> other modules using similar names: detach_pending, send_pending,
> callback_pending)?
>
Done.
> 13)
> - errmsg("could not synchronize replication slot \"%s\"",
> - remote_slot->name),
> - errdetail("Synchronization could lead to data loss, because the
> remote slot needs WAL at LSN %X/%08X and catalog xmin %u, but the
> standby has LSN %X/%08X and catalog xmin %u.",
> - LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> - remote_slot->catalog_xmin,
> - LSN_FORMAT_ARGS(slot->data.restart_lsn),
> - slot->data.catalog_xmin));
> + errmsg("Replication slot \"%s\" is not sync ready; will keep retrying",
> + remote_slot->name),
> + errdetail("Attempting Synchronization could lead to data loss,
> because the remote slot needs WAL at LSN %X/%08X and catalog xmin %u,
> but the standby has LSN %X/%08X and catalog xmin %u.",
> + LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> + remote_slot->catalog_xmin,
> + LSN_FORMAT_ARGS(slot->data.restart_lsn),
> + slot->data.catalog_xmin));
>
> We can retain the same message as it was put after a lot of
> discussion. We can attempt to change if others comment. The idea is
> since a worker dumps it in each subsequent cycle (if such a situation
> arises), on the same basis now the API can also do so because it is
> also performing multiple cycles now. Earlier I had suggested changing
> it for API based on messages 'continuing to wait..' which are no
> longer there now.
>
Done.
On Thu, Aug 14, 2025 at 10:44 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Aug 14, 2025 at 12:14 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > 8)
> > + /* If refreshing, free the original list structures */
> > + if (is_refresh)
> > + {
> > + foreach(lc, remote_slot_list)
> > + {
> > + RemoteSlot *old_slot = (RemoteSlot *) lfirst(lc);
> > + pfree(old_slot);
> > + }
> > + list_free(remote_slot_list);
> > + }
> >
> > We can get rid of 'is_refresh' and can simply check if
> > 'target_slot_list != NIL', free it. We can use list_free_deep instead
> > of freeing each element. Having said that, it looks slightly odd to
> > free the list in this function, I will think more here. Meanwhile, we
> > can do this.
> >
>
> +1. The function prologue doesn't mention that the original list is
> deep freed. So a caller may try to access it after this call, which
> will lead to a crash. As a safe programming practice we should let the
> caller free the original list if it is not needed anymore OR modify
> the input list in-place and return it for the convenience of the
> caller like all list_* interfaces. At least we should document this
> behavior in the function prologue. You could also use foreach_ptr
> instead of foreach.
>
I've changed the logic so that it is the responsibility of the caller
to free the list.
> > 13)
> > - errmsg("could not synchronize replication slot \"%s\"",
> > - remote_slot->name),
> > - errdetail("Synchronization could lead to data loss, because the
> > remote slot needs WAL at LSN %X/%08X and catalog xmin %u, but the
> > standby has LSN %X/%08X and catalog xmin %u.",
> > - LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> > - remote_slot->catalog_xmin,
> > - LSN_FORMAT_ARGS(slot->data.restart_lsn),
> > - slot->data.catalog_xmin));
> > + errmsg("Replication slot \"%s\" is not sync ready; will keep retrying",
> > + remote_slot->name),
> > + errdetail("Attempting Synchronization could lead to data loss,
> > because the remote slot needs WAL at LSN %X/%08X and catalog xmin %u,
> > but the standby has LSN %X/%08X and catalog xmin %u.",
> > + LSN_FORMAT_ARGS(remote_slot->restart_lsn),
> > + remote_slot->catalog_xmin,
> > + LSN_FORMAT_ARGS(slot->data.restart_lsn),
> > + slot->data.catalog_xmin));
> >
> > We can retain the same message as it was put after a lot of
> > discussion. We can attempt to change if others comment. The idea is
> > since a worker dumps it in each subsequent cycle (if such a situation
> > arises), on the same basis now the API can also do so because it is
> > also performing multiple cycles now. Earlier I had suggested changing
> > it for API based on messages 'continuing to wait..' which are no
> > longer there now.
>
> Also we usually don't use capital letters at the start of the error
> message. Any reason this is different?
>
Retained the old message.
> Some more
>
> + * When called from pg_sync_replication_slots, use a fixed 2
> + * second wait time.
>
> Function prologue doesn't mention this. Probably the prologue should
> contain only the first sentence there. Rest of the prologues just
> repeat comments in the function. The function is small enough that a
> reader could read the details from the function instead of the
> prologue.
>
> + wait_time = SLOTSYNC_API_NAPTIME_MS;
> + } else {
>
> } else and { should be on separate lines.
>
I've removed the changes in this function and it is now the same as before.
Attaching patch v7 addressing all the above comments.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Tue, Aug 19, 2025 at 10:55 AM Ajin Cherian <itsajin@gmail.com> wrote: > > Attaching patch v7 addressing all the above comments. > Thank You for the patches. Please find a few comments: 1) We are not resetting 'slot_persistence_pending' to false anywhere. So once it hits the flow which sets it to true, it will never become false even if remote-slot catches up in subsequent cycles, resulting in a hang of the API. We shall reset it before starting a new iteration in SyncReplicationSlots(). 2) We need to clean 'slot_persistence_pending' in reset_syncing_flag() as well which is called at the end of API or in failure of API. Even though the slot sync worker is not using it, we should clean it up in slotsync_worker_onexit() as well. 3) + /* slot has been persisted, no need to retry */ + SlotSyncCtx->slot_persistence_pending |= false; + This will not be needed once we reset this flag before each iteration in SyncReplicationSlots() 4) -synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid) +synchronize_one_slot(WalReceiverConn * wrconn, RemoteSlot * remote_slot, + Oid remote_dbid) wrconn not used anywhere. 5) + bool is_refresh = (target_slot_list!= NIL); is_refresh is not needed. We can simply check if target_slot_list!=NIL, then append it to cmd. 6) * If remote_slot_list is NIL, fetches all failover logical slots from the * primary server. If remote_slot_list is provided, refreshes only those * specific slots with current values from the primary server. The usage of the word 'refreshing' is confusing. Since we are allocating a new remote-list everytime (instead of reusing or refreshing previous one), we can simply say: ------ Fetches the failover logical slots info from the primary server If target_slot_list is NIL, fetches all failover logical slots from the primary server, otherwise fetches only the ones mentioned in target_slot_list ------ The 'Parameters:' can also be adjusted accordingly. 7) * Returns a list of RemoteSlot structures. If refreshing and the query fails, * returns the original list. Slots that no longer exist on the primary will * be removed from the list. This can be removed. 8) - * If restart_lsn, confirmed_lsn or catalog_xmin is invalid but the - * slot is valid, that means we have fetched the remote_slot in its - * RS_EPHEMERAL state. In such a case, don't sync it; we can always - * sync it in the next sync cycle when the remote_slot is persisted - * and has valid lsn(s) and xmin values. - * - * XXX: In future, if we plan to expose 'slot->data.persistency' in - * pg_replication_slots view, then we can avoid fetching RS_EPHEMERAL - * slots in the first place. + * Apply ephemeral slot filtering. Skip slots that are in RS_EPHEMERAL + * state (invalid LSNs/xmin but not explicitly invalidated). We can retain the original comment. 9) Apart from above, there are many changes (alignement, comments etc) which are not related to this particular improvement. We can get rid of those changes. The patch should have the changes pertaining to current improvement alone. thanks Shveta
On Tue, 19 Aug 2025 at 10:25, Ajin Cherian <itsajin@gmail.com> wrote: > > Attaching patch v7 addressing all the above comments. Looks like this thread is not attached to any commitfest entry? If so, can you please create one[0]? This will be beneficial for thread, both simplifying patch review and (possibly) increasing the number of reviewers. [0] https://commitfest.postgresql.org/55/new/ -- Best regards, Kirill Reshke
On Tue, Aug 19, 2025 at 7:42 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Tue, Aug 19, 2025 at 10:55 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > Attaching patch v7 addressing all the above comments. > > > > Thank You for the patches. Please find a few comments: > > 1) > We are not resetting 'slot_persistence_pending' to false anywhere. So > once it hits the flow which sets it to true, it will never become > false even if remote-slot catches up in subsequent cycles, resulting > in a hang of the API. We shall reset it before starting a new > iteration in SyncReplicationSlots(). > > 2) > We need to clean 'slot_persistence_pending' in reset_syncing_flag() as > well which is called at the end of API or in failure of API. Even > though the slot sync worker is not using it, we should clean it up in > slotsync_worker_onexit() as well. > Done. > 3) > + /* slot has been persisted, no need to retry */ > + SlotSyncCtx->slot_persistence_pending |= false; > + > > This will not be needed once we reset this flag before each iteration > in SyncReplicationSlots() > Removed. > 4) > -synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid) > +synchronize_one_slot(WalReceiverConn * wrconn, RemoteSlot * remote_slot, > + Oid remote_dbid) > > wrconn not used anywhere. > Removed. > 5) > + bool is_refresh = (target_slot_list!= NIL); > > is_refresh is not needed. We can simply check if > target_slot_list!=NIL, then append it to cmd. > Changed. > 6) > * If remote_slot_list is NIL, fetches all failover logical slots from the > * primary server. If remote_slot_list is provided, refreshes only those > * specific slots with current values from the primary server. > > The usage of the word 'refreshing' is confusing. Since we are > allocating a new remote-list everytime (instead of reusing or > refreshing previous one), we can simply say: > > ------ > Fetches the failover logical slots info from the primary server > > If target_slot_list is NIL, fetches all failover logical slots from > the primary server, otherwise fetches only the ones mentioned in > target_slot_list > ------ > > The 'Parameters:' can also be adjusted accordingly. > Done. > > 7) > * Returns a list of RemoteSlot structures. If refreshing and the query fails, > * returns the original list. Slots that no longer exist on the primary will > * be removed from the list. > > This can be removed. > Done. > > 8) > - * If restart_lsn, confirmed_lsn or catalog_xmin is invalid but the > - * slot is valid, that means we have fetched the remote_slot in its > - * RS_EPHEMERAL state. In such a case, don't sync it; we can always > - * sync it in the next sync cycle when the remote_slot is persisted > - * and has valid lsn(s) and xmin values. > - * > - * XXX: In future, if we plan to expose 'slot->data.persistency' in > - * pg_replication_slots view, then we can avoid fetching RS_EPHEMERAL > - * slots in the first place. > + * Apply ephemeral slot filtering. Skip slots that are in RS_EPHEMERAL > + * state (invalid LSNs/xmin but not explicitly invalidated). > > We can retain the original comment. > Done. > 9) > Apart from above, there are many changes (alignement, comments etc) > which are not related to this particular improvement. We can get rid > of those changes. The patch should have the changes pertaining to > current improvement alone. > I've removed them. Attaching patch v8 addressing the above comments. regards, Ajin Cherian Fujitsu Australia
Вложения
On Tue, Aug 19, 2025 at 8:57 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > On Tue, 19 Aug 2025 at 10:25, Ajin Cherian <itsajin@gmail.com> wrote: > > > > > Attaching patch v7 addressing all the above comments. > > > Looks like this thread is not attached to any commitfest entry? > If so, can you please create one[0]? This will be beneficial for > thread, both simplifying patch review and (possibly) increasing the > number of reviewers. > > [0] https://commitfest.postgresql.org/55/new/ Done. https://commitfest.postgresql.org/patch/5976/ regards, Ajin Cherian Fujitsu Australia
On Wed, Aug 20, 2025 at 10:53 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> I've removed them.
> Attaching patch v8 addressing the above comments.
>
Thanks for the patch. Please find a few comments:
1)
When the API is in progress, and meanwhile in another session we turn
off hot_standby_feedback, the API session terminates abnormally.
postgres=# SELECT pg_sync_replication_slots();
server closed the connection unexpectedly
It seems slotsync_reread_config() is not adjusted for API. It does
proc_exit assuming it is a worker process.
2)
slotsync_worker_onexit():
SlotSyncCtx->slot_persistence_pending = false;
/*
* If syncing_slots is true, it indicates that the process errored out
* without resetting the flag. So, we need to clean up shared memory and
* reset the flag here.
*/
if (syncing_slots)
{
SlotSyncCtx->syncing = false;
syncing_slots = false;
}
Shall we reset slot_persistence_pending inside 'if (syncing_slots)'?
slot_persistence_pending can not be true without syncing_slots being
true.
3)
reset_syncing_flag():
SpinLockAcquire(&SlotSyncCtx->mutex);
SlotSyncCtx->syncing = false;
+ SlotSyncCtx->slot_persistence_pending = false;
SpinLockRelease(&SlotSyncCtx->mutex);
Here we are changing slot_persistence_pending under mutex, while at
other places, it is not protected by mutex. Is it intentional here?
4)
On rethinking, we maintain anything in shared memory if it has to be
shared between a few processes. 'slot_persistence_pending' OTOH is
required to be set and accessed by only one process at a time. Shall
we move it out of SlotSyncCtxStruct and keep it static similar to
'syncing_slots'? Rest of the setting, resetting flow remains the same.
What do you think?
5)
/* Build the query based on whether we're fetching all or refreshing
specific slots */
Perhaps we can shift this comment to where we actually append
target_slot_list. Better to have it something like:
'If target_slot_list is provided, construct the query only to fetch given slots'
thanks
Shveta
On Fri, Aug 22, 2025 at 3:44 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Aug 20, 2025 at 10:53 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > I've removed them.
> > Attaching patch v8 addressing the above comments.
> >
>
> Thanks for the patch. Please find a few comments:
>
> 1)
> When the API is in progress, and meanwhile in another session we turn
> off hot_standby_feedback, the API session terminates abnormally.
>
> postgres=# SELECT pg_sync_replication_slots();
> server closed the connection unexpectedly
>
> It seems slotsync_reread_config() is not adjusted for API. It does
> proc_exit assuming it is a worker process.
>
I've removed the API calling ProcessSlotSyncInterrupts() as I don't
think the API needs to specifically handle shutdown requests, it works
without calling this. And the other config checks, I don't think the
API needs to handle, I think we should leave it to the user.
> 2)
> slotsync_worker_onexit():
>
> SlotSyncCtx->slot_persistence_pending = false;
>
> /*
> * If syncing_slots is true, it indicates that the process errored out
> * without resetting the flag. So, we need to clean up shared memory and
> * reset the flag here.
> */
> if (syncing_slots)
> {
> SlotSyncCtx->syncing = false;
> syncing_slots = false;
> }
>
> Shall we reset slot_persistence_pending inside 'if (syncing_slots)'?
> slot_persistence_pending can not be true without syncing_slots being
> true.
>
> 3)
> reset_syncing_flag():
>
> SpinLockAcquire(&SlotSyncCtx->mutex);
> SlotSyncCtx->syncing = false;
> + SlotSyncCtx->slot_persistence_pending = false;
> SpinLockRelease(&SlotSyncCtx->mutex);
>
> Here we are changing slot_persistence_pending under mutex, while at
> other places, it is not protected by mutex. Is it intentional here?
>
> 4)
> On rethinking, we maintain anything in shared memory if it has to be
> shared between a few processes. 'slot_persistence_pending' OTOH is
> required to be set and accessed by only one process at a time. Shall
> we move it out of SlotSyncCtxStruct and keep it static similar to
> 'syncing_slots'? Rest of the setting, resetting flow remains the same.
> What do you think?
>
Yes, I agree. I have modified it accordingly.
>
> 5)
> /* Build the query based on whether we're fetching all or refreshing
> specific slots */
>
> Perhaps we can shift this comment to where we actually append
> target_slot_list. Better to have it something like:
> 'If target_slot_list is provided, construct the query only to fetch given slots'
>
Changed.
Attaching patch v9 addressing the above comments.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Aug 22, 2025 at 3:44 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Aug 20, 2025 at 10:53 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > >
> > > I've removed them.
> > > Attaching patch v8 addressing the above comments.
> > >
> >
> > Thanks for the patch. Please find a few comments:
> >
> > 1)
> > When the API is in progress, and meanwhile in another session we turn
> > off hot_standby_feedback, the API session terminates abnormally.
> >
> > postgres=# SELECT pg_sync_replication_slots();
> > server closed the connection unexpectedly
> >
> > It seems slotsync_reread_config() is not adjusted for API. It does
> > proc_exit assuming it is a worker process.
> >
>
> I've removed the API calling ProcessSlotSyncInterrupts() as I don't
> think the API needs to specifically handle shutdown requests, it works
> without calling this. And the other config checks, I don't think the
> API needs to handle, I think we should leave it to the user.
>
> > 2)
> > slotsync_worker_onexit():
> >
> > SlotSyncCtx->slot_persistence_pending = false;
> >
> > /*
> > * If syncing_slots is true, it indicates that the process errored out
> > * without resetting the flag. So, we need to clean up shared memory and
> > * reset the flag here.
> > */
> > if (syncing_slots)
> > {
> > SlotSyncCtx->syncing = false;
> > syncing_slots = false;
> > }
> >
> > Shall we reset slot_persistence_pending inside 'if (syncing_slots)'?
> > slot_persistence_pending can not be true without syncing_slots being
> > true.
> >
> > 3)
> > reset_syncing_flag():
> >
> > SpinLockAcquire(&SlotSyncCtx->mutex);
> > SlotSyncCtx->syncing = false;
> > + SlotSyncCtx->slot_persistence_pending = false;
> > SpinLockRelease(&SlotSyncCtx->mutex);
> >
> > Here we are changing slot_persistence_pending under mutex, while at
> > other places, it is not protected by mutex. Is it intentional here?
> >
> > 4)
> > On rethinking, we maintain anything in shared memory if it has to be
> > shared between a few processes. 'slot_persistence_pending' OTOH is
> > required to be set and accessed by only one process at a time. Shall
> > we move it out of SlotSyncCtxStruct and keep it static similar to
> > 'syncing_slots'? Rest of the setting, resetting flow remains the same.
> > What do you think?
> >
>
> Yes, I agree. I have modified it accordingly.
>
> >
> > 5)
> > /* Build the query based on whether we're fetching all or refreshing
> > specific slots */
> >
> > Perhaps we can shift this comment to where we actually append
> > target_slot_list. Better to have it something like:
> > 'If target_slot_list is provided, construct the query only to fetch given slots'
> >
>
> Changed.
>
> Attaching patch v9 addressing the above comments.
>
Thank You for the patches. Please find a few comments.
1)
Change not needed:
* without waiting for SLOTSYNC_RESTART_INTERVAL_SEC.
+ *
*/
2)
Regarding the naptime in API, I was thinking why not to use
wait_for_slot_activity() directly? If there are no slots activity, it
will keep on doubling the naptime starting from 2sec till max of
30sec. Thoughts?
We can rename MIN_SLOTSYNC_WORKER_NAPTIME_MS and
MAX_SLOTSYNC_WORKER_NAPTIME_MS to MIN_SLOTSYNC_NAPTIME_MS and
MAX_SLOTSYNC_NAPTIME_MS in such a case.
3)
+ * target_slot_list - List of RemoteSlot structures to refresh, or NIL to
+ * fetch all failover slots
Can we please change it to:
List of failover logical slots to fetch from primary, or NIL to fetch
all failover logical slots
4)
In the worker, before each call to synchronize_slots(), we are
starting a new transaction. It aligns with the previous implementation
where StartTransaction was inside synchronize_slots(). But in API, we
are doing StartTransaction once outside of the loop instead of doing
before each synchronize_slots(), is it intentional? It may keep the
transaction open for a long duration for the case where slots are not
getting persisted soon.
5)
With ProcessSlotSyncInterrupts() being removed from API, can you
please check the behaviour of API on smart-shutdown and rest of the
shutdown modes? It should behave like other APIs. And what happens if
I change primary_conninfo to some non-existing server when the API is
running. Does it error out or keep retrying?
thanks
Shveta
On Fri, Aug 29, 2025 at 3:01 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > 4) > In the worker, before each call to synchronize_slots(), we are > starting a new transaction. It aligns with the previous implementation > where StartTransaction was inside synchronize_slots(). But in API, we > are doing StartTransaction once outside of the loop instead of doing > before each synchronize_slots(), is it intentional? It may keep the > transaction open for a long duration for the case where slots are not > getting persisted soon. > I’ll address your other comments separately, but I wanted to respond to this one first. I did try the approach you suggested, but the issue is that we use the remote_slots list across loop iterations. If we end the transaction at the end of each iteration, the list gets freed and is no longer available for the next pass. Each iteration relies on the remote_slots list from the previous one to build the new list, which is why we can’t free it inside the loop. regards, Ajin Cherian Fujitsu Australia
On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Aug 29, 2025 at 3:01 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > 4)
> > In the worker, before each call to synchronize_slots(), we are
> > starting a new transaction. It aligns with the previous implementation
> > where StartTransaction was inside synchronize_slots(). But in API, we
> > are doing StartTransaction once outside of the loop instead of doing
> > before each synchronize_slots(), is it intentional? It may keep the
> > transaction open for a long duration for the case where slots are not
> > getting persisted soon.
> >
>
> I’ll address your other comments separately, but I wanted to respond
> to this one first. I did try the approach you suggested, but the issue
> is that we use the remote_slots list across loop iterations. If we end
> the transaction at the end of each iteration, the list gets freed and
> is no longer available for the next pass. Each iteration relies on the
> remote_slots list from the previous one to build the new list, which
> is why we can’t free it inside the loop.
Isn't that just a matter of allocating the list in appropriate long
lived memory context?
Here are some more comments
<para>
- The logical replication slots on the primary can be synchronized to
- the hot standby by using the <literal>failover</literal> parameter of
+ The logical replication slots on the primary can be enabled for
+ synchronization to the hot standby by using the
+ <literal>failover</literal> parameter of
This change corresponds to an existing feature. Should be a separate
patch, which we may want to backport.
- on the standby, the failover slots can be synchronized periodically in
+ <command>CREATE SUBSCRIPTION</command> during slot creation. After that,
+ synchronization can be be performed either manually by calling
+ <link linkend="pg-sync-replication-slots">
... snip ...
- <note>
- <para>
- While enabling <link linkend="guc-sync-replication-slots">
- <varname>sync_replication_slots</varname></link> allows for automatic
- periodic synchronization of failover slots, they can also be manually
... snip ...
I like the current documentation which separates the discussion of two
methods. I think we should just improve the second paragraph instead
of deleting it and changing the first one.
* without waiting for SLOTSYNC_RESTART_INTERVAL_SEC.
+ *
unnecessary blank line
+/*
+ * Flag used by pg_sync_replication_slots()
+ * to do retries if the slot did not persist while syncing.
+ */
+static bool slot_persistence_pending = false;
I don't think we need to keep a global variable for this. The variable
is used only inside SyncReplicationSlots() and the call depth is not
more than a few calls. From synchronize_slots(), before which the
variable is reset and after which the variable is checked, to
update_and_persist_local_synced_slot() which sets the variable, all
the functions return bool. All of them can be made to return an
integer status instead indicating the result of the operation. If we
do so we could check the return value of synchronize_slots() to decide
whether to retry or not, isntead of maintaining a global variable
which has a much wider scope than required. It's difficult to keep it
updated over the time.
+ * Parameters:
+ * wrconn - Connection to the primary server
+ * target_slot_list - List of RemoteSlot structures to refresh, or NIL to
+ * fetch all failover slots
*
- * Returns TRUE if any of the slots gets updated in this sync-cycle.
Need to describe the return value.
*/
-static bool
-synchronize_slots(WalReceiverConn *wrconn)
+static List *
+fetch_remote_slots(WalReceiverConn *wrconn, List *target_slot_list)
I like the way this function is broken down into two. That breaking down is
useful even without this feature.
- /* Construct the remote_slot tuple and synchronize each slot locally */
+ /* Process the slot information */
Probably these comments don't make much sense or they repeat what's
already there in the function prologue.
else
- /* Create list of remote slots */
+ /* Add to updated list */
Probably these comments don't make much sense or they repeat what's
already there in the function prologue.
@@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
The function is too cute to be useful. The code should be part of
ReplSlotSyncWorkerMain() just like other worker's main functions.
void
SyncReplicationSlots(WalReceiverConn *wrconn)
{
PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
{
Shouldn't this function call CheckForInterrupts() somewhere in the
loop since it could be potentially an infinite loop?
--
Best Wishes,
Ashutosh Bapat
On Fri, Aug 29, 2025 at 2:20 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > On Fri, Aug 29, 2025 at 3:01 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > 4) > > > In the worker, before each call to synchronize_slots(), we are > > > starting a new transaction. It aligns with the previous implementation > > > where StartTransaction was inside synchronize_slots(). But in API, we > > > are doing StartTransaction once outside of the loop instead of doing > > > before each synchronize_slots(), is it intentional? It may keep the > > > transaction open for a long duration for the case where slots are not > > > getting persisted soon. > > > > > > > I’ll address your other comments separately, but I wanted to respond > > to this one first. I did try the approach you suggested, but the issue > > is that we use the remote_slots list across loop iterations. If we end > > the transaction at the end of each iteration, the list gets freed and > > is no longer available for the next pass. Each iteration relies on the > > remote_slots list from the previous one to build the new list, which > > is why we can’t free it inside the loop. > > Isn't that just a matter of allocating the list in appropriate long > lived memory context? > +1. Since we're reallocating the list each time we fetch it from the remote server, it's not suitable for long-lived memory storage. Instead, should we extract the slot names during the initial fetch of failover slots and store them in the appropriate memory context? This extraction would only need to happen only when slot_persistence_pending is true during the first sync cycle. > Here are some more comments > <para> > - The logical replication slots on the primary can be synchronized to > - the hot standby by using the <literal>failover</literal> parameter of > + The logical replication slots on the primary can be enabled for > + synchronization to the hot standby by using the > + <literal>failover</literal> parameter of > > This change corresponds to an existing feature. Should be a separate > patch, which we may want to backport. > > - on the standby, the failover slots can be synchronized periodically in > + <command>CREATE SUBSCRIPTION</command> during slot creation. After that, > + synchronization can be be performed either manually by calling > + <link linkend="pg-sync-replication-slots"> > ... snip ... > - <note> > - <para> > - While enabling <link linkend="guc-sync-replication-slots"> > - <varname>sync_replication_slots</varname></link> allows for automatic > - periodic synchronization of failover slots, they can also be manually > ... snip ... > > I like the current documentation which separates the discussion of two > methods. I think we should just improve the second paragraph instead > of deleting it and changing the first one. > > * without waiting for SLOTSYNC_RESTART_INTERVAL_SEC. > + * > > unnecessary blank line > > +/* > + * Flag used by pg_sync_replication_slots() > + * to do retries if the slot did not persist while syncing. > + */ > +static bool slot_persistence_pending = false; > > I don't think we need to keep a global variable for this. The variable > is used only inside SyncReplicationSlots() and the call depth is not > more than a few calls. From synchronize_slots(), before which the > variable is reset and after which the variable is checked, to > update_and_persist_local_synced_slot() which sets the variable, all > the functions return bool. All of them can be made to return an > integer status instead indicating the result of the operation. If we > do so we could check the return value of synchronize_slots() to decide > whether to retry or not, isntead of maintaining a global variable > which has a much wider scope than required. It's difficult to keep it > updated over the time. > > + * Parameters: > + * wrconn - Connection to the primary server > + * target_slot_list - List of RemoteSlot structures to refresh, or NIL to > + * fetch all failover slots > * > - * Returns TRUE if any of the slots gets updated in this sync-cycle. > > Need to describe the return value. > > */ > -static bool > -synchronize_slots(WalReceiverConn *wrconn) > +static List * > +fetch_remote_slots(WalReceiverConn *wrconn, List *target_slot_list) > > I like the way this function is broken down into two. That breaking down is > useful even without this feature. > > - /* Construct the remote_slot tuple and synchronize each slot locally */ > + /* Process the slot information */ > > Probably these comments don't make much sense or they repeat what's > already there in the function prologue. > > else > - /* Create list of remote slots */ > + /* Add to updated list */ > > Probably these comments don't make much sense or they repeat what's > already there in the function prologue. > > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated) > > The function is too cute to be useful. The code should be part of > ReplSlotSyncWorkerMain() just like other worker's main functions. > I was thinking we can retain wait_for_slot_activity() as this can even be invoked from API flow. See my comment# 2 in [1] [1]: https://www.postgresql.org/message-id/CAJpy0uASzojKbzinpNu29xuYGsSRnSo%3D22CLhXaSt_43TVoBhQ%40mail.gmail.com thanks Shveta
On Fri, Aug 29, 2025 at 2:37 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Fri, Aug 29, 2025 at 2:20 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > > > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated) > > > > The function is too cute to be useful. The code should be part of > > ReplSlotSyncWorkerMain() just like other worker's main functions. > > > > I was thinking we can retain wait_for_slot_activity() as this can even > be invoked from API flow. See my comment# 2 in [1] We want the SQL callable function to finish as fast as possible, and make all the slots sync ready as fast as possible. So a shorter nap time makes sense. We don't want to increase it per iteration. But sync worker is a long running worker and can afford to wait longer. In fact it should wait longer so as not to load the primary and the standby. Given that the naptimes in both cases can not be controlled by the same logic, I think it's better not to use the same function. Each of them should have separate code for napping. That way the logic which decides the nap time is closer to the code that naps making it more readable. -- Best Wishes, Ashutosh Bapat
On Fri, Aug 29, 2025 at 4:14 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > On Fri, Aug 29, 2025 at 2:37 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Fri, Aug 29, 2025 at 2:20 PM Ashutosh Bapat > > <ashutosh.bapat.oss@gmail.com> wrote: > > > > > > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated) > > > > > > The function is too cute to be useful. The code should be part of > > > ReplSlotSyncWorkerMain() just like other worker's main functions. > > > > > > > I was thinking we can retain wait_for_slot_activity() as this can even > > be invoked from API flow. See my comment# 2 in [1] > > We want the SQL callable function to finish as fast as possible, and > make all the slots sync ready as fast as possible. So a shorter nap > time makes sense. We don't want to increase it per iteration. But sync > worker is a long running worker and can afford to wait longer. In fact > it should wait longer so as not to load the primary and the standby. > Given that the naptimes in both cases can not be controlled by the > same logic, I think it's better not to use the same function. Okay, makes sense. thanks Shveta
On Fri, Aug 29, 2025 at 3:01 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Thank You for the patches. Please find a few comments.
>
> 1)
> Change not needed:
>
> * without waiting for SLOTSYNC_RESTART_INTERVAL_SEC.
> + *
> */
>
Removed.
> 2)
> Regarding the naptime in API, I was thinking why not to use
> wait_for_slot_activity() directly? If there are no slots activity, it
> will keep on doubling the naptime starting from 2sec till max of
> 30sec. Thoughts?
>
> We can rename MIN_SLOTSYNC_WORKER_NAPTIME_MS and
> MAX_SLOTSYNC_WORKER_NAPTIME_MS to MIN_SLOTSYNC_NAPTIME_MS and
> MAX_SLOTSYNC_NAPTIME_MS in such a case.
>
Not changing this since further discussion clarified this.
> 3)
> + * target_slot_list - List of RemoteSlot structures to refresh, or NIL to
> + * fetch all failover slots
>
> Can we please change it to:
>
> List of failover logical slots to fetch from primary, or NIL to fetch
> all failover logical slots
>
Changed the variable itself.
> 4)
> In the worker, before each call to synchronize_slots(), we are
> starting a new transaction. It aligns with the previous implementation
> where StartTransaction was inside synchronize_slots(). But in API, we
> are doing StartTransaction once outside of the loop instead of doing
> before each synchronize_slots(), is it intentional? It may keep the
> transaction open for a long duration for the case where slots are not
> getting persisted soon.
>
I've added a new memory context to handle slot_names
> 5)
> With ProcessSlotSyncInterrupts() being removed from API, can you
> please check the behaviour of API on smart-shutdown and rest of the
> shutdown modes? It should behave like other APIs. And what happens if
> I change primary_conninfo to some non-existing server when the API is
> running. Does it error out or keep retrying?
>
I've tested with different types of shutdown and it seems to be
handled corerctly. However, yes, if configuration changed, the API
does not handle. I've written a new function
slotsync_api_reread_config() to specifically handle configuration
changes in API context as it is different from slotsync worker logic.
On Fri, Aug 29, 2025 at 6:50 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, Aug 29, 2025 at 3:01 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Tue, Aug 26, 2025 at 9:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > > 4)
> > > In the worker, before each call to synchronize_slots(), we are
> > > starting a new transaction. It aligns with the previous implementation
> > > where StartTransaction was inside synchronize_slots(). But in API, we
> > > are doing StartTransaction once outside of the loop instead of doing
> > > before each synchronize_slots(), is it intentional? It may keep the
> > > transaction open for a long duration for the case where slots are not
> > > getting persisted soon.
> > >
> >
> > I’ll address your other comments separately, but I wanted to respond
> > to this one first. I did try the approach you suggested, but the issue
> > is that we use the remote_slots list across loop iterations. If we end
> > the transaction at the end of each iteration, the list gets freed and
> > is no longer available for the next pass. Each iteration relies on the
> > remote_slots list from the previous one to build the new list, which
> > is why we can’t free it inside the loop.
>
> Isn't that just a matter of allocating the list in appropriate long
> lived memory context?
>
Yes, changed this.
> Here are some more comments
> <para>
> - The logical replication slots on the primary can be synchronized to
> - the hot standby by using the <literal>failover</literal> parameter of
> + The logical replication slots on the primary can be enabled for
> + synchronization to the hot standby by using the
> + <literal>failover</literal> parameter of
>
> This change corresponds to an existing feature. Should be a separate
> patch, which we may want to backport.
>
Removed.
> - on the standby, the failover slots can be synchronized periodically in
> + <command>CREATE SUBSCRIPTION</command> during slot creation. After that,
> + synchronization can be be performed either manually by calling
> + <link linkend="pg-sync-replication-slots">
> ... snip ...
> - <note>
> - <para>
> - While enabling <link linkend="guc-sync-replication-slots">
> - <varname>sync_replication_slots</varname></link> allows for automatic
> - periodic synchronization of failover slots, they can also be manually
> ... snip ...
>
> I like the current documentation which separates the discussion of two
> methods. I think we should just improve the second paragraph instead
> of deleting it and changing the first one.
>
> * without waiting for SLOTSYNC_RESTART_INTERVAL_SEC.
> + *
>
> unnecessary blank line
>
Changed.
> +/*
> + * Flag used by pg_sync_replication_slots()
> + * to do retries if the slot did not persist while syncing.
> + */
> +static bool slot_persistence_pending = false;
>
> I don't think we need to keep a global variable for this. The variable
> is used only inside SyncReplicationSlots() and the call depth is not
> more than a few calls. From synchronize_slots(), before which the
> variable is reset and after which the variable is checked, to
> update_and_persist_local_synced_slot() which sets the variable, all
> the functions return bool. All of them can be made to return an
> integer status instead indicating the result of the operation. If we
> do so we could check the return value of synchronize_slots() to decide
> whether to retry or not, isntead of maintaining a global variable
> which has a much wider scope than required. It's difficult to keep it
> updated over the time.
>
The problem is that all those calls synchronize_slots() and
update_and_persist_local_synced_slot() are shared with the slotsync
worker logic and API. Hence, changing this will affect slotsync_worker
logic as well. While the API needs to spefically retry only if the
initial sync fails, the slotsync worker will always be retrying. I
feel using a global variable is a more convenient way of doing this.
> + * Parameters:
> + * wrconn - Connection to the primary server
> + * target_slot_list - List of RemoteSlot structures to refresh, or NIL to
> + * fetch all failover slots
> *
> - * Returns TRUE if any of the slots gets updated in this sync-cycle.
>
> Need to describe the return value.
>
Added.
> */
> -static bool
> -synchronize_slots(WalReceiverConn *wrconn)
> +static List *
> +fetch_remote_slots(WalReceiverConn *wrconn, List *target_slot_list)
>
> I like the way this function is broken down into two. That breaking down is
> useful even without this feature.
>
> - /* Construct the remote_slot tuple and synchronize each slot locally */
> + /* Process the slot information */
>
> Probably these comments don't make much sense or they repeat what's
> already there in the function prologue.
>
> else
> - /* Create list of remote slots */
> + /* Add to updated list */
>
> Probably these comments don't make much sense or they repeat what's
> already there in the function prologue.
>
Removed these comments.
> @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
>
> The function is too cute to be useful. The code should be part of
> ReplSlotSyncWorkerMain() just like other worker's main functions.
>
But this wouldn't be part of this feature.
> void
> SyncReplicationSlots(WalReceiverConn *wrconn)
> {
> PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
> {
>
> Shouldn't this function call CheckForInterrupts() somewhere in the
> loop since it could be potentially an infinite loop?
I've tested this and I see that interrupts are being handled by
sending SIGQUIT and SIGINT to the backend process.
Attaching v10 with the above changes.
regards,
Ajin Cerian
Fujitsu Australia
Вложения
On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > Attaching v10 with the above changes. > The patch does not apply on HEAD. Can you please rebase? thanks Shveta
On Wed, Sep 3, 2025 at 6:47 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > Attaching v10 with the above changes. > > > > The patch does not apply on HEAD. Can you please rebase? Rebased and made a small change as well to use TopMemoryContext rather than create a new context for slot_list. regards, Ajin Cherian Fujitsu Australia
Вложения
On Wed, Sep 3, 2025 at 3:19 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 6:47 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > Attaching v10 with the above changes.
> > >
> >
> > The patch does not apply on HEAD. Can you please rebase?
>
> Rebased and made a small change as well to use TopMemoryContext rather
> than create a new context for slot_list.
>
Thanks for the patch. Please find a few comments:
1)
/* Clean up slot_names if allocated in TopMemoryContext */
if (slot_names)
{
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
list_free_deep(slot_names);
MemoryContextSwitchTo(oldcontext);
}
I think we can free slot_names without switching the context. Can you
please check this?
2)
We should add a comment for:
a) why we are using the slot-names from the first cycle instead of
fetching all failover slots in each cycle.
b) why are we relocating remote_slot list everytime.
3)
@@ -1130,7 +1180,7 @@ slotsync_reread_config(void)
- Assert(sync_replication_slots);
+ Assert(!AmLogicalSlotSyncWorkerProcess() || sync_replication_slots);
Do we still need this change after slotsync_api_reread_config?
4)
+static void ProcessSlotSyncInterrupts(void);
This is not needed.
5)
+ /* update flag, so that we retry */
+ slot_persistence_pending = true;
Can we tweak it to: 'Update the flag so that the API can retry'
6)
SyncReplicationSlots():
+ /* Free the current remote_slots list */
+ if (remote_slots)
+ list_free_deep(remote_slots);
Do we need a 'remote_slots' check, won't it manage it internally? We
don't have it in ReplSlotSyncWorkerMain().
7)
slotsync_api_reread_config
+ ereport(ERROR,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("cannot continue slot synchronization due to parameter changes"),
+ errdetail("Critical replication parameters (primary_conninfo,
primary_slot_name, or hot_standby_feedback) have changed since
pg_sync_replication_slots() started."),
+ errhint("Retry pg_sync_replication_slots() to use the updated
configuration.")));
I am unsure if we need to mention '(primary_conninfo,
primary_slot_name, or hot_standby_feedback)', but would like to know
what others think.
thanks
Shveta
On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> On Fri, Aug 29, 2025 at 6:50 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > +/*
> > + * Flag used by pg_sync_replication_slots()
> > + * to do retries if the slot did not persist while syncing.
> > + */
> > +static bool slot_persistence_pending = false;
> >
> > I don't think we need to keep a global variable for this. The variable
> > is used only inside SyncReplicationSlots() and the call depth is not
> > more than a few calls. From synchronize_slots(), before which the
> > variable is reset and after which the variable is checked, to
> > update_and_persist_local_synced_slot() which sets the variable, all
> > the functions return bool. All of them can be made to return an
> > integer status instead indicating the result of the operation. If we
> > do so we could check the return value of synchronize_slots() to decide
> > whether to retry or not, isntead of maintaining a global variable
> > which has a much wider scope than required. It's difficult to keep it
> > updated over the time.
> >
>
> The problem is that all those calls synchronize_slots() and
> update_and_persist_local_synced_slot() are shared with the slotsync
> worker logic and API. Hence, changing this will affect slotsync_worker
> logic as well. While the API needs to spefically retry only if the
> initial sync fails, the slotsync worker will always be retrying. I
> feel using a global variable is a more convenient way of doing this.
AFAICS, it's a matter of expanding the scope of what's returned by
those functions. The worker may not want to use the whole expanded
scope but the API will use it. That shouldn't change the functionality
of the worker, but it will help avoid the global variable - which have
much wider scope and their maintenance can be prone to bugs.
>
> > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
> >
> > The function is too cute to be useful. The code should be part of
> > ReplSlotSyncWorkerMain() just like other worker's main functions.
> >
>
> But this wouldn't be part of this feature.
>
> > void
> > SyncReplicationSlots(WalReceiverConn *wrconn)
> > {
> > PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
> > {
> >
> > Shouldn't this function call CheckForInterrupts() somewhere in the
> > loop since it could be potentially an infinite loop?
>
> I've tested this and I see that interrupts are being handled by
> sending SIGQUIT and SIGINT to the backend process.
Can you please point me to the code (the call to
CHECK_FOR_INTERRUPTS()) which processes these interrupts while
pg_sync_replication_slots() is executing, especially when the function
is waiting while syncing a slot.
--
Best Wishes,
Ashutosh Bapat
Hi, On Wed, Sep 3, 2025 at 3:20 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Wed, Sep 3, 2025 at 6:47 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > > > Attaching v10 with the above changes. > > > > > > > The patch does not apply on HEAD. Can you please rebase? > > Rebased and made a small change as well to use TopMemoryContext rather > than create a new context for slot_list. > If the remote slot is inactive since long and lagging behind the standby, this function (pg_sync_replication_slots) could end up waiting indefinitely. While it can certainly be cancelled manually, that behavior might not be ideal for everyone. That’s my understanding; please let me know if you see it differently. -- With Regards, Ashutosh Sharma.
On Fri, Sep 5, 2025 at 6:52 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > On Fri, Aug 29, 2025 at 6:50 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > +/*
> > > + * Flag used by pg_sync_replication_slots()
> > > + * to do retries if the slot did not persist while syncing.
> > > + */
> > > +static bool slot_persistence_pending = false;
> > >
> > > I don't think we need to keep a global variable for this. The variable
> > > is used only inside SyncReplicationSlots() and the call depth is not
> > > more than a few calls. From synchronize_slots(), before which the
> > > variable is reset and after which the variable is checked, to
> > > update_and_persist_local_synced_slot() which sets the variable, all
> > > the functions return bool. All of them can be made to return an
> > > integer status instead indicating the result of the operation. If we
> > > do so we could check the return value of synchronize_slots() to decide
> > > whether to retry or not, isntead of maintaining a global variable
> > > which has a much wider scope than required. It's difficult to keep it
> > > updated over the time.
> > >
> >
> > The problem is that all those calls synchronize_slots() and
> > update_and_persist_local_synced_slot() are shared with the slotsync
> > worker logic and API. Hence, changing this will affect slotsync_worker
> > logic as well. While the API needs to spefically retry only if the
> > initial sync fails, the slotsync worker will always be retrying. I
> > feel using a global variable is a more convenient way of doing this.
>
> AFAICS, it's a matter of expanding the scope of what's returned by
> those functions. The worker may not want to use the whole expanded
> scope but the API will use it. That shouldn't change the functionality
> of the worker, but it will help avoid the global variable - which have
> much wider scope and their maintenance can be prone to bugs.
>
> >
> > > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
> > >
> > > The function is too cute to be useful. The code should be part of
> > > ReplSlotSyncWorkerMain() just like other worker's main functions.
> > >
> >
> > But this wouldn't be part of this feature.
> >
> > > void
> > > SyncReplicationSlots(WalReceiverConn *wrconn)
> > > {
> > > PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
> > > {
> > >
> > > Shouldn't this function call CheckForInterrupts() somewhere in the
> > > loop since it could be potentially an infinite loop?
> >
> > I've tested this and I see that interrupts are being handled by
> > sending SIGQUIT and SIGINT to the backend process.
>
> Can you please point me to the code (the call to
> CHECK_FOR_INTERRUPTS()) which processes these interrupts while
> pg_sync_replication_slots() is executing, especially when the function
> is waiting while syncing a slot.
>
I noticed that the function libpqrcv_processTuples, which is invoked
by fetch_remote_slots, includes a CHECK_FOR_INTERRUPTS call. This is
currently helping in processing interrupts while we are in an infinite
loop within SyncReplicationSlots(). I’m just pointing this out based
on my observation while reviewing the changes in this patch. Ajin,
please correct me if I’m mistaken. If not, can we always rely on this
particular check for interrupts.
--
With Regards,
Ashutosh Sharma.
On Fri, Sep 5, 2025 at 11:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Wed, Sep 3, 2025 at 3:20 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > On Wed, Sep 3, 2025 at 6:47 PM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > > > > > Attaching v10 with the above changes. > > > > > > > > > > The patch does not apply on HEAD. Can you please rebase? > > > > Rebased and made a small change as well to use TopMemoryContext rather > > than create a new context for slot_list. > > > > If the remote slot is inactive since long and lagging behind the > standby, this function (pg_sync_replication_slots) could end up > waiting indefinitely. While it can certainly be cancelled manually, > that behavior might not be ideal for everyone. That’s my > understanding; please let me know if you see it differently. > Such a case can be addressed by having additional timeout parameters. We can do that as an additional patch if the use case is important enough to address. -- With Regards, Amit Kapila.
On Sat, Sep 6, 2025 at 12:05 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > On Fri, Sep 5, 2025 at 6:52 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > > > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote: > > > > > > I've tested this and I see that interrupts are being handled by > > > sending SIGQUIT and SIGINT to the backend process. > > > > Can you please point me to the code (the call to > > CHECK_FOR_INTERRUPTS()) which processes these interrupts while > > pg_sync_replication_slots() is executing, especially when the function > > is waiting while syncing a slot. > > > > I noticed that the function libpqrcv_processTuples, which is invoked > by fetch_remote_slots, includes a CHECK_FOR_INTERRUPTS call. This is > currently helping in processing interrupts while we are in an infinite > loop within SyncReplicationSlots(). I’m just pointing this out based > on my observation while reviewing the changes in this patch. Ajin, > please correct me if I’m mistaken. If not, can we always rely on this > particular check for interrupts. It doesn't seem good to rely on CHECKF_FOR_INTERRUPTS from so far away. It's better to have one being called from SyncReplicationSlots() which has the wait loop. That's how the other functions which have potentially long wait loops do. -- Best Wishes, Ashutosh Bapat
On Sat, Sep 6, 2025 at 9:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Sep 5, 2025 at 11:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > If the remote slot is inactive since long and lagging behind the > > standby, this function (pg_sync_replication_slots) could end up > > waiting indefinitely. While it can certainly be cancelled manually, > > that behavior might not be ideal for everyone. That’s my > > understanding; please let me know if you see it differently. > > > > Such a case can be addressed by having additional timeout parameters. > We can do that as an additional patch if the use case is important > enough to address. > Or we could rely on statement_timeout or the user cancelling the query explicitly. -- Best Wishes, Ashutosh Bapat
On Mon, Sep 8, 2025 at 9:51 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > On Sat, Sep 6, 2025 at 9:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Sep 5, 2025 at 11:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > > If the remote slot is inactive since long and lagging behind the > > > standby, this function (pg_sync_replication_slots) could end up > > > waiting indefinitely. While it can certainly be cancelled manually, > > > that behavior might not be ideal for everyone. That’s my > > > understanding; please let me know if you see it differently. > > > > > > > Such a case can be addressed by having additional timeout parameters. > > We can do that as an additional patch if the use case is important > > enough to address. > > > > Or we could rely on statement_timeout or the user cancelling the query > explicitly. > Sure. thanks Amit and Ashutosh. -- With Regards, Ashutosh Sharma.
On Fri, Sep 5, 2025 at 6:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > On Fri, Aug 29, 2025 at 6:50 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > +/*
> > > + * Flag used by pg_sync_replication_slots()
> > > + * to do retries if the slot did not persist while syncing.
> > > + */
> > > +static bool slot_persistence_pending = false;
> > >
> > > I don't think we need to keep a global variable for this. The variable
> > > is used only inside SyncReplicationSlots() and the call depth is not
> > > more than a few calls. From synchronize_slots(), before which the
> > > variable is reset and after which the variable is checked, to
> > > update_and_persist_local_synced_slot() which sets the variable, all
> > > the functions return bool. All of them can be made to return an
> > > integer status instead indicating the result of the operation. If we
> > > do so we could check the return value of synchronize_slots() to decide
> > > whether to retry or not, isntead of maintaining a global variable
> > > which has a much wider scope than required. It's difficult to keep it
> > > updated over the time.
> > >
> >
> > The problem is that all those calls synchronize_slots() and
> > update_and_persist_local_synced_slot() are shared with the slotsync
> > worker logic and API. Hence, changing this will affect slotsync_worker
> > logic as well. While the API needs to spefically retry only if the
> > initial sync fails, the slotsync worker will always be retrying. I
> > feel using a global variable is a more convenient way of doing this.
>
> AFAICS, it's a matter of expanding the scope of what's returned by
> those functions. The worker may not want to use the whole expanded
> scope but the API will use it. That shouldn't change the functionality
> of the worker, but it will help avoid the global variable - which have
> much wider scope and their maintenance can be prone to bugs.
>
I think this can be done.
> >
> > > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
> > >
> > > The function is too cute to be useful. The code should be part of
> > > ReplSlotSyncWorkerMain() just like other worker's main functions.
> > >
> >
> > But this wouldn't be part of this feature.
> >
> > > void
> > > SyncReplicationSlots(WalReceiverConn *wrconn)
> > > {
> > > PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
> > > {
> > >
> > > Shouldn't this function call CheckForInterrupts() somewhere in the
> > > loop since it could be potentially an infinite loop?
> >
> > I've tested this and I see that interrupts are being handled by
> > sending SIGQUIT and SIGINT to the backend process.
>
> Can you please point me to the code (the call to
> CHECK_FOR_INTERRUPTS()) which processes these interrupts while
> pg_sync_replication_slots() is executing, especially when the function
> is waiting while syncing a slot.
>
> > I noticed that the function libpqrcv_processTuples, which is invoked
> > by fetch_remote_slots, includes a CHECK_FOR_INTERRUPTS call. This is
> > currently helping in processing interrupts while we are in an infinite
> > loop within SyncReplicationSlots(). I’m just pointing this out based
> > on my observation while reviewing the changes in this patch. Ajin,
> > please correct me if I’m mistaken. If not, can we always rely on this
> > particular check for interrupts.
>
> It doesn't seem good to rely on CHECKF_FOR_INTERRUPTS from so far
> away. It's better to have one being called from SyncReplicationSlots()
> which has the wait loop. That's how the other functions which have
> potentially long wait loops do.
+1
thanks
Shveta
Hi,
Sharing some of my review comments, please look if these make sense to you.
On Wed, Sep 3, 2025 at 3:20 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 6:47 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > Attaching v10 with the above changes.
> > >
> >
> > The patch does not apply on HEAD. Can you please rebase?
>
> Rebased and made a small change as well to use TopMemoryContext rather
> than create a new context for slot_list.
>
+ /*
+ * If we've been promoted, then no point
+ * continuing.
+ */
+ if (SlotSyncCtx->stopSignaled)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exiting from slot synchronization as"
+ " promotion is triggered")));
+ break;
+ }
"break" statement here looks redundant to me.
--
+ *
+ * Repeatedly fetches and updates replication slot information from the
+ * primary until all slots are at least "sync ready". Retry is done after 2
+ * sec wait. Exits early if promotion is triggered or certain critical
+ * configuration parameters have changed.
*/
wait for 2 seconds before retrying - the constant is
SLOTSYNC_API_NAPTIME_MS, so technically it may *not* always be 2s if
the macro changes. Maybe reword to “wait for SLOTSYNC_API_NAPTIME_MS
before retrying” would look better?
--
/* Retry until all slots are sync ready atleast */
and
/* Done if all slots are atleast sync ready */
atleast -> "at least". I am just making this comment because at few
places in the same file I see "at least" and not "atleast".
--
+static void ProcessSlotSyncInterrupts(void);
Is this change related to this patch?
--
+ <command>CREATE SUBSCRIPTION</command> during slot creation. After that,
+ synchronization can be be performed either manually by calling
+ <link linkend="pg-sync-replication-slots">
double "be".
--
With Regards,
Ashutosh Sharma.
On Thu, Sep 4, 2025 at 4:35 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 3:19 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Thanks for the patch. Please find a few comments:
>
> 1)
> /* Clean up slot_names if allocated in TopMemoryContext */
> if (slot_names)
> {
> oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> list_free_deep(slot_names);
> MemoryContextSwitchTo(oldcontext);
> }
>
> I think we can free slot_names without switching the context. Can you
> please check this?
>
Removed this.
> 2)
> We should add a comment for:
> a) why we are using the slot-names from the first cycle instead of
> fetching all failover slots in each cycle.
> b) why are we relocating remote_slot list everytime.
>
I have added a comment, let me know if any more is required.
>
> 3)
> @@ -1130,7 +1180,7 @@ slotsync_reread_config(void)
>
> - Assert(sync_replication_slots);
> + Assert(!AmLogicalSlotSyncWorkerProcess() || sync_replication_slots);
>
> Do we still need this change after slotsync_api_reread_config?
>
Removed.
> 4)
> +static void ProcessSlotSyncInterrupts(void);
>
> This is not needed.
>
Removed.
>
> 5)
>
> + /* update flag, so that we retry */
> + slot_persistence_pending = true;
>
> Can we tweak it to: 'Update the flag so that the API can retry'
Updated.
>
> 6)
> SyncReplicationSlots():
> + /* Free the current remote_slots list */
> + if (remote_slots)
> + list_free_deep(remote_slots);
>
> Do we need a 'remote_slots' check, won't it manage it internally? We
> don't have it in ReplSlotSyncWorkerMain().
>
Changed.
> 7)
> slotsync_api_reread_config
>
> + ereport(ERROR,
> + (errcode(ERRCODE_CONFIG_FILE_ERROR),
> + errmsg("cannot continue slot synchronization due to parameter changes"),
> + errdetail("Critical replication parameters (primary_conninfo,
> primary_slot_name, or hot_standby_feedback) have changed since
> pg_sync_replication_slots() started."),
> + errhint("Retry pg_sync_replication_slots() to use the updated
> configuration.")));
>
> I am unsure if we need to mention '(primary_conninfo,
> primary_slot_name, or hot_standby_feedback)', but would like to know
> what others think.
>
Leaving this as is now.
On Mon, Sep 8, 2025 at 2:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Sat, Sep 6, 2025 at 12:05 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Fri, Sep 5, 2025 at 6:52 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > I've tested this and I see that interrupts are being handled by
> > > > sending SIGQUIT and SIGINT to the backend process.
> > >
> > > Can you please point me to the code (the call to
> > > CHECK_FOR_INTERRUPTS()) which processes these interrupts while
> > > pg_sync_replication_slots() is executing, especially when the function
> > > is waiting while syncing a slot.
> > >
> >
> > I noticed that the function libpqrcv_processTuples, which is invoked
> > by fetch_remote_slots, includes a CHECK_FOR_INTERRUPTS call. This is
> > currently helping in processing interrupts while we are in an infinite
> > loop within SyncReplicationSlots(). I’m just pointing this out based
> > on my observation while reviewing the changes in this patch. Ajin,
> > please correct me if I’m mistaken. If not, can we always rely on this
> > particular check for interrupts.
>
> It doesn't seem good to rely on CHECKF_FOR_INTERRUPTS from so far
> away. It's better to have one being called from SyncReplicationSlots()
> which has the wait loop. That's how the other functions which have
> potentially long wait loops do.
>
Ok, I agree. Added the Check.
On Mon, Sep 8, 2025 at 2:33 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
> Sharing some of my review comments, please look if these make sense to you.
>
>
> + /*
> + * If we've been promoted, then no point
> + * continuing.
> + */
> + if (SlotSyncCtx->stopSignaled)
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("exiting from slot synchronization as"
> + " promotion is triggered")));
> + break;
> + }
> "break" statement here looks redundant to me.
>
Removed.
> --
>
> + *
> + * Repeatedly fetches and updates replication slot information from the
> + * primary until all slots are at least "sync ready". Retry is done after 2
> + * sec wait. Exits early if promotion is triggered or certain critical
> + * configuration parameters have changed.
> */
>
> wait for 2 seconds before retrying - the constant is
> SLOTSYNC_API_NAPTIME_MS, so technically it may *not* always be 2s if
> the macro changes. Maybe reword to “wait for SLOTSYNC_API_NAPTIME_MS
> before retrying” would look better?
>
I've removed the reference to 2 seconds.
> --
>
> /* Retry until all slots are sync ready atleast */
>
> and
>
>
> /* Done if all slots are atleast sync ready */
>
> atleast -> "at least". I am just making this comment because at few
> places in the same file I see "at least" and not "atleast".
>
Changed.
> --
>
> +static void ProcessSlotSyncInterrupts(void);
>
> Is this change related to this patch?
>
It was used earlier, and forgot to change it. Removed now.
> --
>
> + <command>CREATE SUBSCRIPTION</command> during slot creation. After that,
> + synchronization can be be performed either manually by calling
> + <link linkend="pg-sync-replication-slots">
>
> double "be".
>
Fixed
On Fri, Sep 5, 2025 at 11:21 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> AFAICS, it's a matter of expanding the scope of what's returned by
> those functions. The worker may not want to use the whole expanded
> scope but the API will use it. That shouldn't change the functionality
> of the worker, but it will help avoid the global variable - which have
> much wider scope and their maintenance can be prone to bugs.
>
> >
Ok, added a new return parameter for these functions that will return
if there are any slots pending persistence and removed the global
variable.
Attached v11 patch addressing the above comments.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Tue, Sep 9, 2025 at 5:37 PM Ajin Cherian <itsajin@gmail.com> wrote: > > Attached v11 patch addressing the above comments. > Please find a few comments: 1) + Retry is done after 2 + * sec wait. Exits early if promotion is triggered or certain critical We can say: Retry is done after SLOTSYNC_API_NAPTIME_MS wait. 2) + /* + * Fetch remote slot info for the given slot_names. If slot_names is NIL, + * fetch all failover-enabled slots. Note that we reuse slot_names from + * the previous iteration; re-fetching all failover slots each time could + * cause an endless loop. + */ a) the previous iteration --> the first iteration. b) Also we can mention the reason why we take names from first iteration instead of going for pending ones alone, something like: Instead of reprocessing only the pending slots in each iteration, it's better to process all the slots received in the first iteration. This ensures that by the time we're done, all slots reflect the latest values. 3) + remote_slots = fetch_remote_slots(wrconn, slot_names); + + + /* Attempt to synchronize slots */ + synchronize_slots(wrconn, remote_slots, &slot_persistence_pending); One extra blank line can be removed 4) + /* Clean up slot_names if allocated in TopMemoryContext */ + if (slot_names) + list_free_deep(slot_names); Can we please move it before 'ReplicationSlotCleanup'. 5) In case of error in subsequent iteration, slot_names allocated from TopMemoryContext will be left unfreed? 6) + ListCell *lc; + bool first_slot = true; Shall we move these two to concerned if-block: if (slot_names != NIL) 7) * The slot_persistence_pending flag is used by the pg_sync_replication_slots * API to track if any slots could not be persisted and need to be retried. a) Instead of mentioning only about slot_persistence_pending argument in concerned function's header, we shall define all the arguments. b) We can remove the 'flag' term from the comments as it is a function-argument now. 8) I think we should add briefly in the header of the file about the new behaviour of API. thanks Shveta
On Wed, Sep 10, 2025 at 2:45 PM shveta malik <shveta.malik@gmail.com> wrote: > > On Tue, Sep 9, 2025 at 5:37 PM Ajin Cherian <itsajin@gmail.com> wrote: > > > > Attached v11 patch addressing the above comments. > > > > Please find a few comments: > > 1) > > + Retry is done after 2 > + * sec wait. Exits early if promotion is triggered or certain critical > > We can say: Retry is done after SLOTSYNC_API_NAPTIME_MS wait. > Changed. > 2) > + /* > + * Fetch remote slot info for the given slot_names. If slot_names is NIL, > + * fetch all failover-enabled slots. Note that we reuse slot_names from > + * the previous iteration; re-fetching all failover slots each time could > + * cause an endless loop. > + */ > > a) > the previous iteration --> the first iteration. > > b) Also we can mention the reason why we take names from first > iteration instead of going for pending ones alone, something like: > > Instead of reprocessing only the pending slots in each iteration, it's > better to process all the slots received in the first iteration. > This ensures that by the time we're done, all slots reflect the latest values. > > 3) > + remote_slots = fetch_remote_slots(wrconn, slot_names); > + > + > + /* Attempt to synchronize slots */ > + synchronize_slots(wrconn, remote_slots, &slot_persistence_pending); > > One extra blank line can be removed > Fixed. > 4) > > + /* Clean up slot_names if allocated in TopMemoryContext */ > + if (slot_names) > + list_free_deep(slot_names); > > Can we please move it before 'ReplicationSlotCleanup'. > Fixed. > 5) > In case of error in subsequent iteration, slot_names allocated from > TopMemoryContext will be left unfreed? > I've changed the logic so that even on error, slot_names are freed. > 6) > + ListCell *lc; > + bool first_slot = true; > > Shall we move these two to concerned if-block: > if (slot_names != NIL) > Changed. > 7) > * The slot_persistence_pending flag is used by the pg_sync_replication_slots > * API to track if any slots could not be persisted and need to be retried. > > a) Instead of mentioning only about slot_persistence_pending argument > in concerned function's header, we shall define all the arguments. > > b) We can remove the 'flag' term from the comments as it is a > function-argument now. > Changed. > 8) > I think we should add briefly in the header of the file about the new > behaviour of API. > Added. Attaching patch v12 addressing these comments. regards, Ajin Cherian Fujitsu Australia
Вложения
On Mon, Sep 15, 2025 at 6:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Sep 10, 2025 at 2:45 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 5:37 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > Attached v11 patch addressing the above comments.
> > >
> >
> > Please find a few comments:
> >
> > 1)
> >
> > + Retry is done after 2
> > + * sec wait. Exits early if promotion is triggered or certain critical
> >
> > We can say: Retry is done after SLOTSYNC_API_NAPTIME_MS wait.
> >
>
> Changed.
>
> > 2)
> > + /*
> > + * Fetch remote slot info for the given slot_names. If slot_names is NIL,
> > + * fetch all failover-enabled slots. Note that we reuse slot_names from
> > + * the previous iteration; re-fetching all failover slots each time could
> > + * cause an endless loop.
> > + */
> >
> > a)
> > the previous iteration --> the first iteration.
> >
> > b) Also we can mention the reason why we take names from first
> > iteration instead of going for pending ones alone, something like:
> >
> > Instead of reprocessing only the pending slots in each iteration, it's
> > better to process all the slots received in the first iteration.
> > This ensures that by the time we're done, all slots reflect the latest values.
> >
> > 3)
> > + remote_slots = fetch_remote_slots(wrconn, slot_names);
> > +
> > +
> > + /* Attempt to synchronize slots */
> > + synchronize_slots(wrconn, remote_slots, &slot_persistence_pending);
> >
> > One extra blank line can be removed
> >
>
> Fixed.
>
> > 4)
> >
> > + /* Clean up slot_names if allocated in TopMemoryContext */
> > + if (slot_names)
> > + list_free_deep(slot_names);
> >
> > Can we please move it before 'ReplicationSlotCleanup'.
> >
>
> Fixed.
>
> > 5)
> > In case of error in subsequent iteration, slot_names allocated from
> > TopMemoryContext will be left unfreed?
> >
>
> I've changed the logic so that even on error, slot_names are freed.
I see that you have freed 'slot_names' in config-changed and promotion
case but the ERROR can come from other flows as well. The idea was to
somehow to free it (if possible) in slotsync_failure_callback() by
passing it as an argument, like we do for 'wrconn'.
> > 6)
> > + ListCell *lc;
> > + bool first_slot = true;
> >
> > Shall we move these two to concerned if-block:
> > if (slot_names != NIL)
> >
>
> Changed.
>
> > 7)
> > * The slot_persistence_pending flag is used by the pg_sync_replication_slots
> > * API to track if any slots could not be persisted and need to be retried.
> >
> > a) Instead of mentioning only about slot_persistence_pending argument
> > in concerned function's header, we shall define all the arguments.
> >
> > b) We can remove the 'flag' term from the comments as it is a
> > function-argument now.
> >
>
> Changed.
>
> > 8)
> > I think we should add briefly in the header of the file about the new
> > behaviour of API.
> >
>
> Added.
>
> Attaching patch v12 addressing these comments.
>
Thank You for the patch. Please find a few comments:
1)
+ bool slot_persistence_pending = false;
We can move this declaration outside of the loop. And I think we don't
need to initialize as we are resetting it to false before each
iteration.
2)
+ /* Switch to long-lived TopMemoryContext to store slot names */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Extract slot names from the remote slots */
+ slot_names = extract_slot_names(remote_slots);
+
+ MemoryContextSwitchTo(oldcontext);
I think it will be better if we move 'MemoryContextSwitchTo' calls
inside extract_slot_names() itself. The entire logic related to
'slot_names' will then be consolidated in one place
3)
+ * The slot_persistence_pending flag is used by the pg_sync_replication_slots
+ * API to track if any slots could not be persisted and need to be retried.
Can we change it to below. We can have it started in a new line after
a blank line (see how remote_slot_precedes, found_consistent_snapshot
are defined)
*slot_persistence_pending is set to true if any of the slots fail to
persist. It is utilized by the pg_sync_replication_slots() API.
Please change it in both synchronize_one_slot() and
update_and_persist_local_synced_slot()
4)
a)
+ Update the
+ * slot_persistence_pending flag, so the API can retry.
*/
b)
/* update the flag, so that the API can retry */
It will be good if we can remove 'flag' usage from both occurrences in
update_and_persist_local_synced_slot().
5)
Similar to ProcessSlotSyncInterrupts() for worker, shall we have one
such function for API which can have all 3 things:
{
/*
* If we've been promoted, then no point
* continuing.
*/
if (SlotSyncCtx->stopSignaled)
{
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("exiting from slot synchronization as"
" promotion is triggered")));
}
CHECK_FOR_INTERRUPTS();
if (ConfigReloadPending)
slotsync_api_reread_config();
}
And similar to the worker case, we can have it checked in the
beginning of the loop. Thoughts?
thanks
Shveta
On Tue, Sep 16, 2025 at 11:53 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 15, 2025 at 6:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Wed, Sep 10, 2025 at 2:45 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Tue, Sep 9, 2025 at 5:37 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > Attached v11 patch addressing the above comments.
> > > >
> > >
> > > Please find a few comments:
> > >
> > > 1)
> > >
> > > + Retry is done after 2
> > > + * sec wait. Exits early if promotion is triggered or certain critical
> > >
> > > We can say: Retry is done after SLOTSYNC_API_NAPTIME_MS wait.
> > >
> >
> > Changed.
> >
> > > 2)
> > > + /*
> > > + * Fetch remote slot info for the given slot_names. If slot_names is NIL,
> > > + * fetch all failover-enabled slots. Note that we reuse slot_names from
> > > + * the previous iteration; re-fetching all failover slots each time could
> > > + * cause an endless loop.
> > > + */
> > >
> > > a)
> > > the previous iteration --> the first iteration.
> > >
> > > b) Also we can mention the reason why we take names from first
> > > iteration instead of going for pending ones alone, something like:
> > >
> > > Instead of reprocessing only the pending slots in each iteration, it's
> > > better to process all the slots received in the first iteration.
> > > This ensures that by the time we're done, all slots reflect the latest values.
> > >
> > > 3)
> > > + remote_slots = fetch_remote_slots(wrconn, slot_names);
> > > +
> > > +
> > > + /* Attempt to synchronize slots */
> > > + synchronize_slots(wrconn, remote_slots, &slot_persistence_pending);
> > >
> > > One extra blank line can be removed
> > >
> >
> > Fixed.
> >
> > > 4)
> > >
> > > + /* Clean up slot_names if allocated in TopMemoryContext */
> > > + if (slot_names)
> > > + list_free_deep(slot_names);
> > >
> > > Can we please move it before 'ReplicationSlotCleanup'.
> > >
> >
> > Fixed.
> >
> > > 5)
> > > In case of error in subsequent iteration, slot_names allocated from
> > > TopMemoryContext will be left unfreed?
> > >
> >
> > I've changed the logic so that even on error, slot_names are freed.
>
> I see that you have freed 'slot_names' in config-changed and promotion
> case but the ERROR can come from other flows as well. The idea was to
> somehow to free it (if possible) in slotsync_failure_callback() by
> passing it as an argument, like we do for 'wrconn'.
>
Are you suggesting introducing a structure (for example, SlotSyncContext as shown below) that encapsulates both wrconn and slot_names, and then passing a pointer to this structure as the Datum argument to the slotsync_failure_callback cleanup function, so that the callback can handle freeing wrconn and slot_names and maybe some other members within the structure that allocate memory?
/*
* Extended structure that can hold both connection and slot_names info
*/
typedef struct SlotSyncContext
{
WalReceiverConn *wrconn; /* Must be first for compatibility */} SlotSyncContext;
List *slot_names; /* Pointer to slot_names list */
bool extended; /* Flag to indicate extended context */
SyncReplicationSlots(WalReceiverConn *wrconn)
{
SlotSyncContext sync_ctx;
...
...
/* Initialize extended context */
sync_ctx.wrconn = wrconn;
sync_ctx.slot_names_ptr = &slot_names;
sync_ctx.extended = true;
PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(&sync_ctx));
...
}
--
With Regards,
Ashutosh Sharma.
On Tue, Sep 16, 2025 at 5:12 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
>
> On Tue, Sep 16, 2025 at 11:53 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Sep 15, 2025 at 6:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Wed, Sep 10, 2025 at 2:45 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Tue, Sep 9, 2025 at 5:37 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > > >
> > > > > Attached v11 patch addressing the above comments.
> > > > >
> > > >
> > > > Please find a few comments:
> > > >
> > > > 1)
> > > >
> > > > + Retry is done after 2
> > > > + * sec wait. Exits early if promotion is triggered or certain critical
> > > >
> > > > We can say: Retry is done after SLOTSYNC_API_NAPTIME_MS wait.
> > > >
> > >
> > > Changed.
> > >
> > > > 2)
> > > > + /*
> > > > + * Fetch remote slot info for the given slot_names. If slot_names is NIL,
> > > > + * fetch all failover-enabled slots. Note that we reuse slot_names from
> > > > + * the previous iteration; re-fetching all failover slots each time could
> > > > + * cause an endless loop.
> > > > + */
> > > >
> > > > a)
> > > > the previous iteration --> the first iteration.
> > > >
> > > > b) Also we can mention the reason why we take names from first
> > > > iteration instead of going for pending ones alone, something like:
> > > >
> > > > Instead of reprocessing only the pending slots in each iteration, it's
> > > > better to process all the slots received in the first iteration.
> > > > This ensures that by the time we're done, all slots reflect the latest values.
> > > >
> > > > 3)
> > > > + remote_slots = fetch_remote_slots(wrconn, slot_names);
> > > > +
> > > > +
> > > > + /* Attempt to synchronize slots */
> > > > + synchronize_slots(wrconn, remote_slots, &slot_persistence_pending);
> > > >
> > > > One extra blank line can be removed
> > > >
> > >
> > > Fixed.
> > >
> > > > 4)
> > > >
> > > > + /* Clean up slot_names if allocated in TopMemoryContext */
> > > > + if (slot_names)
> > > > + list_free_deep(slot_names);
> > > >
> > > > Can we please move it before 'ReplicationSlotCleanup'.
> > > >
> > >
> > > Fixed.
> > >
> > > > 5)
> > > > In case of error in subsequent iteration, slot_names allocated from
> > > > TopMemoryContext will be left unfreed?
> > > >
> > >
> > > I've changed the logic so that even on error, slot_names are freed.
> >
> > I see that you have freed 'slot_names' in config-changed and promotion
> > case but the ERROR can come from other flows as well. The idea was to
> > somehow to free it (if possible) in slotsync_failure_callback() by
> > passing it as an argument, like we do for 'wrconn'.
> >
>
> Are you suggesting introducing a structure (for example, SlotSyncContext as shown below) that encapsulates both
wrconnand slot_names, and then passing a pointer to this structure as the Datum argument to the
slotsync_failure_callbackcleanup function, so that the callback can handle freeing wrconn and slot_names and maybe some
othermembers within the structure that allocate memory?
>
Yes, as I do not see any other simpler way to take care of this
memory-free in all ERROR scenarios.
> /*
> * Extended structure that can hold both connection and slot_names info
> */
> typedef struct SlotSyncContext
> {
>
> WalReceiverConn *wrconn; /* Must be first for compatibility */
> List *slot_names; /* Pointer to slot_names list */
> bool extended; /* Flag to indicate extended context */
>
> } SlotSyncContext;
>
> SyncReplicationSlots(WalReceiverConn *wrconn)
> {
>
> SlotSyncContext sync_ctx;
> ...
> ...
> /* Initialize extended context */
> sync_ctx.wrconn = wrconn;
> sync_ctx.slot_names_ptr = &slot_names;
> sync_ctx.extended = true;
>
>
> PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(&sync_ctx));
>
Yes, like this.
thanks
Shveta
On Tue, Sep 16, 2025 at 4:23 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 15, 2025 at 6:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> Thank You for the patch. Please find a few comments:
>
> 1)
> + bool slot_persistence_pending = false;
>
> We can move this declaration outside of the loop. And I think we don't
> need to initialize as we are resetting it to false before each
> iteration.
>
Fixed.
> 2)
>
> + /* Switch to long-lived TopMemoryContext to store slot names */
> + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> +
> + /* Extract slot names from the remote slots */
> + slot_names = extract_slot_names(remote_slots);
> +
> + MemoryContextSwitchTo(oldcontext);
>
> I think it will be better if we move 'MemoryContextSwitchTo' calls
> inside extract_slot_names() itself. The entire logic related to
> 'slot_names' will then be consolidated in one place
>
Changed,
> 3)
> + * The slot_persistence_pending flag is used by the pg_sync_replication_slots
> + * API to track if any slots could not be persisted and need to be retried.
>
> Can we change it to below. We can have it started in a new line after
> a blank line (see how remote_slot_precedes, found_consistent_snapshot
> are defined)
>
> *slot_persistence_pending is set to true if any of the slots fail to
> persist. It is utilized by the pg_sync_replication_slots() API.
>
> Please change it in both synchronize_one_slot() and
> update_and_persist_local_synced_slot()
>
Changed.
> 4)
> a)
> + Update the
> + * slot_persistence_pending flag, so the API can retry.
> */
>
> b)
> /* update the flag, so that the API can retry */
>
> It will be good if we can remove 'flag' usage from both occurrences in
> update_and_persist_local_synced_slot().
>
Changed.
> 5)
> Similar to ProcessSlotSyncInterrupts() for worker, shall we have one
> such function for API which can have all 3 things:
>
> {
> /*
> * If we've been promoted, then no point
> * continuing.
> */
> if (SlotSyncCtx->stopSignaled)
> {
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("exiting from slot synchronization as"
> " promotion is triggered")));
> }
>
> CHECK_FOR_INTERRUPTS();
>
> if (ConfigReloadPending)
> slotsync_api_reread_config();
> }
>
> And similar to the worker case, we can have it checked in the
> beginning of the loop. Thoughts?
>
Changed it and added a function - ProcessSlotSyncAPIChanges()
Created a patch v13 with these changes.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> Created a patch v13 with these changes.
>
Please find a few comments:
1)
+ /* update the failure structure so that it can be freed on error */
+ fparams.slot_names = slot_names;
+
Since slot_names is assigned only once, we can make the above
assignment as well only once, inside the if-block where we initialize
slot_names.
2)
extract_slot_names():
+ foreach(lc, remote_slots)
+ {
+ RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
+ char *slot_name;
+
+ /* Switch to long-lived TopMemoryContext to store slot names */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ slot_name = pstrdup(remote_slot->name);
+ slot_names = lappend(slot_names, slot_name);
+
+ MemoryContextSwitchTo(oldcontext);
+ }
It will be better to move 'MemoryContextSwitchTo' calls outside of the
loop. No need to switch the context for each slot.
3)
ProcessSlotSyncAPIChanges() gives a feeling that it is actually
processing API changes where instead it is processing interrupts or
config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()
4)
I prefer version 11's slotsync_api_reread_config() over current
slotsync_api_config_changed(). There, even error was taken care of
inside the function, which to me looked better and similar to how
slotsync worker deals with it.
I have made some comment changes, attached the patch. Please include
it if you find it okay.
thanks
Shveta
Вложения
On Tue, Sep 23, 2025 at 10:29 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > Created a patch v13 with these changes.
> >
>
> Please find a few comments:
>
> 1)
> + /* update the failure structure so that it can be freed on error */
> + fparams.slot_names = slot_names;
> +
>
> Since slot_names is assigned only once, we can make the above
> assignment as well only once, inside the if-block where we initialize
> slot_names.
>
> 2)
> extract_slot_names():
>
> + foreach(lc, remote_slots)
> + {
> + RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
> + char *slot_name;
> +
> + /* Switch to long-lived TopMemoryContext to store slot names */
> + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> +
> + slot_name = pstrdup(remote_slot->name);
> + slot_names = lappend(slot_names, slot_name);
> +
> + MemoryContextSwitchTo(oldcontext);
> + }
>
> It will be better to move 'MemoryContextSwitchTo' calls outside of the
> loop. No need to switch the context for each slot.
>
> 3)
> ProcessSlotSyncAPIChanges() gives a feeling that it is actually
> processing API changes where instead it is processing interrupts or
> config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()
>
> 4)
> I prefer version 11's slotsync_api_reread_config() over current
> slotsync_api_config_changed(). There, even error was taken care of
> inside the function, which to me looked better and similar to how
> slotsync worker deals with it.
>
> I have made some comment changes, attached the patch. Please include
> it if you find it okay.
>
Tested the patch, few more suggestions
5)
Currently the error message is:
postgres=# SELECT pg_sync_replication_slots();
ERROR: cannot continue slot synchronization due to parameter changes
DETAIL: Critical replication parameters (primary_conninfo,
primary_slot_name, or hot_standby_feedback) have changed since
pg_sync_replication_slots() started.
HINT: Retry pg_sync_replication_slots() to use the updated configuration.
a)
To be consistent with other error-messages, can we change ERROR msg
to: 'cannot continue replication slots synchronization due to
parameter changes'
b)
There is double space in DETAIL msg: "have changed since"
Will it be better to shorten the DETAIL as: 'One or more of
primary_conninfo, primary_slot_name, or hot_standby_feedback were
modified.'
6)
postgres=# SELECT pg_sync_replication_slots();
ERROR: exiting from slot synchronization as promotion is triggered
Shall we rephrase it similar to the previous message: 'cannot continue
replication slots synchronization as standby promotion is triggered'
thanks
Shveta
On Tue, Sep 23, 2025 at 2:59 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > Created a patch v13 with these changes.
> >
>
> Please find a few comments:
>
> 1)
> + /* update the failure structure so that it can be freed on error */
> + fparams.slot_names = slot_names;
> +
>
> Since slot_names is assigned only once, we can make the above
> assignment as well only once, inside the if-block where we initialize
> slot_names.
>
Changed.
> 2)
> extract_slot_names():
>
> + foreach(lc, remote_slots)
> + {
> + RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
> + char *slot_name;
> +
> + /* Switch to long-lived TopMemoryContext to store slot names */
> + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> +
> + slot_name = pstrdup(remote_slot->name);
> + slot_names = lappend(slot_names, slot_name);
> +
> + MemoryContextSwitchTo(oldcontext);
> + }
>
> It will be better to move 'MemoryContextSwitchTo' calls outside of the
> loop. No need to switch the context for each slot.
>
Changed.
> 3)
> ProcessSlotSyncAPIChanges() gives a feeling that it is actually
> processing API changes where instead it is processing interrupts or
> config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()
>
Changed.
> 4)
> I prefer version 11's slotsync_api_reread_config() over current
> slotsync_api_config_changed(). There, even error was taken care of
> inside the function, which to me looked better and similar to how
> slotsync worker deals with it.
>
Changed.
> I have made some comment changes, attached the patch. Please include
> it if you find it okay.
Incorporated.
On Tue, Sep 23, 2025 at 4:42 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Tested the patch, few more suggestions
>
> 5)
> Currently the error message is:
>
> postgres=# SELECT pg_sync_replication_slots();
> ERROR: cannot continue slot synchronization due to parameter changes
> DETAIL: Critical replication parameters (primary_conninfo,
> primary_slot_name, or hot_standby_feedback) have changed since
> pg_sync_replication_slots() started.
> HINT: Retry pg_sync_replication_slots() to use the updated configuration.
>
> a)
> To be consistent with other error-messages, can we change ERROR msg
> to: 'cannot continue replication slots synchronization due to
> parameter changes'
>
Changed.
> b)
> There is double space in DETAIL msg: "have changed since"
>
> Will it be better to shorten the DETAIL as: 'One or more of
> primary_conninfo, primary_slot_name, or hot_standby_feedback were
> modified.'
>
Changed.
> 6)
> postgres=# SELECT pg_sync_replication_slots();
> ERROR: exiting from slot synchronization as promotion is triggered
>
> Shall we rephrase it similar to the previous message: 'cannot continue
> replication slots synchronization as standby promotion is triggered'
>
Changed.
Attaching patch v14 incorporating the above changes.
regards,
Ajin Cherian
Fujitsu Australia
Вложения
On Wed, Sep 24, 2025 at 5:35 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 23, 2025 at 2:59 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > >
> > > Created a patch v13 with these changes.
> > >
> >
> > Please find a few comments:
> >
> > 1)
> > + /* update the failure structure so that it can be freed on error */
> > + fparams.slot_names = slot_names;
> > +
> >
> > Since slot_names is assigned only once, we can make the above
> > assignment as well only once, inside the if-block where we initialize
> > slot_names.
> >
>
> Changed.
>
> > 2)
> > extract_slot_names():
> >
> > + foreach(lc, remote_slots)
> > + {
> > + RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
> > + char *slot_name;
> > +
> > + /* Switch to long-lived TopMemoryContext to store slot names */
> > + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> > +
> > + slot_name = pstrdup(remote_slot->name);
> > + slot_names = lappend(slot_names, slot_name);
> > +
> > + MemoryContextSwitchTo(oldcontext);
> > + }
> >
> > It will be better to move 'MemoryContextSwitchTo' calls outside of the
> > loop. No need to switch the context for each slot.
> >
>
> Changed.
>
> > 3)
> > ProcessSlotSyncAPIChanges() gives a feeling that it is actually
> > processing API changes where instead it is processing interrupts or
> > config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()
> >
>
> Changed.
>
> > 4)
> > I prefer version 11's slotsync_api_reread_config() over current
> > slotsync_api_config_changed(). There, even error was taken care of
> > inside the function, which to me looked better and similar to how
> > slotsync worker deals with it.
> >
>
> Changed.
>
> > I have made some comment changes, attached the patch. Please include
> > it if you find it okay.
>
> Incorporated.
>
>
>
> On Tue, Sep 23, 2025 at 4:42 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > Tested the patch, few more suggestions
> >
> > 5)
> > Currently the error message is:
> >
> > postgres=# SELECT pg_sync_replication_slots();
> > ERROR: cannot continue slot synchronization due to parameter changes
> > DETAIL: Critical replication parameters (primary_conninfo,
> > primary_slot_name, or hot_standby_feedback) have changed since
> > pg_sync_replication_slots() started.
> > HINT: Retry pg_sync_replication_slots() to use the updated configuration.
> >
> > a)
> > To be consistent with other error-messages, can we change ERROR msg
> > to: 'cannot continue replication slots synchronization due to
> > parameter changes'
> >
>
> Changed.
It seems it is missed to be changed perhaps due to bringing back
previous implementation of slotsync_api_reread_config()
>
> > b)
> > There is double space in DETAIL msg: "have changed since"
> >
> > Will it be better to shorten the DETAIL as: 'One or more of
> > primary_conninfo, primary_slot_name, or hot_standby_feedback were
> > modified.'
> >
>
> Changed.
>
This too is missed.
> > 6)
> > postgres=# SELECT pg_sync_replication_slots();
> > ERROR: exiting from slot synchronization as promotion is triggered
> >
> > Shall we rephrase it similar to the previous message: 'cannot continue
> > replication slots synchronization as standby promotion is triggered'
> >
> Changed.
>
> Attaching patch v14 incorporating the above changes.
>
Few trivial comments:
1)
slotsync_api_reread_config():
+ * Returns true if conninfo, primary_slot_name or hot_standby_feedback changed.
This comment is no longer valid, we can remove it.
2)
/* We are done with sync, so reset sync flag */
reset_syncing_flag();
+
This extra blank line is not needed.
thanks
Shveta
On Fri, Sep 26, 2025 at 8:14 PM shveta malik <shveta.malik@gmail.com> wrote: > > > It seems it is missed to be changed perhaps due to bringing back > previous implementation of slotsync_api_reread_config() > > > > > > b) > > > There is double space in DETAIL msg: "have changed since" > > > > > > Will it be better to shorten the DETAIL as: 'One or more of > > > primary_conninfo, primary_slot_name, or hot_standby_feedback were > > > modified.' > > > > > > > Changed. > > Fixed. > > This too is missed. > > > > 6) > > > postgres=# SELECT pg_sync_replication_slots(); > > > ERROR: exiting from slot synchronization as promotion is triggered > > > > > > Shall we rephrase it similar to the previous message: 'cannot continue > > > replication slots synchronization as standby promotion is triggered' > > > > > Changed. > > > > Attaching patch v14 incorporating the above changes. > > > This was rephrased. > Few trivial comments: > > 1) > slotsync_api_reread_config(): > + * Returns true if conninfo, primary_slot_name or hot_standby_feedback changed. > > This comment is no longer valid, we can remove it. > Removed. > 2) > /* We are done with sync, so reset sync flag */ > reset_syncing_flag(); > + > > This extra blank line is not needed. Fixed. Attaching patch v15 addressing these comments. regards, Ajin Cherian Fujitsu Australia