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
