maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sachin!

Looks good now, thanks.
See comments below

On Oct 29, Sachin Setiya wrote:
> revision-id: 02362a77d0d (mariadb-10.4.7-111-g02362a77d0d)
> parent(s): 52f3829b95c
> author: Sachin <sachin.setiya@xxxxxxxxxxx>
> committer: Sachin <sachin.setiya@xxxxxxxxxxx>
> timestamp: 2019-10-14 16:10:54 +0530
> message:
> 
> MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key
> 
> Problem:- Insert delayed is not working with Long Unique Index.
> It is failing with
> 1. INSERT DELAYED INTO t1 VALUES();
> 2. INSERT DELAYED INTO t1 VALUES(1);
> 3. Potential Race condition When Insert DELAYED gets dup key error(After fix),
> And it will change original table key_info by calling
> re_setup_keyinfo_hash, And second thread is in check_duplicate_long_entries
> 4. Insert delayed into INVISIBLE COLUMN will also not work.
> 
> There are 4 main issue
> 
> 1. while calling make_new_field we forgot to & LONG_UNIQUE_HASH_FIELD
> flag into new field flags.
> 
> 2. New field created created into get_local_table by make_new_field does
> not respect old field visibility, Assigning old field visibility will
> solve Problem 4 and part of problem 2.
> 
> 3. As we know Problem 3 race condition is caused because table and
> delayed table share same key_info, So we will make a copy of original table
> key_info in get_local_table.
> 
> 4. In parse_vcol_defs we have this code block
>   keypart->field->vcol_info=
>     table->field[keypart->field->field_index]->vcol_info;
>   Which is wrong because we should not change original
> table->field->vcol_info with vcol_info which is create on delayed
> thread.
> 
> ---
>  mysql-test/main/long_unique_bugs.result | 29 +++++++++++++++++++++++++++++
>  mysql-test/main/long_unique_bugs.test   | 32 ++++++++++++++++++++++++++++++++
>  sql/field.cc                            |  7 ++++++-
>  sql/sql_insert.cc                       | 31 +++++++++++++++++++++++++++++++
>  sql/table.cc                            | 31 ++++++++++++++++++++++++++++++-
>  sql/table.h                             |  2 ++
>  6 files changed, 130 insertions(+), 2 deletions(-)
> 
> diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test
> index 13a4e1367a0..365393aa2ea 100644
> --- a/mysql-test/main/long_unique_bugs.test
> +++ b/mysql-test/main/long_unique_bugs.test
> @@ -340,3 +340,35 @@ while ($count)
>  --eval $insert_stmt
>  --enable_query_log
>  drop table t1;
> +#
> +# MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key
> +#
> +CREATE  TABLE t1 (a blob, UNIQUE(a)) ENGINE=MyISAM;
> +INSERT DELAYED t1 () VALUES (1);
> +INSERT t1 () VALUES (2);
> +# Cleanup
> +DROP TABLE t1;
> +CREATE TABLE t1 (a char(50), UNIQUE(a(10)) USING HASH);
> +INSERT DELAYED t1 () VALUES (1);
> +INSERT t1 () VALUES (2);
> +# Cleanup
> +DROP TABLE t1;
> +CREATE TABLE t1 (
> +  a CHAR(128),
> +  b CHAR(128) AS (a),
> +  c varchar(5000),
> +  UNIQUE(c,b(64))
> +) ENGINE=myisam;
> +INSERT DELAYED t1 (a,c) VALUES (1,1);
> +--sleep 1
> +INSERT t1 (a,c) VALUES (2,2);
> +INSERT t1 (a,c) VALUES (3,3);
> +drop table t1;
> +create table t1(a int , b int invisible);
> +insert into t1 values(1);
> +insert delayed into t1(a,b) values(2,2);
> +--echo #Should not fails

"Should not fail"

> +insert delayed into t1 values(2);

I suspect there's a race condition here.
You cannot know that INSERT DELAYED will insert rows before you do your
SELECT.

Other tests usually do FLUSH TABLE t1 before SELECT.
Alternatively you can use include/wait_condition.inc file
to wait for, say, SELECT COUNT(*) = 3

> +select a,b from t1 order by a;
> +# Cleanup
> +DROP TABLE t1;
> diff --git a/sql/field.cc b/sql/field.cc
> index 0eb53f40a54..d1187237f23 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -2373,8 +2373,13 @@ Field *Field::make_new_field(MEM_ROOT *root, TABLE *new_table,
>    tmp->flags&= (NOT_NULL_FLAG | BLOB_FLAG | UNSIGNED_FLAG |
>                  ZEROFILL_FLAG | BINARY_FLAG | ENUM_FLAG | SET_FLAG |
>                  VERS_SYS_START_FLAG | VERS_SYS_END_FLAG |
> -                VERS_UPDATE_UNVERSIONED_FLAG);
> +                VERS_UPDATE_UNVERSIONED_FLAG | LONG_UNIQUE_HASH_FIELD);
>    tmp->reset_fields();
> +  /*
> +    Calling make_new_field will return a VISIBLE field, If caller function
> +    wants original visibility he should change it later.

"visibility it should change"

> +    This is done because view created on invisible fields are visible.

"because invisible fields explicitly named in a view become visible"

> +  */
>    tmp->invisible= VISIBLE;
>    return tmp;
>  }
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index fe6c5fa8ec4..d80044ec37d 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -2607,6 +2607,11 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd)
>    {
>      if (!(*field= (*org_field)->make_new_field(client_thd->mem_root, copy, 1)))
>        goto error;
> +    /*
> +      We want same visibility as of original table because we are just creating
> +      a clone for delayed insert.
> +    */
> +    (*field)->invisible= (*org_field)->invisible;

I'm surprised it matters. I'd thought that all sanity checks, and in
particular whether number of columns matches the number of values, are
done in the connection user thread before submitting the job to the
delayed insert thread.

>      (*field)->unireg_check= (*org_field)->unireg_check;
>      (*field)->orig_table= copy;			// Remove connection
>      (*field)->move_field_offset(adjust_ptrs);	// Point at copy->record[0]
> @@ -2621,7 +2626,33 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd)
>    if (share->virtual_fields || share->default_expressions ||
>        share->default_fields)
>    {
> +    /*
> +       If we have long unique table then delayed insert can modify key structure
> +       (re/setup_keyinfo_hash_all) of original table when it gets insert error,
> +       parse_vcol_defs will also modify key_info structure. So it is better to
> +       clone the table->key_info for copy table.
> +       We will not be cloning key_part_info or even changing any field ptr.
> +       Because re/setup_keyinfo_hash_all only modify key_info array. So it will
> +       be like having new key_info array for copy table with old key_part_info
> +       ptr.
> +    */
> +    if (share->long_unique_table)
> +    {
> +      KEY *key_info;
> +      if (!(key_info= (KEY*) client_thd->alloc(share->keys*sizeof(KEY))))
> +        goto error;
> +      copy->key_info= key_info;
> +      memcpy(key_info, table->key_info, sizeof(KEY)*share->keys);
> +    }
> +    /*
> +      parse_vcol_defs expects key_infos to be in user defined format.
> +    */
> +    copy->setup_keyinfo_hash_all();
>      bool error_reported= FALSE;
> +    /*
> +      We won't be calling re_setup_keyinfo_hash because parse_vcol_defs changes
> +      key_infos to storage engine format
> +    */
>      if (unlikely(parse_vcol_defs(client_thd, client_thd->mem_root, copy,
>                                   &error_reported,
>                                   VCOL_INIT_DEPENDENCY_FAILURE_IS_WARNING)))
> diff --git a/sql/table.cc b/sql/table.cc
> index 718d0dce072..d0fcd449098 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -1223,7 +1223,16 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table,
>                         new (mem_root) Item_field(thd, keypart->field),
>                         new (mem_root) Item_int(thd, length));
>            list_item->fix_fields(thd, NULL);
> -          keypart->field->vcol_info=
> +          /*
> +            Do not change the vcol_info when vcol_info->expr is not NULL
> +            This will happen in the case of Delayed_insert::get_local_table()
> +            And if we change the vcol_info in Delayed insert , then original
> +            table field->vcol_info will be created on delayed insert thread
> +            mem_root.
> +          */
> +          if (!keypart->field->vcol_info ||
> +               !keypart->field->vcol_info->expr)
> +            keypart->field->vcol_info=
>              table->field[keypart->field->field_index]->vcol_info;
>          }
>          else
> @@ -9084,6 +9093,26 @@ void re_setup_keyinfo_hash(KEY *key_info)
>                 key_info->ext_key_parts= 1;
>    key_info->flags&= ~HA_NOSAME;
>  }
> +
> +/*
> +  call setup_keyinfo_hash for all keys in table
> + */
> +void TABLE::setup_keyinfo_hash_all()
> +{
> +  for (uint i= 0; i < s->keys; i++)
> +    if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH)
> +      setup_keyinfo_hash(&key_info[i]);

also... I remember you wanted to rename these methods to something more
descriptive.

> +}
> +
> +/*
> +  call re_setup_keyinfo_hash for all keys in table
> + */
> +void TABLE::re_setup_keyinfo_hash_all()

don't add this method, you aren't using it anywhere

> +{
> +  for (uint i= 0; i < s->keys; i++)
> +    if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH)
> +      re_setup_keyinfo_hash(&key_info[i]);
> +}
>  /**
>    @brief clone of current handler.
>    Creates a clone of handler used in update for

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