Hi Vignesh, Here are my review comments for the latest patchset
v20240819-0001. No changes. No comments.
v20240819-0002. No changes. No comments.
v20240819-0003. See below.
v20240819-0004. See below.
v20240819-0005. No changes. No comments.
///////////////////////
PATCH v20240819-0003
======
src/backend/replication/logical/syncutils.c
3.1.
+typedef enum
+{
+ SYNC_RELATION_STATE_NEEDS_REBUILD,
+ SYNC_RELATION_STATE_REBUILD_STARTED,
+ SYNC_RELATION_STATE_VALID,
+} SyncingRelationsState;
+
+static SyncingRelationsState relation_states_validity =
SYNC_RELATION_STATE_NEEDS_REBUILD;
There is some muddle of singular/plural names here. The
typedef/values/var should all match:
e.g. It could be like:
SYNC_RELATION_STATE_xxx --> SYNC_RELATION_STATES_xxx
SyncingRelationsState --> SyncRelationStates
But, a more radical change might be better.
typedef enum
{
RELATION_STATES_SYNC_NEEDED,
RELATION_STATES_SYNC_STARTED,
RELATION_STATES_SYNCED,
} SyncRelationStates;
~~~
3.2. GENERAL refactoring
I don't think all of the functions moved into syncutil.c truly belong there.
This new module was introduced to be for common/util functions for
tablesync and sequencesync, but with each patchset, it has been
sucking in more and more functions that maybe do not quite belong
here.
For example, AFAIK these below have logic that is *solely* for TABLES
(not for SEQUENCES). Perhaps it was convenient to dump them here
because they are statically called, but I felt they still logically
belong in tablesync.c:
- process_syncing_tables_for_sync(XLogRecPtr current_lsn)
- process_syncing_tables_for_apply(XLogRecPtr current_lsn)
- AllTablesyncsReady(void)
~~~
3.3.
+static bool
+FetchRelationStates(bool *started_tx)
+{
If this function can remain static then the name should change to be
like fetch_table_states, right?
======
src/include/replication/worker_internal.h
3.4.
+extern bool wait_for_relation_state_change(Oid relid, char expected_state);
If this previously static function will be exposed now (it may not
need to be if some other functions are returned tablesync.c) then the
function name should also be changed, right?
////////////////////////
PATCH v20240819-0004
======
src/backend/replication/logical/syncutils.c
4.1 GENERAL refactoring
(this is similar to review comment #3.2 above)
Functions like below have logic that is *solely* for SEQUENCES (not
for TABLES). I felt they logically belong in sequencesync.c, not here.
- process_syncing_sequences_for_apply(void)
~~~
FetchRelationStates:
nit - the comment change about "not-READY tables" (instead of
relations) should be already in patch 0003.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
