On Wednesday, March 4, 2026 12:21 PM Kuroda, Hayato/黒田 隼人 <[email protected]> wrote: > > Thanks for updating the patch. Here are my comments.
Thanks for the comments. > > 01. > ``` > <para> > A <firstterm>sequence synchronization worker</firstterm> will be started > - after executing any of the above subscriber commands, and will exit once > the > - sequences are synchronized. > + after executing any of the above subscriber commands. The worker will > + remain running for the life of the subscription, periodically > + synchronizing all published sequences. > </para> > ``` > > I think it's not accurate, because REFRESH SEQUENCE command does not > need the sequencesync worker anymore. Since the command is changed in 0002, I updated the doc there. > > 02. > ``` > void > GetSequence(Relation seqrel, int64 *last_value, bool *is_called) ``` > > I think GetSequence() itself should conitan the permission check like > SetSequence(). > My idea is to set NULL for last_value and is_called in this case. Changed. > > 03. > ``` > /* > * Verify that the current user has SELECT privilege on the > sequence. > * This is required to read the sequence state below. > */ > aclresult = pg_class_aclcheck(seqoid, GetUserId(), > ACL_SELECT); > > if (aclresult != ACLCHECK_OK) > return COPYSEQ_INSUFFICIENT_PERM; > > /* Get current local sequence state */ > GetSequence(sequence_rel, &local_last_value, > &local_is_called); ``` > > If you accept above comment, this part can be simplified. Changed. > > 04. > ``` > /* > * get_and_validate_seq_info > * > * Extracts remote sequence information from the tuple slot received from the > * publisher, and validates it against the corresponding local sequence > * definition. > */ > static CopySeqResult > get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel, > LogicalRepSequenceInfo > **seqinfo, int *seqidx, > List *seqinfos) > ``` > > It can return COPYSEQ_SUCCESS, but it might be misleading; copying is not > happened yet here. How about returning boolean and add another argument > to indicate the reason if the validation is failed? I have added one more enum value COPYSEQ_ALLOWED for cases where the validation passes to avoid changing the function signature. > > 05. > > LogicalRepSyncSequences() starts the transaction and read sequences every > time. > Can we cache the seqinfos to reuse in the next iteration? My idea is to > introduce a syscache callback for the pg_subscription_relto invalidate the > cached list. > > How about measuring performance once and considering it's a good > improvement? I think the time of scanning sequence might be negligible when most of sequence needs to be updated, since the most cost would be on sequence update. There might be some value to save the CPU cycle when the most of sequence has not drifted in which case the cost of scanning might be noticeable. But we will do some more tests and share later. Best Regards, Hou zj
