maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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