maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Nov 12, Oleksandr Byelkin wrote:
> > 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.
> 
>  I'd prefer to have them symmetric

Right, but these macros already have non-symmetric names and
non-symmetric definition - the second checks for vcol_set.

The way you did it only makes the reader to pause and think in what case
one can get `ptr` outside of table->record[0] for writes.

Because your assert says "yes, it's ok to write into the column if ptr
is outside of table->record[0]". If ptr cannot possibly be outside of
record[0] for write, then it shouldn't be in the assert.

> > >  #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "")
> > >
> > > 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?
> >
> 
> enum enum_mark_columns
> { MARK_COLUMNS_NONE, MARK_COLUMNS_READ, MARK_COLUMNS_WRITE};
> 
>  MARK_COLUMNS_NONE really used in the code, I remember that something was
> marked incorrectly (it is how I found it) but what it is too late to ask
> after many month as fix is done.

this code is inside of

  if (thd->mark_used_columns != MARK_COLUMNS_NONE)

so I don't see what your change could possibly do.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx