RE: [PATCH] Support automatic sequence replication

On Wednesday, March 4, 2026 6:25 PM shveta malik <shveta.malik@gmail.com> wrote:
> 
> On Mon, Mar 2, 2026 at 1:28 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> 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