Re: Logical Replication of sequences - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Logical Replication of sequences |
Date | |
Msg-id | CAHut+PsfsfzyBrmo8E43qFMp9_bmen2tuCsNYN8sX=fa86SdfA@mail.gmail.com Whole thread Raw |
In response to | Re: Logical Replication of sequences (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: Logical Replication of sequences
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: