RE: [PATCH] Support automatic sequence replication - Mailing list pgsql-hackers

From Zhijie Hou (Fujitsu)
Subject RE: [PATCH] Support automatic sequence replication
Date
Msg-id TY4PR01MB169077FE33BDE8363F8795EC5947DA@TY4PR01MB16907.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: [PATCH] Support automatic sequence replication  (Amit Kapila <amit.kapila16@gmail.com>)
Responses RE: [PATCH] Support automatic sequence replication
Re: [PATCH] Support automatic sequence replication
List pgsql-hackers
On Thursday, March 5, 2026 6:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 5, 2026 at 9:35 AM shveta malik <shveta.malik@gmail.com>
> 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

pgsql-hackers by date:

Previous
From: Laurenz Albe
Date:
Subject: Re: Relstats after VACUUM FULL and CLUSTER
Next
From: "Zhijie Hou (Fujitsu)"
Date:
Subject: RE: [PATCH] Support automatic sequence replication