On Wednesday, March 4, 2026 6:25 PM shveta malik <[email protected]> wrote: > > On Mon, Mar 2, 2026 at 1:28 PM Zhijie Hou (Fujitsu) <[email protected]> > wrote: > > > > Rebased the patch to silence compile warning due to a recent commit > > a2c89835. > > > > Thanks for the patch. Please find a few comments for 001:
Thanks for the comments. > > 1) > /* > * 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) > + UpdateSubscriptionRelState(MySubscription->oid, seqoid, > SUBREL_STATE_READY, > + seqinfo->page_lsn, false); > > What if page-lsn has changed and we are in READY state, don't we need to > update that in pg_subscription_rel? Or is that done somewhere else? The check was done in 0002, I moved it to 0001 now. > > 2) > + * > + * If relstate is SUBREL_STATE_READY, only synchronize sequences that > + * have drifted from their publisher values. Otherwise, synchronize > + * all sequences. > + * > + * Returns true/false if any sequences were actually copied. > */ > +static bool > +copy_sequences(WalReceiverConn *conn, List *seqinfos) > > There is no relstate, comments need correction. Changed. > > 3) > Currently we use same state 'COPYSEQ_SUCCESS' for 2 cases: allowed to > copy (as returned by validate_seqsync_state and > get_and_validate_seq_info) and copy-done (by copy_sequences, > copy_sequence). It is slightly confusing. Shall we add one more state for > 'allowed' case, could be COPYSEQ_ELIGIBLE or COPYSEQ_PROCEED or > COPYSEQ_ALLOWED? COPYSEQ_SUCCESS was used for such a case in > previous seq-sync commands too (on HEAD), but now its usage is more in > 'allowed' case as compared to HEAD, so perhaps we can change in this patch. > But I would like to know what others think here. I agree that adding a new value can make the code easier to read. > > > 4) > + * Preliminary check to determine if copying the sequence is allowed. > > How about this comment: > Check whether the user has required privileges on the sequence and whether > the sequence has drifted. Changed as suggested. > > 5) > validate_seqsync_state(): > + /* > + * Skip synchronization if the sequence is already in READY state and > + * 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; > > Since we already have a comment where we check READY state in outer if- > block and since we are not checking READY state here, perhaps we can > change the above comment to simply: > "Skip synchronization if it has not drifted from the publisher's value." Changed. Here is V10 patch set which addressed all comments. I also fixed a bug that Kuroda-San reported off-list, where we were not resetting the StringInfo used to build the query in the copy_sequences loop, which caused the built query to be incorrect. Best Regards, Hou zj
v10-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch
Description: v10-0002-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch
v10-0001-Support-automatic-sequence-replication.patch
Description: v10-0001-Support-automatic-sequence-replication.patch
