Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field

[Aleksey Midenkov]

Sergei,

On Thu, Oct 31, 2019 at 2:03 PM Sergei Golubchik  wrote:

> Hi, Aleksey!
>
> On Oct 30, Aleksey Midenkov wrote:
> > On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik 
> wrote:
> > > On Oct 25, Aleksey Midenkov wrote:
> > > > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d)
> > > > parent(s): 1153950ad0a
> > > > author: Aleksey Midenkov 
> > > > committer: Aleksey Midenkov 
> > > > timestamp: 2019-07-22 15:40:06 +0300
> > > > message:
> > > >
> > > > MDEV-20015 Assertion `!in_use->is_error()' failed in
> TABLE::update_virtual_field
> > > >
> > > > Preserve and restore statement DA.
> > >
> > > This is strange. Diagnostics areas aren't supposed to be temporarily
> > > created on the frame, they aren't arenas.
> >
> > There are already several samples of this pattern in the code. This is
> the
> > main (if not the only) usage of set_stmt_da().
>
> One in sql_prepare.cc, mysql_stmt_get_longdata().
> No idea why, it happened during the merge. The first patch used the
> error handler, which is the supposed way of doing this kind of things.
> Then during the merge it was suddenly replaced with this.
>
> Another one in sql_prepare.cc: Ed_connection::execute_direct(). It's
> dead code, doesn't matter.
>
> Yet another in sql_partition.cc, never mind that, partitioning is a huge
> mess, never take any ideas from it.
>
> And the last one in sql_get_diagnostics.cc, which is the way diagnostic
> areas are supposed to be used. That one is correct.
>
> > > Why TABLE::update_virtual_field() is called at all if there's already
> > > an error?
> >
> > update_virtual_field() is called as part of  REPAIR (MDEV-5800
> > ) which is done on bulk
> insert
> > finish. I doubt it should consider error status in this case as no matter
> > how SQL command is finished it must update the index.
> >
> > #0  TABLE::update_virtual_field (this=0x7f85b4068388, vf=0x7f85b4066948)
> at /home/midenok/src/mariadb/10.2/src/sql/table.cc:7685
> > #1  0x01105d2b in compute_vcols (info=0x7f85b406a5d8,
> record=0x7f85b4006948 "\377\001", keynum=1) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:683
> > #2  0x011142bd in sort_get_next_record
> (sort_param=0x7f860c05edd8) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3657
> > #3  0x01118a33 in sort_key_read (sort_param=0x7f860c05edd8,
> key=0x7f85b4039808) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3121
> > #4  0x011650a9 in find_all_keys (info=0x7f860c05edd8, keys=2,
> sort_keys=0x7f85b40397f8, buffpek=0x7f860c05ea20, maxbuffer=0x7f860c05ea54,
> tempfile=0x7f860c05e898, tempfile_for_exceptions=0x7f860c05e728) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:312
> > #5  0x01164b8d in _create_index_by_sort (info=0x7f860c05edd8,
> no_messages=1 '\001', sortbuff_size=134216704) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:228
> > #6  0x0111763b in mi_repair_by_sort (param=0x7f85b406e300,
> info=0x7f85b406a5d8, name=0x7f860c05f7c0 "./test/t2", rep_quick=1) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:2401
> > #7  0x01107097 in ha_myisam::repair (this=0x7f85b4068f20,
> thd=0x7f85b4000cf8, param=..., do_optimize=false) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1271
> > #8  0x011085d3 in ha_myisam::enable_indexes
> (this=0x7f85b4068f20, mode=2) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1610
> > #9  0x01108db2 in ha_myisam::end_bulk_insert
> (this=0x7f85b4068f20) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1760
> > #10 0x006d0d23 in handler::ha_end_bulk_insert
> (this=0x7f85b4068f20) at
> /home/midenok/src/mariadb/10.2/src/sql/handler.h:2918
> > #11 0x006cd120 in select_insert::abort_result_set
> (this=0x7f85b4012818) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_insert.cc:3959
> > #12 0x007335a7 in handle_select (thd=0x7f85b4000cf8,
> lex=0x7f85b4004820, result=0x7f85b4012818,
> setup_tables_done_option=1073741824) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_select.cc:383
> > #13 0x006eedad in mysql_execute_command (thd=0x7f85b4000cf8) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:4284
> > #14 0x006e8880 in mysql_parse (thd=0x7f85b4000cf8,
> rawbuf=0x7f85b40117f0 "insert into t2 (pk) select a from t1", length=36,
> parser_state=0x7f860c0625f0, is_com_multi=false, is_next_command=false) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:7760
>
> I see. You're right, we have to update vcols even if thd->is_error().
> It used to be a problem, indeed, as Item::fix_length_and_dec() was void,
> and the caller had to check thd->is_error().
>
> But Sanja has fixed it about a year ago. So ideally it would be much
> better if we could just update vcols under thd->is_error() == true.
> Did you try that? Simply removing the assert, I mean.
> 

Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field

[Sergei Golubchik]

Hi, Aleksey!

On Oct 30, Aleksey Midenkov wrote:
> On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik  wrote:
> > On Oct 25, Aleksey Midenkov wrote:
> > > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d)
> > > parent(s): 1153950ad0a
> > > author: Aleksey Midenkov 
> > > committer: Aleksey Midenkov 
> > > timestamp: 2019-07-22 15:40:06 +0300
> > > message:
> > >
> > > MDEV-20015 Assertion `!in_use->is_error()' failed in 
> > > TABLE::update_virtual_field
> > >
> > > Preserve and restore statement DA.
> >
> > This is strange. Diagnostics areas aren't supposed to be temporarily
> > created on the frame, they aren't arenas.
> 
> There are already several samples of this pattern in the code. This is the
> main (if not the only) usage of set_stmt_da().

