MDEV-31182 Check for blob too in create_key_parts_for_pseudo_indexes

[mariadb-YuchenPei]

BLOB and VARCHAR has keys prefixed with length of the payload. This length data itself has length HA_KEY_BLOB_LENGTH which is 2, as demonstrated for example in Field_blob::set_key_image and Field_varstring::set_key_image.

In this patch, when preparing key info for a range analysis, we account for this extra HA_KEY_BLOB_LENGTH in the total key length for BLOB. Previously it was only done for VARCHAR. This way, when doing the actual range analysis, LIKE (Item_func_like::get_mm_leaf) can correctly identify that it is dealing with a BLOB, thereby allowing the space of 2 for the length data. This avoids later Field_blob::set_key_image reading the payload as length data and causing reading the wrong memory segment for the payload.

[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.}

[gemini-code-assist]

Warning

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

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Warning

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

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182 by ensuring that MYSQL_TYPE_BLOB is treated similarly to MYSQL_TYPE_VARCHAR when calculating key part lengths for pseudo-indexes in sql/opt_range.cc. Specifically, it adds HA_KEY_BLOB_LENGTH to the store length when the field type is a BLOB. Additionally, a corresponding test case has been added to verify the fix and prevent ASAN errors. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182, fixing ASAN errors related to String_copier::well_formed_copy and calculate_cond_selectivity_for_table. The changes ensure that MYSQL_TYPE_BLOB is handled correctly when calculating key lengths for pseudo-indexes, and introduces an empty override for set_key_image in Field_geom to prevent incorrect key data parsing. Feedback suggests using the is_real_blob() abstraction in sql/opt_range.cc for better readability, and adding a Doxygen comment to explain the no-op override in Field_geom.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182 by adding test cases and fixing ASAN errors related to geometry and blob types. Key changes include making print_key_part_value virtual, handling MYSQL_TYPE_BLOB in pseudo-index key part creation, and implementing Field_geom::print_key_part_value while asserting on unsupported operations. The review feedback suggests simplifying the Field_geom::print_key_part_value implementation by removing an unnecessary temporary StringBuffer and charset check, as well as correcting minor typos in the newly added comments in sql/sql_type_geom.h.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182 by fixing ASAN errors in String_copier::well_formed_copy and calculate_cond_selectivity_for_table. It makes print_key_part_value virtual, implements it for Field_geom, and updates pseudo-index key part creation to support MYSQL_TYPE_BLOB. The review feedback correctly points out that replacing the fallback behavior in Field_geom::print_key_value with only DBUG_ASSERT(0) leaves the function empty in release builds, which could lead to issues if called; retaining the fallback string append after the assertion is recommended.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

claude review comment on the gis commit:

---

Code Review: MDEV-31182 — Field_geom::set_key_image / spatial-index key printing

Commit: 8f0aef93 — "Make Field_geom::set_key_image do nothing"
Files: sql/field.h, sql/sql_type_geom.{h,cc}, mysql-test/main/gis.{test,result} (+54 / −4)

Overview

The bug is an ASAN error (String_copier::well_formed_copy under calculate_cond_selectivity_for_table) triggered when the optimizer trace prints a range over a SPATIAL index. The trace-printing path (opt_range.cc:16922 → Field::print_key_part_value) called the inherited Field_blob::set_key_image, which does length = uint2korr(buff) on what is actually MBR bytes, then store()s that bogus length — reading past the key buffer.

The fix:

• Makes print_key_part_value virtual and overrides it in Field_geom to print unprintable_geometry_value (or NULL) without calling set_key_image/print_key_value.
• Turns Field_geom::set_key_image and print_key_value into DBUG_ASSERT(0) — asserting they must never be reached, no-ops in release.
• Adds a regression test in gis.test.

This is the right shape of fix: GIS indexes store only the bounding box, so restoring the "key" into the record is meaningless, and the previous behavior was outright unsafe.

Correctness

• ✅ The null-byte handling in the new override is equivalent to the base: real_maybe_null() && *key → print NULL; every other case → the placeholder string. Correct for both nullable and non-nullable geometry.

• ✅ The literal unprintable_geometry_value moved cleanly from print_key_value into print_key_part_value; no other caller of Field_geom::print_key_value exists (the only member call site, field.cc:11803, is inside the base print_key_part_value, which geom now overrides).

• ✅ In release builds, DBUG_ASSERT(0) compiles away, leaving set_key_image/print_key_value as empty no-ops — strictly safer than the previous buffer-overrunning behavior.

