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>
In-Reply-To: 
 <CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 20 May 2022 15:53:12 +1000
Message-ID: 
 <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@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"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPtiomM%2BiyAZHvb2dzfsPvRru266KuBe49hKy2n2h%2Bm_zA%40mail.gmail.com>
Precedence: bulk

Below are my review comments for v6-0002.

======

1. Commit message.
The psql \d family of commands to display excluded tables.

SUGGESTION
The psql \d family of commands can now display excluded tables.

~~~

2. doc/src/sgml/ref/alter_publication.sgml

@@ -22,6 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ADD <replaceable class="parameter">publication_object</replaceable> [,
...]
+ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ADD ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]

The "exception_object" font is wrong. Should look the same as
"publication_object"

~~~

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

@@ -214,6 +220,14 @@ ALTER PUBLICATION sales_publication ADD ALL
TABLES IN SCHEMA marketing, sales;
 </programlisting>
   </para>

+  <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 TABLE
users, departments;
+</programlisting></para>

Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
show TABLE keyword is optional.

~~~

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

An SGML tag error caused building the docs to fail. My fix was
previously reported [1].

~~~

5. doc/src/sgml/ref/create_publication.sgml

@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
-    [ FOR ALL TABLES
+    [ FOR ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]

The "exception_object" font is wrong. Should look the same as
"publication_object"

~~~

6. doc/src/sgml/ref/create_publication.sgml - Examples

@@ -351,6 +366,15 @@ CREATE PUBLICATION production_publication FOR
TABLE users, departments, ALL TABL
 CREATE PUBLICATION sales_publication FOR ALL TABLES IN SCHEMA marketing, sales;
 </programlisting></para>

+  <para>
+   Create a publication that publishes all changes in all the tables except for
+   the changes of <structname>users</structname> and
+   <structname>departments</structname> table:
+<programlisting>
+CREATE PUBLICATION mypublication FOR ALL TABLE EXCEPT TABLE users, departments;
+</programlisting>
+  </para>
+

6a.
Typo: "FOR ALL TABLE" -> "FOR ALL TABLES"

6b.
Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
show TABLE keyword is optional.

~~~

7. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication

@@ -316,18 +316,25 @@ GetTopMostAncestorInPublication(Oid puboid, List
*ancestors, int *ancestor_level
  }
  else
  {
- aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
- if (list_member_oid(aschemaPubids, puboid))
+ List    *aschemapubids = NIL;
+ List    *aexceptpubids = NIL;
+
+ aschemapubids = GetSchemaPublications(get_rel_namespace(ancestor));
+ aexceptpubids = GetRelationPublications(ancestor, true);
+ if (list_member_oid(aschemapubids, puboid) ||
+ (puballtables && !list_member_oid(aexceptpubids, puboid)))
  {

You could re-write this as multiple conditions instead of one. That
could avoid always assigning the 'aexceptpubids', so it might be a
more efficient way to write this logic.

~~~

8. src/backend/catalog/pg_publication.c - CheckPublicationDefValues

+/*
+ * Check if the publication has default values
+ *
+ * Check the following:
+ * Publication is having default options
+ *  Publication is not associated with relations
+ *  Publication is not associated with schemas
+ *  Publication is not set with "FOR ALL TABLES"
+ */
+static bool
+CheckPublicationDefValues(HeapTuple tup)

8a.
Remove the tab. Replace with spaces.

8b.
It might be better if this comment order is the same as the logic order.
e.g.

* Check the following:
*  Publication is not set with "FOR ALL TABLES"
*  Publication is having default options
*  Publication is not associated with schemas
*  Publication is not associated with relations

~~~

9. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables

+/*
+ * Reset the publication.
+ *
+ * Reset the publication options, publication relations and
publication schemas.
+ */
+static void
+AlterPublicationSetAllTables(Relation rel, HeapTuple tup)

The function comment and the function name do not seem to match here;
something looks like a cut/paste error ??

~~~

10. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables

+ /* set all tables option */
+ values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(true);
+ replaces[Anum_pg_publication_puballtables - 1] = true;

SUGGEST (comment)
/* set all ALL TABLES flag */

~~~

11. src/backend/catalog/pg_publication.c - AlterPublication

@@ -1501,6 +1579,20 @@ AlterPublication(ParseState *pstate,
AlterPublicationStmt *stmt)
  aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
     stmt->pubname);

+ if (stmt->for_all_tables)
+ {
+ bool isdefault = CheckPublicationDefValues(tup);
+
+ if (!isdefault)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("Setting ALL TABLES requires publication \"%s\" to have
default values",
+    stmt->pubname),
+ errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));

The errmsg should start with a lowercase letter.

~~~

12. src/backend/catalog/pg_publication.c - AlterPublication

@@ -1501,6 +1579,20 @@ AlterPublication(ParseState *pstate,
AlterPublicationStmt *stmt)
  aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
     stmt->pubname);

+ if (stmt->for_all_tables)
+ {
+ bool isdefault = CheckPublicationDefValues(tup);
+
+ if (!isdefault)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("Setting ALL TABLES requires publication \"%s\" to have
default values",
+    stmt->pubname),
+ errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));

Example test:

postgres=# create table t1(a int);
CREATE TABLE
postgres=# create publication p1 for table t1;
CREATE PUBLICATION
postgres=# alter publication p1 add all tables except t1;
2022-05-20 14:34:49.301 AEST [21802] ERROR:  Setting ALL TABLES
requires publication "p1" to have default values
2022-05-20 14:34:49.301 AEST [21802] HINT:  Use ALTER PUBLICATION ...
RESET to reset the publication
2022-05-20 14:34:49.301 AEST [21802] STATEMENT:  alter publication p1
add all tables except t1;
ERROR:  Setting ALL TABLES requires publication "p1" to have default values
HINT:  Use ALTER PUBLICATION ... RESET to reset the publication
postgres=# alter publication p1 set all tables except t1;

That error message does not quite match what the user was doing.
Firstly, they were adding the ALL TABLES, not setting it. Secondly,
all the values of the publication were already defaults (only there
was an existing table t1 in the publication). Maybe some minor changes
to the message wording can be a better reflect what the user is doing
here.

~~~

13. src/backend/parser/gram.y

@@ -10410,7 +10411,7 @@ AlterOwnerStmt: ALTER AGGREGATE
aggregate_with_argtypes OWNER TO RoleSpec
  *
  * CREATE PUBLICATION name [WITH options]
  *
- * CREATE PUBLICATION FOR ALL TABLES [WITH options]
+ * CREATE PUBLICATION FOR ALL TABLES [EXCEPT TABLE table [, ...]]
[WITH options]

Comment should show the "TABLE" keyword is optional

~~~

14. src/bin/pg_dump/pg_dump.c - dumpPublicationTable

@@ -4332,6 +4380,7 @@ dumpPublicationTable(Archive *fout, const
PublicationRelInfo *pubrinfo)

  appendPQExpBuffer(query, "ALTER PUBLICATION %s ADD TABLE ONLY",
    fmtId(pubinfo->dobj.name));
+
  appendPQExpBuffer(query, " %s",
    fmtQualifiedDumpable(tbinfo));

This additional whitespace seems unrelated to this patch

~~~

15. src/include/nodes/parsenodes.h

15a.
@@ -3999,6 +3999,7 @@ typedef struct PublicationTable
  RangeVar   *relation; /* relation to be published */
  Node    *whereClause; /* qualifications */
  List    *columns; /* List of columns in a publication table */
+ bool except; /* except relation */
 } PublicationTable;

Maybe the comment should be more like similar ones:
/* exclude the relation */

15b.
@@ -4007,6 +4008,7 @@ typedef struct PublicationTable
 typedef enum PublicationObjSpecType
 {
  PUBLICATIONOBJ_TABLE, /* A table */
+ PUBLICATIONOBJ_EXCEPT_TABLE, /* An Except table */
  PUBLICATIONOBJ_TABLES_IN_SCHEMA, /* All tables in schema */
  PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA, /* All tables in first element of

Maybe the comment should be more like:
/* A table to be excluded */

~~~

16. src/test/regress/sql/publication.sql

I did not see any test cases using EXCEPT when the optional TABLE
keyword is omitted.


------
[1] https://www.postgresql.org/message-id/CAHut%2BPtZDfBJ1d%3D3kSexgM5m%2BP_ok8sdsJXKimsXycaMyqXsNA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia