maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Sergei,

On Thu, Oct 31, 2019 at 2:03 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Aleksey!
>
> On Oct 30, Aleksey Midenkov wrote:
> > On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik <serg@xxxxxxxxxxx>
> wrote:
> > > On Oct 25, Aleksey Midenkov wrote:
> > > > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d)
> > > > parent(s): 1153950ad0a
> > > > author: Aleksey Midenkov <midenok@xxxxxxxxx>
> > > > committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> > > > 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
> > <https://jira.mariadb.org/browse/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  0x0000000001105d2b 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  0x00000000011142bd in sort_get_next_record
> (sort_param=0x7f860c05edd8) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3657
> > #3  0x0000000001118a33 in sort_key_read (sort_param=0x7f860c05edd8,
> key=0x7f85b4039808) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/mi_check.c:3121
> > #4  0x00000000011650a9 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  0x0000000001164b8d 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  0x000000000111763b 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  0x0000000001107097 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  0x00000000011085d3 in ha_myisam::enable_indexes
> (this=0x7f85b4068f20, mode=2) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1610
> > #9  0x0000000001108db2 in ha_myisam::end_bulk_insert
> (this=0x7f85b4068f20) at
> /home/midenok/src/mariadb/10.2/src/storage/myisam/ha_myisam.cc:1760
> > #10 0x00000000006d0d23 in handler::ha_end_bulk_insert
> (this=0x7f85b4068f20) at
> /home/midenok/src/mariadb/10.2/src/sql/handler.h:2918
> > #11 0x00000000006cd120 in select_insert::abort_result_set
> (this=0x7f85b4012818) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_insert.cc:3959
> > #12 0x00000000007335a7 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 0x00000000006eedad in mysql_execute_command (thd=0x7f85b4000cf8) at
> /home/midenok/src/mariadb/10.2/src/sql/sql_parse.cc:4284
> > #14 0x00000000006e8880 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?
>

I put this assertion as part of MDEV-16222 because
TABLE::update_virtual_field()
returns in_use->is_error(). I believe we discussed this in that ticket. The
idea was: wrongly mixed semantics of error status
before update_virtual_field() and the status returned
by update_virtual_field(). The former can falsely influence the latter.


>
> 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, please, add a
> test case where virtual column expression generates an error - this
> error will end up in your temporary Diagnostics_area, let's see what
> happens then.
>

Probably it will be lost as compute_vcols() doesn't check return status
of update_virtual_field():

  for (; kp < end; kp++)
  {
    Field *f= table->field[kp->fieldnr - 1];
    if (f->vcol_info)
      table->update_virtual_field(f);
  }


>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>


-- 
All the best,

Aleksey Midenkov
@midenok