Skip to content

MDEV-39412: parse error reading tabs in ranges #5049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bsrikanth-mariadb wants to merge 1 commit into
base: bb-12.3-MDEV-39368-test-replay
Choose a base branch
from
Open

MDEV-39412: parse error reading tabs in ranges #5049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions mysql-test/main/opt_context_replay_basic.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,5 +272,35 @@ set optimizer_replay_context='opt_context';
EXPLAIN SELECT MAX(a) FROM t1;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Select tables optimized away
set optimizer_replay_context='';
drop table t1;
#
# MDEV-39412: Failed to parse saved optimizer context: error reading ranges value
#
set optimizer_record_context=0;
CREATE TABLE t1(
a VARCHAR(8),
b VARCHAR(8),
KEY(A),
KEY(B)
);
INSERT INTO t1 SELECT REPEAT('a',8), REPEAT('b',8) FROM seq_1_to_10;
set optimizer_record_context=1;
EXPLAIN
SELECT * FROM t1 FORCE INDEX(a,b) WHERE a LIKE 'a%' OR b LIKE 'b%'
ORDER BY a,b;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE t1 index_merge a,b a,b 35,35 NULL 10 Using sort_union(a,b); Using where; Using filesort
select context into dumpfile "../../tmp/dump1.sql"
from information_schema.optimizer_context;
drop table t1;
set optimizer_replay_context='opt_context';
# Same query as above, must have same explain:
EXPLAIN
SELECT * FROM t1 FORCE INDEX(a,b) WHERE a LIKE 'a%' OR b LIKE 'b%'
ORDER BY a,b;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE t1 index_merge a,b a,b 35,35 NULL 10 Using sort_union(a,b); Using where; Using filesort
set optimizer_replay_context='';
drop table t1;
drop database db1;
39 changes: 39 additions & 0 deletions mysql-test/main/opt_context_replay_basic.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,45 @@ set optimizer_replay_context='opt_context';
--echo # Same query as above, must have same explain:
EXPLAIN SELECT MAX(a) FROM t1;

set optimizer_replay_context='';
--remove_file "$MYSQLTEST_VARDIR/tmp/dump1.sql"
drop table t1;

--echo #
--echo # MDEV-39412: Failed to parse saved optimizer context: error reading ranges value
--echo #
set optimizer_record_context=0;

CREATE TABLE t1(
a VARCHAR(8),
b VARCHAR(8),
KEY(A),
KEY(B)
);
INSERT INTO t1 SELECT REPEAT('a',8), REPEAT('b',8) FROM seq_1_to_10;

set optimizer_record_context=1;
EXPLAIN
SELECT * FROM t1 FORCE INDEX(a,b) WHERE a LIKE 'a%' OR b LIKE 'b%'
ORDER BY a,b;

select context into dumpfile "../../tmp/dump1.sql"
from information_schema.optimizer_context;
drop table t1;

--disable_query_log
--disable_result_log
--source "$MYSQLTEST_VARDIR/tmp/dump1.sql"
--enable_query_log
--enable_result_log
set optimizer_replay_context='opt_context';
--echo # Same query as above, must have same explain:
EXPLAIN
SELECT * FROM t1 FORCE INDEX(a,b) WHERE a LIKE 'a%' OR b LIKE 'b%'
ORDER BY a,b;

set optimizer_replay_context='';

--remove_file "$MYSQLTEST_VARDIR/tmp/dump1.sql"
drop table t1;

