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
