public inbox for [email protected]
help / color / mirror / Atom feedFrom: vignesh C <[email protected]>
To: Peter Smith <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Sat, 21 May 2022 11:06:16 +0530
Message-ID: <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
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>
On Fri, May 20, 2022 at 11:23 AM Peter Smith <[email protected]> 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%40ma...
Regards,
Vignesh
view thread (377+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox