Hi Shlok, Some review comments for patch v28-0001.
====== src/backend/commands/publicationcmds.c AlterPublicationReset: 1. + values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(PUB_DEFAULT_ALL_TABLES); + replaces[Anum_pg_publication_puballtables - 1] = true; + + values[Anum_pg_publication_puballsequences - 1] = BoolGetDatum(PUB_DEFAULT_ALL_SEQUENCES); + replaces[Anum_pg_publication_puballsequences - 1] = true; The PUB_DEFAULT_xxx made sense for all the publication parameters, but these are just flags that say if the ALL TABLES or ALL SEQUENCES clauses are present in the command, so I'm not sure that "default" macros are appropriate here. e.g. it seems better to reset those using hardwired booleans: values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(false); values[Anum_pg_publication_puballsequences - 1] = BoolGetDatum(false); (This would also be consistent with what the function comment is saying) ====== src/include/catalog/pg_publication.h 2. +/* default values for flags and publication parameters */ +#define PUB_DEFAULT_ACTION_INSERT true +#define PUB_DEFAULT_ACTION_UPDATE true +#define PUB_DEFAULT_ACTION_DELETE true +#define PUB_DEFAULT_ACTION_TRUNCATE true +#define PUB_DEFAULT_VIA_ROOT false +#define PUB_DEFAULT_ALL_TABLES false +#define PUB_DEFAULT_ALL_SEQUENCES false +#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE As in the previous comment #1, I don't think we should define PUB_DEFAULT_ALL_TABLES and PUB_DEFAULT_ALL_SEQUENCES. Also, change the comment to remove the words "flags and". ====== src/test/regress/sql/publication.sql 3. I don't think the parameter names should be written in uppercase because that's not how they normally appear in the docs and examples. ~~~ 4. +-- Verify that 'ALL TABLES', 'ALL SEQUENCES' flag is reset typo: /flag is reset/flags are reset/ ~~~ 5. +-- Verify that associated tables, schemas and the publication parameters +-- 'PUBLISH', 'PUBLISH_VIA_PARTITION_ROOT', and 'PUBLISH_GENERATED_COLUMNS' +-- are removed from the publication after RESET The comment makes it sound like parameters are "removed". Maybe some rewording is needed. SUGGESTION Verify that a publication RESET removes the associated tables and schemas, and sets default values for publication parameters 'publish', 'publish_via_partition_root', and 'publish_generated_columns'. ====== Kind Regards, Peter Smith. Fujitsu Australia
