On Thursday, March 5, 2026 6:47 PM Amit Kapila <[email protected]> wrote:
> On Thu, Mar 5, 2026 at 9:35 AM shveta malik <[email protected]>
> wrote:
> >
> >
> > 3)
> > + /* Sleep for the configured interval */
> > + (void) WaitLatch(MyLatch,
> > + WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, sleep_ms,
> > + WAIT_EVENT_LOGICAL_SYNC_STATE_CHANGE);
> >
> > I don't think this wait-event is appropriate. Unlike tablesync, we are
> > not waiting for any state change here. Shall we add a new one for our
> > case? How about WAIT_EVENT_LOGICAL_SEQSYNC_MAIN? Thoughts?
> >
> 
> +1 for a new wait event. Few other minor comments:

Added.

> 
> 1.
> + * Check if the subscription includes sequences and start a
> + sequencesync
> + * worker if one is not already running. The active sequencesync worker
> + will
> + * handle all pending sequence synchronization. If any sequences remain
> + * unsynchronized after it exits, a new worker can be started in the
> + next
> + * iteration.
>   *
> - * Start a sequencesync worker if one is not already running. The active
> - * sequencesync worker will handle all pending sequence synchronization. If
> any
> - * sequences remain unsynchronized after it exits, a new worker can be
> started
> - * in the next iteration.
> 
> Why did this comment change? The earlier one sounds okay to me.

I think either version is fine, so reverted this change now.

> 
> 2.
>   break;
> +
>   case COPYSEQ_INSUFFICIENT_PERM:
> 
> Why does this patch add additional new lines? We use both styles (existing
> and what this patch does) in the code but it seems unnecessary to change for
> this patch.

Removed.

> 
> 3.
> - ProcessSequencesForSync();
> +
> + /* Check if sequence worker needs to be started */
> + MaybeLaunchSequenceSyncWorker();
> 
> No need for an additional line and a comment here.

Removed.

Here is the V11 patch which addressed all above comments and [1][2].

[1] 
https://www.postgresql.org/message-id/CAJpy0uAfu-VPqCknLLvJ%2BPUx_cyoR-b70xUNT6Pyv8N-odKizQ%40mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAJpy0uBeAdz6-3P26Eryeq3TyjA-GiKY3z0hFMxzZD%3DAYGqQ3Q%40mail.gmail.com

Best Regards,
Hou zj

Attachment: v11-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch
Description: v11-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch

Attachment: v11-0001-Support-automatic-sequence-replication.patch
Description: v11-0001-Support-automatic-sequence-replication.patch

Reply via email to