Re: [Maria-developers] 688ad5ed67f: MDEV-26830: Wrong ROW_NUMBER in diagnostics upon INSERT IGNORE with CHECK violation

2021-10-18 Thread Sergei Golubchik
Hi, Rucha!

On Oct 18, Rucha Deodhar wrote:
> revision-id: 688ad5ed67f (mariadb-10.6.1-138-g688ad5ed67f)
> parent(s): c27f04ede5a
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-10-18 13:40:56 +0530
> message:
> 
> MDEV-26830: Wrong ROW_NUMBER in diagnostics upon INSERT IGNORE with
> CHECK violation
> 
> Analysis: When there is constraint fail we return non-zero value for
> view_check_option(). So we continue the loop which doesn't increment the
> counter because it increments at the end of the loop.
> Fix: Increment m_current_row_for_warning() at the beginning of loop. This
> will also fix similar bugs if any, about counter not incrementing
> correctly because of continue.
> 
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index 3dff6722a3d..11289779f7c 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -1007,8 +1007,10 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list,
>goto values_loop_end;
>  }
>  
> +thd->get_stmt_da()->reset_current_row_for_warning(0);

This changes the behavior in array binding case, let's not do it in this
commit.

Better change the previous reset_current_row_for_warning() to reset to
0, not to 1.

The rest is ok.

>  while ((values= its++))
>  {
> +  thd->get_stmt_da()->inc_current_row_for_warning();
>if (fields.elements || !value_count)
>{
>  /*
> @@ -1125,7 +1127,6 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list,
>if (unlikely(error))
>  break;
>info.accepted_rows++;
> -  thd->get_stmt_da()->inc_current_row_for_warning();
>  }
>  its.rewind();
>  iteration++;
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp


Re: [Maria-developers] 8a84f5a40c1: MDEV-24176 Preparations

2021-10-18 Thread Aleksey Midenkov
Hi Sergei!

On Sun, Oct 17, 2021 at 4:55 PM Sergei Golubchik  wrote:
>
> Hi, Aleksey!
>
> On Oct 17, Aleksey Midenkov wrote:
> > commit 8a84f5a40c1
> > Author: Aleksey Midenkov 
> > Date:   Thu May 27 17:00:14 2021 +0300
> >
> > MDEV-24176 Preparations
> >
> > 1. moved fix_vcol_exprs() call to open_table()
> >
> > mysql_alter_table() doesn't do lock_tables() so it cannot win from
> > fix_vcol_exprs() from there. Tests affected: main.default_session
>
> This is likely wrong, but the old code was wrong too. Neither open_table
> nor lock_tables is called under LOCK TABLES, but the session
> environment can change, I suspect.

Why, open_table() is called under LOCK TABLES. Please see there `if
(best_table)` and a jump to reset where vcol_fix_exprs() is called.
Added test case for LOCK TABLES.

>
> >
> > 2. cleanup_excluding_fields_processor removed.
> >
> > That was just a quick hack to exclude wrongly working Item_field from
> > processing. Now it works due to correct execution environment (see
> > next commit). Related to MDEV-10355
>
> does it work now, in that commit, or only after the next commit?

It is for the next commit actually. Moved that there.


> what exactly do you mean by correct execution environment?

This is what Vcol_expr_context does.

>
> >
> > 3. Vanilla cleanups and comments.
> >
> > diff --git a/sql/item.h b/sql/item.h
> > index cc1914a7ad4..7b7fe04f0b2 100644
> > --- a/sql/item.h
> > +++ b/sql/item.h
> > @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field
> >const char *db_name;
> >const char *table_name;
> >const char *field_name;
> > +  /*
> > + NOTE: came from TABLE::alias_name_used and this is only a hint! It 
> > works
> > + only in need_correct_ident() condition. On other cases it is FALSE 
> > even if
> > + table_name is alias! It cannot be TRUE in these cases, lots of 
> > spaghetti
> > + logic depends on that.
>
> could you elaborate on that?

See in next commit check_vcol_func_processor(). I could use `if
(alias_name_used)` instead of `if (table_name)` if alias_name_used
were updated correctly. But it is used only for limited subset of
cases (so is not always true when alias is specified). Improving
alias_name_used caused me some failures (in find_field_in_tables()
AFAIR), so I had to mark VCOL_TABLE_ALIAS for every existing
Item_ident::table_name. That triggers redundant expr update for some
cases.

>
> > +  */
> >bool alias_name_used; /* true if item was resolved against alias */
> >/*
> >  Cached value of index for this field in table->field array, used by 
> > prep.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org



--
@midenok

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp