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>
In-Reply-To: 
 <CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Tue, 31 May 2022 16:20:48 +1000
Message-ID: 
 <CAHut+PtsyfwL3opi2u4gwak7nip66agq7rQynRBL97oXBjKkcg@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: vignesh C <vignesh21@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/CAHut%2BPtsyfwL3opi2u4gwak7nip66agq7rQynRBL97oXBjKkcg%40mail.gmail.com>
Precedence: bulk

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 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 super=
user.
+   To create a publication that publishes all tables or all tables in sche=
ma
+   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.

~~~

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</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</lite=
ral>,
    <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?

~~~

3. doc/src/sgml/ref/alter_publication.sgml - examples

+  <para>
+   Alter publication <structname>production_publication</structname> to pu=
blish
+   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")

~~~

4. doc/src/sgml/ref/create_publication.sgml - examples

+  <para>
+   Create a publication that publishes all changes in all the tables excep=
t 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")

~~~

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? Ev=
en
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);
}

------

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"

~~~

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 alr=
eady?

~~~

8. src/backend/commands/publicationcmds.c - AlterPublicationSetAllTables

+ /* set ALL TABLES flag */

Use uppercase 'S' to match other comments.

~~~

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"

~~~

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] ...

~~~

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.

~~~

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

~~~

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.

~~~

14. .../t/032_rep_changes_except_table.pl

+# Test replication with publications created using FOR ALL TABLES EXCEPT T=
ABLE
+# option.
+# Create schemas and tables on publisher

"option" -> "clause"

------
[1] https://www.postgresql.org/message-id/TYCPR01MB83730A2F1D6A5303E9C1416A=
EDD99%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia