Re: Skipping schema changes in publication - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Skipping schema changes in publication |
Date | |
Msg-id | CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com Whole thread Raw |
In response to | Re: Skipping schema changes in publication (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: Skipping schema changes in publication
|
List | pgsql-hackers |
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
pgsql-hackers by date: