maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Nov 15, Nikita Malyavin wrote:
> revision-id: 6ac19d09c66 (mariadb-10.4.4-280-g6ac19d09c66)
> parent(s): 67ddb6507d5
> author: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> timestamp: 2019-08-20 17:56:07 +1000
> message:
> 
> MDEV-16978 Application-time periods: WITHOUT OVERLAPS
> 
> * The overlaps check is implemented on a handler level per row
> command. It creates a separate cursor (actually, another handler
> instance) and caches it inside the original handler, when
> ha_update_row or ha_insert_row is issued. Cursor closes on unlocking
> the handler.
> 
> * Containing the same key in index means unique constraint violation
> even in usual terms. So we fetch left and right neighbours and check
> that they have same key prefix, excluding from the key only the period
> part. If it doesnt match, then there's no such neighbour, and the
> check passes. Otherwise, we check if this neighbour intersects with
> the considered key.
> 
> * the check does introduce new error and fails with ER_DUPP_KEY error.
> This might break REPLACE workflow and should be fixed separately
> 
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 94cffd69b75..560f6316602 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -6913,6 +6928,130 @@ void handler::set_lock_type(enum thr_lock_type lock)
>    table->reginfo.lock_type= lock;
>  }
>  
> +int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data)
> +{
> +  DBUG_ASSERT(new_data);
> +  if (!table_share->period.unique_keys)
> +    return 0;
> +  if (table->versioned() && !table->vers_end_field()->is_max())
> +    return 0;
> +
> +  bool is_update= old_data != NULL;
> +  if (!check_overlaps_buffer)
> +    check_overlaps_buffer= (uchar*)alloc_root(&table_share->mem_root,
> +                                              table_share->max_unique_length
> +                                              + table_share->reclength);
> +  auto *record_buffer= check_overlaps_buffer + table_share->max_unique_length;
> +  auto *handler= this;
> +  if (handler->inited != NONE)
> +  {
> +    if (!check_overlaps_handler)
> +    {
> +      check_overlaps_handler= clone(table_share->normalized_path.str,
> +                                    &table_share->mem_root);

wouldn't it be simpler and cheaper to use HA_EXTRA_REMEMBER_POS and
HA_EXTRA_RESTORE_POS, as we've discussed?

> +      int error= -1;
> +      if (check_overlaps_handler != NULL)
> +        error= check_overlaps_handler->ha_external_lock(table->in_use, F_RDLCK);
> +      if (error)
> +        return error;
> +    }
> +    handler= check_overlaps_handler;
> +  }

why "if (handler->inited != NONE)" ?
What happens if it is NONE?

> +
> +  for (uint key_nr= 0; key_nr < table_share->keys; key_nr++)
> +  {
> +    const KEY *key_info= table->key_info + key_nr;
> +    const uint key_parts= key_info->user_defined_key_parts;
> +    if (!key_info->without_overlaps)
> +      continue;
> +
> +    key_copy(check_overlaps_buffer, new_data, key_info, 0);
> +    if (is_update)
> +    {
> +      bool key_used= false;
> +      for (uint k= 0; k < key_parts && !key_used; k++)
> +        key_used= bitmap_is_set(table->write_set,
> +                                key_info->key_part[k].fieldnr - 1);
> +      if (!key_used)
> +        continue;
> +    }

Good.

> +
> +    int error= handler->ha_index_init(key_nr, 0);
> +    if (error)
> +      return error;
> +
> +    for (int run= 0; run < 2; run++)
> +    {
> +      if (run == 0)
> +      {
> +        error = handler->ha_index_read_map(record_buffer,
> +                                           check_overlaps_buffer,
> +                                           key_part_map((1 << key_parts) - 1),
> +                                           HA_READ_KEY_OR_PREV);
> +        if (error == HA_ERR_KEY_NOT_FOUND)
> +          continue;
> +      }
> +      else
> +      {
> +        error = handler->ha_index_next(record_buffer);
> +        if (error == HA_ERR_END_OF_FILE)
> +          continue;
> +      }
> +
> +      if (error)
> +      {
> +        handler->ha_index_end();
> +        return error;
> +      }

I found this "for (int run= 0; run < 2; run++)" rather confusing.
Particularly as you have an if() to have different code for the first
and the second runs.

it would be clearer to unroll the loop and move the common code (period
comparison) into a function. Like

  handler->ha_index_read_map(...);
  compare_periods();
  handler->ha_index_next();
  compare_periods();

But it can be done better. You search for the key with the same value
and a period start <= than the period start of new row. And then you
have to check two rows for overlaps. If you'll search for a key with the
period start <= than the _period end_ of the new row, then you'll only
need to check one row (may be one more, if updating).

Also, why do you store both period ends in the index? It seems like it'd
be enough to store only one end - only the start or only the end. Both
ends help if you use keyreads, but you don't. On the second thought,
perhaps, you should use keyreads, there's no need to fetch the whole row
for this overlap check. On the yet another thought it might not work
with HA_EXTRA_REMEMBER_POS.

> +
> +      /* In case of update it could appear that the nearest neighbour is
> +       * a record we are updating. It means, that there are no overlaps
> +       * from this side. */
> +      if (is_update && memcmp(old_data + table->s->null_bytes,
> +                              record_buffer + table->s->null_bytes,
> +                              table->s->reclength - table->s->null_bytes) == 0)
> +      {
> +        continue;
> +      }

I'd rather compare row positions here.

> +
> +      uint period_key_part_nr= key_parts - 2;
> +      int cmp_res= 0;
> +      for (uint part_nr= 0; !cmp_res && part_nr < period_key_part_nr; part_nr++)
> +      {
> +        Field *f= key_info->key_part[part_nr].field;
> +        cmp_res= f->cmp(f->ptr_in_record(new_data),
> +                        f->ptr_in_record(record_buffer));
> +      }
> +      if (cmp_res)
> +        continue; /* key is different => no overlaps */
> +
> +      int period_cmp[2][2]= {/* l1 > l2, l1 > r2, r1 > l2, r1 > r2 */};
> +      for (int i= 0; i < 2; i++)
> +      {
> +        for (int j= 0; j < 2; j++)
> +        {
> +          Field *lhs= key_info->key_part[period_key_part_nr + i].field;
> +          Field *rhs= key_info->key_part[period_key_part_nr + j].field;
> +
> +          period_cmp[i][j]= lhs->cmp(lhs->ptr_in_record(new_data),
> +                                     rhs->ptr_in_record(record_buffer));
> +        }
> +      }

this can be simplified too if you'll change the code as I suggested
above to do only one index read - you'll only need one comparison,
instead of four.

> +
> +      if ((period_cmp[0][0] <= 0 && period_cmp[1][0] > 0)
> +          || (period_cmp[0][0] >= 0 && period_cmp[0][1] < 0))
> +      {
> +        handler->ha_index_end();
> +        return HA_ERR_FOUND_DUPP_KEY;
> +      }
> +    }
> +    error= handler->ha_index_end();
> +    if (error)
> +      return error;
> +  }
> +  return 0;
> +}
> +
>  #ifdef WITH_WSREP
>  /**
>    @details
> diff --git a/sql/handler.h b/sql/handler.h
> index 63d0bf2215c..71debf9bab7 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -3237,6 +3243,8 @@ class handler :public Sql_alloc
>      DBUG_ASSERT(cached_table_flags < (HA_LAST_TABLE_FLAG << 1));
>      return cached_table_flags;
>    }
> +  /** PRIMARY KEY WITHOUT OVERLAPS check is done globally */

what do you mean "globally"?

> +  int ha_check_overlaps(const uchar *old_data, const uchar* new_data);
>    /**
>      These functions represent the public interface to *users* of the
>      handler class, hence they are *not* virtual. For the inheritance
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 9bb1d98152b..5aba86003c6 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -4549,25 +4552,57 @@ static bool vers_prepare_keys(THD *thd, HA_CREATE_INFO *create_info,
>      if (key->type != Key::PRIMARY && key->type != Key::UNIQUE)
>        continue;
>  
> -    Key_part_spec *key_part= NULL;
> -    List_iterator<Key_part_spec> part_it(key->columns);
> -    while ((key_part=part_it++))
> +    if (create_info->versioned())
>      {
> -      if (!my_strcasecmp(system_charset_info,
> -                         row_start_field,
> -                         key_part->field_name.str) ||
> -
> -          !my_strcasecmp(system_charset_info,
> -                         row_end_field,
> -                         key_part->field_name.str))
> -        break;
> +      Key_part_spec *key_part=NULL;
> +      List_iterator<Key_part_spec> part_it(key->columns);
> +      while ((key_part=part_it++))
> +      {
> +        if (row_start_field.streq(key_part->field_name) ||
> +            row_end_field.streq(key_part->field_name))
> +          break;
> +      }
> +      if (!key_part)
> +        key->columns.push_back(new Key_part_spec(&row_end_field, 0));
>      }
> -    if (key_part)
> -      continue; // Key already contains Sys_start or Sys_end
> +  }
>  
> -    Key_part_spec *key_part_sys_end_col=
> -        new (thd->mem_root) Key_part_spec(&create_info->vers_info.as_row.end, 0);
> -    key->columns.push_back(key_part_sys_end_col);
> +  key_it.rewind();
> +  while ((key=key_it++))
> +  {
> +    if (key->without_overlaps)
> +    {
> +      if (key->type != Key::PRIMARY && key->type != Key::UNIQUE)
> +      {
> +        my_error(ER_PERIOD_WITHOUT_OVERLAPS_NON_UNIQUE, MYF(0), key->period.str);
> +        return true;
> +      }
> +      if (!create_info->period_info.is_set()
> +          || !key->period.streq(create_info->period_info.name))
> +      {
> +        my_error(ER_PERIOD_NOT_FOUND, MYF(0), key->period.str);
> +        return true;
> +      }
> +      if (thd->work_part_info)
> +      {
> +        my_error(ER_PERIOD_WITHOUT_OVERLAPS_PARTITIONED, MYF(0));

why?

> +        return true;
> +      }
> +      const auto &period_start= create_info->period_info.period.start;
> +      const auto &period_end= create_info->period_info.period.end;
> +      List_iterator<Key_part_spec> part_it(key->columns);
> +      while (Key_part_spec *key_part= part_it++)
> +      {
> +        if (period_start.streq(key_part->field_name)
> +            || period_end.streq(key_part->field_name))
> +        {
> +          my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key->name.str);
> +          return true;
> +        }
> +      }
> +      key->columns.push_back(new Key_part_spec(&period_start, 0));
> +      key->columns.push_back(new Key_part_spec(&period_end, 0));
> +    }
>    }
>  
>    return false;
> diff --git a/sql/table.h b/sql/table.h
> index 2b866159fe0..6511960488e 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -1730,10 +1731,18 @@ class IS_table_read_plan;
>  
>  /** number of bytes used by field positional indexes in frm */
>  constexpr uint frm_fieldno_size= 2;
> +/** number of bytes used by key position number in frm */
> +constexpr uint frm_keyno_size= 2;
>  static inline uint16 read_frm_fieldno(const uchar *data)
>  { return uint2korr(data); }
> -static inline void store_frm_fieldno(const uchar *data, uint16 fieldno)
> +static inline void store_frm_fieldno(uchar *data, uint16 fieldno)
>  { int2store(data, fieldno); }
> +static inline uint16 read_frm_keyno(const uchar *data)
> +{ return uint2korr(data); }
> +static inline void store_frm_keyno(uchar *data, uint16 fieldno)
> +{ int2store(data, fieldno); }
> +static inline size_t frm_ident_stored_size(size_t len)
> +{ return len + (len > 255 ? 3 : 1); }

this is exactly extra2_str_size() function. Don't duplicate it, reuse.
(and rename, if you'd like a more generic name)

>  
>  class select_unit;
>  class TMP_TABLE_PARAM;
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 7130b3e5d8a..1e95242786e 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -485,6 +486,16 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING &table,
>      store_frm_fieldno(pos, get_fieldno_by_name(create_info, create_fields,
>                                         create_info->period_info.period.end));
>      pos+= frm_fieldno_size;
> +    store_frm_keyno(pos, create_info->period_info.unique_keys);
> +    pos+= frm_keyno_size;
> +    for (uint key= 0; key < keys; key++)
> +    {
> +      if (key_info[key].without_overlaps)
> +      {
> +        store_frm_keyno(pos, key);
> +        pos+= frm_keyno_size;
> +      }
> +    }

This will make 10.5 frms look corrupted in 10.4. Better use a new EXTRA2
flag for that. With a number > EXTRA2_ENGINE_IMPORTANT.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx