MDEV-40383:innodb_gis.point_basic fails on replay by bsrikanth-mariadb · Pull Request #5446 · MariaDB/server

bsrikanth-mariadb · 2026-07-23T09:21:07Z

There are 2 problems: -

The REPLACE statement that is recorded doesn't store the
value of geometry type field correctly.
The table definition that got recorded has fields with non-null constraint,
and no default value is specified.
Also, the "REPLACE INTO" statement that gets stored in the context,
doesn't have any value specified for these non-null fields.

Solution is to: -

When using REPLACE INTO statement, store all the non-numeric values in HEX,
whenever conversion from field's charset to output's charset is lossy.
Instead of storing only the column values that were projected in the
query, store all the non-virtual column values into the recorded
REPLACE INTO statement.

Implementation details: -

Introduce a new method is_charset_conversion_lossless() in filesort.cc,
to check if the output charset to which field's data is being written to,
results in a lossless conversion. If so, non-numeric values being witten
using REPLACE INTO statement are stored in string representation,
else they are converted to HEX.
From join_read_const(), and join_read_system() methods in sql_select.cc
re-read the const row for all the non-virtual fields in the table.
Similarly, for min/max optimization in opt_sum_query() of opt)_sum.cc,
include all the non-virtual fields to be dumped into the "REPLACE INTO"
statement.
After the row is re-read and recorded, restore the table->read_set,
table->status, and the const row, to the value that was before with
the help of widen_read_set_no_vcols() method.

gemini-code-assist · 2026-07-23T09:29:07Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

mariadb-pavithrapandith · 2026-07-24T06:36:08Z

Test failure is fixed with the changes

vuvova · 2026-07-24T15:59:19Z

+  join_read_system()/join_read_const() only fetch the columns present in
+  table->read_set. That's sufficient for execution, but when we record the
+  const row for replay it yields an incomplete REPLACE INTO -- columns that
+  are NOT NULL and have no default then depend on a relaxed sql_mode (see


just make sure all columns always have defaults and you won't need any of the below!

@vuvova , do you suggest generating default values for any possible column type? How feasible is this given that there may be something fancy like GEOMETRY?

Olernov · 2026-07-24T18:24:09Z

    {
      Field *field= *pfield;
+      /* Generated columns cannot be assigned a value in REPLACE INTO */
+      if (field->vcol_info)


This function is also used by dbug_format_row() / dbug_print_row(), so this brings in a regression for those (same applies below too)

Agree. However, I believe dbug_format_row() / dbug_print_row() are only used when debug is enabled. Since it is low risk method, we clubbed our changes with this. How, about if we always set print_names=true?

At least, I'd suggest adding an argument like print_virtual_cols to format_and_store_row(), otherwise silently dropping vcols looks unexpected

if we always set print_names=true

Yes, this looks more robust than positional printing

gemini-code-assist · 2026-07-27T12:29:09Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Olernov · 2026-07-28T10:47:14Z

  return length;
 }

+static bool is_target_cs_superset(CHARSET_INFO *from_cs, CHARSET_INFO *to_cs)


This name is not descriptive, consider renaming to something like:

charset_conversion_is_lossless

can_convert_losslessly

Or even invert it and rename to charset_conversion_may_lose_data, so that the code on the call site becomes more readable:

if (require_quote && tmp.length() && charset_conversion_may_lose_data(field->charset(), output.charset()))

Please, make sure @abarkov reviews it. And may be we already have a method to do it, no need to invent a new one

Sure, tried using the function can_be_safely_converted_to() defined in sql_string.h. But, it doesn't work correctly.

spetrunia · 2026-08-01T13:24:24Z

One missed case:
Context stores table rows not only for const tables but also for the MIN/MAX records in MIN/MAX optimization.

Saving stats for

EXPLAIN SELECT MAX(a) FROM t1;

I can see the same error:

REPLACE INTO db1.t1(a) VALUES (2)' failed: ER_CANT_CREATE_GEOMETRY_OBJECT (1416): Cannot get geometry object from data you send to the GEOMETRY field

Full diff:

diff --git a/mysql-test/main/opt_context_replay_basic.test b/mysql-test/main/opt_context_replay_basic.test
index 17a6354e276..ba2c12d11e9 100644
--- a/mysql-test/main/opt_context_replay_basic.test
+++ b/mysql-test/main/opt_context_replay_basic.test
@@ -392,6 +392,25 @@ set optimizer_replay_context='opt_context';
 --echo # Same query as above, must have same explain:
 EXPLAIN SELECT a, ST_AsText(p) FROM t1 WHERE a = 2 AND p = ST_GeomFromText('POINT(20 20)');
 
+
+--echo # More tests 
+set optimizer_replay_context=NULL;
+alter table t1 add index(a);
+select a from t1;
+SELECT MAX(a) FROM t1;
+set optimizer_record_context=1;
+EXPLAIN SELECT MAX(a) FROM t1;
+select context into dumpfile "../../tmp/dump2.sql"
+from information_schema.optimizer_context;
+set optimizer_record_context=0;
+drop table t1;
+--disable_query_log
+--disable_result_log
+--source "$MYSQLTEST_VARDIR/tmp/dump2.sql"
+--enable_query_log
+--enable_result_log
+set optimizer_replay_context='opt_context';
+
 set optimizer_replay_context='';
 --remove_file "$MYSQLTEST_VARDIR/tmp/dump1.sql"
 drop table t1;

spetrunia · 2026-08-01T14:31:39Z

I think, dumping MIN/MAX rows can be done in a similar way.
The difference is that MIN/MAX row is typically not the only row in the table.
It should be possible to get the full row by re-reading the row we've just read:

  // save the handler state: is it inited for reading index
  //   (save h->active_index) or for full table scan? 
  h->position(); 
  // Set all needed columns in table->read_set
  h->rnd_int(scan=false);
  h->rnd_pos(h->ref);

spetrunia · 2026-08-01T14:33:56Z

record_const_row_full() should be in opt_context_store_replay.*, not in sql_select.cc

spetrunia · 2026-08-01T14:48:09Z

Please apply this cleanup diff

diff --git a/sql/filesort.cc b/sql/filesort.cc
index a2c828b41e2..e5c31f7eeaa 100644
--- a/sql/filesort.cc
+++ b/sql/filesort.cc
@@ -3024,8 +3024,8 @@ static uint make_packed_sortkey(Sort_param *param, uchar *to)
   return length;
 }
 
