Hi Vignesh, Here are my review comments for latest v20240812* patchset:
patch v20240812-0001. No comments.
patch v20240812-0002. Fixed docs.LGTM
patch v20240812-0003. This is new refactoring. See below.
patch v20240812-0004. (was 0003). See below.
patch v20240812-0005. (was 0004). No comments.
//////
patch v20240812-0003.
3.1. GENERAL
Hmm. I am guessing this was provided as a separate patch to aid review
by showing that existing functions are moved? OTOH you can't really
judge this patch properly without already knowing details of what will
come next in the sequencesync. i.e. As a *standalone* patch without
the sequencesync.c the refactoring doesn't make much sense.
Maybe it is OK later to combine patches 0003 and 0004. Alternatively,
keep this patch separated but give greater emphasis in the comment
header to say this patch only exists separately in order to help the
review.
======
Commit message
3.2.
Reorganized tablesync code to generate a syncutils file which will
help in sequence synchronization worker code.
~
"generate" ??
======
src/backend/replication/logical/syncutils.c
3.3. "common code" ??
FYI - There are multiple code comments mentioning "common code..."
which, in the absence of the sequencesync worker (which comes in the
next patch), have nothing "common" about them at all. Fixing them and
then fixing them again in the next patch might cause unnecessary code
churn, but OTOH they aren't correct as-is either. I have left them
alone for now.
~
3.4. function names
With the re-shuffling that this patch does, and changing several from
static to not-static, should the function names remain as they are?
They look random to me.
- finish_sync_worker(void)
- invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
- FetchTableStates(bool *started_tx)
- process_syncing_tables(XLogRecPtr current_lsn)
I think using a consistent naming convention would be better. e.g.
SyncFinishWorker
SyncInvalidateTableStates
SyncFetchTableStates
SyncProcessTables
~~~
nit - file header comment
======
src/backend/replication/logical/tablesync.c
3.5.
-static void
+void
process_syncing_tables_for_sync(XLogRecPtr current_lsn)
-static void
+void
process_syncing_tables_for_apply(XLogRecPtr current_lsn)
Since these functions are no longer static should those function names
be changed to use the CamelCase convention for non-static API?
//////////
patch v20240812-0004.
======
src/backend/replication/logical/syncutils.c
nit - file header comment (made same as patch 0003)
~
FetchRelationStates:
nit - IIUC sequence states are only INIT -> READY. So the comments in
this function dont need to specifically talk about sequence INIT
state.
======
src/backend/utils/misc/guc_tables.c
4.1.
{"max_sync_workers_per_subscription",
PGC_SIGHUP,
REPLICATION_SUBSCRIBERS,
- gettext_noop("Maximum number of table synchronization workers per
subscription."),
+ gettext_noop("Maximum number of relation synchronization workers per
subscription."),
NULL,
},
I was wondering if "relation synchronization workers" is meaningful to
the user because that seems like new terminology.
Maybe it should say "... of table + sequence synchronization workers..."
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/syncutils.c
b/src/backend/replication/logical/syncutils.c
index 4e39836..4bbc481 100644
--- a/src/backend/replication/logical/syncutils.c
+++ b/src/backend/replication/logical/syncutils.c
@@ -1,5 +1,5 @@
/*-------------------------------------------------------------------------
- * sequencesync.c
+ * syncutils.c
* PostgreSQL logical replication: common synchronization code
*
* Copyright (c) 2024, PostgreSQL Global Development Group
@@ -8,8 +8,8 @@
* src/backend/replication/logical/syncutils.c
*
* NOTES
- * This file contains common code for synchronization of tables that
will be
- * help apply worker and table synchronization worker.
+ * This file contains code common to table synchronization workers, and
+ * the sequence synchronization worker.
*-------------------------------------------------------------------------
*/
diff --git a/src/backend/replication/logical/syncutils.c
b/src/backend/replication/logical/syncutils.c
index ed353f1..1702be9 100644
--- a/src/backend/replication/logical/syncutils.c
+++ b/src/backend/replication/logical/syncutils.c
@@ -8,9 +8,8 @@
* src/backend/replication/logical/syncutils.c
*
* NOTES
- * This file contains common code for synchronization of tables and
- * sequences that will be help apply worker, table synchronization worker
- * and sequence synchronization.
+ * This file contains code common to table synchronization workers, and
+ * the sequence synchronization worker.
*-------------------------------------------------------------------------
*/
@@ -93,7 +92,7 @@ invalidate_syncing_relation_states(Datum arg, int cacheid,
uint32 hashvalue)
* Common code to fetch the up-to-date sync state info into the static lists.
*
* Copy tables that are not READY state into table_states_not_ready, and
- * sequences that have INIT state into sequence_states_not_ready. The
+ * sequences that are not READY state into sequence_states_not_ready. The
* pg_subscription_rel catalog is shared by tables and sequences. Changes to
* either sequences or tables can affect the validity of relation states, so we
* update both table_states_not_ready and sequence_states_not_ready
@@ -132,10 +131,7 @@ FetchRelationStates(void)
started_tx = true;
}
- /*
- * Fetch tables that are in non-ready state, and sequences that
are in
- * init state.
- */
+ /* Fetch tables and sequences that are in non-READY state. */
rstates = GetSubscriptionRelations(MySubscription->oid, true,
true,
false);
