MIME-Version: 1.0
References: 
 <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
 <CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
 <CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
 <CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
 <CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
 <CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
 <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
 <CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>
 <TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ@mail.gmail.com>
 <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
 <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com>
 <CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com>
 <CAHut+PtsyfwL3opi2u4gwak7nip66agq7rQynRBL97oXBjKkcg@mail.gmail.com>
In-Reply-To: 
 <CAHut+PtsyfwL3opi2u4gwak7nip66agq7rQynRBL97oXBjKkcg@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Fri, 3 Jun 2022 15:50:08 +0530
Message-ID: 
 <CALDaNm1T1pj6aDG-U_4Zxzykv-uxzYVAP7W_QAaYNcDKW+3xRA@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: Peter Smith <smithpb2250@gmail.com>
Cc: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm1T1pj6aDG-U_4Zxzykv-uxzYVAP7W_QAaYNcDKW%2B3xRA%40mail.gmail.com>
Precedence: bulk

On Tue, May 31, 2022 at 11:51 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for patch v7-0002.
>
> =3D=3D=3D=3D=3D=3D
>
> 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 o=
n 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 sup=
eruser.
> +   To create a publication that publishes all tables or all tables in sc=
hema
> +   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=3D"parameter">name</replaceable> RESET
>
>    <para>
>     You must own the publication to use <command>ALTER PUBLICATION</comma=
nd>.
> -   Adding a table to a publication additionally requires owning that tab=
le.
> -   The <literal>ADD ALL TABLES IN SCHEMA</literal>,
> +   Adding a table to or excluding a table from a publication additionall=
y
> +   requires owning that table. The <literal>ADD ALL TABLES IN SCHEMA</li=
teral>,
>     <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 exc=
ept for
> +   the changes of <structname>users</structname> and
> +   <structname>departments</structname> tables:
> +<programlisting>
> +CREATE PUBLICATION mypublication FOR ALL TABLES EXCEPT users, department=
s;
> +</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 =3D lfirst_oid(lc);
> - List    *apubids =3D GetRelationPublications(ancestor);
> - List    *aschemaPubids =3D NIL;
> + List    *apubids =3D GetRelationPublications(ancestor, false);
> + List    *aschemapubids =3D NIL;
> + List    *aexceptpubids =3D NIL;
>
>   level++;
>
> - if (list_member_oid(apubids, puboid))
> + /* check if member of table publications */
> + if (!list_member_oid(apubids, puboid))
>   {
> - topmost_relid =3D ancestor;
> -
> - if (ancestor_level)
> - *ancestor_level =3D level;
> - }
> - else
> - {
> - aschemaPubids =3D GetSchemaPublications(get_rel_namespace(ancestor));
> - if (list_member_oid(aschemaPubids, puboid))
> + /* check if member of schema publications */
> + aschemapubids =3D GetSchemaPublications(get_rel_namespace(ancestor));
> + if (!list_member_oid(aschemapubids, puboid))
>   {
> - topmost_relid =3D ancestor;
> -
> - if (ancestor_level)
> - *ancestor_level =3D level;
> + /*
> + * If the publication is all tables publication and the table
> + * is not part of exception tables.
> + */
> + if (puballtables)
> + {
> + aexceptpubids =3D GetRelationPublications(ancestor, true);
> + if (list_member_oid(aexceptpubids, puboid))
> + goto next;
> + }
> + else
> + goto next;
>   }
>   }
>
> + topmost_relid =3D ancestor;
> +
> + if (ancestor_level)
> + *ancestor_level =3D 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=E2=80=99t 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 =3D lfirst_oid(lc);
> List    *apubids =3D GetRelationPublications(ancestor);
> List    *aschemaPubids =3D NIL;
> List    *aexceptpubids =3D NIL;
> bool set_top =3D false;
> level++;
>
> set_top =3D list_member_oid(apubids, puboid);
> if (!set_top)
> {
> aschemaPubids =3D GetSchemaPublications(get_rel_namespace(ancestor));
> set_top =3D list_member_oid(aschemaPubids, puboid);
>
> if (!set_top && puballtables)
> {
> aexceptpubids =3D GetRelationPublications(ancestor, true);
> set_top =3D !list_member_oid(aexceptpubids, puboid);
> }
> }
> if (set_top)
> {
> topmost_relid =3D ancestor;
>
> if (ancestor_level)
> *ancestor_level =3D 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 a=
lready?

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 =E2=80=98AlterPublicationSetAllTables=E2=80=
=99:
publicationcmds.c:1250:29: error: unused variable =E2=80=98pubform=E2=80=99
[-Werror=3Dunused-variable]
 1250 |         Form_pg_publication pubform =3D (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 =3D exceptinfo.head; cell; cell =3D cell->next)
> + {
> + PublicationRelInfo *pubrinfo =3D (PublicationRelInfo *) cell->ptr;
> + PublicationInfo *relpubinfo =3D pubrinfo->publication;
> + TableInfo  *tbinfo;
> +
> + if (pubinfo =3D=3D 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 TABL=
E.

> ~~~
>
> 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%3DrYo5dQK=
6uFTJn_0FKj3XG1Nv4D-qw%40mail.gmail.com

Regards,
Vignesh