Re: Logical Replication of sequences

Hi, here are more review comments for patch v20240720-0003.

======
src/backend/catalog/pg_subscription.c

(Numbers are starting at #4 because this is a continuation of the docs review)

4. GetSubscriptionRelations

nitpick - rearranged the function header comment

~

5.
TBH, I'm thinking that just passing 2 parameters:
- bool get_tables
- bool get_sequences
where one or both can be true, would have resulted in simpler code,
instead of introducing this new enum SubscriptionRelKind.

~

6.
The 'not_all_relations' parameter/logic feels really awkward. IMO it
needs a better name and reverse the meaning to remove all the "nots".

For example, commenting it and calling it like below could be much simpler.

'all_relations'
If returning sequences, if all_relations=true get all sequences,
otherwise only get sequences that are in 'init' state.
If returning tables, if all_relation=true get all tables, otherwise
only get tables that have not reached 'READY' state.

======
src/backend/commands/subscriptioncmds.c

AlterSubscription_refresh:

nitpick - this function comment is difficult to understand. I've
rearranged it a bit but it could still do with some further
improvement.
nitpick - move some code comments
nitpick - I adjusted the "stop worker" comment slightly. Please check
it is still correct.
nitpick - add a blank line

~

7.
The logic seems over-complicated. For example, why is the sequence
list *always* fetched, but the tables list is only sometimes fetched?
Furthermore, this 'refresh_all_sequences' parameter seems to have a
strange interference with tables (e.g. even though it is possible to
refresh all tables and sequences at the same time). It is as if the
meaning is 'refresh_publication_sequences' yet it is not called that
(???)

These gripes may be related to my other thread [1] about the new ALTER
syntax. (I feel that there should be the ability to refresh ALL TABLES
or ALL SEQUENCES independently if the user wants to). IIUC, it would
simplify this function logic as well as being more flexible. Anyway, I
will leave the discussion about syntax to that other thread.

~

8.
+ if (relkind != RELKIND_SEQUENCE)
+ logicalrep_worker_stop(sub->oid, relid);

/*
* For READY state, we would have already dropped the
* tablesync origin.
*/
- if (state != SUBREL_STATE_READY)
+ if (state != SUBREL_STATE_READY && relkind != RELKIND_SEQUENCE)

It might be better to have a single "if (relkind != RELKIND_SEQUENCE)"
here and combine both of these codes under that.

~

9.
ereport(DEBUG1,
- (errmsg_internal("table \"%s.%s\" removed from subscription \"%s\"",
+ (errmsg_internal("%s \"%s.%s\" removed from subscription \"%s\"",
+ get_namespace_name(get_rel_namespace(relid)),
+ get_rel_name(relid),
+ sub->name,
+ get_rel_relkind(relid) == RELKIND_SEQUENCE ? "sequence" : "table")));

IIUC prior conDitions mean get_rel_relkind(relid) == RELKIND_SEQUENCE
will be impossible here.

~~~

10. AlterSubscription

+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ...
REFRESH PUBLICATION SEQUENCES");

IIUC the docs page for ALTER SUBSCRIPTION was missing this information
about "REFRESH PUBLICATION SEQUENCES" in transactions. Docs need more
updates.

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

logicalrep_worker_find:
nitpick - tweak comment to say "or" instead of "and"

~~~

11.
+/*
+ * Return the pid of the apply worker for one that matches given
+ * subscription id.
+ */
+static LogicalRepWorker *
+logicalrep_apply_worker_find(Oid subid, bool only_running)

The function comment is wrong. This is not returning a PID.

~~~

12.
+ if (is_sequencesync_worker)
+ Assert(!OidIsValid(relid));

Should we the Assert to something more like:
Assert(!is_sequencesync_worker || !OidIsValid(relid));

Otherwise, in NODEBUG current code will compile into an empty
condition statement, which is a bit odd.

~~~

logicalrep_seqsyncworker_failuretime:
nitpick - tweak function comment
nitpick - add blank line

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

13. fetch_remote_sequence_data

The "current state" mentioned in the function comment is a bit vague.
Can't tell from this comment what it is returning without looking
deeper into the function code.

~

nitpick - typo "scenarios" in comment

~~~

copy_sequence:
nitpick - typo "withe" in function comment
nitpick - typo /retreived/retrieved/
nitpick - add/remove blank lines

~~~

LogicalRepSyncSequences:
nitpick - move a comment.
nitpick - remove blank line

14.
+ /*
+ * Verify whether the current batch of sequences is synchronized or if
+ * there are no remaining sequences to synchronize.
+ */
+ if ((((curr_seq + 1) % MAX_SEQUENCES_SYNC_PER_BATCH) == 0) ||
+ (curr_seq + 1) == seq_count)

All this "curr_seq + 1" maths seems unnecessarily tricky. Can't we
just increment the cur_seq? before this calculation?

~

nitpick - simplify the comment about batching
nitpick - added a comment to the commit

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

finish_sync_worker:
nitpick - added an Assert so the if/else is less risky.
nitpick - modify the comment about failure time when it is a clean exit

~~~

15. process_syncing_sequences_for_apply

+ /* We need up-to-date sync state info for subscription sequences here. */
+ FetchTableStates(&started_tx, SUB_REL_KIND_ALL);

Should that say SUB_REL_KIND_SEQUENCE?

~

16.
+ /*
+ * If there are free sync worker slot(s), start a new sequence
+ * sync worker, and break from the loop.
+ */
+ if (nsyncworkers < max_sync_workers_per_subscription)

Should this "if" have some "else" code to log a warning if we have run
out of free workers? Otherwise, how will the user know that the system
may need tuning?

~~~

17. FetchTableStates

/* Fetch all non-ready tables. */
- rstates = GetSubscriptionRelations(MySubscription->oid, true);
+ rstates = GetSubscriptionRelations(MySubscription->oid, rel_type, true);

This feels risky. IMO there needs to be some prior Assert about the
rel_type. For example, if it happened to be SUB_REL_KIND_SEQUENCE then
this function code doesn't seem to make sense.

~~~

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

18. SetupApplyOrSyncWorker

+
+ if (isSequenceSyncWorker(MyLogicalRepWorker))
+ before_shmem_exit(logicalrep_seqsyncworker_failuretime, (Datum) 0);

Probably that should be using macro am_sequencesync_worker(), right?

======
src/include/catalog/pg_subscription_rel.h

19.
+typedef enum
+{
+ SUB_REL_KIND_TABLE,
+ SUB_REL_KIND_SEQUENCE,
+ SUB_REL_KIND_ALL,
+} SubscriptionRelKind;
+

I was not sure how helpful this is; it might not be needed. e.g. see
review comment for GetSubscriptionRelations

~~~

20.
+extern List *GetSubscriptionRelations(Oid subid, SubscriptionRelKind reltype,
+ bool not_ready);

There is a mismatch with the ‘not_ready’ parameter name here and in
the function implementation

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

nitpick - removed a blank line

======
99.
Please also see the attached diffs patch which implements all the
nitpicks mentioned above.

======
[1] syntax - https://www.postgresql.org/message-id/CAHut%2BPuFH1OCj-P1UKoRQE2X4-0zMG%2BN1V7jdn%3DtOQV4RNbAbw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia