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. Thanks! Regards, Lucas Jeffrey 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. > > -- > Thanks, Amit Langote
