| From: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
|---|---|
| To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com> |
| Cc: | Ajin Cherian <itsajin(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | RE: [PATCH] Support automatic sequence replication |
| Date: | 2026-03-05 11:34:51 |
| Message-ID: | TY4PR01MB169077FE33BDE8363F8795EC5947DA@TY4PR01MB16907.jpnprd01.prod.outlook.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Thursday, March 5, 2026 6:47 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Mar 5, 2026 at 9:35 AM shveta malik <shveta(dot)malik(at)gmail(dot)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 | Content-Type | Size |
|---|---|---|
| v11-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch | application/octet-stream | 18.5 KB |
| v11-0001-Support-automatic-sequence-replication.patch | application/octet-stream | 42.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Zhijie Hou (Fujitsu) | 2026-03-05 11:34:53 | RE: [PATCH] Support automatic sequence replication |
| Previous Message | Laurenz Albe | 2026-03-05 10:58:20 | Re: Relstats after VACUUM FULL and CLUSTER |