Re: Skipping schema changes in publication

On Thu, Apr 28, 2022 at 4:50 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Wednesday, April 27, 2022 9:50 PM vignesh C <vignesh21(at)gmail(dot)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.

Slightly reworded and modified

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

Modified

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

Modified

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

Modified

> (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".

Modified

The attached v4 patch has the changes for the same.

Regards,
Vignesh