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>
In-Reply-To: 
 <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Sat, 21 May 2022 11:06:16 +0530
Message-ID: 
 <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@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"
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ%40mail.gmail.com>
Precedence: bulk

On Fri, May 20, 2022 at 11:23 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

Modified

> ~~~
>
> 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"

Modified

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

Modified

> ~~~
>
> 4. doc/src/sgml/ref/create_publication.sgml
>
> An SGML tag error caused building the docs to fail. My fix was
> previously reported [1].

Modified

> ~~~
>
> 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"

Modified

> ~~~
>
> 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"

Modified

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

Modified

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

Modified

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

Modified

> 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

Modified

> ~~~
>
> 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 ??

Modified

> ~~~
>
> 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 */

Modified

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

Modified

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

Modified

> ~~~
>
> 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

Modified

> ~~~
>
> 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

Modified

> ~~~
>
> 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 */

Modified

> 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 */

Modified

> ~~~
>
> 16. src/test/regress/sql/publication.sql
>
> I did not see any test cases using EXCEPT when the optional TABLE
> keyword is omitted.

Added a test

Thanks for the comments, the v7 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm3EpX3%2BRu%3DSNaYi%3DUW5ZLE6nNhGRHZ7a8-fXPZ_-gLdxQ%40mail.gmail.com

Regards,
Vignesh