Hi,
On Fri, Feb 20, 2026 at 2:08 PM vignesh C <[email protected]> wrote:
>
> On Thu, 19 Feb 2026 at 16:06, Amit Kapila <[email protected]> wrote:
> >
> > On Thu, Feb 19, 2026 at 10:13 AM Shlok Kyal <[email protected]>
> > 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.
>
SIGABRT with normal tables:
--------------------------------------
ashutosh@test=# select
pg_get_publication_effective_tables('t1'::regclass,
'{all_pub}'::text[]);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
Since the function is user-facing, I think we should have test coverage for it.
--
With Regards,
Ashutosh Sharma.
