Re: [PATCH] Support automatic sequence replication

public inbox for [email protected]  
help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [PATCH] Support automatic sequence replication
Date: Wed, 4 Mar 2026 15:54:35 +0530
Message-ID: <CAJpy0uA1txsV5RhjZjLBDrUjvxVyBDtMXzHr6=DzLHf7ybBrqg@mail.gmail.com> (raw)
In-Reply-To: <TY4PR01MB1690715895CDE6FEFA13C2A2C947EA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
	<CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com>
	<CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
	<CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
	<CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
	<CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@mail.gmail.com>
	<CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
	<CAE9k0PmTyCU1A9YEf+MRgfeZ8yK1bAYJu=o0bH8DNUTzXejQyQ@mail.gmail.com>
	<CAA4eK1L6czEzG4mLNZSyjYC5nX0FMSjjk3csKuxPD3Ph5-7Yvw@mail.gmail.com>
	<CAJpy0uAhGQJ=msVsn2GsqWXr+YESJK6x9NBvrUtKvtvp1OVuKQ@mail.gmail.com>
	<CAJpy0uAOuu-M6wobH2wHOdTymm-cX9+MqwPyRNoOt=sPKBdCew@mail.gmail.com>
	<CAFPTHDZiWYXoKoo4VcBYNH9a=gxDZhfkcBeXt5w6cLw4_ysyKw@mail.gmail.com>
	<OS9PR01MB12149D9054CC7F2DC3F0D26A1F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
	<CAA4eK1KYxQALt46k5uWOO6SUtNjvjOaXwfNjH0AU656YrcGZEw@mail.gmail.com>
	<CAFPTHDZYonM+SXG19VVjgWduXQJSuDhcOUWq0NCiiuQubCSt6g@mail.gmail.com>
	<CAFPTHDYud1zr0VyizhyhEQXfHMgXVcHrPzE56WUKGCFNskQq2A@mail.gmail.com>
	<CAA4eK1JTau3fV7br6xwAV+LXXwM65RuGCuM2J3PQpCONtL1KXA@mail.gmail.com>
	<OS9PR01MB1691377CDB1468CDC9820BBEB9470A@OS9PR01MB16913.jpnprd01.prod.outlook.com>
	<TY4PR01MB1690715895CDE6FEFA13C2A2C947EA@TY4PR01MB16907.jpnprd01.prod.outlook.com>

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

view thread (58+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Support automatic sequence replication
  In-Reply-To: <CAJpy0uA1txsV5RhjZjLBDrUjvxVyBDtMXzHr6=DzLHf7ybBrqg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox