Hi, Oleksandr! See questions below. There are few changes that I'm not quite sure about.
On Aug 08, Oleksandr Byelkin wrote: > revision-id: 6df4e4a855fe223988c12681f8caec6c49b2f629 > (mariadb-10.2.16-66-g6df4e4a855f) > parent(s): 4ddcb4eb46c62cf459936554d43351db740ba14d > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-08-08 12:28:15 +0200 > 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 firld marking for write based really on the thd->mark_used_columns > flag > > --- > mysql-test/r/func_default.result | 2 +- > mysql-test/r/func_time.result | 28 ++++++++++++++++++++++++++++ > mysql-test/t/func_time.test | 21 +++++++++++++++++++++ > sql/field.cc | 21 +++++++++++++++++++-- > sql/field.h | 1 + > sql/item.cc | 25 ++++++++++++++++++++++++- > sql/item.h | 5 +++++ > sql/sql_base.cc | 2 +- > 8 files changed, 100 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/r/func_default.result > b/mysql-test/r/func_default.result > index 535be10da86..1f331af287c 100644 > --- a/mysql-test/r/func_default.result > +++ b/mysql-test/r/func_default.result > @@ -8,7 +8,7 @@ explain extended select default(str), default(strnull), > default(intg), default(r > id select_type table type possible_keys key key_len ref > rows filtered Extra > 1 SIMPLE t1 system NULL NULL NULL NULL 1 100.00 > Warnings: > -Note 1003 select default('') AS `default(str)`,default('') AS > `default(strnull)`,default(0) AS `default(intg)`,default(0) AS `default(rel)` > from dual > +Note 1003 select default(`str`) AS `default(str)`,default(`strnull`) AS > `default(strnull)`,default(`intg`) AS `default(intg)`,default(`rel`) AS > `default(rel)` from dual This looks rather fishy. There can be no columns str and strnull in dual. Please add a test case with a view, to make sure you only get this effect (column names and dual) in explain extended (where it doesn't matter), but not in a view (where it does). > select * from t1 where str <> default(str); > str strnull intg rel > 0 0 > diff --git a/mysql-test/r/func_time.result b/mysql-test/r/func_time.result > index 55c2c93eb56..51f51820efc 100644 > --- a/mysql-test/r/func_time.result > +++ b/mysql-test/r/func_time.result > @@ -3260,3 +3260,31 @@ DROP TABLE t1,t2; > # > # End of 10.1 tests > # > +# > +# MDEV-16217: Assertion `!table || (!table->read_set || > +# bitmap_is_set(table->read_set, field_index))' > +# failed in Field_num::get_date > +# > +CREATE TABLE t1 (pk int default 0, a1 date); > +INSERT INTO t1 VALUES (1,'1900-01-01'),(2,NULL),(3,NULL),(4,NULL); > +CREATE VIEW v1 AS > +SELECT t1.pk AS pk, t1.a1 AS a1 FROM t1; > +SELECT a1 BETWEEN (('2018-08-24')) AND (DEFAULT(pk)) FROM v1; > +a1 BETWEEN (('2018-08-24')) AND (DEFAULT(pk)) > +0 > +NULL > +NULL > +NULL > +SELECT a1 BETWEEN (('2018-08-24')) AND (~ DEFAULT(pk)) FROM v1; > +a1 BETWEEN (('2018-08-24')) AND (~ DEFAULT(pk)) > +0 > +NULL > +NULL > +NULL > +Warnings: > +Warning 1292 Incorrect datetime value: '18446744073709551615' > +DROP VIEW v1; > +DROP TABLE t1; > +# > +# End of 10.2 tests > +# > diff --git a/mysql-test/t/func_time.test b/mysql-test/t/func_time.test > index 3503b6d5bc6..c8859feee93 100644 > --- a/mysql-test/t/func_time.test > +++ b/mysql-test/t/func_time.test > @@ -1857,3 +1857,24 @@ DROP TABLE t1,t2; > --echo # > --echo # End of 10.1 tests > --echo # > + > +--echo # > +--echo # MDEV-16217: Assertion `!table || (!table->read_set || > +--echo # bitmap_is_set(table->read_set, field_index))' > +--echo # failed in Field_num::get_date > +--echo # > +CREATE TABLE t1 (pk int default 0, a1 date); > +INSERT INTO t1 VALUES (1,'1900-01-01'),(2,NULL),(3,NULL),(4,NULL); > + > +CREATE VIEW v1 AS > +SELECT t1.pk AS pk, t1.a1 AS a1 FROM t1; > + > +SELECT a1 BETWEEN (('2018-08-24')) AND (DEFAULT(pk)) FROM v1; > +SELECT a1 BETWEEN (('2018-08-24')) AND (~ DEFAULT(pk)) FROM v1; > + > +DROP VIEW v1; > +DROP TABLE t1; > + > +--echo # > +--echo # End of 10.2 tests > +--echo # > diff --git a/sql/field.cc b/sql/field.cc > index 9ca9663f066..e12adbf47b3 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -70,8 +70,25 @@ 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) && \ > + !(ptr >= table->record[1] && \ > + ptr < table->record[1] + table->s->reclength)))) why did you add record[1] to the assert? I think, having just record[0] would've been enough > + > +#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) && \ > + !(ptr >= table->record[1] && \ > + ptr < table->record[1] + table->s->reclength))) || \ > + (table->vcol_set && bitmap_is_set(table->vcol_set, > field_index))) > > #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "") > > diff --git a/sql/field.h b/sql/field.h > index 22c276478b6..55c3ed4c4bd 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -630,6 +630,7 @@ class Virtual_column_info: public Sql_alloc > bool utf8; /* Already in utf8 */ > Item *expr; > LEX_STRING name; /* Name of constraint */ > + /* see VCOL_* (VCOL_FIELD_REF, ...) */ > uint flags; > > Virtual_column_info() > diff --git a/sql/item.cc b/sql/item.cc > index 58d2b7dbfc0..91fe7ff03aa 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -8917,8 +8917,19 @@ bool Item_default_value::fix_fields(THD *thd, Item > **items) > fixed= 1; > return FALSE; > } > + > + /* > + DEFAULT() do not need table field so should not ask handler to bring > + field value (mark column for read) > + */ > + enum_mark_columns save_mark_used_columns= thd->mark_used_columns; > + thd->mark_used_columns= MARK_COLUMNS_NONE; > if (!arg->fixed && arg->fix_fields(thd, &arg)) > + { > + thd->mark_used_columns= save_mark_used_columns; > goto error; > + } > + thd->mark_used_columns= save_mark_used_columns; Hmm, I thought that the field wasn't marked in read_set, and that failed the assert. Now you're saying that it was? > real_arg= arg->real_item(); > @@ -8938,12 +8949,16 @@ 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 ah, good. thanks! > + // 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); better call update_used_tables() from here. > @@ -8970,6 +8985,14 @@ void Item_default_value::print(String *str, > enum_query_type query_type) > return; > } > str->append(STRING_WITH_LEN("default(")); > + /* > + We take DEFAULT from a field so do not need it value in case of const > + tables but its name so we set QT_NO_DATA_EXPANSION (as we print for > + table definition, also we do not need table and database name) > + */ > + query_type= (enum_query_type) (query_type | QT_NO_DATA_EXPANSION | > + QT_ITEM_IDENT_SKIP_DB_NAMES | > + QT_ITEM_IDENT_SKIP_TABLE_NAMES); in a view definition you might need table and database names. For example, a view that joins two tables from different databases, and they both have a column named `a`. And the query uses DEFAULT(db1.t1.a). > arg->print(str, query_type); > str->append(')'); > } > diff --git a/sql/item.h b/sql/item.h > index 8fad8dadf22..d252a256f76 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5316,6 +5316,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); > + } > Field *get_tmp_table_field() { return 0; } > Item *get_tmp_table_item(THD *thd) { return this; } > Item_field *field_for_view_update() { return 0; } Regards, Sergei Chief Architect MariaDB and secur...@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