Re: [Maria-developers] Review request for MDEV-12325 Unexpected data type and truncation when using CTE
Hello Monty. The patch looks OK for me. I have just one question below: On 8/5/22 7:15 PM, Michael Widenius wrote:
Hi!
Just to make things easy, here is the patch: commit a556c4d972eaa09bb8ad586b9bda6cfb2a7b574a Author: Monty <monty@mariadb.org> Date: Fri Aug 5 17:57:27 2022 +0300
MDEV-12325 Unexpected data type and truncation when using CTE
When creating a recursive CTE, the column types are taken from the non recursive part of the CTE (this is according to the SQL standard).
This patch adds code to abort the CTE if the calculated values in the recursive part does not fit in the fields in the created temporary table.
The new code only affects recursive CTE, so it should not cause any notable problems for old applications.
<cut>
diff --git a/sql/sql_union.cc b/sql/sql_union.cc index 48d8c16db68..d271a919a4e 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -111,6 +111,8 @@ int select_unit::send_data(List<Item> &values) { int rc; int not_reported_error= 0; + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; + if (unit->offset_limit_cnt) { // using limit offset,count unit->offset_limit_cnt--; @@ -118,8 +120,10 @@ int select_unit::send_data(List<Item> &values) } if (thd->killed == ABORT_QUERY) return 0; + if (table->no_rows_with_nulls) table->null_catch_flags= CHECK_ROW_FOR_NULLS_TO_REJECT; + if (intersect_mark) { fill_record(thd, table, table->field + 1, values, TRUE, FALSE); @@ -127,6 +131,8 @@ int select_unit::send_data(List<Item> &values) } else fill_record(thd, table, table->field, values, TRUE, FALSE); + thd->count_cuted_fields= save_count_cuted_fields; + if (unlikely(thd->is_error())) { rc= 1; @@ -297,7 +303,13 @@ bool select_unit::send_eof()
Why do you need above changes in select_unit::send_data()? Shouldn't it be enough just to have below changes in select_union_recursive::send_data()?
int select_union_recursive::send_data(List<Item> &values) { + Abort_on_warning_instant_set handle_abort(thd, 1); + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; + + /* For recursive CTE's give warnings for wrong field info */ + thd->count_cuted_fields= CHECK_FIELD_WARN; int rc= select_unit::send_data(values); + thd->count_cuted_fields= save_count_cuted_fields;
if (rc == 0 && write_err != HA_ERR_FOUND_DUPP_KEY && <cut>
Hello Monty, On 8/5/22 10:35 PM, Alexander Barkov wrote:
Hello Monty.
The patch looks OK for me. I have just one question below:
On 8/5/22 7:15 PM, Michael Widenius wrote:
Hi!
Just to make things easy, here is the patch: commit a556c4d972eaa09bb8ad586b9bda6cfb2a7b574a <cut>
Why do you need above changes in select_unit::send_data()?
Shouldn't it be enough just to have below changes in select_union_recursive::send_data()?
int select_union_recursive::send_data(List<Item> &values) { + Abort_on_warning_instant_set handle_abort(thd, 1); + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; + + /* For recursive CTE's give warnings for wrong field info */ + thd->count_cuted_fields= CHECK_FIELD_WARN; int rc= select_unit::send_data(values); + thd->count_cuted_fields= save_count_cuted_fields;
if (rc == 0 && write_err != HA_ERR_FOUND_DUPP_KEY && <cut>
I compiled your patch. The changes to select_unit::send_data() are not needed. We only need to fix select_union_recursive::send_data(). Also, in non-strict mode it should produce warnings on tructation, without aborting SELECT or CREATE..SELECT statements. After some experimenting I came to this patch:
diff --git a/sql/sql_union.cc b/sql/sql_union.cc index 48d8c16db68..60aba118f04 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -297,7 +297,13 @@ bool select_unit::send_eof()
int select_union_recursive::send_data(List<Item> &values) { + Abort_on_warning_instant_set handle_abort(thd, thd->is_strict_mode()); + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; + + /* For recursive CTE's give warnings for wrong field info */ + thd->count_cuted_fields= CHECK_FIELD_WARN; int rc= select_unit::send_data(values); + thd->count_cuted_fields= save_count_cuted_fields;
if (rc == 0 && write_err != HA_ERR_FOUND_DUPP_KEY &&
In strict mode it still works fine and cte_recursive.test passes. In non-strict modes it: - now continues the statement (in your original version it aborted the statement on error). - SELECT and CREATE..SELECT produce the same result set, with the same set of warnings. - Issues truncation warnings Please consider applying this version of the patch. Note, a test for non-strict mode is additionally needed. Also, I think the patch should be further extended to handle IGNORE properly: CREATE TABLE t2 IGNORE AS WITH RECURSIVE .. SELECT ..; "Strict mode + IGNORE" should work like non-strict mode. A new test is also needed here. In order to process IGNORE easy, a "bool ignore;" member can be added to the class select_union_recursive and initialized in constructor: - either from a new constructor parameter - or using thd_arg->lex->ignore. Please investigate what is more appropriate. See classes st_copy_info, multi_update, Alter_inplace_info as examples having "ignore" as a member. Greetings.
participants (1)
-
Alexander Barkov