maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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.