-static bool is_charset_conversion_lossless(CHARSET_INFO *from_cs,
-                                           CHARSET_INFO *to_cs)
+static bool is_charset_conversion_lossless(const CHARSET_INFO *from_cs,
+                                           const CHARSET_INFO *to_cs)
 {
   if (to_cs == &my_charset_bin)
     return true; // binary swallows any bytes
@@ -3182,8 +3182,7 @@ void format_and_store_row(TABLE *table, const uchar *rec, bool print_names,
       {
         if (require_quote)
           output.append('\'');
-        output.append_for_single_quote_opt_convert(tmp.ptr(), tmp.length(),
-                                                   field->charset());
+        output.append_for_single_quote_opt_convert(tmp);
         if (require_quote)
           output.append('\'');
       }

spetrunia

See the input above

spetrunia · 2026-08-02T08:30:57Z

Another question: Previously, the problem of "Not having DEFAULT value for columns that were also not in the read_set" was fixed for some datatypes by clearing STRICT_{ALL,TRANS}_TABLES for the REPLACE statement in the context:

{code:sql}
SET STATEMENT sql_mode=REPLACE(REPLACE(@@sql_mode,'STRICT_ALL_TABLES',''),'STRICT_TRANS_TABLES','') FOR
REPLACE INTO test.t1 ...
{code}

If we've resolved the issue with other means, is clearing the flags now redundant? If yes, let's remove it.

There are 2 problems: - 1. The REPLACE statement that is recorded doesn't store the value of geometry type field correctly. 2. The table definition that got recorded has fields with non-null constraint, and no default value is specified. Also, the "REPLACE INTO" statement that gets stored in the context, doesn't have any value specified for these non-null fields. Solution is to: - 1. When using REPLACE INTO statement, store all the non-numeric values in HEX, whenever conversion from field's charset to output's charset is lossy. 2. Instead of storing only the column values that were projected in the query, store all the non-virtual column values into the recorded REPLACE INTO statement. Implementation details: - 1. Introduce a new method is_charset_conversion_lossless() in filesort.cc, to check if the output charset to which field's data is being written to, results in a lossless conversion. If so, non-numeric values being witten using REPLACE INTO statement are stored in string representation, else they are converted to HEX. 2. From join_read_const(), and join_read_system() methods in sql_select.cc re-read the const row for all the non-virtual fields in the table. Similarly, for min/max optimization in opt_sum_query() of opt)_sum.cc, include all the non-virtual fields to be dumped into the "REPLACE INTO" statement. After the row is re-read and recorded, restore the table->read_set, table->status, and the const row, to the value that was before with the help of widen_read_set_no_vcols() method.

bsrikanth-mariadb · 2026-08-03T05:52:01Z

Another question: Previously, the problem of "Not having DEFAULT value for columns that were also not in the read_set" was fixed for some datatypes by clearing STRICT_{ALL,TRANS}_TABLES for the REPLACE statement in the context:

{code:sql} SET STATEMENT sql_mode=REPLACE(REPLACE(@@sql_mode,'STRICT_ALL_TABLES',''),'STRICT_TRANS_TABLES','') FOR REPLACE INTO test.t1 ... {code}

If we've resolved the issue with other means, is clearing the flags now redundant? If yes, let's remove it.

This can be removed, and the new tests that were added to opt_context_replay_basic.test, confirms it. However, there is one off test noticed in main.type_enum.test, which is failing.

CREATE TABLE t1(a enum('a','b','c','d'));
INSERT INTO t1 VALUES (4),(1),(0),(3); // -> fails for INNODB. But, it succeeds in MyISAM and is the cause of concern

EXPLAIN SELECT a FROM t1 WHERE a=0;
SELECT a FROM t1 WHERE a=0;

ALTER TABLE t1 ADD PRIMARY KEY (a);

EXPLAIN SELECT a FROM t1 WHERE a=0;
//Here, with out lowering the constraint, The REPLACE INTO statement would try to insert a NULL value into field a.
//which result in an error

REPLACE INTO test.t1(a) VALUES (''): Data truncated for column 'a' at row 1

bsrikanth-mariadb added the MariaDB Corporation label Jul 23, 2026

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch 2 times, most recently from d7ceeb9 to 12df834 Compare July 24, 2026 02:51

bsrikanth-mariadb requested a review from spetrunia July 24, 2026 09:10

vuvova reviewed Jul 24, 2026

View reviewed changes

Comment thread sql/filesort.cc

vuvova reviewed Jul 24, 2026

View reviewed changes

Olernov reviewed Jul 24, 2026

View reviewed changes

Comment thread sql/filesort.cc Outdated

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from 12df834 to 4ebbfec Compare July 27, 2026 11:29

bsrikanth-mariadb marked this pull request as draft July 27, 2026 11:47

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch 3 times, most recently from 9f50d3c to cf67fb5 Compare July 27, 2026 12:28

bsrikanth-mariadb marked this pull request as ready for review July 27, 2026 12:29

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from cf67fb5 to d817206 Compare July 27, 2026 12:35

Olernov reviewed Jul 28, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 28, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 28, 2026

View reviewed changes

bsrikanth-mariadb requested a review from abarkov July 28, 2026 13:23

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from d817206 to b4d06f3 Compare July 30, 2026 04:21

bsrikanth-mariadb marked this pull request as draft July 30, 2026 07:09

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from b4d06f3 to 46c4a5e Compare July 30, 2026 10:24

bsrikanth-mariadb marked this pull request as ready for review July 30, 2026 10:24

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from 46c4a5e to a1df1fb Compare August 1, 2026 02:50

spetrunia requested changes Aug 1, 2026

View reviewed changes

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from a1df1fb to 6b93976 Compare August 3, 2026 02:35

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from 6b93976 to ebeb028 Compare August 3, 2026 05:45

Uh oh!

Uh oh!

Conversation

bsrikanth-mariadb commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot commented Jul 23, 2026

Uh oh!

mariadb-pavithrapandith commented Jul 24, 2026

Uh oh!

Uh oh!

vuvova Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot commented Jul 27, 2026

Uh oh!

Uh oh!

Uh oh!

Olernov Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

vuvova Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrunia commented Aug 1, 2026

Uh oh!

spetrunia commented Aug 1, 2026

Uh oh!

spetrunia commented Aug 1, 2026

Uh oh!

spetrunia commented Aug 1, 2026

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

spetrunia commented Aug 2, 2026

Uh oh!

bsrikanth-mariadb commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

bsrikanth-mariadb commented Jul 23, 2026 •

edited

Loading

Olernov Jul 24, 2026 •

edited

Loading

Olernov Jul 26, 2026 •

edited

Loading

bsrikanth-mariadb Jul 30, 2026 •

edited

Loading

bsrikanth-mariadb commented Aug 3, 2026 •

edited

Loading