Re: Skipping schema changes in publication

On Tue, 17 Feb 2026 at 17:08, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Tue, Feb 17, 2026 at 12:01 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Tue, Feb 17, 2026 at 11:13 AM vignesh C <vignesh21(at)gmail(dot)com> 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.
>
Based on the discussion in [1]. We are still analysing this change.
Will address it in the next version of patch.

> 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.
>
Modified it as per Amit's suggestion in [2].

> 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 for reviewing the patch. I have addressed the remaining
comments in the v46 patch..

[1]: https://www.postgresql.org/message-id/CAA4eK1J%3D41eienWGzgAnB_-fAGjZdB2daP_N%3D84NSm1QT56YVQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1KWqttt3UMdR8P0wYyqDO6cuLhuvGb5cDpuctG8F10EFA%40mail.gmail.com

Thanks,
Shlok Kyal