maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 security@xxxxxxxxxxx