Skip to content

MDEV-41026 ER_DUP_KEY on '(temporary)' converting a subquery cache - #5637

Open
arcivanov wants to merge 1 commit into
MariaDB:11.8from
arcivanov:MDEV-41026-11.8
Open

MDEV-41026 ER_DUP_KEY on '(temporary)' converting a subquery cache#5637
arcivanov wants to merge 1 commit into
MariaDB:11.8from
arcivanov:MDEV-41026-11.8

Conversation

@arcivanov

Copy link
Copy Markdown
Contributor

https://jira.mariadb.org/browse/MDEV-41026

This is the same change as #5636, targeted at 11.8 instead of main. Fix Version/s on the ticket starts at 11.8, and a fix committed to main never reaches 11.8 or 12.3, so this is the one that should land; #5636 becomes redundant once this merges up.

The defect

A subquery expression cache whose parameter is NULL for many rows fills its in-memory table with rows that all carry a NULL key. TABLE::add_tmp_key() builds that key without HA_NULL_ARE_EQUAL, ha_heap::create() propagates the absence, and hp_write_key() then skips the duplicate check for any key value containing NULL, so such rows are legal there. They accumulate rather than being exceptional: ref.null_rejecting is set, so every NULL parameter is a cache miss and adds one more NULL-keyed row.

The on-disk creators disagreed. Both the key and the unique constraint forced HA_NULL_ARE_EQUAL on, making the destination of a conversion stricter about NULLs than the table being converted from. create_internal_tmp_table_from_heap() re-inserts every stored row and treats a duplicate among them as fatal, so the second NULL-keyed row was rejected.

The reported symptom is ER_DUP_KEY. The same defect surfaces as ER_DUP_ENTRY once the cache table is already on disk, and as ER_DUP_UNIQUE when the key is too wide for the on-disk engine to index and is built as a unique constraint over a row hash instead.

The fix

Make KEY::flags & HA_NULL_ARE_EQUAL the single source of truth and read it at all four creator sites - keydef and uniquedef, in each of the Aria and the MyISAM creator - rather than deciding it there.

For that flag to be truthful, the GROUP BY key has to carry it when it is built as a unique constraint as well. The assignment sat inside the branch that only the real-key case takes, which also lays the group buffer out around the NULL flag, so it is now set in the unique-constraint case too. This is behaviour-neutral on Aria, whose creator tests HA_UNIQUE_HASH explicitly and therefore always took the uniquedef branch for such a key, where null_are_equal was hardcoded to 1.

choose_engine() routes every m_using_unique_constraint table to the on-disk engine, so that change cannot reach ha_heap::create(), the only other place a KEY's HA_NULL_ARE_EQUAL is read.

Testing

mysql-test/main/subquery_cache_null_convert covers both on-disk shapes. Each block prints Created_tmp_disk_tables, so the test cannot pass by quietly ceasing to overflow, and neither cached query aborts on error, so a regression in either shape is reported rather than hidden behind the other one. Reverting the fix and running the test unchanged gives, in one run on this branch:

shape error
narrow key, stays a key on disk ERROR 23000: Can't write; duplicate key in table '(temporary)' (1022)
key too wide for Aria, becomes a unique constraint ERROR 23000: Can't write, because of unique constraint, to table '(temporary)' (1169)

main, heap and maria suites on 11.8: 1393 of 1393 pass.

Not covered

The MyISAM halves of both creators compile only with -DUSE_ARIA_FOR_TMP_TABLES=OFF, which is not the default. They are changed to match and verified to compile warning-free in that configuration, but no test run has executed them.

A subquery expression cache whose parameter is NULL for many rows
fills its in-memory table with rows that all carry a NULL key.
`TABLE::add_tmp_key()` builds that key without `HA_NULL_ARE_EQUAL`,
`ha_heap::create()` propagates the absence, and `hp_write_key()` then
skips the duplicate check for any key value containing NULL, so such
rows are legal there.  They accumulate rather than being exceptional:
`ref.null_rejecting` is set, so every NULL parameter is a miss and
adds one more NULL-keyed row.

The on-disk creators disagreed.  Both the key and the unique
constraint forced `HA_NULL_ARE_EQUAL` on, making the destination of a
conversion stricter about NULLs than the table being converted from.
`create_internal_tmp_table_from_heap()` re-inserts every stored row
and treats a duplicate among them as fatal, so the second NULL-keyed
row was rejected.  The user saw either

    Can't write; duplicate key in table '(temporary)'

or, when the key is too wide for the on-disk engine to index and is
built as a unique constraint over a row hash instead,

    Can't write, because of unique constraint, to table '(temporary)'

Make `KEY::flags & HA_NULL_ARE_EQUAL` the single source of truth and
read it at all four creator sites rather than deciding it there.  For
the flag to be truthful the GROUP BY key has to carry it when it is
built as a unique constraint as well; the assignment sat inside the
branch that only the real-key case takes, which also lays the group
buffer out around the NULL flag.

The MyISAM halves of both creators compile only with
`-DUSE_ARIA_FOR_TMP_TABLES=OFF`, which is not the default.  They are
changed to match and verified to compile warning-free in that
configuration, but **no test run has executed them**.

The test covers both on-disk shapes: a narrow key that stays a key,
and a key too wide for Aria that becomes a unique constraint.  Each
block reports whether the conversion happened at all, so the test
cannot pass by quietly ceasing to overflow.  The count itself is not
asserted: `--ps-protocol` executes the statement twice and so
converts twice.  Neither cached query aborts on error, so a
regression in either shape is reported rather than hidden behind the
other one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant