Re: Skipping schema changes in publication
От | vignesh C |
---|---|
Тема | Re: Skipping schema changes in publication |
Дата | |
Msg-id | CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Skipping schema changes in publication (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
RE: Skipping schema changes in publication
RE: Skipping schema changes in publication Re: Skipping schema changes in publication Re: Skipping schema changes in publication RE: Skipping schema changes in publication |
Список | pgsql-hackers |
On Fri, May 13, 2022 at 9:37 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, May 12, 2022 at 2:24 PM vignesh C <vignesh21@gmail.com> wrote: > > > ... > > The attached patch has the implementation for "ALTER PUBLICATION > > pubname RESET". This command 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. > > > > Please see below my review comments for the v1-0001 (RESET) patch > > ====== > > 1. Commit message > > This patch adds a new RESET option to ALTER PUBLICATION which > > Wording: "RESET option" -> "RESET clause" Modified > ~~~ > > 2. doc/src/sgml/ref/alter_publication.sgml > > + <para> > + The <literal>RESET</literal> clause will reset the publication to default > + state which includes resetting the publication options, setting > + <literal>ALL TABLES</literal> option to <literal>false</literal> > and drop the > + relations and schemas that are associated with the publication. > </para> > > 2a. Wording: "to default state" -> "to the default state" Modified > 2b. Wording: "and drop the relations..." -> "and dropping all relations..." Modified > ~~~ > > 3. doc/src/sgml/ref/alter_publication.sgml > > + invoking user to be a superuser. <literal>RESET</literal> of publication > + requires invoking user to be a superuser. To alter the owner, you must also > > Wording: "requires invoking user" -> "requires the invoking user" Modified > ~~~ > > 4. doc/src/sgml/ref/alter_publication.sgml - Example > > @@ -207,6 +220,12 @@ ALTER PUBLICATION sales_publication ADD ALL > TABLES IN SCHEMA marketing, sales; > <structname>production_publication</structname>: > <programlisting> > ALTER PUBLICATION production_publication ADD TABLE users, > departments, ALL TABLES IN SCHEMA production; > +</programlisting></para> > + > + <para> > + Resetting the publication <structname>production_publication</structname>: > +<programlisting> > +ALTER PUBLICATION production_publication RESET; > > Wording: "Resetting the publication" -> "Reset the publication" Modified > ~~~ > > 5. src/backend/commands/publicationcmds.c > > + /* Check and reset the options */ > > IMO the code can just reset all these options unconditionally. I did > not see the point to check for existing option values first. I feel > the simpler code outweighs any negligible performance difference in > this case. Modified > ~~~ > > 6. src/backend/commands/publicationcmds.c > > + /* Check and reset the options */ > > Somehow it seemed a pity having to hardcode all these default values > true/false in multiple places; e.g. the same is already hardcoded in > the parse_publication_options function. > > To avoid multiple hard coded bools you could just call the > parse_publication_options with an empty options list. That would set > the defaults which you can then use: > values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactiondefs->insert); > > Alternatively, maybe there should be #defines to use instead of having > the scattered hardcoded bool defaults: > #define PUBACTION_DEFAULT_INSERT true > #define PUBACTION_DEFAULT_UPDATE true > etc I have used #define for default value and used it in both the functions. > ~~~ > > 7. src/include/nodes/parsenodes.h > > @@ -4033,7 +4033,8 @@ typedef enum AlterPublicationAction > { > AP_AddObjects, /* add objects to publication */ > AP_DropObjects, /* remove objects from publication */ > - AP_SetObjects /* set list of objects */ > + AP_SetObjects, /* set list of objects */ > + AP_ReSetPublication /* reset the publication */ > } AlterPublicationAction; > > Unusual case: "AP_ReSetPublication" -> "AP_ResetPublication" Modified > ~~~ > > 8. src/test/regress/sql/publication.sql > > 8a. > +-- Test for RESET PUBLICATION > SUGGESTED > +-- Tests for ALTER PUBLICATION ... RESET Modified > 8b. > +-- Verify that 'ALL TABLES' option is reset > SUGGESTED: > +-- Verify that 'ALL TABLES' flag is reset Modified > 8c. > +-- Verify that publish option and publish via root option is reset > SUGGESTED: > +-- Verify that publish options and publish_via_partition_root option are reset Modified > 8d. > +-- Verify that only superuser can execute RESET publication > SUGGESTED > +-- Verify that only superuser can reset a publication Modified Thanks for the comments, the attached v5 patch has the changes for the same. Also I have made the changes for SKIP Table based on the new syntax, the changes for the same are available in v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch. Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления: