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?
> 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.
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use different column lists for table \"%s.%s\" in
different publications",
nspname, relname));
Few other minor comments:
1.
+ * Combining different exclusion lists with varying settings for
+ * 'publish_via_partition_root' can result in ambiguous publication
+ * identities.
It would be better if you give one such example case so that it is
easy for reader to understand why we don't support this combination.
2.
+ *
+ * Publications declared with FOR ALL TABLES should use
+ * GetAllPublicationRelations() to obtain the complete set of tables covered by
+ * the publication.
+ */
+List *
+GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partopt)
Shouldn't this mention the ALL SEQUENCES case as well in the comments
as we were previously doing for GetPublicationRelations.
--
With Regards,
Amit Kapila.
