On Wed, Feb 18, 2026 at 11:41 AM Amit Kapila <[email protected]> wrote:
>
> On Tue, Feb 17, 2026 at 5:08 PM shveta malik <[email protected]> wrote:
> >
> > On Tue, Feb 17, 2026 at 12:01 PM shveta malik <[email protected]>
> > wrote:
> > >
> > > On Tue, Feb 17, 2026 at 11:13 AM vignesh C <[email protected]> wrote:
> > > >
> > > > Thanks for the comments. The attached v45 version patch has the
> > > > changes for the same.
> > > >
> > >
> > > Thanks Vignesh, please find a few comments from v44 itself. Please
> > > ignore if already addressed. Will switch to v45 now.
> > >
> > >
> > > 1)
> > > publication_has_any_exception:
> > > + ScanKeyInit(&skey,
> > > + Anum_pg_publication_rel_prpubid,
> > > + BTEqualStrategyNumber, F_OIDEQ,
> > > + ObjectIdGetDatum(pubid));
> > > +
> > > + scan = systable_beginscan(pubRel,
> > > + PublicationRelPrrelidPrpubidIndexId,
> > > + true, NULL, 1, &skey);
> > >
> > > PublicationRelPrrelidPrpubidIndexId is index on relid and pubid, I
> > > don't think it will be used in this case where we have only pubid key.
> > > Shall we use PublicationRelPrpubidIndexId instead?
> > >
> > > 2)
> > > +/*
> > > + * is_relid_published_explicitly
> > > + *
> > > + * Checks if the given relation OID is explicitly part of the
> > > publication.
> > > + * This corresponds to the 'FOR TABLE' syntax.
> > > + */
> > > +static bool
> > > +is_relid_published_explicitly(Oid relid, Oid pubid)
> > > +{
> > > + /*
> > > + * Search the syscache for pg_publication_rel using the (relid, pubid)
> > > + * index.
> > > + */
> > > + return SearchSysCacheExists2(PUBLICATIONRELMAP,
> > > + ObjectIdGetDatum(relid),
> > > + ObjectIdGetDatum(pubid));
> > > +}
> > >
> > > How are we ensuring that it is not fetching the one with except-flag
> > > as true? Shall we assert when pub is all-tables to rule out that
> > > case/mistake? Or if we code-flow is expected to come to this function
> > > even for 'all-tables' pub (it appears to me t by looking at the
> > > caller), we shall return in such a case instead of Assert.
> > >
> > >
> > > 3)
> > > Shall we rename these:
> > > 'is_relid_published_as_except' as 'is_relid_excepted'.
> > > 'is_relid_published_as_except_with_ancestors' as
> > > 'is_relid_or_ancestor_excepted'
> > >
> > > These will be to match tones of:
> > > is_schema_published
> > > is_relid_published_explicitly
> > >
> > > I feel even for 'is_relid_published_explicitly', we can simply say
> > > 'is_relid_published'. Comments (and assert/checks) can explain that it
> > > is for FOR-TABLE pub.
> > >
> > > 4)
> > > + List *except_leaves = NIL;
> > > + List *allowed_leaves = NIL;
> > >
> > > Similar to allowed_leaves, shall we have excepted_leaves instead of
> > > except_leaves?
> > >
> > > 5)
> > > pg_get_publication_effective_tables():
> > >
> > > +
> > > + /*
> > > + * Check whether the table itself or its schema is
> > > + * included in this publication.
> > > + */
> > > + if (is_relid_published_explicitly(curr_relid, pubid) ||
> > > + is_schema_published(get_rel_namespace(curr_relid), pubid))
> > > + {
> > > + allowed_leaves = lappend_oid(allowed_leaves, curr_relid);
> > > + }
> > > + else
> > > + {
> > > + List *ancestors = get_partition_ancestors(curr_relid);
> > > +
> > > + /*
> > > + * Check whether any ancestor, or its schema, is
> > > + * included in this publication.
> > > + */
> > > + foreach_oid(anc_oid, ancestors)
> > > + {
> > > + if (is_relid_published_explicitly(anc_oid, pubid) ||
> > > + is_schema_published(get_rel_namespace(anc_oid), pubid))
> > > + allowed_leaves = lappend_oid(allowed_leaves, curr_relid);
> > > + }
> > > + }
> > >
> > > Do you think we can convert this code part to
> > > is_relid_or_ancestor_published(), similar to
> > > is_relid_published_as_except_with_ancestors()?
> > >
> > > is_relid_or_ancestor_published() can check both pg_publication_rel and
> > > pg_publication_namespace. I do not see individual use-case scenarios
> > > for is_relid_published_explicitly() and is_schema_published() and thus
> > > it is better to club these in one function. So at the end we will be
> > > left with 2 such functions:
> > >
> > > is_relid_or_ancestor_published
> > > is_relid_or_ancestor_excepted/excluded (choose the name based on
> > > others comments too)
> > >
> >
> > A few more:
> >
> > 6)
> > postgres=# CREATE PUBLICATION pub4 for ALL TABLES EXCEPT TABLE (tab1);
> > ERROR: cannot add relation "tab1" to publication
> > DETAIL: This operation is not supported for temporary tables.
> >
> > postgres=# CREATE PUBLICATION pub4 for ALL TABLES EXCEPT TABLE (tab2);
> > ERROR: cannot add relation "tab2" to publication
> > DETAIL: This operation is not supported for unlogged tables.
> >
> > Shall we change the error message here as we are not trying to add
> > relation here.
> >
>
> But aren't these existing messages? As these are not added by this
> patch and equally apply to existing code, so, isn't it better to
> discuss these separately if you think these are not suitable?
>
I do not have strong opinion here but this is what I originally had in mind:
1) 'cannot add relation .. to publication' is confusing when the user
is actually trying to exclude them.
2) Since these tables are already excluded because of their unlogged
and temporary nature, the error is further misleading.
Perhaps we shall have:
ERROR: cannot specify relation "tab1" in publication
DETAIL: Unlogged tables are automatically excluded from publication.
Or something better? Or do you think we are okay as is?
> > 7)
> > Currently the error i s:
> >
> > postgres=# create subscription sub1 connection '...' publication
> > pub1,pub2,pub3;
> > ERROR: publications "pub1", "pub2", "pub3" are defined with EXCEPT TABLE
> > HINT: Subscription cannot be created using multiple publications that
> > specify EXCEPT TABLE.
> >
> > Hint looks more like DETAIL. Shall we have this:
> >
> > ERROR: cannot create subscription with multiple publications that
> > specify EXCEPT TABLE
> > DETAIL: The publications "pub1", "pub2", and "pub3" define EXCEPT
> > TABLE clauses.
> >
>
> So, you are talking about below error:
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publications %s are defined with EXCEPT TABLE", pubnames.data),
> + errhint("Subscription cannot be created using multiple publications
> that specify EXCEPT TABLE."));
>
> Apart from the message, the error code here should be
> ERRCODE_FEATURE_NOT_SUPPORTED. How about an errmsg like: "cannot
> combine publications %s with EXCEPT TABLE clauses",
> except_publications? If we choose a message like that then we don't
> need a hint. I suggest you can refer to the following column list
> message to form a message for the except clause.
The proposed message looks good.
thanks
Shveta
