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.
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.
8)
pg_get_publication_effective_tables():
+ /* Return root immediately if no filtering logic is needed */
+ if (has_clean_all_tables_pub || !has_any_except)
I think we do not need any additional boolean 'has_any_except' for
this purpose. We can simply rely on 'except_pub_names' being Nil.
9)
+ /* Return root immediately if no filtering logic is needed */
+ if (has_clean_all_tables_pub || !has_any_except)
+ {
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ final_output = list_make1_oid(root_relid);
+ MemoryContextSwitchTo(oldcontext);
+ }
+ else
Can we please write a comment atop 'else' block to say what it is
going to attempt? This is because there is a big chunk of code in
'else' block and it is difficult to construct our logic by reading
each part of it.
10)
CreatePublication() has changed the way we process publications.
Earlier, we had explicit checks for publication types such as
'for_all_tables' and 'for_all_sequences' etc, which made the code
easier to follow. That differentiation based on publication type is no
longer there. As an example,
we now invoke functions like TransformPubWhereClauses() and
CheckPubRelationColumnList() even for FOR ALL TABLES ... EXCEPT
publications, which are not needed. We could consider restoring the
previous structure, where logic was clearly separated based on
publication type.
thanks
Shveta
