Sergei, On Thu, Oct 31, 2019 at 2:03 PM Sergei Golubchik <s...@mariadb.org> wrote:
> Hi, Aleksey! > > On Oct 30, Aleksey Midenkov wrote: > > On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik <s...@mariadb.org> > wrote: > > > On Oct 25, Aleksey Midenkov wrote: > > > > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) > > > > parent(s): 1153950ad0a > > > > author: Aleksey Midenkov <mide...@gmail.com> > > > > committer: Aleksey Midenkov <mide...@gmail.com> > > > > 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 secur...@mariadb.org > -- All the best, Aleksey Midenkov @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
