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: 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? 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. 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. 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. 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." thanks Shveta
