Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field
Hi, Aleksey! On Oct 25, Aleksey Midenkov wrote:
revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) parent(s): 1153950ad0a author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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. 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(&tmp_stmt_da); + } in_use->set_n_backup_active_arena(expr_arena, &backup_arena); bitmap_clear_all(&tmp_set); vf->vcol_info->expr->walk(&Item::update_vcol_processor, 0, &tmp_set); vf->vcol_info->expr->save_in_field(vf, 0); in_use->restore_active_arena(expr_arena, &backup_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 security@mariadb.org
Hi Sergei, On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Oct 25, Aleksey Midenkov wrote:
revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) parent(s): 1153950ad0a author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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().
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
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(&tmp_stmt_da); + } in_use->set_n_backup_active_arena(expr_arena, &backup_arena); bitmap_clear_all(&tmp_set); vf->vcol_info->expr->walk(&Item::update_vcol_processor, 0, &tmp_set); vf->vcol_info->expr->save_in_field(vf, 0); in_use->restore_active_arena(expr_arena, &backup_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 security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Oct 30, Aleksey Midenkov wrote:
On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 25, Aleksey Midenkov wrote:
revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) parent(s): 1153950ad0a author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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? 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@mariadb.org
Sergei, On Thu, Oct 31, 2019 at 2:03 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Fri, Oct 25, 2019 at 9:44 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Oct 25, Aleksey Midenkov wrote:
revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) parent(s): 1153950ad0a author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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
On Oct 30, Aleksey Midenkov wrote: 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@mariadb.org
-- All the best, Aleksey Midenkov @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik