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
