On Wed, Jul 8, 2026 at 2:22 PM shveta malik <[email protected]> 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. > > 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. > > 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. thanks Shveta
