public inbox for [email protected]
help / color / mirror / Atom feedFrom: shveta malik <[email protected]>
To: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [PATCH] Support automatic sequence replication
Date: Thu, 5 Mar 2026 09:35:21 +0530
Message-ID: <CAJpy0uAfu-VPqCknLLvJ+PUx_cyoR-b70xUNT6Pyv8N-odKizQ@mail.gmail.com> (raw)
In-Reply-To: <TY4PR01MB1690739DE978BCBD12358478E947DA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
<CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com>
<CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
<CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
<CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
<CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@mail.gmail.com>
<CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
<CAE9k0PmTyCU1A9YEf+MRgfeZ8yK1bAYJu=o0bH8DNUTzXejQyQ@mail.gmail.com>
<CAA4eK1L6czEzG4mLNZSyjYC5nX0FMSjjk3csKuxPD3Ph5-7Yvw@mail.gmail.com>
<CAJpy0uAhGQJ=msVsn2GsqWXr+YESJK6x9NBvrUtKvtvp1OVuKQ@mail.gmail.com>
<CAJpy0uAOuu-M6wobH2wHOdTymm-cX9+MqwPyRNoOt=sPKBdCew@mail.gmail.com>
<CAFPTHDZiWYXoKoo4VcBYNH9a=gxDZhfkcBeXt5w6cLw4_ysyKw@mail.gmail.com>
<OS9PR01MB12149D9054CC7F2DC3F0D26A1F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
<CAA4eK1KYxQALt46k5uWOO6SUtNjvjOaXwfNjH0AU656YrcGZEw@mail.gmail.com>
<CAFPTHDZYonM+SXG19VVjgWduXQJSuDhcOUWq0NCiiuQubCSt6g@mail.gmail.com>
<CAFPTHDYud1zr0VyizhyhEQXfHMgXVcHrPzE56WUKGCFNskQq2A@mail.gmail.com>
<CAA4eK1JTau3fV7br6xwAV+LXXwM65RuGCuM2J3PQpCONtL1KXA@mail.gmail.com>
<OS9PR01MB1691377CDB1468CDC9820BBEB9470A@OS9PR01MB16913.jpnprd01.prod.outlook.com>
<TY4PR01MB1690715895CDE6FEFA13C2A2C947EA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
<CAJpy0uA1txsV5RhjZjLBDrUjvxVyBDtMXzHr6=DzLHf7ybBrqg@mail.gmail.com>
<TY4PR01MB1690739DE978BCBD12358478E947DA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
On Thu, Mar 5, 2026 at 8:16 AM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
>
> Here is V10 patch set which addressed all comments.
>
Thank You. Please find a few comments on 001:
1)
+ /*
+ * Skip synchronization if the current user does not have sufficient
+ * privileges to read the sequence data.
+ */
+ if (local_last_value == 0)
+ return COPYSEQ_INSUFFICIENT_PERM;
I don't think it is the right way to handle this. The local_last_value
can be genuinely 0 for some cases and we may end up giving the wrong
ERROR.
Try this:
CREATE SEQUENCE my_seq START WITH 0 INCREMENT BY 1 MINVALUE 0;
And then set-up pub-sub.
We get:
2026-03-05 08:57:39.591 IST [92281] WARNING: insufficient privileges
on sequence ("public.my_seq")
2026-03-05 08:57:39.591 IST [92281] ERROR: logical replication
sequence synchronization failed for subscription "subi1"
Either we shall move back the acl check to the caller of GetSequence
or pass the info of acl-check failure in a new argument or return
value.
2)
+ /*
+ * For sequences in INIT state, always sync. Otherwise, for
+ * sequences in READY state, only sync if there's drift.
+ */
if (sync_status == COPYSEQ_SUCCESS)
- sync_status = copy_sequence(seqinfo,
- sequence_rel->rd_rel->relowner);
+ sync_status = copy_sequence(seqinfo, sequence_rel);
We shall add such a comment atop copy_sequence as well. I am unsure if
we need it here or not.
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?
4)
+ relstate = subrel->srsubstate;
it will be good to move it just after below part:
/* Skip if the relation is not a sequence */
5)
}
+ /* Check if there are any sequences. */
+ has_subsequences = (seq_states != NIL);
One blank line before new change will improve readability.
6)
##########
## ALTER SUBSCRIPTION ... REFRESH PUBLICATION should cause sync of new
-# sequences of the publisher, but changes to existing sequences should
-# not be synced.
+# sequences of the publisher.
##########
# Create a new sequence 'regress_s2', and update existing sequence 'regress_s1'
Last comment needs to be changed. Remove this please: 'and update
existing sequence 'regress_s1''
thanks
Shveta
view thread (58+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Support automatic sequence replication
In-Reply-To: <CAJpy0uAfu-VPqCknLLvJ+PUx_cyoR-b70xUNT6Pyv8N-odKizQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox