Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications

From: Nisha Moond <[email-masked]>
Date: Fri, 10 Jul 2026 17:03:43 +0530

On Wed, Jul 8, 2026 at 2:58 PM shveta malik <[email-masked]> wrote:
>
> On Wed, Jul 8, 2026 at 2:22 PM shveta malik <[email-masked]> 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

---

From: Nisha Moond <[email-masked]>
Date: Fri, 10 Jul 2026 17:07:40 +0530

On Thu, Jul 9, 2026 at 2:53 PM shveta malik <[email-masked]> wrote:
>
> >
> > To reduce duplicate code, I moved the schema
> > qualification and eligibility checks for both cases to execution time
>>  and removed the corresponding processing from gram.y.
> >
> > Since this changes a fair amount of code, I've kept it as a separate
> > patch (v19-0002) to make review easier. Depending on the feedback, we
> > can optimize the implementation or move parts of it back into gram.y
> > before merging it into patch 0001.
> > ~~~
> >
>
> Nisha,
>
> - * Also flattens except_tables from TABLES IN SCHEMA nodes into the list so
> - * that ObjectsInPublicationToOids() sees them as top-level
> EXCEPT_TABLE entries.
> + * The except_tables attached to TABLES IN SCHEMA nodes are left in place;
> + * ObjectsInPublicationToOids() qualifies names, validates schema membership,
> + * and flattens them once the schema OID is known.
>
> Can you please explain what does 'flatten' mean here.
>

Each TABLES IN SCHEMA ... EXCEPT (...) entry stores its own nested
list of EXCEPT tables. Flatten means collecting the PublicationTable
from each nested list into a single top-level except_pubtables list,
giving it the same structure as a FOR TABLE list that can be passed
directly to OpenTableList().

I've reworded the comment. We can also remove it if it seems unnecessary here.

> The flatten logic here is added by your patch001 only, it is not an
> existing logic. To me (in my initial round of review), it seems okay
> if we moved it to ObjectsInPublicationToOids(). The only thing is that
> the one error (table in EXCEPT clause does not belong to schema) which
> is possible to get at the parsing stage will now be emitted during
> pub-creation stage.

Yes, the logic was introduced in patch 0001 itself. I just highlighted
it for the case you mentioned, where the error is deferred instead of
being raised at parse time.

> But since it will keep the entire logic of
> except-list verification at one place and considering that
> creat/alter-pub is not so frequent operation, I am fine with it. But
> lets see if others have any comment here.
>

My thought was the same: to keep the logic in one place for now. Let's
see if others have any opinions on it as well.

> ~~
>
> I had a quick look at the changes:
>
> + * Include the ONLY-ness of this entry in its signature. EXCEPT
> + * (TABLE parent) excludes parent and its inheritance children,
> + * while EXCEPT (TABLE ONLY parent) excludes only parent itself.
>
> I think this will unconditionally apply to paritioned tables as well
> but for paritioned table, EXCEPT doc says that ONLY and * has no
> effect, but here it shows the effect:
>
> Thsi gives error:
> postgres=# CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT
> (TABLE ONLY tab_root), TABLES IN SCHEMA public EXCEPT (TABLE
> tab_root);
> ERROR:  42P17: schema "public" specified with conflicting EXCEPT lists
> LOCATION:  ProcessSchemaExceptTables, publicationcmds.c:299
>
> While this does not:
> postgres=#  CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT
> (TABLE tab_root), TABLES IN SCHEMA public EXCEPT (TABLE tab_root);
> CREATE PUBLICATION
>
> But I also checked column-list errors and found that even if columns
> are exact sames, we get error:
>
> postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1(i), T1(i);
> ERROR:  42710: conflicting or redundant column lists for table "t1"
> LOCATION:  OpenTableList, publicationcmds.c:2312
>
> While this works:
> postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1, T1;
> CREATE PUBLICATION
>
> This makes me think that even introducing the smartness of identifying
> ONLY in case of inherited tables and distinguishing it from
> parititoned tables is not required. We can just change your error
> message to 'conflicting or redundant EXCEPT lists for schmea "s1"' and
> throw error in all cases where schmea with except list repeats without
> actually comparing them. Does this sound reasonable?
>

Yes, that works for me as we have FOR TABLES example to follow. I've
made the changes in v20.

--
Thanks,
Nisha