Re: Logical Replication of sequences - Mailing list pgsql-hackers

Hi Vignesh,

Some review comments for v20251029_2-0001

======
General.

1.
I think it's more readable to consistently write the state name in
uppercase. There are a few affected comments:

e.g.
+ * we identify non-ready tables and non-ready sequences together to ensure
becomes
+ * we identify non-READY tables and non-READY sequences together to ensure

e.g.
+ * has_pending_subtables: true if the subscription has one or more tables that
+ * are not in ready state, otherwise false.
+ * has_pending_subsequences: true if the subscription has one or more sequences
+ * that are not in ready state, otherwise false.
becomes
+ * has_pending_subtables: true if the subscription has one or more tables that
+ * are not in READY state, otherwise false.
+ * has_pending_subsequences: true if the subscription has one or more sequences
+ * that are not in READY state, otherwise false.

e.g.
+ /* Fetch tables and sequences that are in non-ready state. */
becomes
+ /* Fetch tables and sequences that are in non-READY state. */

======
.../replication/logical/sequencesync.c

2.
+ * A single sequencesync worker is responsible for synchronizing all sequences
+ * marked in pg_subscription_rel. It begins by retrieving the list of sequences

typo? /marked in pg_subscription_rel/mapped in pg_subscription_rel/

~~~

3.
+static void
+report_sequence_errors(StringInfo insuffperm_seqs, StringInfo mismatched_seqs,
+    StringInfo insuffperm_seqs)

Perhaps this function should also have an assertion:

Assert(insuffperm_seqs->len || mismatched_seqs->len || insuffperm_seqs->len);

~~~

copy_sequences:

4.
+ int current_index = 0;

It's not really a current index. Maybe a better name for this variable
is something more like 'cur_batch_base_index'?

~~~

LogicalRepSyncSequences:

5.
+ if (!seqinfos)
+ return;
+

Maybe this needs some comment to say it is an early exit, for when
somehow the sequencesync worker started to do some work, but then it
found that there was nothing to do. Maybe also describe how this could
happen?

======
src/backend/replication/logical/syncutils.c

launch_sync_worker

6.
+ * wtype: sync worker type.
+ * nsyncworkers: Number of currently running sync workers for the subscription.
+ * relid:  InvalidOid for sequencesync worker, actual relid for tablesync
+ * worker.
+ * last_start_time: Pointer to the last start time of the worker.
+ */
+void
+launch_sync_worker(LogicalRepWorkerType wtype, int nsyncworkers, Oid relid,
+    TimestampTz *last_start_time)


Now it might be nice to have a sanity assert to match that function
comment. e.g.

Assert((wtype == WORKERTYPE_TABLESYNC) == OidIsValid(relid));

~~~

FetchRelationStates:

7.
  foreach(lc, rstates)
  {
- rstate = palloc(sizeof(SubscriptionRelState));
- memcpy(rstate, lfirst(lc), sizeof(SubscriptionRelState));
- table_states_not_ready = lappend(table_states_not_ready, rstate);
+ SubscriptionRelState *subrel = (SubscriptionRelState *) lfirst(lc);

Should we change that to:
foreach_ptr(SubscriptionRelState, subrel, rstates)

======
src/backend/replication/logical/tablesync.c

ProcessSyncingTablesForApply:

8.
bool started_tx = false;
bool should_exit = false;
Relation rel = NULL;

Assert(!IsTransactionState());

/* We need up-to-date sync state info for subscription tables here. */
FetchRelationStates(NULL, NULL, &started_tx);

~

It's not necessary here to assign that `started_tx` to false at the
declaration, because FetchRelationStates will set it.
ProcessSyncingSequencesForApply() does not assign a declaration like
this, so I felt here should not do it either.

Alternatively, leave this code as-is, change
ProcessSyncingSequencesForApply() to assign it false too. And then
Assert(*started_tx == false) inside the FetchRelationStates function.

Whatever way you choose, the point is that all caller code should be consistent.

~~~

AllTablesyncsReady:

9.
Ditto previous comment about the inconsistent auto-assignment of started_tx.

~~~

HasSubscriptionTablesCached:

10.
Ditto previous comment about the inconsistent auto-assignment of started_tx.

======
src/test/subscription/t/036_sequences.pl

11.
The comments can be more helpful if they also name the sequences
involved -- It saves having to go hunting to see what is new and what
is existing. I mean only small changes like below:

BEFORE
# Check - existing sequence is not synced
# Check - newly published sequence is synced

SUGGESTION
# Check - existing sequence ('regress_s1') is not synced
# Check - newly published sequence ('regress_s2') is synced

etc in multiple places.

======
Kind Regards,
Peter Smith.
Fujitsu Australia