Re: Support EXCEPT for TABLES IN SCHEMA publications

On Wed, Jul 8, 2026 at 2:58 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, Jul 8, 2026 at 2:22 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > Nisha, the patch (testing wise) is okay, but some code-optimization
> > might help. I am stuck on reveiw of GetTopMostAncestorInPublication()
> > and all its callers.
> >

Thanks, Shveta.

> > 1)
> > caller 1: pub_rf_contains_invalid_column():
> >
> > It is not understood why pub_rf_contains_invalid_column() does not
> > pass 'exceptPubids' to GetTopMostAncestorInPublication(). Perhaps a
> > comment will help. IIUC, a parittion (for which we are trying to
> > verify row-filter expression) can not co-exist in a subscription where
> > its ROOT is already excluded. This assumption will be true once you
> > address the issue of CREATE SUB (in [1]) sent in my previous email.
> > After that, this will still need a comment.
> >

You're right. Neither pub_rf_contains_invalid_column() nor
pub_contains_invalid_column() depends on
GetTopMostAncestorInPublication(), since
RelationBuildPublicationDesc() already prepares puboids after
excluding GetRelationExcludedPublications(). This follows the same
approach used for the FOR ALL TABLES case.

With your suggestion below, the except_pubids argument is no longer
needed, so I don't think any additional comments are necessary.

> > 2)
> > Both caller 2 (get_rel_sync_entry) and caller 3
> > (is_table_publishable_in_publication) are trying to see if partition
> > should be considered as included in publication and thus are trying to
> > see if ROOT is excluded. But the implementation is very different.
> >
> > The get_rel_sync_entry() does that by passing except_pubids to
> > GetTopMostAncestorInPublication(). This except_pubids was computed for
> > topmost ROOT of the parition in the caller and then it skips checking
> > pg_pub_namepspace if given pubid is part of except_pubids arguement.
> >
> > While is_table_publishable_in_publication() does that by selecting
> > topmost ancestor incuded in a given pubid using
> > GetTopMostAncestorInPublication(), without consulting/computing
> > except_pubids. And then later checks if the ancestor returned by
> > GetTopMostAncestorInPublication() is in pg_publicaiton_rel with
> > 'prexcept' true.
> >
> > IMO, GetTopMostAncestorInPublication() should itself be inclusive of
> > logic where it filters out the table (does not return it as result) if
> > ROOT is excluded. And even we should not be passing an argument for
> > that (this is my initial thought). By making such a logic, we need not
> > to bother about all
> > the callers to see if caller has correct logic to deal with output of
> > GetTopMostAncestorInPublication(). Can you think on this line and
> > check the feasibility.
> >
>
> I thought more on this. Do you think we can do this?
>
> GetTopMostAncestorInPublication() is already accepting a list of
> ancestors in a ordered fashion, root at the end. We get root from this
> ancestor list, check if root is in pg_publication_rel with
> except=true. If so, we skip the rest of the logic and return
> InvalidOid from GetTopMostAncestorInPublication().
>
> The 'except_pubids' argument is not required. Callers need not to have
> special logic to send this arguement or to have extra-processing on
> output as done by is_table_publishable_in_publication() currently.
> Let me know if I have missed anything.

The suggested optimization looks reasonable to me. I verified the test
cases for both callers (2 and 3) and finalized the changes.

During verification, I found it easy to miss cross-schema partition
cases i.e., when the partition root is excluded from one schema while
a child partition belongs to another included schema. To cover this, I
added tests in patch 0001 that tests both get_rel_sync_entry() and
is_table_publishable_in_publication().

Attached is the v20 patch set.

--
Thanks,
Nisha