Horton

Re: Skipping schema changes in publication

vignesh C <vignesh21@gmail.com>

View thread
From: vignesh C <vignesh21@gmail.com>
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Euler Taveira <euler@eulerto.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-04-29T11:42:59Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix miscellaneous issues in EXCEPT publication clause.

  2. Change syntax of EXCEPT TABLE clause in publication commands.

  3. Add support for EXCEPT TABLE in ALTER PUBLICATION.

  4. Allow table exclusions in publications via EXCEPT TABLE.

  5. Add wait_for_subscription_sync for TAP tests.

Attachments

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