maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Feb 29, Michael Widenius wrote:
> commit 5365db70cb8
> Author: Michael Widenius <monty@xxxxxxxxxxx>
> Date:   Mon Jan 13 18:30:13 2020 +0200
> 
>     Improve update handler (long unique keys on blobs)
>     
>     MDEV-21606 Improve update handler (long unique keys on blobs)
>     MDEV-21470 MyISAM and Aria start_bulk_insert doesn't work with long unique
>     MDEV-21606 Bug fix for previous version of this code
>     MDEV-21819 2 Assertion `inited == NONE || update_handler != this'
>     
>     - Move update_handler from TABLE to handler
>     - Move out initialization of update handler from ha_write_row() to
>       prepare_for_insert()
>     - Fixed that INSERT DELAYED works with update handler
>     - Give an error if using long unique with an autoincrement column
>     - Added handler function to check if table has long unique hash indexes
>     - Disable write cache in MyISAM and Aria when using update_handler as
>       if cache is used, the row will not be inserted until end of statement
>       and update_handler would not find conflicting rows.
>     - Removed not used handler argument from
>       check_duplicate_long_entries_update()
>     - Syntax cleanups
>       - Indentation fixes
>       - Don't use single character indentifiers for arguments
>     
>     squash! 1ca2f0871ecfb550268ffc1a5cc23d7043d6b855

you may want to remove this line from the commit comment

> 
> diff --git a/mysql-test/main/long_unique.result b/mysql-test/main/long_unique.result
> index a4955b3e7b5..fee8e7721bf 100644
> --- a/mysql-test/main/long_unique.result
> +++ b/mysql-test/main/long_unique.result
> @@ -1477,4 +1477,28 @@ id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>  SELECT t2.b FROM t1 JOIN t2 ON t1.d = t2.f WHERE t2.pk >= 20;
>  b
>  drop table t1,t2;
> +#
> +# MDEV-21470 MyISAM start_bulk_insert doesn't work with long unique
> +#
> +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MyISAM;
> +INSERT INTO t1 VALUES (1,'foo'),(2,'bar');
> +CREATE TABLE t2 (c BIT, d BLOB, UNIQUE(d)) ENGINE=MyISAM;
> +INSERT INTO t2 SELECT * FROM t1;
> +Warnings:
> +Warning	1264	Out of range value for column 'c' at row 2
> +DROP TABLE t1, t2;
> +#
> +# MDEV-19338 Using AUTO_INCREMENT with long unique
> +#
> +CREATE TABLE t1 (pk INT, a TEXT NOT NULL DEFAULT '', PRIMARY KEY (pk), b INT AUTO_INCREMENT, UNIQUE(b), UNIQUE (a,b)) ENGINE=myisam;
> +ERROR HY000: Function or expression 'AUTO_INCREMENT' cannot be used in the UNIQUE clause of `a`

Quite confusing error message :(
I understand where it comes from, but for a user, I'm afraid, it just won't
make any sense.

A reasonable message could be, for example,

  AUTO_INCREMENT column %`s cannot be used in the UNIQUE index %`s

> +#
> +# MDEV-21819 Assertion `inited == NONE || update_handler != this'
> +# failed in handler::ha_write_row
> +#
> +CREATE OR REPLACE TABLE t1 (a INT, b BLOB, s DATE, e DATE, PERIOD FOR app(s,e), UNIQUE(b)) ENGINE=MyISAM PARTITION BY HASH(a) PARTITIONS 2;
> +INSERT INTO t1 VALUES (1,'foo','2022-01-01', '2025-01-01');
> +DELETE FROM t1 FOR PORTION OF app FROM '2023-01-01' TO '2024-01-01';
> +ERROR 23000: Duplicate entry 'foo' for key 'b'
> +DROP TABLE t1;
>  set @@GLOBAL.max_allowed_packet= @allowed_packet;
> diff --git a/mysql-test/suite/versioning/r/long_unique.result b/mysql-test/suite/versioning/r/long_unique.result
> new file mode 100644
> index 00000000000..da07bc66e22
> --- /dev/null
> +++ b/mysql-test/suite/versioning/r/long_unique.result
> @@ -0,0 +1,8 @@
> +#
> +# Assertion `inited == NONE || update_handler != this' failed in
> +# handler::ha_write_row
> +#
> +CREATE TABLE t1 (f VARCHAR(4096), s DATE, e DATE, PERIOD FOR app(s,e), UNIQUE(f)) ENGINE=MyISAM;
> +INSERT INTO t1 VALUES ('foo', '2023-08-30', '2025-07-09'),('bar', '2021-01-01', '2021-12-31');
> +DELETE FROM t1 FOR PORTION OF app FROM '2023-08-29' TO '2025-07-01';
> +DROP TABLE t1;

you've had almost the same test (MDEV-21819) in main/long_unique.test

it's a bit strange to have to very similar tests in different suites

I suggest to move MDEV-21819 test from main/long_unique.test to this file.
and then move this whole file from versioning suite to the period suite.

> diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> index 73191907aac..c7e61864366 100644
> --- a/sql/ha_partition.cc
> +++ b/sql/ha_partition.cc
> @@ -4340,6 +4339,10 @@ int ha_partition::write_row(const uchar * buf)
>    }
>    m_last_part= part_id;
>    DBUG_PRINT("info", ("Insert in partition %u", part_id));
> +  if (table->s->long_unique_table &&
> +      m_file[part_id]->update_handler == m_file[part_id] && inited == RND)
> +    m_file[part_id]->prepare_for_insert(0);

This looks quite incomprehensible. If the table has a long unique key
and the update_handler is the handler itself you _prepare for insert_ ???

if it's an insert it should've been prepared earlier, like, normally for
an insert, and it don't have to do anything with long uniques.

What you actually do here (as I've found out looking firther in the patch)
you prepare update_handler. That is "prepare_for_insert" is VERY confusing
here, it sounds like it prepares for a normal insert and should be
called at the beginning of mysql_insert(). And it's nothing like that.

It would remove half of the questions if you'd call it
prepare_update_handler() or, better, prepare_auxiliary_handler() (because
it'll be used not only for long uniques) or something like that.

Still, some questions are left:

1. why partitioning needs special code for that?
2. when a handler itself is its update_handler AND inited==RND?
   the handler itself can be the update_handler on INSERT, but then inited is NONE
   Is it UPDATE with system versioning?

> +
>    start_part_bulk_insert(thd, part_id);
>  
>    tmp_disable_binlog(thd); /* Do not replicate the low-level changes. */
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 0700b1571e5..1e3f987b4e5 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -2648,6 +2649,39 @@ handler *handler::clone(const char *name, MEM_ROOT *mem_root)
>    return NULL;
>  }
>  
> +/**
> +  @brief clone of current handler.

you generally don't have to write @brief in these cases. Just

 /** clone of current handler.

   Creates a clone of handler used in update for
   unique hash key.
 */

> +  Creates a clone of handler used in update for
> +  unique hash key.
> +*/
> +bool handler::clone_handler_for_update()
> +{
> +  handler *tmp;
> +  DBUG_ASSERT(table->s->long_unique_table);
> +
> +  if (update_handler != this)
> +    return 0;                                   // Already done
> +  if (!(tmp= clone(table->s->normalized_path.str, table->in_use->mem_root)))
> +    return 1;
> +  update_handler= tmp;
> +  update_handler->ha_external_lock(table->in_use, F_RDLCK);

Looks strange. Why F_RDLCK if it's an *update* handler?
This definitely needs a comment, why you're using read lock for updates.

> +  return 0;
> +}
> +
> +/**
> + @brief Deletes update handler object
> +*/
> +void handler::delete_update_handler()
> +{
> +  if (update_handler != this)
> +  {
> +    update_handler->ha_external_lock(table->in_use, F_UNLCK);
> +    update_handler->ha_close();
> +    delete update_handler;
> +  }
> +  update_handler= this;
> +}
> +
>  LEX_CSTRING *handler::engine_name()
>  {
>    return hton_name(ht);
> @@ -6481,6 +6515,8 @@ int handler::ha_reset()
>    DBUG_ASSERT(inited == NONE);
>    /* reset the bitmaps to point to defaults */
>    table->default_column_bitmaps();
> +  if (update_handler != this)
> +    delete_update_handler();

you already have an if() inside the delete_update_handler().
Either remove it from here or from there.

>    pushed_cond= NULL;
>    tracker= NULL;
>    mark_trx_read_write_done= 0;
> @@ -6671,10 +6717,35 @@ static int check_duplicate_long_entries_update(TABLE *table, handler *h, uchar *
>        }
>      }
>    }
> -  exit:
> -  return error;
> +  return 0;
> +}
> +
> +
> +/**
> +  Do all initialization needed for insert
> +
> +  @param force_update_handler  Set to TRUE if we should always create an
> +                               update handler.  Needed if we don't know if we
> +                               are going to do inserted while a scan is in

"going to do inserts"

> +                               progress.
> +*/
> +
> +int handler::prepare_for_insert(bool force_update_handler)
> +{
> +  /* Preparation for unique of blob's */
> +  if (table->s->long_unique_table && (inited == RND || force_update_handler))
> +  {
> +    /*
> +      When doing a scan we can't use the same handler to check
> +      duplicate rows. Create a new temporary one
> +    */
> +    if (clone_handler_for_update())
> +      return 1;
> +  }
> +  return 0;
>  }
>  
> +
>  int handler::ha_write_row(const uchar *buf)
>  {
>    int error;
> @@ -6977,6 +7044,11 @@ void handler::set_lock_type(enum thr_lock_type lock)
>    table->reginfo.lock_type= lock;
>  }
>  
> +bool handler::has_long_unique()
> +{
> +  return table->s->long_unique_table;
> +}

I don't see why you need it for Aria.
It can always check table->s->long_unique_table without an extra call

> +
>  #ifdef WITH_WSREP
>  /**
>    @details
> diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> index 6a4ce266af2..e55ae4f97e6 100644
> --- a/sql/sql_delete.cc
> +++ b/sql/sql_delete.cc
> @@ -752,6 +752,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds,
>            && !table->versioned()
>            && table->file->has_transactions();
>  
> +  if (table->versioned(VERS_TIMESTAMP) ||
> +      (table_list->has_period() && !portion_of_time_through_update))
> +    table->file->prepare_for_insert(1);

Ok, now I think that your idea of the default update_handler=this
is rather fragile. If the default is NULL, than we can put in many places

   DBUG_ASSERT(update_handler != NULL);

to make sure it was initialized when needed. Now we cannot do it.
And there are many places in the code and non-trivial conditions when
the update_handler has to be initialized and these places and conditions
will change in the future. An assert would be helpful here.

> +
>    THD_STAGE_INFO(thd, stage_updating);
>    while (likely(!(error=info.read_record())) && likely(!thd->killed) &&
>           likely(!thd->is_error()))
> diff --git a/sql/table.h b/sql/table.h
> index 6ce92ee048e..1a0b9514d23 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -1182,6 +1181,7 @@ struct TABLE
>    /* Map of keys dependent on some constraint */
>    key_map constraint_dependent_keys;
>    KEY  *key_info;			/* data of keys in database */
> +  KEY_PART_INFO *base_key_part;         /* Where key parts are stored */


again, this is *only* needed for INSERT DELAYED that very few people
ever use. It should be in some INSERT DELAYED specific data structure,
not in the common TABLE.

By the way, why cannot you get the list of keyparts
from table->key_info->key_part ?

>  
>    Field **field;                        /* Pointer to fields */
>    Field **vfield;                       /* Pointer to virtual fields*/
> diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
> index 38091dae0ba..39147396359 100644
> --- a/storage/myisam/ha_myisam.cc
> +++ b/storage/myisam/ha_myisam.cc
> @@ -1740,7 +1740,7 @@ void ha_myisam::start_bulk_insert(ha_rows rows, uint flags)
>      enable_indexes() failed, thus it's essential that indexes are
>      disabled ONLY for an empty table.
>    */
> -  if (file->state->records == 0 && can_enable_indexes &&
> +  if (file->state->records == 0 && can_enable_indexes && !has_long_unique() &&
>        (!rows || rows >= MI_MIN_ROWS_TO_DISABLE_INDEXES))

why do you disable bulk inserts for long uniques?

>    {
>      if (file->open_flag & HA_OPEN_INTERNAL_TABLE)
> diff --git a/sql/table.cc b/sql/table.cc
> index 718efa5767c..18a942964c3 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -1241,31 +1241,46 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table,
>        Item *list_item;
>        KEY *key= 0;
>        uint key_index, parts= 0;
> +      KEY_PART_INFO *key_part= table->base_key_part;
> +
>        for (key_index= 0; key_index < table->s->keys; key_index++)
>        {
> -        key=table->key_info + key_index;
> -        parts= key->user_defined_key_parts;
> -        if (key->key_part[parts].fieldnr == field->field_index + 1)
> -          break;
> +        /*
> +          We have to use key from share as this function may have changed
> +          table->key_info if it was ever invoked before. This could happen
> +          in case of INSERT DELAYED.
> +        */
> +        key= table->s->key_info + key_index;
> +        if (key->algorithm == HA_KEY_ALG_LONG_HASH)
> +        {
> +          parts= key->user_defined_key_parts;
> +          if (key_part[parts].fieldnr == field->field_index + 1)
> +            break;
> +          key_part++;
> +        }

Here, again, you've basically duplicated the logic of how long uniques
are represented in the keys and keyparts. All for the sake of INSERT DELAYED.

I'd really prefer INSERT DELAYED problems to be solved inside
the Delayed_insert class. And it can use setup_keyinfo_hash()
and re_setup_keyinfo_hash() functions to avoid duplicating the logic.

> +        key_part+= key->ext_key_parts;
>        }
> -      if (!key || key->algorithm != HA_KEY_ALG_LONG_HASH)
> +      if (key_index == table->s->keys)
>          goto end;
> -      KEY_PART_INFO *keypart;
> -      for (uint i=0; i < parts; i++)
> +
> +      /* Correct the key & key_parts if this function has been called before */
> +      key= table->key_info + key_index;
> +      key->key_part= key_part;
> +
> +      for (uint i=0; i < parts; i++, key_part++)
>        {
> -        keypart= key->key_part + i;
> -        if (keypart->key_part_flag & HA_PART_KEY_SEG)
> +        if (key_part->key_part_flag & HA_PART_KEY_SEG)
>          {
> -          int length= keypart->length/keypart->field->charset()->mbmaxlen;
> +          int length= key_part->length/key_part->field->charset()->mbmaxlen;
>            list_item= new (mem_root) Item_func_left(thd,
> -                       new (mem_root) Item_field(thd, keypart->field),
> +                       new (mem_root) Item_field(thd, key_part->field),
>                         new (mem_root) Item_int(thd, length));
>            list_item->fix_fields(thd, NULL);
> -          keypart->field->vcol_info=
> -            table->field[keypart->field->field_index]->vcol_info;
> +          key_part->field->vcol_info=
> +            table->field[key_part->field->field_index]->vcol_info;
>          }
>          else
> -          list_item= new (mem_root) Item_field(thd, keypart->field);
> +          list_item= new (mem_root) Item_field(thd, key_part->field);
>          field_list->push_back(list_item, mem_root);
>        }
>        Item_func_hash *hash_item= new(mem_root)Item_func_hash(thd, *field_list);

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