MDEV-40383:innodb_gis.point_basic fails on replay

[bsrikanth-mariadb]

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

[gemini-code-assist]

Caution

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

[mariadb-pavithrapandith]

Test failure is fixed with the changes

[vuvova]

you know the charset of tmp and charset of output. You must always use hex if tmp cannot be converted to output charset without losses. Otherwise you don't need hex. It's not an external property that you need to pass as an argument.

[bsrikanth-mariadb]

May be I should narrow down the scope of change. For Geometry types I can use HEX, and for others, existing string representation should work, as it was already taken care to use charsets.

[vuvova]

No! This is the whole point. Geometry is just a special case of a binary string that cannot necessarily be converted to the target charset. It is not a property of a type, it's a property of the charset. You must use hex in all cases where you cannot perform a lossless conversion from the source to the target charset.

[bsrikanth-mariadb]

updated it now.

[vuvova]

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

[Olernov]

@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]

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

[bsrikanth-mariadb]

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?

[Olernov]

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

[Olernov]

if we always set print_names=true

Yes, this looks more robust than positional printing

[gemini-code-assist]

Caution

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

[Olernov]

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()))

[vuvova]

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

[bsrikanth-mariadb]

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

[spetrunia]

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]

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]

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

[spetrunia]

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