RE: [PATCH] Support automatic sequence replication
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>
Cc: Ajin Cherian <itsajin@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-05T11:34:51Z
Lists: pgsql-hackers
Attachments
- v11-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch (application/octet-stream) patch v11-0002
- v11-0001-Support-automatic-sequence-replication.patch (application/octet-stream) patch v11-0001
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
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Don't include wait_event.h in pgstat.h
- 868825aaeb40 19 (unreleased) cited
-
Don't include proc.h in shm_mq.h
- a2c89835f512 19 (unreleased) cited
-
Fix unsafe RTE_GROUP removal in simplify_EXISTS_query
- 77c7a17a6e5f 19 (unreleased) cited
-
Add sequence synchronization for logical replication.
- 5509055d6956 19 (unreleased) cited