Re: Skipping schema changes in publication - Mailing list pgsql-hackers

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

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