Hi, Sergei! Am 09.08.2018 um 12:34 schrieb Sergei Golubchik:
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. it is how table elimination work, table is eliminated (table is read) but default values is taking from it.
Also previous output was also fishy (if not more) what is default('') ? maybe putting there full defined field (with table/database (as discussed later) will helps to make it a bit more clear).
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).
OK
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? originaly it was set (that is why all worked with tables), but as soon as we move some tables around (view merge) and try recalculate used
just for safety, I think record 1 still can be used for reading/writing values during update (old/new values) tables which actually also reset read_set: 1. clear all bit-mask 2. call update_used_tabes() for all expressions (and that call for Item_field set the bit back). The cache did not do it.
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.
OK, but actually it should be more or less the same
@@ -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).
OK, I will test it additionally. Also I found that QT_NO_DATA_EXPANSION prevent expansion of '?' on prepare, i fixed this in my other patch about similar problem (on Igor's review now) so I will fix it there. (do not add QT_NO_DATA_EXPANSION but special mode for view print which prevent constant substitution in the view, actually it is not a big problem there is not constant optimization during CRETE VIEW, but bigger problem is that SHOW CREATE VIEW shows not the same what is written in .frm).
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 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