Re: Skipping schema changes in publication
От | vignesh C |
---|---|
Тема | Re: Skipping schema changes in publication |
Дата | |
Msg-id | CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Skipping schema changes in publication (vignesh C <vignesh21@gmail.com>) |
Ответы |
RE: Skipping schema changes in publication
("osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>)
Re: Skipping schema changes in publication (Peter Smith <smithpb2250@gmail.com>) Re: Skipping schema changes in publication (Peter Smith <smithpb2250@gmail.com>) |
Список | pgsql-hackers |
On Sat, May 21, 2022 at 11:06 AM vignesh C <vignesh21@gmail.com> wrote: > > 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 Attached v7 patch which fixes the buildfarm warning for an unused warning in release mode as in [1]. [1] - https://cirrus-ci.com/task/6220288017825792 Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Bharath RupireddyДата:
Сообщение: Re: Enforce "max_wal_size/ min_wal_size must be at least twice wal_segment_size" limit while setting GUCs
Следующее
От: Dilip KumarДата:
Сообщение: Re: postgres_fdw has insufficient support for large object