MDEV-40486 [fixup] Clamp max_length at MAX_FIELD_VARCHARLENGTH in Ite…

[mariadb-YuchenPei]

…m_func_vec_fromtext::fix_length_and_dec

This allows

create table t1 (v vector(64) not null);
insert into t1 select vec_fromtext(concat('[',group_concat(1),']')) from seq_1_to_64;

which was banned in the previous fix
bb0ac43, though this also introduces the inconsistency(?) where

create table t1 as select vec_fromtext(concat('[',group_concat(1),']')) from seq_1_to_64;

still fails ER_TRUNCATED_WRONG_VALUE

see updated tests

Pre-approved by: Oleksandr Byelkin sanja@mariadb.com

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mariadb-YuchenPei]

claude:

---

Reviewed f6433caf (fetched via origin/bb-11.8-mdev-40486) — "MDEV-40486 [fixup] Clamp max_length at MAX_FIELD_VARCHARLENGTH in Item_func_vec_fromtext::fix_length_and_dec". The fix_length_and_dec clamp itself is fine (MAX_FIELD_VARCHARLENGTH = 65532 is a multiple of 4, so it maps cleanly to VECTOR(16383)); all the problems are in the new two-line guard added to val_str.

Findings:

• /home/user/server/sql/item_vectorfunc.cc:213 — return nullptr without setting null_value. Line 210 has just assigned null_value= false, so the new early-out breaks the val_str contract. Item::save_str_in_field (/home/user/server/sql/item.cc:7278) documents "If null_value == FALSE, result must be not NULL" and then dereferences result->ptr() → segfault on e.g. CREATE TABLE t1 (v VECTOR(2)); INSERT INTO t1 VALUES (VEC_FROMTEXT(CONCAT('[1.', REPEAT('0',70000), ',2]')));. Debug builds hit DBUG_ASSERT(item->null_value) in Type_handler_string_result::Item_send_str (/home/user/server/sql/sql_type.cc:7720) on a plain SELECT. Also reachable with a short multibyte literal such as SELECT VEC_FROMTEXT('😀😀😀') (3 chars → max_length 4, but 12 bytes).

• /home/user/server/sql/item_vectorfunc.cc:213 — the guard compares the input text byte length against max_length, which bounds the output binary vector. Wrong direction both ways: valid long-text/small-vector inputs are rejected (VEC_FROMTEXT(CONCAT('[1.', REPEAT('0',70000), ',2]')) is a valid 2-dim vector), and outputs still exceed max_length (the 16383-repeat case in the updated .result produces 65536 bytes vs max_length 65532). Checking buf->length() after the parse loop would fix both. There is also a char-vs-byte unit mismatch, since max_length derives from max_char_length() while value->length() is bytes.

• /home/user/server/sql/item_vectorfunc.cc:213 — silent NULL with no warning, unlike every other failure path in the function (ER_VECTOR_FORMAT_INVALID / ER_TRUNCATED_WRONG_VALUE / JSON errors). Users get an unexplained NULL and an empty SHOW WARNINGS.

The test/result changes themselves are consistent with the code change; the acknowledged CREATE TABLE ... AS SELECT vs INSERT ... SELECT asymmetry is called out in the commit message and pre-approved, so I did not report it.

[mariadb-YuchenPei]

claude:

---

Both commits found. Let me review them.

Found both (the branch was force-updated: f6433caf → 999b669b, which differs from the old one only in vector_utf16.result — the code is unchanged, so my earlier review of it still applies). Reviewed 999b669b..b33e137d statically; there's no build in this container, so I did not run main.vector2.

Verdict

Both changes are correct, and all three findings from the previous review are resolved. No correctness issues found in the new commits. Details of what I verified, then some nits.

a046922 — the check is now on the produced vector and folded into the existing failure branch, so null_value and the warning are set on every return nullptr path (item_vectorfunc.cc:261). Two things confirm the false-NULL bug is really gone rather than just moved: the VECTOR(2) / 70000-digit and emoji cases you added, and the fact that the net vector_utf16.result diff across the whole series is empty — 999b669b had recorded hex(vec_fromtext('[1,2,3]')) as NULL under utf16 (7 chars → max_length 12 vs 14 bytes, exactly the byte-vs-char confusion), and this commit reverts it. Worth keeping in mind that 999b669b on its own is a knowingly-broken bisect point, so it needs the squash before it goes anywhere permanent.

b33e137 — 4*floor((chars-1)/2) via & ~3ULL is the tight bound, so a CTAS column can always accept the widest vector its argument can produce (CHAR(4) → vector(1), and '[1]' inserts). I also checked that keeping max_char_length() rather than the octet-based variant I floated was the better call: with octets, CHAR(4) in utf8mb4 would infer vector(7) and reject the only value it can produce — reintroducing the very MDEV-40751 symptom. And the concern I had about octets being needed for correctness doesn't apply, because Field_blob::max_display_length() multiplies by mbmaxlen (sql/field.cc:11431), so max_char_length() is a valid character bound for TEXT/BLOB too. Combined with the clamp — any argument whose char bound reaches 32767 gets max_length exactly 65532, which equals the val_str cap — max_length remains a true upper bound on the returned length. That matters because it's what rules out silent truncation when the item is materialized into a tmp-table Field_vector or a sort key.

Also checked: if (maxlen > 0) is load-bearing (without it (0-1)*2 wraps and the clamp would yield 65532); MAX_FIELD_VARCHARLENGTH is 65532, itself a multiple of 4, so the clamp and the rounding compose; and no existing vector2.result expectation shifts (CHAR(1), CHAR(2), NULL, NULL+NULL, varchar(5), the 16383/16384-dim boundary and the ? placeholder cases were all already multiples of 4), which matches the append-only test diff.

Nits

• sql/item_vectorfunc.cc:189 — stale comment. "Clamp at 4 … and UINT_MAX32 - 1 (to avoid overflow)" no longer describes the code: the clamp is MAX_FIELD_VARCHARLENGTH and overflow is now handled by the ulonglong type. Worth rewriting while you're in there, since the next reader will look for the UINT_MAX32 branch.

• sql/item_vectorfunc.cc:197 — fix_length_and_charset(maxlen & ~3ULL, …) narrows ulonglong to the uint32 parameter implicitly. Value-safe after the clamp, but MSVC's C4244 fires on this at the default warning level; an explicit (uint32) keeps Windows builds quiet.

• sql/item_vectorfunc.cc:261 — the cap now only fires after the entire array has been parsed and buffered, so a LONGTEXT argument of '[1,1,…]' allocates roughly twice the input before being rejected (the removed input-side check used to bound that, accidentally). Moving it into the loop right after the append at line 250 and jumping to the shared warning block bounds the buffer at 64K with identical behavior — and then the post-loop condition goes back to being just is_valid.

• sql/sql_type_vector.cc:122 — optional hardening for MDEV-40751: DBUG_ASSERT(attr.max_length % sizeof(float) == 0) in make_table_field would catch any future vector-producing item that reintroduces the bug. Safe to assert there specifically, since the frm path goes through make_table_field_from_def and won't crash debug builds on a corrupt frm.

• Two design/hygiene points, take or leave: the 16384-dim case now reports Truncated incorrect vector value: '[1,1,…' where bb0ac437 said Column length too big … (max = 16383) — the new message no longer tells the user what the dimension limit is; and the two new cases in a046922c land inside the "Conversion to max vector dimension" block with no --echo saying what they guard, so a future bisect won't know they're regression tests for the char-vs-byte confusion.