public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: Chao Li <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: [PATCH] Support automatic sequence replication
Date: Fri, 13 Mar 2026 07:12:09 +0000
Message-ID: <TY4PR01MB16907AB8416E53B843A506E659445A@TY4PR01MB16907.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <[email protected]>
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>
<CAJpy0uAfu-VPqCknLLvJ+PUx_cyoR-b70xUNT6Pyv8N-odKizQ@mail.gmail.com>
<CAA4eK1K3S=FaeDS62qoidt=uUaOzRiyzq=DVV1uhV-jcva8KxA@mail.gmail.com>
<TY4PR01MB169077FE33BDE8363F8795EC5947DA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
<[email protected]>
On Tuesday, March 10, 2026 2:44 PM Chao Li <[email protected]> wrote:
> Thanks for your clarification in the other email. I have reviewed v11 and here
> comes my comments:
Thanks for the comments.
>
> 1 - 0001
> ```
> static CopySeqResult
> -copy_sequence(LogicalRepSequenceInfo *seqinfo, Oid seqowner)
> +validate_seqsync_state(LogicalRepSequenceInfo *seqinfo, Relation
> sequence_rel)
> {
> - UserContext ucxt;
> AclResult aclresult;
> + Oid seqoid = seqinfo->localrelid;
> +
> + /* Perform drift check if it's not the initial sync */
> + if (seqinfo->relstate == SUBREL_STATE_READY)
> + {
> + int64 local_last_value;
> + bool local_is_called;
> +
> + /*
> + * Skip synchronization if the current user does not have
> sufficient
> + * privileges to read the sequence data.
> + */
> + if (!GetSequence(sequence_rel, &local_last_value,
> &local_is_called))
> + return COPYSEQ_INSUFFICIENT_PERM;
> +
> + /*
> + * Skip synchronization if the sequence has not drifted from
> the
> + * publisher's value.
> + */
> + if (local_last_value == seqinfo->last_value &&
> + local_is_called == seqinfo->is_called)
> + return COPYSEQ_NO_DRIFT;
> + }
> +
> + /* Verify that the current user can update the sequence */
> + aclresult = pg_class_aclcheck(seqoid, GetUserId(), ACL_UPDATE);
> +
> + if (aclresult != ACLCHECK_OK)
> + return COPYSEQ_INSUFFICIENT_PERM;
> +
> + return COPYSEQ_ALLOWED;
> +}
> ```
>
> I think we can move pg_class_aclcheck() to before the drift check, because it’s
> a local check, should be cheaper than a remote check. If the local check fails,
> we don’t need to touch remote.
It makes more sense to structure the permission check around the actual
requirement. Since we only update the sequence when a drift is detected, the
permission check should naturally follow that order. If the permission check
fails, an error will be raised anyway, and in that case, performance is less of
a concern.
>
> 2 - 0001
> ```
> +static CopySeqResult
> +copy_sequence(LogicalRepSequenceInfo *seqinfo, Relation sequence_rel)
> +{
> + UserContext ucxt;
> bool run_as_owner = MySubscription->runasowner;
> Oid seqoid = seqinfo->localrelid;
> + CopySeqResult result;
> + XLogRecPtr local_page_lsn;
> +
> + (void) GetSubscriptionRelState(MySubscription->oid,
> +
> RelationGetRelid(sequence_rel),
> +
> &local_page_lsn);
> ```
>
> I think GetSubscriptionRelState is called to get local_page_lsn. But
> local_page_lsn is only used very late in this function, and there is an early
> return branch, so can we move this call to right before where local_page_lsn
> is used?
Yes, we can do that. Changed.
>
> 3 - 0001
> ```
> /*
> * Record the remote sequence's LSN in pg_subscription_rel and mark
> the
> - * sequence as READY.
> + * sequence as READY if updating a sequence that is in INIT state.
> */
> - UpdateSubscriptionRelState(MySubscription->oid, seqoid,
> SUBREL_STATE_READY,
> - seqinfo->page_lsn,
> false);
> + if (seqinfo->relstate == SUBREL_STATE_INIT ||
> + seqinfo->page_lsn != local_page_lsn)
> + UpdateSubscriptionRelState(MySubscription->oid, seqoid,
> SUBREL_STATE_READY,
> + seqinfo-
> >page_lsn, false);
> ```
>
> In the comment, I think you don’t have to add “if updating .. that is in INIT
> state”, but if you do, then you should also mention the lsn condition.
Changed.
>
> 4 - 0001
> ```
> + else
> + {
> + /*
> + * Double the sleep time, but not beyond the
> maximum allowable
> + * value.
> + */
> + sleep_ms = Min(sleep_ms * 2,
> SEQSYNC_MAX_SLEEP_MS);
> + }
> ```
>
> Double the sleep time when no drift is an optimization. But looks like the
> doubling happens only when all sequences have no drift. Say, there are 1000
> sequences, and only one is hot, then the it will still fetch the 1000 sequences
> from remote every 2 seconds, making the optimization much less efficient. I
> think the worker can wake up every 2 seconds, but next fetch time should be
> per sequence.
I'm unsure whether the complexity of per-sequence interval adjustment is
justified. We could consider this as a future enhancement based on user
feedback.
> 5 - 0001
> ```
> + * If the state of sequence is SUBREL_STATE_READY, only synchronize
> sequences
> ```
>
> “The state of sequence is SUBREL_STATE_READY” is inaccurate, I think it
> should be “the state of sequence sync is SUBREL_STATE_READY”.
I slightly reworded the comments.
> 6 - 0001
>
> start_sequence_sync runs an infinite loop to periodically sync sequences. I
> don’t it has an auto reconnect mechanism. When something wrong happens,
> the sync worker will exit, how can the worker
The comment seems incomplete.
>
> 7 - 0001
>
> Say a major version upgrade use case that uses logical replication. Before
> shutdown the publication side (old version), if there are 1000 sequences, how
> can a user decide if all sequences have been synced? From this perspective,
> would it make sense to log a INFO message when all sequences have no drift?
> If next round still no drift, then don’t repeat the message. In other words,
> when the states between (all no drift) and (any drift) switch, log a INFO
> message.
To determine whether a sequence needs to be resynchronized, users should check
srsublsn in pg_subscription_rel, as documented in [1]. To ensure all sequences
are fully synced, they can simply execute REFRESH SEQUENCE as the final step.
>
> 8 - 0001
> ```
> +bool
> +GetSequence(Relation seqrel, int64 *last_value, bool *is_called)
> ```
>
> This function name feels too general. How about ReadSequenceState or
> GetSequenceLastValueAndIsCalled?
I am not sure if the suggested name is better, considering that we already
have SetSequence().
>
> 9 - 0001
> ```
> + elog(ERROR, "unrecognized Sequence
> replication result: %d", (int) sync_status);
> ```
> Nit: The “S” seems not have to be capital.
Changed.
>
> 10 - 0002
0002 includes significant changes in this version, though I have made an effort to address
the comments that are still applicable.
[1] https://www.postgresql.org/docs/devel/logical-replication-sequences.html#SEQUENCES-OUT-OF-SYNC
Best Regards,
Hou zj
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], [email protected]
Subject: RE: [PATCH] Support automatic sequence replication
In-Reply-To: <TY4PR01MB16907AB8416E53B843A506E659445A@TY4PR01MB16907.jpnprd01.prod.outlook.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