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
