Hi Vignesh, Here are my review comments for the latest patchset:
Patch v20240813-0001. No comments
Patch v20240813-0002. No comments
Patch v20240813-0003. No comments
Patch v20240813-0004. See below
Patch v20240813-0005. No comments
//////
Patch v20240813-0004
======
src/backend/catalog/pg_subscription.
GetSubscriptionRelations:
nit - modify a condition for readability
======
src/backend/commands/subscriptioncmds.c
fetch_sequence_list:
nit - changed the WARNING message. /parameters differ
between.../parameters differ for.../ (FYI, Chat-GPT agrees that 2nd
way is more correct)
nit - other minor changes to the message and hint
======
.../replication/logical/sequencesync.c
1. LogicalRepSyncSequences
+ ereport(DEBUG1,
+ errmsg("logical replication synchronization for subscription \"%s\",
sequence \"%s\" has finished",
+ get_subscription_name(subid, false), get_rel_name(done_seq->relid)));
DEBUG logs should use errmsg_internal. (fixed also nitpicks attachment).
~
nit - minor change to the log message counting the batched sequences
~~~
process_syncing_sequences_for_apply:
nit - /sequence sync worker/seqeuencesync worker/
======
src/backend/utils/misc/guc_tables.c
nit - /max workers/maximum number of workers/ (for consistency because
all other GUCs are verbose like this; nothing just says "max".)
======
src/test/subscription/t/034_sequences.pl
nit - adjust the expected WARNING message (which was modified above)
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/catalog/pg_subscription.c
b/src/backend/catalog/pg_subscription.c
index d938e57..af2bfe1 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -565,9 +565,11 @@ GetSubscriptionRelations(Oid subid, bool get_tables, bool
get_sequences,
relkind = get_rel_relkind(subrel->srrelid);
/* Skip sequences if they were not requested */
- if ((relkind == RELKIND_SEQUENCE && !get_sequences) ||
- /* Skip tables if they were not requested */
- (relkind != RELKIND_SEQUENCE && !get_tables))
+ if (relkind == RELKIND_SEQUENCE && !get_sequences)
+ continue;
+
+ /* Skip tables if they were not requested */
+ if (relkind != RELKIND_SEQUENCE && !get_tables)
continue;
relstate = (SubscriptionRelState *)
palloc(sizeof(SubscriptionRelState));
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 4166210..7d0be40 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2577,9 +2577,9 @@ fetch_sequence_list(WalReceiverConn *wrconn, char
*subname, List *publications)
*/
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("parameters differ between the remote
and local sequences %s for subscription \"%s\"",
+ errmsg("parameters differ for the remote and
local sequences (%s) for subscription \"%s\"",
warning_sequences->data, subname),
- errhint("Alter/Re-create the sequence using the
same parameter as in remote."));
+ errhint("Alter/Re-create local sequences to
have the same parameters as the remote sequences."));
pfree(warning_sequences);
}
diff --git a/src/backend/replication/logical/sequencesync.c
b/src/backend/replication/logical/sequencesync.c
index aaedba9..e2e0421 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -342,12 +342,12 @@ LogicalRepSyncSequences(void)
done_seq = (SubscriptionRelState *)
lfirst(list_nth_cell(sequences_not_synced, i));
ereport(DEBUG1,
- errmsg("logical replication
synchronization for subscription \"%s\", sequence \"%s\" has finished",
+ errmsg_internal("logical
replication synchronization for subscription \"%s\", sequence \"%s\" has
finished",
get_subscription_name(subid, false), get_rel_name(done_seq->relid)));
}
ereport(LOG,
- errmsg("logical replication
synchronized %d sequences of %d sequences for subscription \"%s\" ",
+ errmsg("logical replication
synchronized %d of %d sequences for subscription \"%s\" ",
curr_seq, seq_count,
get_subscription_name(subid, false)));
/* Commit this batch, and prepare for next batch. */
@@ -477,7 +477,7 @@ process_syncing_sequences_for_apply(void)
LWLockRelease(LogicalRepWorkerLock);
/*
- * If there are free sync worker slot(s), start a new sequence
sync
+ * If there are free sync worker slot(s), start a new
sequencesync
* worker, and break from the loop.
*/
if (nsyncworkers < max_sync_workers_per_subscription)
diff --git a/src/test/subscription/t/034_sequences.pl
b/src/test/subscription/t/034_sequences.pl
index 999561f..38fd7a3 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]+:)? parameters differ between the remote and
local sequences "public.regress_s4" for subscription "regress_seq_sub"/,
+ qr/WARNING: ( [A-Z0-9]+:)? parameters differ for the remote and local
sequences \("public.regress_s4"\) for subscription "regress_seq_sub"/,
"Refresh publication sequences should throw a warning if the sequence
definition is not the same"
);
