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.