One in sql_prepare.cc, mysql_stmt_get_longdata().
No idea why, it happened during the merge. The first patch used the
error handler, which is the supposed way of doing this kind of things.
Then during the merge it was suddenly replaced with this.

Another one in sql_prepare.cc: Ed_connection::execute_direct(). It's
dead code, doesn't matter.

Yet another in sql_partition.cc, never mind that, partitioning is a huge
mess, never take any ideas from it.

And the last one in sql_get_diagnostics.cc, which is the way diagnostic
areas are supposed to be used. That one is correct.

> > Why TABLE::update_virtual_field() is called at all if there's already
> > an error?
> 
> update_virtual_field() is called as part of  REPAIR (MDEV-5800
> ) which is done on bulk insert
> finish. I doubt it should consider error status in this case as no matter
> how SQL command is finished it must update the index.
> 
> #0  TABLE::update_virtual_field (this=0x7f85b4068388, vf=0x7f85b4066948) at 
> /home/midenok/src/mariadb/10.2/src/sql/table.cc:7685
> #1  0x01105d2b in compute_vcols (info=0x7f85b406a5d8, 
> record=0x7f85b4006948 "\377\001", keynum=1) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:683
> #2  0x011142bd in sort_get_next_record (sort_param=0x7f860c05edd8) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3657
> #3  0x01118a33 in sort_key_read (sort_param=0x7f860c05edd8, 
> key=0x7f85b4039808) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3121
> #4  0x011650a9 in find_all_keys (info=0x7f860c05edd8, keys=2, 
> sort_keys=0x7f85b40397f8, buffpek=0x7f860c05ea20, maxbuffer=0x7f860c05ea54, 
> tempfile=0x7f860c05e898, tempfile_for_exceptions=0x7f860c05e728) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:312
> #5  0x01164b8d in _create_index_by_sort (info=0x7f860c05edd8, 
> no_messages=1 '\001', sortbuff_size=134216704) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:228
> #6  0x0111763b in mi_repair_by_sort (param=0x7f85b406e300, 
> info=0x7f85b406a5d8, name=0x7f860c05f7c0 "./test/t2", rep_quick=1) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:2401
> #7  0x01107097 in ha_myisam::repair (this=0x7f85b4068f20, 
> thd=0x7f85b4000cf8, param=..., do_optimize=false) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1271
> #8  0x011085d3 in ha_myisam::enable_indexes (this=0x7f85b4068f20, 
> mode=2) at /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1610
> #9  0x01108db2 in ha_myisam::end_bulk_insert (this=0x7f85b4068f20) at 
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1760
> #10 0x006d0d23 in handler::ha_end_bulk_insert (this=0x7f85b4068f20) 
> at /home/midenok/src/mariadb/10.2/src/sql/handler.h:2918
> #11 0x006cd120 in select_insert::abort_result_set 
> (this=0x7f85b4012818) at 
> /home/midenok/src/mariadb/10.2/src/sql/sql_insert.cc:3959
> #12 0x007335a7 in handle_select (thd=0x7f85b4000cf8, 
> lex=0x7f85b4004820, result=0x7f85b4012818, 
> setup_tables_done_option=1073741824) at 
> /home/midenok/src/mariadb/10.2/src/sql/sql_select.cc:383
> #13 0x006eedad in mysql_execute_command (thd=0x7f85b4000cf8) at 
> /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:4284
> #14 0x006e8880 in mysql_parse (thd=0x7f85b4000cf8, 
> rawbuf=0x7f85b40117f0 "insert into t2 (pk) select a from t1", length=36, 
> parser_state=0x7f860c0625f0, is_com_multi=false, is_next_command=false) at 
> /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:7760

I see. You're right, we have to update vcols even if thd->is_error().
It used to be a problem, indeed, as Item::fix_length_and_dec() was void,
and the caller had to check thd->is_error().

But Sanja has fixed it about a year ago. So ideally it would be much
better if we could just update vcols under thd->is_error() == true.
Did you try that? Simply removing the assert, I mean.
If it doesn't work, is it difficult to fix?

If it doesn't work and is difficult to fix, then, I agree, your fix with
a temporary Diagnostics_area is what we should use. But, 

Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field

[Aleksey Midenkov]

Hi Sergei,

On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik  wrote:

> Hi, Aleksey!
>
> On Oct 25, Aleksey Midenkov wrote:
> > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d)
> > parent(s): 1153950ad0a
> > author: Aleksey Midenkov 
> > committer: Aleksey Midenkov 
> > timestamp: 2019-07-22 15:40:06 +0300
> > message:
> >
> > MDEV-20015 Assertion `!in_use->is_error()' failed in
> TABLE::update_virtual_field
> >
> > Preserve and restore statement DA.
>
> This is strange. Diagnostics areas aren't supposed to be temporarily
> created on the frame, they aren't arenas.
>

There are already several samples of this pattern in the code. This is the
main (if not the only) usage of set_stmt_da().


>
> Why TABLE::update_virtual_field() is called at all if there's already
> an error?
>

update_virtual_field() is called as part of  REPAIR (MDEV-5800
) which is done on bulk insert
finish. I doubt it should consider error status in this case as no matter
how SQL command is finished it must update the index.

#0  TABLE::update_virtual_field (this=0x7f85b4068388,
vf=0x7f85b4066948) at
/home/midenok/src/mariadb/10.2/src/sql/table.cc:7685
#1  0x01105d2b in compute_vcols (info=0x7f85b406a5d8,
record=0x7f85b4006948 "\377\001", keynum=1) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:683
#2  0x011142bd in sort_get_next_record
(sort_param=0x7f860c05edd8) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3657
#3  0x01118a33 in sort_key_read (sort_param=0x7f860c05edd8,
key=0x7f85b4039808) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3121
#4  0x011650a9 in find_all_keys (info=0x7f860c05edd8, keys=2,
sort_keys=0x7f85b40397f8, buffpek=0x7f860c05ea20,
maxbuffer=0x7f860c05ea54, tempfile=0x7f860c05e898,
tempfile_for_exceptions=0x7f860c05e728) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:312
#5  0x01164b8d in _create_index_by_sort (info=0x7f860c05edd8,
no_messages=1 '\001', sortbuff_size=134216704) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/sort.c:228
#6  0x0111763b in mi_repair_by_sort (param=0x7f85b406e300,
info=0x7f85b406a5d8, name=0x7f860c05f7c0 "./test/t2", rep_quick=1) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:2401
#7  0x01107097 in ha_myisam::repair (this=0x7f85b4068f20,
thd=0x7f85b4000cf8, param=..., do_optimize=false) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1271
#8  0x011085d3 in ha_myisam::enable_indexes
(this=0x7f85b4068f20, mode=2) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1610
#9  0x01108db2 in ha_myisam::end_bulk_insert
(this=0x7f85b4068f20) at
/home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1760
#10 0x006d0d23 in handler::ha_end_bulk_insert
(this=0x7f85b4068f20) at
/home/midenok/src/mariadb/10.2/src/sql/handler.h:2918
#11 0x006cd120 in select_insert::abort_result_set
(this=0x7f85b4012818) at
/home/midenok/src/mariadb/10.2/src/sql/sql_insert.cc:3959
#12 0x007335a7 in handle_select (thd=0x7f85b4000cf8,
lex=0x7f85b4004820, result=0x7f85b4012818,
setup_tables_done_option=1073741824) at
/home/midenok/src/mariadb/10.2/src/sql/sql_select.cc:383
#13 0x006eedad in mysql_execute_command (thd=0x7f85b4000cf8)
at /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:4284
#14 0x006e8880 in mysql_parse (thd=0x7f85b4000cf8,
rawbuf=0x7f85b40117f0 "insert into t2 (pk) select a from t1",
length=36, parser_state=0x7f860c0625f0, is_com_multi=false,
is_next_command=false) at
/home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:7760



>
> > diff --git a/sql/table.cc b/sql/table.cc
> > index f5b5bad99cc..65611d78bde 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -7682,15 +7682,25 @@ int TABLE::update_virtual_fields(handler *h,
> enum_vcol_update_mode update_mode)
> >
> >  int TABLE::update_virtual_field(Field *vf)
> >  {
> > -  DBUG_ASSERT(!in_use->is_error());
> > +  Diagnostics_area *stmt_da= NULL;
> > +  Diagnostics_area tmp_stmt_da(in_use->query_id, false, true);
> > +  bool error;
> >Query_arena backup_arena;
> >DBUG_ENTER("TABLE::update_virtual_field");
> > +  if (unlikely(in_use->is_error()))
> > +  {
> > +stmt_da= in_use->get_stmt_da();
> > +in_use->set_stmt_da(_stmt_da);
> > +  }
> >in_use->set_n_backup_active_arena(expr_arena, _arena);
> >bitmap_clear_all(_set);
> >vf->vcol_info->expr->walk(::update_vcol_processor, 0, _set);
> >vf->vcol_info->expr->save_in_field(vf, 0);
> >in_use->restore_active_arena(expr_arena, _arena);
> > -  DBUG_RETURN(in_use->is_error());
> > +  error= in_use->is_error();
> > +  if (stmt_da)
> > +in_use->set_stmt_da(stmt_da);
> > +  DBUG_RETURN(error);
> >  }
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
>


-- 
All the best,

Aleksey Midenkov
@midenok
___

Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field

[Sergei Golubchik]

Hi, Aleksey!

On Oct 25, Aleksey Midenkov wrote:
> revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d)
> parent(s): 1153950ad0a
> author: Aleksey Midenkov 
> committer: Aleksey Midenkov 
> timestamp: 2019-07-22 15:40:06 +0300
> message:
> 
> MDEV-20015 Assertion `!in_use->is_error()' failed in 
> TABLE::update_virtual_field
> 
> Preserve and restore statement DA.

This is strange. Diagnostics areas aren't supposed to be temporarily
created on the frame, they aren't arenas.

Why TABLE::update_virtual_field() is called at all if there's already
an error?

> diff --git a/sql/table.cc b/sql/table.cc
> index f5b5bad99cc..65611d78bde 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -7682,15 +7682,25 @@ int TABLE::update_virtual_fields(handler *h, 
> enum_vcol_update_mode update_mode)
>  
>  int TABLE::update_virtual_field(Field *vf)
>  {
> -  DBUG_ASSERT(!in_use->is_error());
> +  Diagnostics_area *stmt_da= NULL;
> +  Diagnostics_area tmp_stmt_da(in_use->query_id, false, true);
> +  bool error;
>Query_arena backup_arena;
>DBUG_ENTER("TABLE::update_virtual_field");
> +  if (unlikely(in_use->is_error()))
> +  {
> +stmt_da= in_use->get_stmt_da();
> +in_use->set_stmt_da(_stmt_da);
> +  }
>in_use->set_n_backup_active_arena(expr_arena, _arena);
>bitmap_clear_all(_set);
>vf->vcol_info->expr->walk(::update_vcol_processor, 0, _set);
>vf->vcol_info->expr->save_in_field(vf, 0);
>in_use->restore_active_arena(expr_arena, _arena);
> -  DBUG_RETURN(in_use->is_error());
> +  error= in_use->is_error();
> +  if (stmt_da)
> +in_use->set_stmt_da(stmt_da);
> +  DBUG_RETURN(error);
>  }

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