Re: Skipping schema changes in publication

On Thu, 19 Feb 2026 at 16:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 19, 2026 at 10:13 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > Thanks for reviewing the patch. I have addressed the remaining
> > comments in the v46 patch..
> >
>
> Can we think of some ideas to split this patch? One possibility is
> that in the first patch we give an ERROR, if a non-root partitioned
> table is present in EXCEPT Clause. I see that a lot of code is written
> to handle partitions in EXCEPT clause. I feel such a split will make
> code easier to review and validate.

Split it accordingly.

> Few other comments:
> =================
> 1.
>   if (stmt->for_all_tables)
>   {
> + /* Process EXCEPT table list */
> + if (relations != NIL)
> + {
> + Assert(rels != NIL);
> + PublicationAddTables(puboid, rels, true, NULL);
> + }
> +
>   /*
>   * Invalidate relcache so that publication info is rebuilt. Sequences
>   * publication doesn't require invalidation, as replica identity
> CacheInvalidateRelcacheAll();
>
> Here, the relations listed in the except table list will be
> invalidated twice, once inside
> PublicationAddTables->publication_add_relation, and second time via
> CacheInvalidateRelcacheAll. Can we avoid that by adding a parameter to
> PublicationAddTables?

Fixed this

> 2.
> - root_relids = GetPublicationRelations(pubform->oid,
> -   PUBLICATION_PART_ROOT);
> + root_relids = GetIncludedRelationsByPub(pubform->oid,
> + PUBLICATION_PART_ROOT);
>
> To follow the previous function naming pattern, can we rename
> GetIncludedRelationsByPub to GetIncludedPublicationRelations? If we
> agree to this then rename the excluded version of the function as
> well.

Modified

> 3.
> +/*
> + * Return the list of relation Oids for a publication.
> + *
> + * For a FOR TABLE publication, this returns the list of relations explicitly
> + * included in the publication.
> + *
> + * Publications declared with FOR ALL TABLES/SEQUENCES should use
> + * GetAllPublicationRelations() to obtain the complete set of tables/sequences
> + * covered by the publication.
> + */
> +List *
> +GetIncludedRelationsByPub(Oid pubid, PublicationPartOpt pub_partopt)
>
> This is equivalent to the existing function GetPublicationRelations()
> which has more precise comments atop. We can use the same comments
> unless there is any functionality difference.

Updated it

> 4.
> --- a/src/backend/catalog/pg_publication.c
> +++ b/src/backend/catalog/pg_publication.c
> @@ -29,6 +29,7 @@
>  #include "catalog/pg_publication.h"
>  #include "catalog/pg_publication_namespace.h"
>  #include "catalog/pg_publication_rel.h"
> +#include "catalog/pg_subscription.h"
>
> It appears odd to include pg_subscription.h in the publication code.
> Is there a reason for the same? If not then avoid it.

This was required because we now started using GetPublicationsStr. I
have moved GetPublicationsStr to logical.c from pg_subscription which
is common to both pub and sub.

> Apart from above, a few cosmetic changes are attached.

Merged them.

The attached v47 version patch has the changes for the same.

Regards,
Vignesh