maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Aug 09, Oleksandr Byelkin wrote:
> >> 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.

Sure, but the result is not a valid statement. What if you do

 create view v1 as select default(str), default(strnull) from t1;

will this work?

> Also previous output was also fishy (if not more) what is default('') ?

May be default(constant) simply returns this constant?
Anyway, just try the view as above and make sure that it works...

> >> 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)

Yes, but first the row is read into record[0] and then copied into record[1].
So, I think, it's always enough to check only record[0] access.

> >> +  // 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

I hope it'll be exactly the same :)

The point is to have this default_value->expr->walk(&Item::register_field_in_read_map)
logic only in one place (in update_used_tables), without changing the behavior.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx