Hi Vignesh,
v20240809-0001. No comments.
v20240809-0002. See below.
v20240809-0003. See below.
v20240809-0004. No comments.
//////////
Here are my review comments for patch v20240809-0002.
nit - Tweak wording in new docs example, because a publication only
publishes the sequences; it doesn't "synchronize" anything.
//////////
Here are my review comments for patch v20240809-0003.
fetch_sequence_list:
nit - move comment
nit - minor rewording for parameter WARNING message
======
.../replication/logical/sequencesync.c
src/backend/replication/logical/tablesync.c
1.
Currently the declaration 'sequence_states_not_ready' list seems
backwards. IMO it makes more sense for the declaration to be in
sequencesync.c, and the extern in the tablesync.c. (please also see
review comment #3 below which might affect this too).
~~~
2.
static bool
-FetchTableStates(bool *started_tx)
+FetchTableStates(void)
{
- static bool has_subrels = false;
-
- *started_tx = false;
+ static bool has_subtables = false;
+ bool started_tx = false;
Maybe give the explanation why 'has_subtables' is declared static here.
~~~
3.
I am not sure that it was an improvement to move the
process_syncing_sequences_for_apply() function into the
sequencesync.c. Calling the sequence code from the tablesync code
still looks strange. OTOH, I see why you don't want to leave it in
tablesync.c.
Perhaps it would be better to refactor/move all following functions
back to the (apply) worker.c instead:
- process_syncing_relations
- process_syncing_sequences_for_apply(void)
- process_syncing_tables_for_apply(void)
Actually, now that there are 2 kinds of 'sync' workers, maybe you
should introduce a new module (e.g. 'commonsync.c' or
'syncworker.c...), where you can put functions such as
process_syncing_relations() plus any other code common to both
tablesync and sequencesync. That might make more sense then having one
call to the other.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index 92758c7..64214ba 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -427,8 +427,8 @@ CREATE PUBLICATION all_sequences FOR ALL SEQUENCES;
</para>
<para>
- Create a publication that publishes all changes in all tables and
- synchronizes all sequences:
+ Create a publication that publishes all changes in all tables, and
+ all sequences for synchronization:
<programlisting>
CREATE PUBLICATION all_tables_sequences FOR ALL TABLES, SEQUENCES;
</programlisting>
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 2ac63c7..c595873 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2546,6 +2546,7 @@ fetch_sequence_list(WalReceiverConn *wrconn, List
*publications)
seqform = (Form_pg_sequence) GETSTRUCT(tup);
+ /* Build a list of sequences that don't match in publisher and
subscriber */
if (seqform->seqtypid != seqtypid || seqform->seqmin != seqmin
||
seqform->seqmax != seqmax || seqform->seqstart !=
seqstart ||
seqform->seqincrement != seqincrement ||
@@ -2556,7 +2557,6 @@ fetch_sequence_list(WalReceiverConn *wrconn, List
*publications)
else
appendStringInfoString(warning_sequences, ", ");
- /* Add the sequences that don't match in publisher and
subscriber */
appendStringInfo(warning_sequences, "\"%s.%s\"",
get_namespace_name(get_rel_namespace(relid)),
get_rel_name(relid));
@@ -2575,7 +2575,7 @@ fetch_sequence_list(WalReceiverConn *wrconn, List
*publications)
*/
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("Sequence parameter in remote and local
is not same for %s",
+ errmsg("Parameters differ for remote and local
sequences %s",
warning_sequences->data),
errhint("Alter/Re-create the sequence using the
same parameter as in remote."));
pfree(warning_sequences);
diff --git a/src/test/subscription/t/034_sequences.pl
b/src/test/subscription/t/034_sequences.pl
index 7cc8c8c..52453cc 100644
--- a/src/test/subscription/t/034_sequences.pl
+++ b/src/test/subscription/t/034_sequences.pl
@@ -177,7 +177,7 @@ $node_subscriber->safe_psql(
ALTER SUBSCRIPTION regress_seq_sub REFRESH PUBLICATION SEQUENCES");
like(
$stderr,
- qr/WARNING: ( [A-Z0-9]+:)? Sequence parameter in remote and local is
not same for "public.regress_s4"/,
+ qr/WARNING: ( [A-Z0-9]+:)? Parameters differ for remote and local
sequences "public.regress_s4"/,
"Refresh publication sequences should throw a warning if the sequence
definition is not the same"
);
