On Wed, Jul 20, 2022 at 2:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jul 20, 2022 at 9:01 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Wed, Jul 20, 2022 at 12:11 PM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > > > > On Tue, Jul 19, 2022 at 7:28 PM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > Why do you think we can't remove > > > ReorderBufferInitialXactsSetCatalogChanges() from the back branch > > > patch? I think we don't need to change the existing function > > > ReorderBufferXidHasCatalogChanges() but instead can have a wrapper > > > like SnapBuildXidHasCatalogChanges() similar to master branch patch. > > > > IIUC we need to change SnapBuildCommitTxn() but it's exposed. > > > > Currently, we call like DecodeCommit() -> SnapBuildCommitTxn() -> > > ReorderBufferXidHasCatalogChanges(). If we have a wrapper function, we > > call like DecodeCommit() -> SnapBuildCommitTxn() -> > > SnapBuildXidHasCatalogChanges() -> > > ReorderBufferXidHasCatalogChanges(). In > > SnapBuildXidHasCatalogChanges(), we need to check if the transaction > > has XACT_XINFO_HAS_INVALS, which means DecodeCommit() needs to pass > > either parsed->xinfo or (parsed->xinfo & XACT_XINFO_HAS_INVALS != 0) > > down to SnapBuildXidHasCatalogChanges(). However, since > > SnapBuildCommitTxn(), between DecodeCommit() and > > SnapBuildXidHasCatalogChanges(), is exposed we cannot change it. > > > > Agreed. > > > Another idea would be to have functions, say > > SnapBuildCommitTxnWithXInfo() and SnapBuildCommitTxn_ext(). The latter > > does actual work of handling transaction commits and both > > SnapBuildCommitTxn() and SnapBuildCommit() call > > SnapBuildCommitTxnWithXInfo() with different arguments. > > > > Do you want to say DecodeCommit() instead of SnapBuildCommit() in > above para?
I meant that we will call like DecodeCommit() -> SnapBuildCommitTxnWithXInfo() -> SnapBuildCommitTxn_ext(has_invals = true) -> SnapBuildXidHasCatalogChanges(has_invals = true) -> ... If SnapBuildCommitTxn() gets called, it calls SnapBuildCommitTxn_ext() with has_invals = false and behaves the same as before. > Yet another idea could be to have another flag > RBTXN_HAS_INVALS which will be set by DecodeCommit for top-level TXN. > Then, we can retrieve it even for each of the subtxn's if and when > required. Do you mean that when checking if the subtransaction has catalog changes, we check if its top-level XID has this new flag? Why do we need the new flag? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/