Re: [Maria-developers] 031efde365c: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
Hi, Oleksandr! On Nov 07, Oleksandr Byelkin wrote:
revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2018-11-07 14:29:47 +0100 message:
MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
- clean up DEFAULT() to work only with default value and correctly print itself. - fix of DBUG_ASSERT about fields read/write - fix of field marking for write based really on the thd->mark_used_columns flag
diff --git a/sql/field.cc b/sql/field.cc index caa84dc9932..6cd8940a893 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -70,8 +70,21 @@ const char field_separator=','; #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1))
-#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) +// Column marked for read or the field set to read out or record[0] or [1] +#define ASSERT_COLUMN_MARKED_FOR_READ \ + DBUG_ASSERT(!table || \ + (!table->read_set || \ + bitmap_is_set(table->read_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength)))) + +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ + DBUG_ASSERT(is_stat_field || !table || \ + (!table->write_set || \ + bitmap_is_set(table->write_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength))) || \ + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))
Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ.
#define FLAGSTR(S,F) ((S) & (F) ? #F " " : "")
diff --git a/sql/item.cc b/sql/item.cc index 2adec33491b..6828a74f9ff 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -8813,15 +8824,19 @@ bool Item_default_value::fix_fields(THD *thd, Item **items) goto error; memcpy((void *)def_field, (void *)field_arg->field, field_arg->field->size_of()); - IF_DBUG(def_field->is_stat_field=1,); // a hack to fool ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED + // If non-constant default value expression if (def_field->default_value && def_field->default_value->flags) { uchar *newptr= (uchar*) thd->alloc(1+def_field->pack_length()); if (!newptr) goto error; + /* + Even if DEFAULT() do not read tables fields, the default value + expression can do it. + */ fix_session_vcol_expr_for_read(thd, def_field, def_field->default_value); if (thd->mark_used_columns != MARK_COLUMNS_NONE) - def_field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0); + def_field->default_value->expr->update_used_tables();
what's the difference?
def_field->move_field(newptr+1, def_field->maybe_null() ? newptr : 0, 1); } else diff --git a/sql/item.h b/sql/item.h index 8d02d981d38..5866f328f38 100644 --- a/sql/item.h +++ b/sql/item.h @@ -5229,6 +5229,11 @@ class Item_default_value : public Item_field return false; } table_map used_tables() const; + virtual void update_used_tables() + { + if (field && field->default_value) + field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0);
why not field->default_value->expr->update_used_tables() ?
+ } Field *get_tmp_table_field() { return 0; } Item *get_tmp_table_item(THD *thd) { return this; } Item_field *field_for_view_update() { return 0; } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index c282db42fdd..0deb5ec1362 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -5737,7 +5737,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, TABLE *table= field_to_set->table; if (thd->mark_used_columns == MARK_COLUMNS_READ) bitmap_set_bit(table->read_set, field_to_set->field_index); - else + else if (thd->mark_used_columns == MARK_COLUMNS_WRITE) bitmap_set_bit(table->write_set, field_to_set->field_index);
what does it affect?
} }
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi! Am 12.11.18 um 13:24 schrieb Sergei Golubchik:
Hi, Oleksandr!
On Nov 07, Oleksandr Byelkin wrote:
revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2018-11-07 14:29:47 +0100 message:
MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
- clean up DEFAULT() to work only with default value and correctly print itself. - fix of DBUG_ASSERT about fields read/write - fix of field marking for write based really on the thd->mark_used_columns flag
diff --git a/sql/field.cc b/sql/field.cc index caa84dc9932..6cd8940a893 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -70,8 +70,21 @@ const char field_separator=','; #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1))
-#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) +// Column marked for read or the field set to read out or record[0] or [1] +#define ASSERT_COLUMN_MARKED_FOR_READ \ + DBUG_ASSERT(!table || \ + (!table->read_set || \ + bitmap_is_set(table->read_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength)))) + +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ + DBUG_ASSERT(is_stat_field || !table || \ + (!table->write_set || \ + bitmap_is_set(table->write_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength))) || \ + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index))) Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ.
Appeared that yes, it is needed, because set_default uses save_in_field: #3 0x00007ffff5b43412 in __GI___assert_fail (assertion=0x55555663d230 "is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))", file=0x55555663ceb0 "sql/field.cc", line=4205, function=0x55555663faa0 <Field_long::store(long long, bool)::__PRETTY_FUNCTION__> "virtual int Field_long::store(longlong, bool)") at assert.c:101 #4 0x0000555555d5629a in Field_long::store (this=0x7fffd4013888, nr=6, unsigned_val=false) at sql/field.cc:4205 #5 0x0000555555da2939 in Item::save_in_field (this=0x7fffd4107ba8, field=0x7fffd4013888, no_conversions=false) at sql/item.cc:6386 #6 0x0000555555d4f44f in Field::set_default (this=0x7fffd4013888) at sql/field.cc:2351 #7 0x0000555555da9f3e in Item_default_value::calculate (this=0x7fffd4012858) at sql/item.cc:8876 [skip]
Hi, Oleksandr! On Nov 13, Oleksandr Byelkin wrote:
Am 12.11.18 um 13:24 schrieb Sergei Golubchik:
On Nov 07, Oleksandr Byelkin wrote:
revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2018-11-07 14:29:47 +0100 message:
MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
- clean up DEFAULT() to work only with default value and correctly print itself. - fix of DBUG_ASSERT about fields read/write - fix of field marking for write based really on the thd->mark_used_columns flag
diff --git a/sql/field.cc b/sql/field.cc index caa84dc9932..6cd8940a893 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -70,8 +70,21 @@ const char field_separator=','; #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1))
-#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) +// Column marked for read or the field set to read out or record[0] or [1] +#define ASSERT_COLUMN_MARKED_FOR_READ \ + DBUG_ASSERT(!table || \ + (!table->read_set || \ + bitmap_is_set(table->read_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength)))) + +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ + DBUG_ASSERT(is_stat_field || !table || \ + (!table->write_set || \ + bitmap_is_set(table->write_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength))) || \ + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index))) Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ.
Appeared that yes, it is needed, because set_default uses save_in_field:
#3 0x00007ffff5b43412 in __GI___assert_fail (assertion=0x55555663d230 "is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))", file=0x55555663ceb0 "sql/field.cc", line=4205, function=0x55555663faa0 <Field_long::store(long long, bool)::__PRETTY_FUNCTION__> "virtual int Field_long::store(longlong, bool)") at assert.c:101 #4 0x0000555555d5629a in Field_long::store (this=0x7fffd4013888, nr=6, unsigned_val=false) at sql/field.cc:4205 #5 0x0000555555da2939 in Item::save_in_field (this=0x7fffd4107ba8, field=0x7fffd4013888, no_conversions=false) at sql/item.cc:6386 #6 0x0000555555d4f44f in Field::set_default (this=0x7fffd4013888) at sql/field.cc:2351 #7 0x0000555555da9f3e in Item_default_value::calculate (this=0x7fffd4012858) at sql/item.cc:8876
I see. What statement have caused it? Did it start crashing only after your patch? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Am 13.11.18 um 11:07 schrieb Sergei Golubchik:
Hi, Oleksandr!
On Nov 13, Oleksandr Byelkin wrote:
Am 12.11.18 um 13:24 schrieb Sergei Golubchik:
On Nov 07, Oleksandr Byelkin wrote:
revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2018-11-07 14:29:47 +0100 message:
MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
- clean up DEFAULT() to work only with default value and correctly print itself. - fix of DBUG_ASSERT about fields read/write - fix of field marking for write based really on the thd->mark_used_columns flag
diff --git a/sql/field.cc b/sql/field.cc index caa84dc9932..6cd8940a893 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -70,8 +70,21 @@ const char field_separator=','; #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1))
-#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) +// Column marked for read or the field set to read out or record[0] or [1] +#define ASSERT_COLUMN_MARKED_FOR_READ \ + DBUG_ASSERT(!table || \ + (!table->read_set || \ + bitmap_is_set(table->read_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength)))) + +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ + DBUG_ASSERT(is_stat_field || !table || \ + (!table->write_set || \ + bitmap_is_set(table->write_set, field_index) || \ + (!(ptr >= table->record[0] && \ + ptr < table->record[0] + table->s->reclength))) || \ + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index))) Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ.
Appeared that yes, it is needed, because set_default uses save_in_field:
#3 0x00007ffff5b43412 in __GI___assert_fail (assertion=0x55555663d230 "is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))", file=0x55555663ceb0 "sql/field.cc", line=4205, function=0x55555663faa0 <Field_long::store(long long, bool)::__PRETTY_FUNCTION__> "virtual int Field_long::store(longlong, bool)") at assert.c:101 #4 0x0000555555d5629a in Field_long::store (this=0x7fffd4013888, nr=6, unsigned_val=false) at sql/field.cc:4205 #5 0x0000555555da2939 in Item::save_in_field (this=0x7fffd4107ba8, field=0x7fffd4013888, no_conversions=false) at sql/item.cc:6386 #6 0x0000555555d4f44f in Field::set_default (this=0x7fffd4013888) at sql/field.cc:2351 #7 0x0000555555da9f3e in Item_default_value::calculate (this=0x7fffd4012858) at sql/item.cc:8876 I see.
What statement have caused it?
select default(a),b from t1; assert/assert.c:89(__assert_fail_base)[0x7f260f59139a] /lib/x86_64-linux-gnu/libc.so.6(+0x30412)[0x7f260f591412] sql/field.cc:4206(Field_long::store(long long, bool))[0x563ded3fe29a] sql/item.cc:6386(Item::save_in_field(Field*, bool))[0x563ded44a939] sql/field.cc:2351(Field::set_default())[0x563ded3f744f] sql/item.cc:8877(Item_default_value::calculate())[0x563ded451f3e] sql/item.cc:8913(Item_default_value::send(Protocol*, String*))[0x563ded452096] sql/protocol.cc:979(Protocol::send_result_set_row(List<Item>*))[0x563ded0e775d] sql/sql_class.cc:2708(select_send::send_data(List<Item>&))[0x563ded168bc6] sql/sql_select.cc:19918(end_send(JOIN*, st_join_table*, bool))[0x563ded222252] sql/sql_select.cc:18970(evaluate_join_record(JOIN*, st_join_table*, int))[0x563ded21fd15] sql/sql_select.cc:18750(sub_select(JOIN*, st_join_table*, bool))[0x563ded21f601] sql/sql_select.cc:18294(do_select(JOIN*, Procedure*))[0x563ded21eb88] sql/sql_select.cc:3609(JOIN::exec_inner())[0x563ded1f967c] sql/sql_select.cc:3405(JOIN::exec())[0x563ded1f8b28] sql/sql_select.cc:3806(mysql_select(THD*, TABLE_LIST*, unsigned int, List<Item>&, Item*, unsigned int, st_order*, st_order*, Item*, st_order*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*))[0x563ded1f9cee] sql/sql_select.cc:364(handle_select(THD*, LEX*, select_result*, unsigned long))[0x563ded1ee013] sql/sql_parse.cc:6478(execute_sqlcom_select(THD*, TABLE_LIST*))[0x563ded1b9814]
Did it start crashing only after your patch? Of course (before it was marked for default() (which actually do not need table to be read/write)).
Regards, Sergei Chief Architect MariaDB and security@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
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik