On Wed, Nov 4, 2026 at 2:30 AM Lucas Jeffrey <[email protected]> wrote: > El mar, 3 nov 2026 a las 10:41, Amit Langote > (<[email protected]>) escribió: > > > > On Tue, Nov 3, 2026 at 2:39 AM Tomas Vondra <[email protected]> wrote: > > > On 10/26/26 10:23, Amit Langote wrote: > > > > On Sat, Oct 24, 2026 at 4:01 PM Amit Langote <[email protected]> > > > > wrote: > > > >> > > > >> On Fri, Oct 23, 2026 at 10:26 PM Lucas Jeffrey > > > >> <[email protected]> wrote: > > > >>> 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). > > > >> > > > >> Thanks for reporting that. Will fix. > > > > > > > > Tomas, given that you have looked at Lucas's patch and said it's on > > > > the right track, I thought to check whether you like the patch after I > > > > changed it in the following ways: > > > > > > > > * Instead of a hash table to track running SPI plans, it uses an array > > > > to represent a stack of running plans. ri_PerformCheck() pushes the > > > > plan onto the stack before calling SPI_execute_snapshot() and pops it > > > > once it returns normally or if an error occurs as described below > > > > > > > > * Instead of using ResourceOwner to handle the pop during transaction > > > > or subtransaction unwinding, which is only available in 17 and later, > > > > so Lucas's patch already used PG_TRY in the older branches, the > > > > SPI_execute_snapshot() call is wrapped in PG_TRY/PG_FINALLY, so any > > > > error during that call pops the plan from the stack as well. That > > > > makes the fix the same in all branches. > > > > > > > > I chose this approach because the hash lookup and the resource owner > > > > entry add a fixed cost to every check, on the normal path where no > > > > tracking is needed, and I'd rather not add that cost to older > > > > branches, even if it's small. The stack array approach adds depth (or > > > > array size) check, a stack index increment, an array store and a > > > > decrement, plus the PG_TRY, which Lucas's patch needed in the older > > > > branches anyway. It also ends up being less code overall. > > > > > > > > If you think there are any reasons to stick to the original approach, > > > > please let me know. If not, I'd like to commit v10 in the next couple > > > > days after doing the fixes mentioned by Lucas. > > > > > > I like this approach. It seems to be bit simpler than the reference > > > counting approach, and it has the advantages that you explained. Plus it > > > has the advantage of working on all branches. I can't think of a clear > > > reason to stick to the reference counting. > > > > Thanks for chiming in and for the review. > > > > > A couple comments from a quick review: > > > > > > 1) Isn't it a bit weird all regression tests pass even if I comment out > > > the "ri_executing_depth = exec_depth;" line in ri_PerformCheck? There's > > > a comment in the foreign_key test that says: > > > > > > ... must leave the stack of executing RI plans exact (an assertion > > > at end of transaction checks that) ... > > > > > > So how come it didn't catch this? Where's the assertion that comment > > > references, actually? I don't see any assertions added to AtEOXact_RI. > > > > Actually, there used to be an Assert there that I removed, because I > > believed that PG_FINALLY() would not get a chance in some cases (FATAL > > errors) to reset the stack and the Assert would fail. AtEOXact_RI() > > itself already sets it to 0 so ri_FreeDeadPlans() can free all plans > > in ri_plan_dead_list. > > > > I've now revised the Assert to this: > > > > /* > > * No RI check can be in progress here, so clear the stack. On commit > > it > > * must be empty already, since ri_PerformCheck() pops its plan whether > > * the query returns or throws. On abort it may not be, because a FATAL > > * error skips those pops. > > */ > > Assert(ri_executing_depth == 0 || !isCommit); > > > > As for why nothing failed, commenting out "ri_executing_depth = > > exec_depth;" means nothing is ever popped, so every plan invalidated > > during the transaction is held in ri_plan_dead_list until > > AtEOXact_RI() frees it at the end, which is fine but it means that for > > a long statement where every row uses a new plan after the old one is > > invalidated, many such plans will accumulate. The stack approach > > avoids that accumulation, but none of that is observable with a > > regression test. > > > > > 2) On 14, the first test says "Unlike the case above", but that test is > > > missing on 14. > > > > You're right. The test case it's talking about is only present in 19 > > and master because it got added with fast-path work. > > > > Fixed the comment in the patches for 14-18 like this: > > > > --- Unlike the case above, this reproduces the crash without cache > > clobbering. > > +-- This reproduces the crash without cache clobbering. > > > > > 3) The comment in ri_FetchPreparedPlan says: > > > > > > ... to be freed once no check is in progress. > > > > > > Maybe it should instead say "once no check is executing the plan"? > > > > Ok, I chose wording similar in meaning but matching nearby text: > > > > * returns. If the plan has been invalidated in between, freeing it > > here > > * would pull the SPIPlan out from under the outer > > _SPI_execute_plan(). So > > * in that case queue it on ri_plan_dead_list, to be freed once no > > check > > - * is in progress. > > + * is executing it any more. > > > > > 4) The commit message seems to have a lot of details related to > > > buildfarm animals, fast-path checks (which only affects 19/master), etc. > > > Maybe that's too much detail, esp. for backbranches. Not sure. > > > > I think it's better to have the details about both Lucas's report and > > Tom's buildfarm report, but I agree it's not great to have the same > > message in all the branches. The message for 19 and master keeps the > > buildfarm details, because the history matters there. Checks needing > > a cast function went through the fast path, which didn’t have this > > bug, until 6aecabcd5ed moved them to SPI, and that is what made > > trilobite fail. Older branches never had that history but they still > > would have crashes under CLOBBER_CACHE_ALWAYS if the test case had > > existed, so their message only says how the bug was found. > > > > Hi Amit, Tomas > > I tested and reviewed v11, I think comments are fine, and all the issues > Tomas raised were addressed. > > +1 to bringing the Assert back. > > I tested v11 on master and REL_14 through REL_19, built with cassert, > and make check passes on all of them. LGTM, I think it's ready to > commit.
Pushed after trimming the commit messages a bit to reduce duplication of the bug description text. Thank you all for sharing your thoughts and for reviews. -- Thanks, Amit Langote
