Re: Skipping schema changes in publication - Mailing list pgsql-hackers
From | vignesh C |
---|---|
Subject | Re: Skipping schema changes in publication |
Date | |
Msg-id | CALDaNm1T1pj6aDG-U_4Zxzykv-uxzYVAP7W_QAaYNcDKW+3xRA@mail.gmail.com Whole thread Raw |
In response to | Re: Skipping schema changes in publication (Peter Smith <smithpb2250@gmail.com>) |
List | pgsql-hackers |
On Tue, May 31, 2022 at 11:51 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Here are my review comments for patch v7-0002. > > ====== > > 1. doc/src/sgml/logical-replication.sgml > > @@ -1167,8 +1167,9 @@ CONTEXT: processing remote data for replication > origin "pg_16395" during "INSER > <para> > To add tables to a publication, the user must have ownership rights on the > table. To add all tables in schema to a publication, the user must be a > - superuser. To create a publication that publishes all tables or > all tables in > - schema automatically, the user must be a superuser. > + superuser. To add all tables to a publication, the user must be a superuser. > + To create a publication that publishes all tables or all tables in schema > + automatically, the user must be a superuser. > </para> > > I felt that maybe this whole paragraph should be rearranged. Put the > "create publication" parts before the "alter publication" parts; > Re-word the sentences more similarly. I also felt the ALL TABLES and > ALL TABLES IN SCHEMA etc should be written uppercase/literal since > that is what was meant. > > SUGGESTION > To create a publication using FOR ALL TABLES or FOR ALL TABLES IN > SCHEMA, the user must be a superuser. To add ALL TABLES or ALL TABLES > IN SCHEMA to a publication, the user must be a superuser. To add > tables to a publication, the user must have ownership rights on the > table. Modified > ~~~ > > 2. doc/src/sgml/ref/alter_publication.sgml > > @@ -82,8 +88,8 @@ ALTER PUBLICATION <replaceable > class="parameter">name</replaceable> RESET > > <para> > You must own the publication to use <command>ALTER PUBLICATION</command>. > - Adding a table to a publication additionally requires owning that table. > - The <literal>ADD ALL TABLES IN SCHEMA</literal>, > + Adding a table to or excluding a table from a publication additionally > + requires owning that table. The <literal>ADD ALL TABLES IN SCHEMA</literal>, > <literal>SET ALL TABLES IN SCHEMA</literal> to a publication and > > Isn't this missing some information that says ADD ALL TABLES requires > the invoking user to be a superuser? Modified > ~~~ > > 3. doc/src/sgml/ref/alter_publication.sgml - examples > > + <para> > + Alter publication <structname>production_publication</structname> to publish > + all tables except <structname>users</structname> and > + <structname>departments</structname> tables: > +<programlisting> > +ALTER PUBLICATION production_publication ADD ALL TABLES EXCEPT users, > departments; > +</programlisting></para> > + > > I didn't think it needs to say "tables" 2x (e.g. remove the last "tables") Modified > ~~~ > > 4. doc/src/sgml/ref/create_publication.sgml - examples > > + <para> > + Create a publication that publishes all changes in all the tables except for > + the changes of <structname>users</structname> and > + <structname>departments</structname> tables: > +<programlisting> > +CREATE PUBLICATION mypublication FOR ALL TABLES EXCEPT users, departments; > +</programlisting> > + </para> > > I didn't think it needs to say "tables" 2x (e.g. remove the last "tables") Modified > ~~~ > > 5. src/backend/catalog/pg_publication.c > > foreach(lc, ancestors) > { > Oid ancestor = lfirst_oid(lc); > - List *apubids = GetRelationPublications(ancestor); > - List *aschemaPubids = NIL; > + List *apubids = GetRelationPublications(ancestor, false); > + List *aschemapubids = NIL; > + List *aexceptpubids = NIL; > > level++; > > - if (list_member_oid(apubids, puboid)) > + /* check if member of table publications */ > + if (!list_member_oid(apubids, puboid)) > { > - topmost_relid = ancestor; > - > - if (ancestor_level) > - *ancestor_level = level; > - } > - else > - { > - aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor)); > - if (list_member_oid(aschemaPubids, puboid)) > + /* check if member of schema publications */ > + aschemapubids = GetSchemaPublications(get_rel_namespace(ancestor)); > + if (!list_member_oid(aschemapubids, puboid)) > { > - topmost_relid = ancestor; > - > - if (ancestor_level) > - *ancestor_level = level; > + /* > + * If the publication is all tables publication and the table > + * is not part of exception tables. > + */ > + if (puballtables) > + { > + aexceptpubids = GetRelationPublications(ancestor, true); > + if (list_member_oid(aexceptpubids, puboid)) > + goto next; > + } > + else > + goto next; > } > } > > + topmost_relid = ancestor; > + > + if (ancestor_level) > + *ancestor_level = level; > + > +next: > list_free(apubids); > - list_free(aschemaPubids); > + list_free(aschemapubids); > + list_free(aexceptpubids); > } > > > I felt those negative (!) conditions and those goto are making this > logic hard to understand. Can’t it be simplified more than this? Even > just having another bool flag might help make it easier. > > e.g. Perhaps something a bit like this (but add some comments) > > foreach(lc, ancestors) > { > Oid ancestor = lfirst_oid(lc); > List *apubids = GetRelationPublications(ancestor); > List *aschemaPubids = NIL; > List *aexceptpubids = NIL; > bool set_top = false; > level++; > > set_top = list_member_oid(apubids, puboid); > if (!set_top) > { > aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor)); > set_top = list_member_oid(aschemaPubids, puboid); > > if (!set_top && puballtables) > { > aexceptpubids = GetRelationPublications(ancestor, true); > set_top = !list_member_oid(aexceptpubids, puboid); > } > } > if (set_top) > { > topmost_relid = ancestor; > > if (ancestor_level) > *ancestor_level = level; > } > > list_free(apubids); > list_free(aschemapubids); > list_free(aexceptpubids); > } Modified > ------ > > 6. src/backend/commands/publicationcmds.c - CheckPublicationDefValues > > +/* > + * Check if the publication has default values > + * > + * Check the following: > + * a) Publication is not set with "FOR ALL TABLES" > + * b) Publication is having default options > + * c) Publication is not associated with schemas > + * d) Publication is not associated with relations > + */ > +static bool > +CheckPublicationDefValues(HeapTuple tup) > > I think Osumi-san already gave a review [1] about this same comment. > > So I only wanted to add that it should not say "options" here: > "default options" -> "default publication parameter values" Modified > ~~~ > > 7. src/backend/commands/publicationcmds.c - AlterPublicationSetAllTables > > +#ifdef USE_ASSERT_CHECKING > + Assert(!pubform->puballtables); > +#endif > > Why is this #ifdef needed? Isn't that logic built into the Assert macro already? pubform is used only for assert case. If we don't use it within #ifdef or PG_USED_FOR_ASSERTS_ONLY, it will throw a unused variable error without --enable-cassert like: publicationcmds.c: In function ‘AlterPublicationSetAllTables’: publicationcmds.c:1250:29: error: unused variable ‘pubform’ [-Werror=unused-variable] 1250 | Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup); | ^~~~~~~ cc1: all warnings being treated as errors > ~~~ > > 8. src/backend/commands/publicationcmds.c - AlterPublicationSetAllTables > > + /* set ALL TABLES flag */ > > Use uppercase 'S' to match other comments. Modified > ~~~ > > 9. src/backend/commands/publicationcmds.c - AlterPublication > > + if (!isdefault) > + ereport(ERROR, > + errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > + errmsg("adding ALL TABLES requires the publication to have default > publication options, no tables/schemas associated and ALL TABLES flag > should not be set"), > + errhint("Use ALTER PUBLICATION ... RESET to reset the publication")); > > IMO this errmsg text is not very good but I think Osumi-san [1] has > also given a review comment about the same errmsg. > > So I only wanted to add that should not say "options" here: > "default publication options" -> "default publication parameter values" Modified > ~~~ > > 10. src/backend/parser/gram.y > > /***************************************************************************** > * > * ALTER PUBLICATION name SET ( options ) > * > * ALTER PUBLICATION name ADD pub_obj [, ...] > * > * ALTER PUBLICATION name DROP pub_obj [, ...] > * > * ALTER PUBLICATION name SET pub_obj [, ...] > * > * ALTER PUBLICATION name RESET > * > * pub_obj is one of: > * > * TABLE table_name [, ...] > * ALL TABLES IN SCHEMA schema_name [, ...] > * > *****************************************************************************/ > > - > > Should the above comment be updated to mention also ADD ALL TABLES > ... EXCEPT [TABLE] ... Modified > ~~~ > > 11. src/bin/pg_dump/pg_dump.c - dumpPublication > > + /* Include exception tables if the publication has except tables */ > + for (cell = exceptinfo.head; cell; cell = cell->next) > + { > + PublicationRelInfo *pubrinfo = (PublicationRelInfo *) cell->ptr; > + PublicationInfo *relpubinfo = pubrinfo->publication; > + TableInfo *tbinfo; > + > + if (pubinfo == relpubinfo) > > I am unsure if that variable 'relpubinfo' is of much use; it is only > used one time. Removed relpubinfo > ~~~ > > 12. src/bin/pg_dump/t/002_pg_dump.pl > > I think there should be more test cases here: > > E.g.1. EXCEPT TABLE should also test a list of tables > > E.g.2. EXCEPT with optional TABLE keyword ommitted Added a test for list of tables and modified one of the test to remove TABLE. > ~~~ > > 13. src/bin/psql/describe.c - question about the SQL > > Since the new 'except' is a boolean column, wouldn't it be more > natural if all the SQL was treating it as one? > > e.g. should the SQL be saying "IS preexpect", "IS NOT prexcept"; > instead of comparing preexpect to 't' and 'f' character. modified > ~~~ > > 14. .../t/032_rep_changes_except_table.pl > > +# Test replication with publications created using FOR ALL TABLES EXCEPT TABLE > +# option. > +# Create schemas and tables on publisher > > "option" -> "clause" Modified. Thanks for the comments. The v8 patch attached at [1] has the fixes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm0sAU4s1KTLOEWv%3DrYo5dQK6uFTJn_0FKj3XG1Nv4D-qw%40mail.gmail.com Regards, Vignesh
pgsql-hackers by date: