MDEV-26940 Item_cond::remove_eq_conds leaves corrupt Item_equal

[mariadb-RexJohnston]

pushdown_cond_for_derived/merge_into_list/remove_eq_conds leaves an Item_equal in an invalid state. This Item_equal is later used by find_producing_item causing an assert in debug builds and perhaps incorrect results in a release build.

Affected queries will likely have an outer condition pushed down into 2 different derived tables based on the same base table.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[DaveGosselin-MariaDB]

Inline are my review comments. Here are some comments from Claude as well:

Orphan left corrupt after merge — sql/item_cmpfunc.cc:7046 · correctness · PLAUSIBLE
merge() repoints only members still in item->equal_items, leaving item itself with with_const==TRUE over a non-const head. It misses (a) fields fi.remove() stripped from the merged-in equality before merge() ran (item_cmpfunc.cc:7109, which ran once in this test), and (b) a popped const that is a const-table field. Either can trip the new assert (debug) or mis-substitute (release) on other queries. Not reproduced by the committed test.

Unconditional set_item_equal may steal upper-level back-pointers — sql/item_cmpfunc.cc:7056 · correctness · PLAUSIBLE (low confidence)
On the direct-merge path (check_simple_equality, sql_select.cc:16943 — untested here), copied equalities share member Item objects; repointing them unconditionally can redirect a field's item_equal away from the upper-level original, risking wrong results. No concrete repro.

[DaveGosselin-MariaDB]

The original implementation implied that equal_items.head() could return NULL. Is it safe to assume, in the assertion, that equal_items.head() is always non-NULL when with_const is true? I suppose that if the equal_items.head() returns NULL, then we will have an assertion-like behavior in that the null pointer dereference will crash, but that's probably not intended.

Should the assertion instead be something like:

DBUG_ASSERT(!with_const || (equal_items.head() && equal_items.head()->const_item()));

[mariadb-RexJohnston]

I don't think we could ever get an empty equal_items List in our Item_equal object. Even one one item doesn't make any sense.

[DaveGosselin-MariaDB]

You should be able to use List_iterator_fast here instead because you're walking the list and updating elements in the list but not mutating the list itself. List_iterator_fast avoids some extra bookkeeping on the presumption that you're not altering the list itself.

[mariadb-RexJohnston]

nice catch, thanks.

[mariadb-RexJohnston]

Unconditional set_item_equal may steal upper-level back-pointers — sql/item_cmpfunc.cc:7056 · correctness · PLAUSIBLE (low confidence) On the direct-merge path (check_simple_equality, sql_select.cc:16943 — untested here), copied equalities share member Item objects; repointing them unconditionally can redirect a field's item_equal away from the upper-level original, risking wrong results. No concrete repro.

As there is only one item_equal pointer, we need to look at what is used for and whether it matters which Item_equal object we are pointing at. It's main use is obtaining the contents of containing Item_equal, not the object itself (like find_producing_item()).

Aside: multiple Item_equal's containing the same item is exactly what our test case contains, and it's the merge process itself that should leave only one (or zero) instances of Item_equal behind.