MDEV-36896 - Assertion `marked_for_read()' failed in virtual String *Field_varstring::val_str(String *, String *) by pranavktiwari · Pull Request #5158 · MariaDB/server

pranavktiwari · 2026-06-01T13:06:20Z

Problem:

Executing queries that require virtual/generated column evaluation during filesort trigger sdebug assertions due to missing columns in read_set.

Cause:

find_all_keys() temporarily assigns TABLE::tmp_set as both read_set and write_set. Later, TABLE::update_virtual_field() clears tmp_set before evaluating virtual column dependencies.

Since all three pointers reference the same bitmap, clearing tmp_set also clears the active column maps, causing required columns to be missing during execution.

Fix:

Remove the bitmap_clear_all(&tmp_set) call from TABLE::update_virtual_field(). The dependency walk populates the required bits for virtual column evaluation, and clearing the shared bitmap can unintentionally invalidate the active read_set/write_set.

gemini-code-assist

Code Review

This pull request comments out the column_bitmaps_set call in sql/filesort.cc to address an assertion failure. However, the reviewer notes that this is incorrect because it bypasses the temporary read map optimization and pollutes the original read set. The recommended approach is to identify the missing field and ensure it is properly registered instead of disabling the bitmap switch.

Copilot

Pull request overview

Fixes MDEV-36896 where virtual/generated column evaluation during filesort can trip marked_for_read() assertions due to column bitmap aliasing, and adds a regression test to cover the scenario.

Changes:

Adjusts TABLE::update_virtual_field() bitmap handling to avoid clearing shared column maps during virtual column evaluation.
Extends vcol_misc test coverage with a new MDEV-36896 regression case (InnoDB + filesort + virtual column).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
`sql/table.cc`	Changes how the dependency-walk bitmap is handled during single virtual column evaluation.
`mysql-test/suite/vcol/t/vcol_misc.test`	Adds an MDEV-36896 regression scenario and introduces an InnoDB requirement.
`mysql-test/suite/vcol/r/vcol_misc.result`	Updates expected output for the new regression scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pranavktiwari · 2026-06-24T06:22:27Z

  in_use->set_n_backup_active_arena(expr_arena, &backup_arena);
-  bitmap_clear_all(&tmp_set);
  vf->vcol_info->expr->walk(&Item::update_vcol_processor, 0, &tmp_set);
  DBUG_FIX_WRITE_SET(vf);


not needed, It will add the required ones.

pranavktiwari · 2026-06-24T06:21:03Z

 --source include/have_ucs2.inc
 --source include/have_debug.inc
+--source include/have_innodb.inc


pranavktiwari · 2026-06-24T06:21:35Z

+SET tx_isolation='READ-UNCOMMITTED';
+CREATE TABLE t (a VARCHAR(10),b INT,c CHAR(10) GENERATED ALWAYS AS (a) VIRTUAL,KEY(b,c)) ENGINE=InnoDB;
+INSERT INTO t (a) VALUES ('A');
+SELECT * FROM t WHERE b IS NULL ORDER BY a;


pranavktiwari · 2026-06-24T06:13:01Z

+SET tx_isolation='READ-UNCOMMITTED';
+CREATE TABLE t (a VARCHAR(10),b INT,c CHAR(10) GENERATED ALWAYS AS (a) VIRTUAL,KEY(b,c)) ENGINE=InnoDB;
+INSERT INTO t (a) VALUES ('A');
+SELECT * FROM t WHERE b IS NULL ORDER BY a;
+a	b	c
+A	NULL	A


sanja-byelkin

Main description should be in commit comment. (what is in PR comment irrelevant and will be lost.

Investigate please this statement (not necessary correct):
Since the nested-vcol recursion guard was added (Item_field::update_vcol_processor uses bitmap_fast_test_and_set at item.cc:984), tmp_set doubles as the per-call "visited" set. Deleting the clear means it's never reset across calls on a reused TABLE. For a vcol-on-vcol dependency, the nested column's bit can already be set from a prior call, so it gets silently skipped and reuses a stale value — no error.

sanja-byelkin · 2026-06-23T08:31:21Z

+SET tx_isolation='READ-UNCOMMITTED';
+CREATE TABLE t (a VARCHAR(10),b INT,c CHAR(10) GENERATED ALWAYS AS (a) VIRTUAL,KEY(b,c)) ENGINE=InnoDB;
+INSERT INTO t (a) VALUES ('A');
+SELECT * FROM t WHERE b IS NULL ORDER BY a;


Version end marking.

sanja-byelkin · 2026-06-23T08:44:27Z

 --echo # End of 10.5 tests
 --echo #
+
+#


--echo is absent

sanja-byelkin · 2026-06-23T08:46:29Z

+#
+
+SET tx_isolation='READ-UNCOMMITTED';
+CREATE TABLE t (a VARCHAR(10),b INT,c CHAR(10) GENERATED ALWAYS AS (a) VIRTUAL,KEY(b,c)) ENGINE=InnoDB;


the test incorrecly cleanup (no DELETE). Have you run the test alone? nothing was suspiciouse?

sanja-byelkin · 2026-06-23T08:48:03Z

+# MDEV-36896 Assertion `marked_for_read()' failed in virtual String *Field_varstring::val_str(String *, String *)
+#
+
+SET tx_isolation='READ-UNCOMMITTED';


