On Wednesday, April 27, 2022 9:50 PM vignesh C <vignes...@gmail.com> wrote: > Thanks for the comments, the attached v3 patch has the changes for the same. Hi
Thank you for updating the patch. Several minor comments on v3. (1) commit message The new syntax allows specifying schemas. For example: CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2; OR ALTER PUBLICATION pub1 ADD EXCEPT TABLE t1,t2; We have above sentence, but it looks better to make the description a bit more accurate. Kindly change From : "The new syntax allows specifying schemas" To : "The new syntax allows specifying excluded relations" Also, kindly change "OR" to "or", because this description is not syntax. (2) publication_add_relation @@ -396,6 +400,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, ObjectIdGetDatum(pubid); values[Anum_pg_publication_rel_prrelid - 1] = ObjectIdGetDatum(relid); + values[Anum_pg_publication_rel_prexcept - 1] = + BoolGetDatum(pri->except); + /* Add qualifications, if available */ It would be better to remove the blank line, because with this change, we'll have two blank lines in a row. (3) pg_dump.h & pg_dump_sort.c @@ -80,6 +80,7 @@ typedef enum DO_REFRESH_MATVIEW, DO_POLICY, DO_PUBLICATION, + DO_PUBLICATION_EXCEPT_REL, DO_PUBLICATION_REL, DO_PUBLICATION_TABLE_IN_SCHEMA, DO_SUBSCRIPTION @@ -90,6 +90,7 @@ enum dbObjectTypePriorities PRIO_FK_CONSTRAINT, PRIO_POLICY, PRIO_PUBLICATION, + PRIO_PUBLICATION_EXCEPT_REL, PRIO_PUBLICATION_REL, PRIO_PUBLICATION_TABLE_IN_SCHEMA, PRIO_SUBSCRIPTION, @@ -144,6 +145,7 @@ static const int dbObjectTypePriority[] = PRIO_REFRESH_MATVIEW, /* DO_REFRESH_MATVIEW */ PRIO_POLICY, /* DO_POLICY */ PRIO_PUBLICATION, /* DO_PUBLICATION */ + PRIO_PUBLICATION_EXCEPT_REL, /* DO_PUBLICATION_EXCEPT_REL */ PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */ PRIO_PUBLICATION_TABLE_IN_SCHEMA, /* DO_PUBLICATION_TABLE_IN_SCHEMA */ PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */ How about having similar order between pg_dump.h and pg_dump_sort.c, like we'll add DO_PUBLICATION_EXCEPT_REL after DO_PUBLICATION_REL in pg_dump.h ? (4) GetAllTablesPublicationRelations + /* + * pg_publication_rel and pg_publication_namespace will only have except + * tables in case of all tables publication, no need to pass except flag + * to get the relations. + */ + List *exceptpubtablelist = GetPublicationRelations(pubid, PUBLICATION_PART_ALL); + There is one unnecessary space in a comment "...pg_publication_namespace will only have...". Kindly remove it. Then, how about diving the variable declaration and the insertion of the return value of GetPublicationRelations ? That might be aligned with other places in this file. (5) GetTopMostAncestorInPublication @@ -302,8 +303,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level foreach(lc, ancestors) { Oid ancestor = lfirst_oid(lc); - List *apubids = GetRelationPublications(ancestor); + List *apubids = GetRelationPublications(ancestor, false); List *aschemaPubids = NIL; + List *aexceptpubids; level++; @@ -317,7 +319,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level else { aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor)); - if (list_member_oid(aschemaPubids, puboid)) + aexceptpubids = GetRelationPublications(ancestor, true); + if (list_member_oid(aschemaPubids, puboid) || + (puballtables && !list_member_oid(aexceptpubids, puboid))) { topmost_relid = ancestor; It seems we forgot to call list_free for "aexceptpubids". Best Regards, Takamichi Osumi