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)
thanks
Shveta