the some with the variable. old value should be saved and restored.

sanja-byelkin · 2026-06-23T10:01:04Z

 # End of 10.5 tests
 #
+SET tx_isolation='READ-UNCOMMITTED';
+CREATE TABLE t (a VARCHAR(10),b INT,c CHAR(10) GENERATED ALWAYS AS (a) VIRTUAL,KEY(b,c)) ENGINE=InnoDB;


add also test case with nested fields (fields in expression like a=c)

…Field_varstring::val_str(String *, String *) Problem: Executing queries that require virtual/generated column evaluation during filesort trigger sdebug assertions due to missing columns in read_set. Cause: find_all_keys() temporarily assigns TABLE::tmp_set as both read_set and write_set. Later, TABLE::update_virtual_field() clears tmp_set before evaluating virtual column dependencies. Since all three pointers reference the same bitmap, clearing tmp_set also clears the active column maps, causing required columns to be missing during execution. Fix: Remove the bitmap_clear_all(&tmp_set) call from TABLE::update_virtual_field(). The dependency walk populates the required bits for virtual column evaluation, and clearing the shared bitmap can unintentionally invalidate the active read_set/write_set.

pranavktiwari · 2026-06-24T10:55:30Z

Investigate please this statement (not necessary correct):
Since the nested-vcol recursion guard was added (Item_field::update_vcol_processor uses bitmap_fast_test_and_set at item.cc:984), tmp_set doubles as the per-call "visited" set. Deleting the clear means it's never reset across calls on a reused TABLE. For a vcol-on-vcol dependency, the nested column's bit can already be set from a prior call, so it gets silently skipped and reuses a stale value — no error.

bool Item_field::update_vcol_processor(void *arg)
{
  MY_BITMAP *map= (MY_BITMAP *) arg;
  if (field->vcol_info &&
      !bitmap_fast_test_and_set(map, field->field_index))
  {
    field->vcol_info->expr->walk(&Item::update_vcol_processor, 0, arg);
    field->vcol_info->expr->save_in_field(field, 0);
  }
  return 0;
}

In this method we are recursively just checking for virtual columns but before calling this method read_set is being cleared, hence it will never have bit set for actual columns.

But I think, removing bitmap_clear_all(&tmp_set); this line is definitely not a good solution.

Rather this update_vcol_processor method should evolve to handle this case as well.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

+  MY_BITMAP local_set;
+  my_bitmap_init(&local_set, NULL, s->fields);
+  bitmap_set_bit(&local_set, vf->field_index);


pranavktiwari marked this pull request as draft June 1, 2026 13:06

gemini-code-assist Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread sql/filesort.cc Outdated

pranavktiwari force-pushed the 10.11-MDEV-36896 branch from 834f750 to 5b140dc Compare June 2, 2026 06:10

pranavktiwari changed the title ~~DRAFT - Assertion `marked_for_read()' failed in virtual String *Field_varstring::val_str(String *, String *)~~ MDEV-36896 - Assertion `marked_for_read()' failed in virtual String *Field_varstring::val_str(String *, String *) Jun 2, 2026

pranavktiwari marked this pull request as ready for review June 2, 2026 06:24

gkodinov added the MariaDB Corporation label Jun 2, 2026

sanja-byelkin requested a review from Copilot June 23, 2026 09:54

Copilot started reviewing on behalf of sanja-byelkin June 23, 2026 09:54 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

sanja-byelkin requested changes Jun 23, 2026

View reviewed changes

pranavktiwari force-pushed the 10.11-MDEV-36896 branch 2 times, most recently from f809da3 to 3dd20f7 Compare June 24, 2026 07:22

pranavktiwari force-pushed the 10.11-MDEV-36896 branch from 3dd20f7 to 4428320 Compare June 24, 2026 08:16

pranavktiwari added 2 commits June 29, 2026 18:43

Added logic to update the all fileds in map.

747242c

Added a new bit map

b52f86e

pranavktiwari requested a review from Copilot June 30, 2026 11:08

Copilot started reviewing on behalf of pranavktiwari June 30, 2026 11:08 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Comment thread sql/table.cc

Comment on lines +9191 to +9193

MY_BITMAP local_set;

my_bitmap_init(&local_set, NULL, s->fields);

bitmap_set_bit(&local_set, vf->field_index);

Uh oh!

Uh oh!

Conversation

pranavktiwari commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Cause:

Fix:

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanja-byelkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavktiwari commented Jun 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

pranavktiwari commented Jun 1, 2026 •

edited

Loading