• ✅ I audited the other set_key_image call sites for reachability with a geom field, since the assert is a strong claim. All are gated:

  • EITS histogram/selectivity (opt_histogram_json.cc, sql_statistics.cc:4307/4316/...) and the pseudo-index selectivity path (opt_range.cc:3175) are guarded by is_eits_usable(), which explicitly excludes MYSQL_TYPE_GEOMETRY (sql_statistics.cc:4504).
  • Partition pruning (sql_partition.cc) — geometry cannot be a partitioning column.
  • key_restore (key.cc:253) is the HA_VAR_LENGTH_PART branch; blob/geom key parts take the HA_BLOB_PART branch (key.cc:241) instead, so it isn't hit.
  • print_sel_arg_key (opt_range.cc:16446) is a DBUG-only helper operating on pseudo-indexes, which now exclude geom.

  So the "should not be called at all" premise holds for the paths I could find.

Style / conventions

• Commit message vs. behavior (minor): The title says "make set_key_image do nothing," but the implementation is DBUG_ASSERT(0), which aborts in debug builds. It only "does nothing" in release. Consider rewording to "…must never be called (DBUG_ASSERT(0))" so the message matches the code.
• Copy-pasted comment (minor): In the new Field_geom::print_key_part_value, the comment "Otherwise, print the key value starting immediately after the null-byte" is carried over from the base function but no longer applies — the override ignores the key bytes and length entirely and prints a constant. Trim the comment to just explain the NULL check, since length/post-null-byte data are deliberately unused here.
• ✅ virtual + override usage and the placement of the new definition in sql_type_geom.cc follow the file's conventions.

Test coverage

• ✅ The test reproduces the exact scenario (SPATIAL KEY + optimizer_trace='enabled=on' + MBRCONTAINS) and would have crashed under ASAN before the fix, so it's an effective regression guard.
• Suggestion (optional): It only asserts the query runs. Since the whole point is what gets printed into the trace, you could additionally SELECT ... FROM information_schema.optimizer_trace and confirm unprintable_geometry_value appears — that would lock in the behavior, not just "doesn't crash." Skip it if the trace output is too verbose/unstable to match reliably.

Risks

• Low. The main residual risk is the DBUG_ASSERT(0) firing on some geom path not covered above; my audit found none reachable, but any future code that starts calling set_key_image on a nullable/geom field would now assert in debug rather than silently misbehave — arguably a feature.
• Release behavior is unambiguously improved (no-op vs. out-of-bounds read).

Verdict

Approve — correct, well-scoped, and defensively reasoned fix. Only nits: align the commit message with the DBUG_ASSERT(0) behavior, prune the stale copied comment, and optionally strengthen the test to check the trace output.

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182, fixing ASAN errors related to geometry and blob fields during selectivity calculations. The changes include adding test cases, making print_key_part_value virtual, handling MYSQL_TYPE_BLOB in pseudo-index key part creation, and overriding key-related methods in Field_geom with assertions. The review feedback highlights potential compiler warnings in release builds where DBUG_ASSERT(0) is compiled out, leaving parameters unused in set_key_image, print_key_value, and print_key_part_value. It is recommended to comment out or omit these unused parameter names to prevent build warnings or errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182 by fixing ASAN errors related to geometry and blob types. Key changes include making Field::print_key_part_value virtual, implementing it for Field_geom, and updating pseudo-index key part creation to handle MYSQL_TYPE_BLOB. The review feedback suggests removing the print_key_value override in Field_geom that asserts 0, as the base class implementation already delegates to the newly virtual print_key_part_value, making the override redundant and potentially prone to debug crashes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

claude review of the msan fix below

---

Code Review: MDEV-31182 — Fix MSAN uninitialized read in print_range for GIS indexes

,Commit: 13c5c02c (follow-up to 8f0aef93, split out for targeted review)
,File: sql/opt_range.cc (+2 / −2)

Overview

This is a minimal follow-up to the parent commit. The parent's new test (SPATIAL KEY + optimizer_trace) added coverage that routes a GIS range through print_range(), which unconditionally computed:

key_part_map keypart_map= range->start_key.keypart_map | range->end_key.keypart_map;

For GIS ranges, range->end_key is never populated. I confirmed this in sel_arg_range_seq_next (opt_range_mrr.cc:251-261): the GEOM_FLAG branch sets only range->start_key.* and returns, leaving end_key.keypart_map uninitialized (or stale from a prior iteration of the reused KEY_MULTI_RANGE) — the non-GEOM branch at :276 is the only place it's assigned. Hence the MSAN uninitialized-value report.

The fix initializes keypart_map from start_key only, and defers OR-ing in end_key.keypart_map until after the GEOM_FLAG early-return, so the uninitialized field is never read on the GIS path.

Correctness

