Hi Vignesh, Here are my v20240807-0003 review comments.
======
1. GENERAL DOCS.
IMO the replication of SEQUENCES is a big enough topic that it
deserves to have its own section in the docs chapter 31 [1].
Some of the create/alter subscription docs content would stay where it
is in, but a new chapter would just tie everything together better. It
could also serve as a better place to describe the other sequence
replication content like:
(a) getting a WARNING for mismatched sequences and how to handle it.
(b) how can the user know when a subscription refresh is required to
(re-)synchronise sequences
(c) pub/sub examples
======
doc/src/sgml/logical-replication.sgml
2. Restrictions
Sequence data is not replicated. The data in serial or identity
columns backed by sequences will of course be replicated as part of
the table, but the sequence itself would still show the start value on
the subscriber. 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 ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES or by
copying the current data from the publisher (perhaps using pg_dump) or
by determining a sufficiently high value from the tables themselves.
~
2a.
The paragraph starts by saying "Sequence data is not replicated.". It
seems wrong now. Doesn't that need rewording or removing?
~
2b.
Should the info "If, however, some kind of switchover or failover..."
be mentioned in the "Logical Replication Failover" section [2],
instead of here?
======
doc/src/sgml/ref/alter_subscription.sgml
3.
Sequence values may occasionally become out of sync due to updates in
the publisher. To verify this, compare the
pg_subscription_rel.srsublsn on the subscriber with the page_lsn
obtained from the pg_sequence_state for the sequence on the publisher.
If the sequence is still using prefetched values, the page_lsn will
not be updated. In such cases, you will need to directly compare the
sequences and execute REFRESH PUBLICATION SEQUENCES if required.
~
3a.
This whole paragraph may be better put in the new chapter that was
suggested earlier in review comment #1.
~
3b.
Is it only "Occasionally"? I expected subscriber-side sequences could
become stale quite often.
~
3c.
Is this advice very useful? It's saying if the LSN is different then
the sequence is out of date, but if the LSN is not different then you
cannot tell. Why not ignore LSN altogether and just advise the user to
directly compare the sequences in the first place?
======
Also, there are more minor suggestions in the attached nitpicks diff.
======
[1] https://www.postgresql.org/docs/current/logical-replication.html
[2]
file:///usr/local/pg_oss/share/doc/postgresql/html/logical-replication-failover.html
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 6b320b1..bb6aa8e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -726,7 +726,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
recordDependencyOnOwner(SubscriptionRelationId, subid, owner);
/*
- * XXX todo: If the subscription is for a sequence-only publication,
+ * XXX: If the subscription is for a sequence-only publication,
* creating this origin is unnecessary at this point. It can be created
* later during the ALTER SUBSCRIPTION ... REFRESH command, if the
* publication is updated to include tables or tables in schemas.
@@ -756,7 +756,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
PG_TRY();
{
- bool hastables = false;
+ bool has_tables;
List *relations;
char table_state;
@@ -771,16 +771,14 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
table_state = opts.copy_data ? SUBREL_STATE_INIT :
SUBREL_STATE_READY;
/*
- * Get the table list from publisher and build local
table status
- * info.
+ * Build local relation status info. Relations are for
both tables and
+ * sequences from the publisher.
*/
relations = fetch_table_list(wrconn, publications);
- if (relations != NIL)
- hastables = true;
-
- /* Include the sequence list from publisher. */
+ has_tables = relations != NIL;
relations = list_concat(relations,
fetch_sequence_list(wrconn, publications));
+
foreach_ptr(RangeVar, rv, relations)
{
Oid relid;
@@ -800,7 +798,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
* won't use the initial snapshot for anything, so no
need to
* export it.
*
- * XXX todo: If the subscription is for a sequence-only
publication,
+ * XXX: If the subscription is for a sequence-only
publication,
* creating this slot is not necessary at the moment.
It can be
* created during the ALTER SUBSCRIPTION ... REFRESH
command if the
* publication is updated to include tables or tables
in schema.
@@ -827,7 +825,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
* PENDING, to allow ALTER SUBSCRIPTION ...
REFRESH
* PUBLICATION to work.
*/
- if (opts.twophase && !opts.copy_data &&
hastables)
+ if (opts.twophase && !opts.copy_data &&
has_tables)
twophase_enabled = true;
walrcv_create_slot(wrconn, opts.slot_name,
false, twophase_enabled,
@@ -2475,7 +2473,7 @@ fetch_sequence_list(WalReceiverConn *wrconn, List
*publications)
" LATERAL
pg_get_publication_sequences(p.pubname::text) gps(relid), pg_class c\n"
" JOIN pg_namespace n
ON n.oid = c.relnamespace\n"
" JOIN pg_sequence s ON
c.oid = s.seqrelid\n"
- "WHERE c.oid = gps.relid AND
p.pubname IN (\n");
+ "WHERE c.oid = gps.relid AND
p.pubname IN (");
get_publications_str(publications, &cmd, true);
appendStringInfoChar(&cmd, ')');
diff --git a/src/backend/replication/logical/sequencesync.c
b/src/backend/replication/logical/sequencesync.c
index 2935c53..a79c8a3 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -18,8 +18,8 @@
* ALTER SUBSCRIPTION ... REFRESH PUBLICATION
* ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES
*
- * Apply worker will periodically check if there are any sequences in INIT
- * state and start a sequencesync worker.
+ * The apply worker will periodically check if there are any sequences in INIT
+ * state and will start a sequencesync worker if needed.
*
* The sequencesync worker retrieves the sequences to be synchronized from the
* pg_subscription_rel catalog table. It synchronizes multiple sequences per
@@ -35,16 +35,18 @@
* (100) sequences are synchronized per transaction. The locks on the sequence
* relation will be periodically released at each transaction commit.
*
- * An alternative design was considered where the launcher process would
+ * XXX: An alternative design was considered where the launcher process would
* periodically check for sequences that need syncing and then start the
- * sequence sync worker. However, the approach of having the apply worker
- * manage the sequence sync worker was chosen for the following reasons: a) It
- * avoids overloading the launcher, which handles various other subscription
- * requests. b) It offers a more straightforward path for extending support for
- * incremental sequence synchronization. c) It utilizes the existing tablesync
- * worker code to start the sequencesync process, thus preventing code
- * duplication in the launcher. d) It simplifies code maintenance by
- * consolidating changes to a single location rather than multiple components.
+ * sequencesync worker. However, the approach of having the apply worker
+ * manage the sequencesync worker was chosen for the following reasons:
+ * a) It avoids overloading the launcher, which handles various other
+ * subscription requests.
+ * b) It offers a more straightforward path for extending support for
+ * incremental sequence synchronization.
+ * c) It utilizes the existing tablesync worker code to start the sequencesync
+ * process, thus preventing code duplication in the launcher.
+ * d) It simplifies code maintenance by consolidating changes to a single
+ * location rather than multiple components.
*-------------------------------------------------------------------------
*/
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 5800f21..011c579 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1700,7 +1700,7 @@ copy_table_done:
static bool
FetchTableStates(void)
{
- static bool has_subrels = false;
+ static bool has_subtables = false;
bool started_tx = false;
if (relation_states_validity != SYNC_TABLE_STATE_VALID)
@@ -1752,7 +1752,7 @@ FetchTableStates(void)
* if table_states_not_ready was empty we still need to check
again to
* see if there are 0 tables.
*/
- has_subrels = (table_states_not_ready != NIL) ||
+ has_subtables = (table_states_not_ready != NIL) ||
HasSubscriptionTables(MySubscription->oid);
/*
@@ -1772,7 +1772,7 @@ FetchTableStates(void)
pgstat_report_stat(true);
}
- return has_subrels;
+ return has_subtables;
}
/*
