Hi Amit, Thanks for v10, and for the credit in the commit message.
I tested v10 on master and on REL_14 through REL_19, built with cassert, and make check passes on all of them, including the new tests in foreign_key. It addresses everything we found, so it LGTM. I found just one small thing, non-locking: the new test comment in foreign_key.sql (and in foreign_key.out) says "(an assertion at end of transaction checks that)", but that assertion was removed in v10, so I would remove that part of the comment before commiting (doesn't needs a v11). Regards, Lucas Jeffrey El vie, 23 oct 2026 a las 6:58, Amit Langote (<[email protected]>) escribió: > > On Fri, Oct 23, 2026 at 6:01 AM Lucas Jeffrey > <[email protected]> wrote: > > El jue, 22 oct 2026 a las 8:18, Amit Langote (<[email protected]>) > > escribió: > >> On Wed, Oct 21, 2026 at 9:02 PM Amit Langote <[email protected]> > >> wrote: > >> > Thanks for the patch. I'm going to study it and post an updated > >> > version tomorrow. I'd like to handle errors as well as Lucas's patch > >> > does but I'd stick to using PG_TRY in all the branches instead of > >> > resowner in some and PG_TRY in others. I believe subtransaction > >> > aborts are handled correctly by PG_TRY-based solution but I'd need to > >> > test that. In any case, I want to avoid hash table and resowner calls > >> > in the per-row path and keep the tracking code as minimal as possible. > >> > >> Sorry, ran out of time today, so will post the patch tomorrow. > > > > Thanks both for the work on this. I've been iterating on Amit's v1 plus > > Trakshan's stack, and added error handling on top. Patch attached (v9). > > > > Amit, does the approach used to defer freeing the plans look OK to you? > > To keep it conservative I don't free anything while the error is being > > handled; maybe that's not needed and we could free them in the > > PG_FINALLY instead. > > Thanks for v9. I have more or less the same patch in my local tree > that I was about to post. Yeah, it makes sense to keep plan freeing > out of the error handling path. > > So, here's v10, which streamlines the stack code by putting it in > functions instead of directly in ri_PerformCheck as in your v9. It > also keeps the tests from your v8-0001 and adds tests for behavior on > subxact rollbacks. You'll notice that the commit message also details > the history of what you found, what the buildfarm reported, etc. > > I've also attached patch files for the back-branches, which I've > tested down to v14. v14 has a test file conflict so it needs a > different patch file. v15 through v18 share the same patch file. > > As mentioned earlier, the commit message lists you as the author, with > Trakshan and me as co-authors, which I think reflects how this came > together. > > I’d like to commit this early next week unless there are objections. > > -- > Thanks, Amit Langote