• ✅ GEOM path: now uses start_key.keypart_map only. This is both the MSAN fix and semantically correct — GIS ranges have no upper bound (max is +inf), so end_key is meaningless. print_keyparts_name at :16790 gets the right map.
• ✅ Non-GEOM path unchanged: keypart_map is only consumed at :16804 (print_keyparts_name), which is now after keypart_map |= range->end_key.keypart_map. The final value is still start | end, identical to before. No behavior change for regular ranges.
• ✅ No other consumer reads keypart_map between the new init and the GEOM branch, so the reordering is safe.

Code quality / style

• ✅ Minimal, targeted, and the commit-splitting rationale (isolating the MSAN fix from the parent's review) is good practice.
• Suggestion — add a comment (main nit): The split between line 16780 and the deferred keypart_map |= range->end_key.keypart_map; looks arbitrary to a future reader, who may well "simplify" it straight back and reintroduce the bug. A one-liner would prevent that, e.g.:

  // end_key.keypart_map is not populated for GIS ranges (see
  // sel_arg_range_seq_next); only reference it on the non-GEOM path below.
  keypart_map|= range->end_key.keypart_map;

Potential issues / risks

• Symptom vs. source (worth a note, not a blocker): This fixes the consumer (print_range). The underlying condition — sel_arg_range_seq_next leaving end_key uninitialized for GEOM ranges — remains. An arguably more robust fix would zero/initialize range->end_key in the GEOM_FLAG branch at the source, protecting any other current or future consumer of these ranges (e.g. MRR paths) from the same uninitialized read. The author's choice of the localized fix is defensible given the MSAN report was specifically in the print path and the source-side change carries broader risk to the actual scan; I'd just confirm no other code path dereferences end_key.keypart_map for GEOM ranges. If that's been checked, the current fix is fine.
• Low overall risk: two-line change, no functional change for non-GIS ranges, and it removes an undefined-behavior read.

Test coverage

• ✅ Covered transitively — the parent commit's testcase is precisely what exposes this MSAN failure, so no new test is needed here (and the split commit correctly relies on it). This is only observable under an MSAN build; the existing test will catch regressions there.

Verdict

,Approve. Correct, minimal, and well-reasoned fix for a genuine uninitialized-read. Only recommendations: add a brief comment explaining why the end_key.keypart_map OR is deferred past the GEOM branch, and (optionally) confirm no non-print consumer reads end_key for GIS ranges — if one does, prefer initializing end_key at the source in sel_arg_range_seq_next.

[mariadb-YuchenPei]

Symptom vs. source (worth a note, not a blocker): This fixes the consumer (print_range). The underlying condition — sel_arg_range_seq_next leaving end_key uninitialized for GEOM ranges — remains. An arguably more robust fix would zero/initialize range->end_key in the GEOM_FLAG branch at the source, protecting any other current or future consumer of these ranges (e.g. MRR paths) from the same uninitialized read.

This is a good point, but it is not clear what should be the correct value in that branch

[mariadb-YuchenPei]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses MDEV-31182, resolving ASAN errors in String_copier::well_formed_copy and calculate_cond_selectivity_for_table. Key changes include making print_key_part_value virtual in Field, implementing it for Field_geom to handle unprintable geometry values, and updating create_key_parts_for_pseudo_indexes to correctly handle MYSQL_TYPE_BLOB. Additionally, the logic for dropping statistic fields during table alteration was refined in sql_table.cc to only trigger when the field's comparison type handler changes. Since there are no review comments, no further feedback is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[mariadb-YuchenPei]

claude review of the new commit on dropping column stats:

---

Code Review: MDEV-31182 [wip] — Drop column stats on incompatible ALTER changes

Commit: 42529a53 (marked [wip]) — sql/sql_table.cc (+5/−3), plus test updates (+13/−4)

Overview

The goal: ALTER TABLE ... MODIFY/CHANGE should drop persistent column statistics (mysql.column_stats) whenever the type change makes the stored stats meaningless — regardless of ALTER algorithm. Previously the drop was wired into fill_alter_inplace_info(), which only runs on the inplace/no-copy path and keyed off can_convert_nocopy() (an inplace-feasibility test, not a stats-validity test). So a COPY-algorithm incompatible change silently kept stale stats.

The change:

• Removes the drop_stat_fields.push_back(field, …) from fill_alter_inplace_info() (sql_table.cc:7016), leaving the index-stat-drop loop there untouched.
• Adds the column-stat drop to mysql_prepare_alter_table() (runs for all algorithms), gated on field->type_handler_for_comparison() != def->type_handler()->type_handler_for_comparison().

I confirmed the plumbing is sound: drop_stat_fields is consumed by apply_statistics_deletes_renames(), which is called on both the inplace path (sql_table.cc:8203) and the copy path (sql_table.cc:11808). So relocating the population into mysql_prepare_alter_table correctly extends the behavior to COPY.

Correctness

• ✅ Core intent works. The type_blob and analyze (CHANGE b c INT, ORDER BY b — forces COPY) test deltas both go from stats-present to stats-dropped, which is the correct new behavior for genuinely incompatible changes.
• ✅ Rename preservation verified. I traced the varchar-swap tests: varchar(100)→varchar(200) keeps the same comparison handler, so stats are no longer dropped but renamed/preserved. The new avg_length rows (b=4.5, c=2.0, d=50.5 after the cyclic rename) are internally consistent — this is an improvement, not a regression.
• ✅ No double-drop. In the inplace path, mysql_prepare_alter_table now populates column drops and fill_alter_inplace_info no longer does, so each field is pushed once. The DROP / DROP-SYSTEM-VERSIONING pushes (:8571, :8595) are on disjoint paths.
• ✅ Short-circuit ordering is safe. The cheap comparison and tmp_table checks precede push_back, so the list mutation only happens when a drop is actually warranted, and DBUG_RETURN(true) fires only on OOM. Signatures check out (Field::type_handler_for_comparison at field.h:1100; def->type_handler() on Create_field).

The two open questions in the commit message

1. Is type_handler_for_comparison() equality sufficient? — Likely not. Concrete gaps where the handler is unchanged but the stats become invalid:

• Charset/collation change on a string column (e.g. MODIFY b VARCHAR(100) CHARACTER SET utf8mb4 from latin1). Same comparison handler, but min_value/max_value byte representations, avg_length, and histogram bucket boundaries are all collation-dependent → stale/incorrect stats would be retained. Note the old can_convert_nocopy logic would have dropped these (charset change forces a copy on the inplace path), so this is a potential regression to check.
• Signedness change (INT → INT UNSIGNED): worth verifying whether these share a comparison handler — if they do, the retained min/max would be misinterpreted.
• Length shrink (VARCHAR(300)→VARCHAR(100)): handler unchanged, stats kept, but stored max may exceed the new length. Old inplace behavior likely dropped these. Debatable whether it matters (selectivity-only), but it's a behavior change worth a conscious decision.

Suggestion: consider !field->is_equal(*def) (or a dedicated stats-compat predicate that also accounts for charset/collation) rather than comparison-handler equality alone.

2. Do affected index stats need dropping too? — Yes, and there's now an asymmetry. The index-stat drop (add_stat_drop_index loop) remains in fill_alter_inplace_info(), which is inplace-only. So a COPY-algorithm incompatible column change now drops column stats but not index stats on that column. Mirroring the same criterion for index stats in mysql_prepare_alter_table (or moving that loop) would make the two consistent.

Code quality / style

• Density (minor): collapsing the semantic decision, the tmp_table guard, and the OOM-error push_back into one compound if reads less clearly than the surrounding style (:8569, :8593 use a nested if (tmp_table == NO_TMP_TABLE) { if (push_back(...)) return; }). Matching that shape would be more consistent and separate "should we drop" from "did allocation fail":

  if (field->type_handler_for_comparison() !=
        def->type_handler()->type_handler_for_comparison() &&
      table->s->tmp_table == NO_TMP_TABLE)
  {
    if (alter_info->drop_stat_fields.push_back(field, thd->mem_root))
      DBUG_RETURN(true);
  }

• Placement inside if (def && field->invisible < INVISIBLE_SYSTEM) is right — pure metadata changes (comment/default) keep the handler and correctly retain stats.
• Commit message: strip [wip] and resolve the (?)/TODOs before merge; the message is otherwise clear about intent.

Test coverage

• ✅ Good additions covering the COPY path (type_blob) and the CHAR→INT + ORDER BY (COPY) case.
• Gaps to add once the criterion is settled:
  • A charset/collation-only MODIFY, asserting stats are dropped (this is the case most likely to expose insufficiency of the current check).
  • An index-stat drop on a COPY-algorithm incompatible change (to lock in the fix for open question Implementation of mdev-5871 #2).
  • A signed→unsigned MODIFY to pin down that behavior.

Risks

• Regression risk: charset/collation changes may now retain stale string stats that the old path dropped — the highest-priority item to confirm.
• Inconsistency: column vs. index stat dropping now differ by algorithm.
• Otherwise low: the change is small, the apply path is shared across algorithms, and existing tests were updated to reflect (correct) new behavior.

Verdict

Right direction, not yet mergeable (as the [wip] marker acknowledges). The relocation to mysql_prepare_alter_table correctly generalizes stat-dropping to the COPY algorithm, and the verified test deltas are improvements. Before finalizing: (1) replace or augment the comparison-handler check to catch charset/collation (and verify signedness), (2) make index-stat dropping symmetric across algorithms, and (3) add the corresponding tests.