Expand Down
14 changes: 10 additions & 4 deletions sql/opt_context_store_replay.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,18 @@ void dump_mrr_info_calls(List<Multi_range_read_const_call_record> *mrr_list,
Json_writer_object irc_wrapper(ctx_writer);
irc_wrapper.add("index_name", irc->idx_name);

List_iterator rc_li(irc->range_list);
Json_writer_array ranges_wrapper(ctx_writer, "ranges");
while (const char *range_str= rc_li++)
{
Json_writer_array ranges_wrapper(ctx_writer, "ranges");
List_iterator rc_li(irc->range_list);
while (const char *range_str= rc_li++)
ranges_wrapper.add(range_str, strlen(range_str));
const String range_info(range_str, strlen(range_str),
system_charset_info);
StringBuffer<128> escaped_range_info;
json_escape_to_string(&range_info, &escaped_range_info);
ranges_wrapper.add(escaped_range_info.c_ptr_safe(),
escaped_range_info.length());
}
ranges_wrapper.end();
Comment on lines +259 to +270
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation re-allocates the StringBuffer and its internal storage on every iteration of the loop. It is more efficient to move the buffer declaration outside the loop and reuse it by clearing its length in each iteration. Additionally, using a scoped block for the Json_writer_array is a cleaner way to ensure the array is closed correctly via its destructor, rather than calling end() explicitly.

    {
      Json_writer_array ranges_wrapper(ctx_writer, "ranges");
      List_iterator rc_li(irc->range_list);
      StringBuffer<128> escaped_range_info;
      while (const char *range_str= rc_li++)
      {
        const String range_info(range_str, strlen(range_str),
                                system_charset_info);
        escaped_range_info.length(0);
        json_escape_to_string(&range_info, &escaped_range_info);
        ranges_wrapper.add(escaped_range_info.c_ptr_safe(),
                           escaped_range_info.length());
      }
    }


irc_wrapper.add("num_rows", irc->rows);
{
Expand Down
12 changes: 1 addition & 11 deletions sql/sql_json_lib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,7 @@ bool read_string(MEM_ROOT *mem_root, json_engine_t *je, const char *read_elem_ke
if (check_reading_of_elem_key(je, read_elem_key, err_buf))
return true;

StringBuffer<128> val_buf;
if (json_unescape_to_string((const char *) je->value, je->value_len,
&val_buf))
{
err_buf->append(STRING_WITH_LEN("un-escaping error of "));
err_buf->append(read_elem_key, strlen(read_elem_key));
err_buf->append(STRING_WITH_LEN(" element"));
return true;
}

value= strdup_root(mem_root, val_buf.c_ptr_safe());
value= strmake_root(mem_root, (const char *) je->value, je->value_len);
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Removing the unescaping logic from read_string breaks the standard behavior of a JSON parser. JSON strings are expected to be unescaped to correctly handle sequences like \n, \t, \", and \\.

The issue described in the PR (where \\t becomes \t but should remain \\t) is actually addressed by the double-escaping introduced in dump_mrr_info_calls. With double-escaping, the JSON will contain \\\\t, which after SQL unescaping becomes \\t, and then the JSON reader's unescaping will turn it into \t (the literal backslash and 't').

By removing unescaping here, you are making the JSON reader return the raw, escaped content of the JSON string, which is non-standard and will break other potential users of this utility function. Please restore the unescaping logic.

  StringBuffer<128> val_buf;
  if (json_unescape_to_string((const char *) je->value, je->value_len,
                              &val_buf))
  {
    err_buf->append(STRING_WITH_LEN("un-escaping error of "));
    err_buf->append(read_elem_key, strlen(read_elem_key));
    err_buf->append(STRING_WITH_LEN(" element"));
    return true;
  }

  value= strdup_root(mem_root, val_buf.c_ptr_safe());

return false;
}

Expand Down
14 changes: 9 additions & 5 deletions unittest/sql/json_reader-t.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,29 @@ int main(int args, char **argv)
MEM_ROOT alloc;
json_engine_t je;
int rc;
const char *esc_str_val= "a\\bc";
init_alloc_root(0, &alloc, 32768, 0, 0);
mem_root_dynamic_array_init(&alloc, 0, &je.stack,
sizeof(int), NULL, JSON_DEPTH_DEFAULT,
JSON_DEPTH_INC, MYF(0));
system_charset_info= &my_charset_utf8mb3_bin;
const char *js_doc="{ \"str_val\": \"abc\", \"double_val\": 1234.5 }";
const char *js_doc= "{ \"str_val\": \"abc\", \"double_val\": 1234.5, "
"\"esc_str_val\": \"a\\bc\" }";
json_scan_start(&je, &my_charset_utf8mb3_bin, (const uchar *) js_doc,
(const uchar *) js_doc + strlen(js_doc));

char *parsed_name;
double parsed_dbl;
char *parsed_esc_str;
Read_named_member array[]= {
{"str_val", Read_string(&alloc, &parsed_name), false},
{"str_val", Read_string(&alloc, &parsed_name), false},
{"double_val", Read_double(&parsed_dbl), false},
{NULL, Read_double(NULL), false }
};
{"esc_str_val", Read_string(&alloc, &parsed_esc_str), false},
{NULL, Read_double(NULL), false}};
String err_buf;

rc= json_read_object(&je, array, &err_buf);
rc= json_read_object(&je, array, &err_buf) ||
strcmp(parsed_esc_str, esc_str_val);
Comment on lines +63 to +64
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test change validates that the reader does NOT unescape, which is incorrect for a JSON library. For example, a JSON string "a\\bc" should be unescaped to a\bc (where \b is a backspace character if following standard JSON rules, or literal backslash and 'b' if escaped as \\b). Once read_string is restored to its correct behavior, this test should be updated to verify that unescaping is handled correctly.

ok(!rc, "Basic object read");
free_root(&alloc, 0);
#if 0
Expand Down
Loading