Re: Logical Replication of sequences - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Logical Replication of sequences |
Date | |
Msg-id | CAHut+PtexjCsZyFWbAT0-dTazUkm61G88JQcCnYB-tf4KPuxvg@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 |
Here are my review comments for the patch v20240703-0002 ====== doc/src/sgml/ref/create_publication.sgml nitpick - consider putting the "FOR ALL SEQUENCES" para last, because eventually when more sequence syntax is added IMO it will be better to describe all the TABLES together, and then describe all the SEQUENCES together. nitpick - /synchronizing changes/synchronizes changes/ Question: Was there a reason you chose wording "synchronizes changes" instead of having same "replicates changes" wording of FOR ALL TABLES? ====== src/backend/catalog/system_views.sql 1. Should there be some new test for the view? Otherwise, AFAICT this patch has no tests that will exercise the new function pg_get_publication_sequences. ====== src/backend/commands/publicationcmds.c 2. + errmsg("must be superuser to create FOR ALL %s publication", + stmt->for_all_tables ? "TABLES" : "SEQUENCES"))); nitpick - the combined error message may be fine, but I think translators will prefer the substitution to be the full "FOR ALL TABLES" and "FOR ALL SEQUENCES" instead of just the keywords that are different. ====== src/backend/parser/gram.y 3. Some of these new things maybe could be named better? 'preprocess_allpubobjtype_list' => 'preprocess_pub_all_objtype_list' 'AllPublicationObjSpec *allpublicationobjectspec;' => 'PublicationAllObjSpec *publicationallobjectspec;' (I didn't include these in nitpicks diffs because you probably have better ideas than I do for good names) ~~~ nitpick - typo in comment /SCHEMAS/SEQUENCES/ preprocess_allpubobjtype_list: nitpick - typo /allbjects_list/all_objects_list/ nitpick - simplify /allpubob/obj/ nitpick - add underscores in the enums ====== src/bin/pg_dump/pg_dump.c 4. + if (pubinfo->puballtables || pubinfo->puballsequences) + { + appendPQExpBufferStr(query, " FOR ALL"); + if (pubinfo->puballtables && pubinfo->puballsequences) + appendPQExpBufferStr(query, " TABLES, SEQUENCES"); + else if (pubinfo->puballtables) + appendPQExpBufferStr(query, " TABLES"); + else + appendPQExpBufferStr(query, " SEQUENCES"); + } nitpick - it seems over-complicated; See nitpicks diff for my suggestion. ====== src/include/nodes/parsenodes.h nitpick - put underscores in the enum values ~~ 5. - bool for_all_tables; /* Special publication for all tables in db */ + List *for_all_objects; /* Special publication for all objects in + * db */ Is this OK? Saying "for all objects" seemed misleading. ====== src/test/regress/sql/publication.sql nitpick - some small changes to comments, e.g. writing keywords in uppercase ~~~ 6. I asked this before in a previous review [1-#17] -- I didn't understand the point of the sequence 'testpub_seq0' since nobody seems to be doing anything with it. Should it just be removed? Or is there a missing test case to use it? ~~~ 7. Other things to consider: (I didn't include these in my attached diff) * could use a single CREATE SEQUENCE stmt instead of multiple * could use a single DROP PUBLICATION stmt instead of multiple * shouldn't all publication names ideally have a 'regress_' prefix? ====== 99. Please refer to the attached nitpicks diff which has implementation for the nitpicks cited above. ====== [1] https://www.postgresql.org/message-id/CAHut%2BPvrk75vSDkaXJVmhhZuuqQSY98btWJV%3DBMZAnyTtKRB4g%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
Attachment
pgsql-hackers by date: