maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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?

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.
 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx