MIME-Version: 1.0
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>
In-Reply-To: <OS9PR01MB12149D9054CC7F2DC3F0D26A1F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 24 Feb 2026 11:06:15 +0530
Message-ID: <CAA4eK1JqGJWjL4G6vvxudhiT8G4RXfaxXNozKDDh+m3GXQ4AKg@mail.gmail.com>
Subject: Re: [PATCH] Support automatic sequence replication
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Ajin Cherian <itsajin@gmail.com>, shveta malik <shveta.malik@gmail.com>, 
	Ashutosh Sharma <ashu.coek88@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1JqGJWjL4G6vvxudhiT8G4RXfaxXNozKDDh%2Bm3GXQ4AKg%40mail.gmail.com>
Precedence: bulk

On Mon, Feb 23, 2026 at 6:56=E2=80=AFAM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Thanks for updating the patch. Here are my comments.
>
> 01. start_sequence_sync()
> ```
>    /* Need to start transaction for cache lookup */
>    StartTransactionCommand();
> ```
>
> Here, we must check additional parameter changes, such as conninfo and pa=
sswordrequired.
> Also, it's inefficient because transactions are started each time.
>
> Can we re-use maybe_reread_subscription() here? Some parameters do not ta=
ke
> effect for the sequence sync worker, but it is OK to exit even if they
> are changed. If we use the function, no need to include "storage/ipc.h".
>

I also think it is better to restart the sequencesync worker on
subscription parameter change and probably it is okay to use
maybe_reread_subscription() but need to be careful to avoid anything
apply_worker specific in that function. BTW, what happens now without
this change when apply_worker stops due to parameter change, does
sequencesync worker continue? If so, we should make it restart as we
are doing for apply worker.

Also, shouldn't we need to invoke AcceptInvalidationMessages() as we
are doing in apply worker when not in a remote transaction? I think it
will be required to get local_sequence definition changes , if any.

> 02. match_previous_words
>
> No need to remove "REFRESH SEQUENCES" anymore.
>
> 03. CopySeqResult
> ```
>  COPYSEQ_NOWORK,
> ```
>
> It describes why the copying is skipped. How about "COPYSEQ_NO_DRIFT"?
>

+1.

> 04. LogicalRepSyncSequences()
>
> ```
>   oldctx =3D MemoryContextSwitchTo(ApplyContext);
>
>   seq =3D palloc0_object(LogicalRepSequenceInfo);
>   seq->localrelid =3D subrel->srrelid;
>   seq->nspname =3D get_namespace_name(RelationGetNamespace(sequence_rel))=
;
>   seq->seqname =3D pstrdup(RelationGetRelationName(sequence_rel));
>   seq->relstate =3D relstate;
>   seqinfos =3D lappend(seqinfos, seq);
>
>   MemoryContextSwitchTo(oldctx);
> ```
>
> ISTM they are palloc'd but not pfree'd.
>

They are freed, see following code. But it seems nspname and seqname
should be freed separately for each sequence element and each sequence
element also needs to be freed independently.

+ /* Clean up */
+ list_free(seqinfos);

> Since the sequencesync worker now has a long lifetime, we must take care =
of the
> memory allocation/freeing more carefully. How about introducing per-inter=
action
> memory context like ApplyMessageContext?
>

Yeah, I feel that would be a better approach than retail pfree
especially because it is also required at other places in sequencesync
worker (see places where currently ApplyContext is used in
sequencesync worker). I think it would be better if we name this new
context as SequenceSyncContext.

Few other miscellaneous comments
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
1.
  <sect2 id=3D"sequences-out-of-sync">
-   <title>Refreshing Out-of-Sync Sequences</title>
-   <para>
-    Subscriber sequence values will become out of sync as the publisher
-    advances them.
-   </para>
-   <para>
-    To detect this, compare the
-    <link linkend=3D"catalog-pg-subscription-rel">pg_subscription_rel</lin=
k>.<structfield>srsublsn</structfield>
-    on the subscriber with the <structfield>page_lsn</structfield> obtaine=
d
-    from the <link
linkend=3D"func-pg-get-sequence-data"><function>pg_get_sequence_data</funct=
ion></link>
-    function for the sequence on the publisher. Then run
-    <link linkend=3D"sql-altersubscription-params-refresh-sequences">
-    <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link> to
-    re-synchronize if necessary.
-   </para>
+   <title>Out-of-Sync Sequences</title>
    <warning>
     <para>
      Each sequence caches a block of values (typically 32) in memory befor=
e
@@ -1961,16 +1941,6 @@ Publications:
 ------------+-----------+------------
         610 | t         | 0/017CEDF8
 (1 row)
-</programlisting></para>
-
-   <para>
-    The difference between the sequence page LSNs on the publisher and the
-    sequence page LSNs on the subscriber indicates that the sequences are =
out
-    of sync. Re-synchronize all sequences known to the subscriber using
-    <link linkend=3D"sql-altersubscription-params-refresh-sequences">
-    <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link>.
-<programlisting>
-/* sub # */ ALTER SUBSCRIPTION sub1 REFRESH SEQUENCES;

...
...
-   <listitem>
-    <para>
-     Incremental sequence changes are not replicated.  Although the data i=
n
-     serial or identity columns backed by sequences will be replicated as =
part
-     of the table, the sequences themselves do not replicate ongoing chang=
es.
-     On the subscriber, a sequence will retain the last value it synchroni=
zed
-     from the publisher. If the subscriber is used as a read-only database=
,
-     then this should typically not be a problem.  If, however, some kind =
of
-     switchover or failover to the subscriber database is intended, then t=
he
-     sequences would need to be updated to the latest values, either by
-     executing <link linkend=3D"sql-altersubscription-params-refresh-seque=
nces">
-     <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link>
-     or by copying the current data from the publisher (perhaps using
-     <command>pg_dump</command>) or by determining a sufficiently high val=
ue
-     from the tables themselves.
-    </para>
-   </listitem>
-

I think this can still happen after this patch but chances are much
lower, so we need some of this before upgrade. We should reword it
accordingly. Similarly check other parts of the doc you removed.

2.
- "logical replication synchronization for subscription \"%s\",
sequence \"%s.%s\" has finished",
+ "logical replication sync for subscription \"%s\", sequence
\"%s.%s\" has been updated",

Is there a need to shorten synchronization to sync in above message?

3.
  elog(DEBUG1,
- "logical replication sequence synchronization for subscription
\"%s\" - batch #%d =3D %d attempted, %d succeeded, %d mismatched, %d
insufficient permission, %d missing from publisher, %d skipped",
- MySubscription->name,
- (cur_batch_base_index / MAX_SEQUENCES_SYNC_PER_BATCH) + 1,
- batch_size, batch_succeeded_count, batch_mismatched_count,
- batch_insuffperm_count, batch_missing_count, batch_skipped_count);
+ "logical replication sequence synchronization for subscription
\"%s\" - batch #%d =3D %d attempted, %d succeeded, %d mismatched, %d
insufficient permission, %d missing from publisher, %d skipped, %d no
drift",
+ MySubscription->name,
+ (cur_batch_base_index / MAX_SEQUENCES_SYNC_PER_BATCH) + 1,
+ batch_size, batch_succeeded_count, batch_mismatched_count,
+ batch_insuffperm_count, batch_missing_count, batch_skipped_count,
batch_no_drift);

Here, the formatting of message is incorrect.

4.
  table_states_not_ready =3D NIL;
-
  if (!IsTransactionState())

Spurious line removal.

5.
MemoryContextSwitchTo(oldctx);
-
+ list_free_deep(seq_states);
  /*
  * Does the subscription have tables?
  *
@@ -260,7 +253,6 @@ FetchRelationStates(bool *has_pending_subtables,
  */
  has_subtables =3D (table_states_not_ready !=3D NIL) ||
  HasSubscriptionTables(MySubscription->oid);
-
  /*
  * If the subscription relation cache has been invalidated since we
  * entered this routine, we still use and return the relations we just
@@ -271,10 +263,8 @@ FetchRelationStates(bool *has_pending_subtables,
  if (relation_states_validity =3D=3D SYNC_RELATIONS_STATE_REBUILD_STARTED)
  relation_states_validity =3D SYNC_RELATIONS_STATE_VALID;
  }
-
  if (has_pending_subtables)
  *has_pending_subtables =3D has_subtables;
-
  if (has_pending_subsequences)
...
...

  }

+
  if (rc & WL_TIMEOUT)

All above places have spurious line removals and additions which made
code harder to understand.

6.
  else if (Matches("ALTER", "SUBSCRIPTION", MatchAny))
  COMPLETE_WITH("CONNECTION", "ENABLE", "DISABLE", "OWNER TO",
-   "RENAME TO", "REFRESH PUBLICATION", "REFRESH SEQUENCES",
-   "SET", "SKIP (", "ADD PUBLICATION", "DROP PUBLICATION");
+   "RENAME TO", "REFRESH PUBLICATION", "SET", "SKIP (",
+   "ADD PUBLICATION", "DROP PUBLICATION");

unrelated change.

7.
+ else
+ {
+
+ /*
+ * Double the sleep time, but not beyond
+ * the maximum allowable value.
+ */
+ sleep_ms =3D Min(sleep_ms * 2, SEQSYNC_MAX_SLEEP_MS);

Comments are not properly aligned.

--=20
With Regards,
Amit Kapila.