On Mon, May 16, 2022 at 2:53 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Below are my review comments for v5-0001. > > There is some overlap with comments recently posted by Osumi-san [1]. > > (I also have review comments for v5-0002; will post them tomorrow) > > ====== > > 1. Commit message > > This patch adds a new RESET clause to ALTER PUBLICATION which will reset > the publication to default state which includes resetting the publication > options, setting ALL TABLES option to false and dropping the relations and > schemas that are associated with the publication. > > SUGGEST > "to default state" -> "to the default state" > "ALL TABLES option" -> "ALL TABLES flag"
Modified > ~~~ > > 2. doc/src/sgml/ref/alter_publication.sgml > > + <para> > + The <literal>RESET</literal> clause will reset the publication to the > + default state which includes resetting the publication options, setting > + <literal>ALL TABLES</literal> option to <literal>false</literal> and > + dropping all relations and schemas that are associated with the > publication. > </para> > > "ALL TABLES option" -> "ALL TABLES flag" Modified > ~~~ > > 3. doc/src/sgml/ref/alter_publication.sgml > > + invoking user to be a superuser. <literal>RESET</literal> of publication > + requires the invoking user to be a superuser. To alter the owner, you must > > SUGGESTION > To <literal>RESET</literal> a publication requires the invoking user > to be a superuser. I have combined it with the earlier sentence. > ~~~ > > 4. src/backend/commands/publicationcmds.c > > @@ -53,6 +53,13 @@ > #include "utils/syscache.h" > #include "utils/varlena.h" > > +#define PUB_ATION_INSERT_DEFAULT true > +#define PUB_ACTION_UPDATE_DEFAULT true > +#define PUB_ACTION_DELETE_DEFAULT true > +#define PUB_ACTION_TRUNCATE_DEFAULT true > +#define PUB_VIA_ROOT_DEFAULT false > +#define PUB_ALL_TABLES_DEFAULT false > > 4a. > Typo: "ATION" -> "ACTION" Modified > 4b. > I think these #defines deserve a 1 line comment. > e.g. > /* CREATE PUBLICATION default values for flags and options */ Added comment > 4c. > Since the "_DEFAULT" is a common part of all the names, maybe it is > tidier if it comes first. > e.g. > #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 Modified The v6 patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ%40mail.gmail.com Regards, Vignesh