← Back to team overview

maria-developers team mailing list archive

Re: 6df4e4a855f: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date

 

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
just for safety, I think record 1 still can be used for reading/writing values during update (old/new values)
+
+#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 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@xxxxxxxxxxx

_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp




Follow ups

References