Re: Logical Replication of sequences - Mailing 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