On Mon, Feb 23, 2026 at 6:56 AM Hayato Kuroda (Fujitsu)
<[email protected]> 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
> passwordrequired.
> Also, it's inefficient because transactions are started each time.
>
> Can we re-use maybe_reread_subscription() here? Some parameters do not take
> 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 = MemoryContextSwitchTo(ApplyContext);
>
> seq = palloc0_object(LogicalRepSequenceInfo);
> seq->localrelid = subrel->srrelid;
> seq->nspname = get_namespace_name(RelationGetNamespace(sequence_rel));
> seq->seqname = pstrdup(RelationGetRelationName(sequence_rel));
> seq->relstate = relstate;
> seqinfos = 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-interaction
> 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
=================================
1.
<sect2 id="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="catalog-pg-subscription-rel">pg_subscription_rel</link>.<structfield>srsublsn</structfield>
- on the subscriber with the <structfield>page_lsn</structfield> obtained
- from the <link
linkend="func-pg-get-sequence-data"><function>pg_get_sequence_data</function></link>
- function for the sequence on the publisher. Then run
- <link linkend="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 before
@@ -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="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 in
- serial or identity columns backed by sequences will be replicated as part
- of the table, the sequences themselves do not replicate ongoing changes.
- On the subscriber, a sequence will retain the last value it synchronized
- 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 the
- sequences would need to be updated to the latest values, either by
- executing <link linkend="sql-altersubscription-params-refresh-sequences">
- <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 value
- 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 = %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 = %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 = 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 = (table_states_not_ready != 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 == SYNC_RELATIONS_STATE_REBUILD_STARTED)
relation_states_validity = SYNC_RELATIONS_STATE_VALID;
}
-
if (has_pending_subtables)
*has_pending_subtables = 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 = Min(sleep_ms * 2, SEQSYNC_MAX_SLEEP_MS);
Comments are not properly aligned.
--
With Regards,
Amit Kapila.
