maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #12029
Re: 10670133744: MDEV-16978 Application-time periods: WITHOUT OVERLAPS
Hi, Nikita!
On Dec 16, Nikita Malyavin wrote:
> revision-id: 10670133744 (mariadb-10.4.4-492-g10670133744)
> parent(s): 251c6e17269
> author: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> timestamp: 2019-11-26 19:22:04 +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
wrap long lines, pease.
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 46a0c313c80..ed36f3c5bd3 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -6402,6 +6407,14 @@ int handler::ha_external_lock(THD *thd, int lock_type)
> mysql_audit_external_lock(thd, table_share, lock_type);
> }
>
> + if (lock_type == F_UNLCK && check_overlaps_handler)
> + {
> + check_overlaps_handler->ha_external_lock(table->in_use, F_UNLCK);
> + check_overlaps_handler->close();
> + check_overlaps_handler= NULL;
> + overlaps_error_key= -1;
> + }
I'm still thinking about how avoid this overhead or at least to simplify the
code.
One option is to use HA_EXTRA_REMEMBER_POS
It doesn't nest, you're right, but it can be fixed.
Another, simpler, option is to use TABLE::update_handler.
This is a second auxillary handler, a clone, exactly as yours, created for
to check long uniques. So I don't see a need to create a yet another clone
when you can simply use TABLE::update_handler. It's never used for scanning,
only for point lookups, so there is no position that you can disrupt.
upd: you seem to have got the same idea. and you're right, it should be
in the handler class, not in the TABLE as I originally wanted.
> +
> if (MYSQL_HANDLER_RDLOCK_DONE_ENABLED() ||
> MYSQL_HANDLER_WRLOCK_DONE_ENABLED() ||
> MYSQL_HANDLER_UNLOCK_DONE_ENABLED())
> @@ -6935,6 +6956,134 @@ 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;
Please, add a comment that on INSERT handler->inited can be NONE
> + if (handler->inited != NONE)
> + {
> + if (!check_overlaps_handler)
> + {
> + check_overlaps_handler= clone(table_share->normalized_path.str,
> + &table_share->mem_root);
> + 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;
> +
> + // Needs to compare record refs later is old_row_found()
> + if (is_update)
> + position(old_data);
> + }
> +
> + // Save and later restore this handler's keyread
> + int old_this_keyread= this->keyread;
what's that for? you aren't using `this` anywhere below.
Unless handler == this, but then this->keyread cannot be enabled.
> + DBUG_ASSERT(this->ha_end_keyread() == 0);
Eh. Never put any code with side effects into an assert.
Assert are conditionally compiled.
> +
> + int error= 0;
> +
> + for (uint key_nr= 0; key_nr < table_share->keys && !error; 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;
> +
> + 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;
> + }
> +
> + error= handler->ha_index_init(key_nr, 0);
> + if (error)
> + return error;
> +
> + auto old_row_found= [is_update, old_data, record_buffer, this, handler](){
There is no reason to use a lambda here.
could you rewrite that, please? In this particular case old_row_found() is
only used once, so you can inline your lambda there.
> + if (!is_update)
> + return false;
> + /* 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.
> + *
> + * An assumption is made that during update we always have the last
> + * fetched row in old_data. Therefore, comparing ref's is enough
> + * */
> + DBUG_ASSERT(handler != this && inited != NONE);
as a general rule please use
DBUG_ASSERT(x);
DBUG_ASSERT(y);
and not
DBUG_ASSERT(x && y);
> + DBUG_ASSERT(ref_length == handler->ref_length);
> +
> + handler->position(record_buffer);
> + return memcmp(ref, handler->ref, ref_length) == 0;
> + };
> +
> + error= handler->ha_start_keyread(key_nr);
> + DBUG_ASSERT(!error);
> +
> + const uint period_field_length= key_info.key_part[key_parts - 1].length;
> + const uint key_base_length= key_info.key_length - 2 * period_field_length;
> +
> + key_copy(check_overlaps_buffer, new_data, &key_info, 0);
> +
> + /* Copy period_end to period_start.
> + * the value in period_end field is not significant, but anyway let's leave
> + * it defined to avoid uninitialized memory access
> + */
please format your multi-line comments to follow the existing server code
conventions
> + memcpy(check_overlaps_buffer + key_base_length,
> + check_overlaps_buffer + key_base_length + period_field_length,
> + period_field_length);
> +
> + /* Find row with period_start < (period_end of new_data) */
> + error = handler->ha_index_read_map(record_buffer,
> + check_overlaps_buffer,
> + key_part_map((1 << key_parts) - 1),
> + HA_READ_BEFORE_KEY);
> +
> + if (!error && old_row_found())
> + error= handler->ha_index_prev(record_buffer);
> +
> + if (!error && table->check_period_overlaps(key_info, key_info,
> + new_data, record_buffer) == 0)
> + error= HA_ERR_FOUND_DUPP_KEY;
> +
> + if (error == HA_ERR_KEY_NOT_FOUND || error == HA_ERR_END_OF_FILE)
> + error= 0;
> +
> + if (error == HA_ERR_FOUND_DUPP_KEY)
> + overlaps_error_key= key_nr;
> +
> + int end_error= handler->ha_end_keyread();
> + DBUG_ASSERT(!end_error);
> +
> + end_error= handler->ha_index_end();
> + if (!error && end_error)
> + error= end_error;
> + }
> +
> + // Restore keyread of this handler, if it was enabled
> + if (old_this_keyread < MAX_KEY)
> + DBUG_ASSERT(this->ha_start_keyread(old_this_keyread) == 0);
> +
> + return error;
> +}
> +
> #ifdef WITH_WSREP
> /**
> @details
> diff --git a/sql/key.cc b/sql/key.cc
> index bf50094a9e4..d4f33467e2b 100644
> --- a/sql/key.cc
> +++ b/sql/key.cc
> @@ -899,3 +899,45 @@ bool key_buf_cmp(KEY *key_info, uint used_key_parts,
> }
> return FALSE;
> }
> +
> +
a comment please. What does the function return? -1/0/1 ?
> +int key_period_compare_bases(const KEY &lhs_key, const KEY &rhs_key,
> + const uchar *lhs, const uchar *rhs)
> +{
> + uint base_part_nr= lhs_key.user_defined_key_parts - 2;
> + int cmp_res= 0;
> + for (uint part_nr= 0; !cmp_res && part_nr < base_part_nr; part_nr++)
> + {
> + Field *f= lhs_key.key_part[part_nr].field;
> + cmp_res= f->cmp(f->ptr_in_record(lhs),
> + rhs_key.key_part[part_nr].field->ptr_in_record(rhs));
> + }
> +
> + return cmp_res;
> +}
> +
a comment please. What does the function return?
> +int key_period_compare_periods(const KEY &lhs_key, const KEY &rhs_key,
> + const uchar *lhs, const uchar *rhs)
> +{
> + uint base_part_nr= lhs_key.user_defined_key_parts - 2;
> +
> + Field *lhs_fields[]= {lhs_key.key_part[base_part_nr].field,
> + lhs_key.key_part[base_part_nr + 1].field};
> +
> + Field *rhs_fields[]= {rhs_key.key_part[base_part_nr].field,
> + rhs_key.key_part[base_part_nr + 1].field};
> +
> + int 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++)
> + {
> + cmp[i][j]= lhs_fields[0]->cmp(lhs_fields[i]->ptr_in_record(lhs),
> + rhs_fields[j]->ptr_in_record(rhs));
> + }
> + }
> +
> + bool overlaps = (cmp[0][0] <= 0 && cmp[1][0] > 0)
> + || (cmp[0][0] >= 0 && cmp[0][1] < 0);
> + return overlaps ? 0 : cmp[0][0];
Couldn't this be simplifed? Like
if (cmp[1][0] <= 0) // r1 <= l2
return -1;
if (cmp[0][1] >= 0) // l1 >= r2
return 1;
return 0;
and I think it'd be clearer to remove this cmp[][] array and write
the condition directly. May be with shortcuts like
const Field const * f=lhs_fields[0];
const uchar const * l1=lhs_fields[0]->ptr_in_record(lhs), ...
if (f->cmp(r1, l2) <= 0)
return -1;
if (f->cmp(l1, r2) >= 0)
return 1;
return 0;
> +}
> \ No newline at end of file
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 36188e58624..bb2f0fc5296 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7943,3 +7943,9 @@ ER_WARN_HISTORY_ROW_START_TIME
> eng "Table `%s.%s` history row start '%s' is later than row end '%s'"
> ER_PART_STARTS_BEYOND_INTERVAL
> eng "%`s: STARTS is later than query time, first history partition may exceed INTERVAL value"
> +ER_KEY_CONTAINS_PERIOD_FIELDS
> + eng "Key %`s should not contain period fields in order to make it WITHOUT OVERLAPS"
I'd say "Key %`s cannot explicitly include column %`s"
or "WITHOUT OVERLAPS key %`s cannot explicitly include column %`s"
but I think the latter looks weird, I like the first variant more
> +ER_PERIOD_WITHOUT_OVERLAPS_PARTITIONED
> + eng "Period WITHOUT OVERLAPS is not implemented for partitioned tables"
I don't think we should create a separate error message for every
feature that doesn't work with partitioning. There's already
ER_FOREIGN_KEY_ON_PARTITIONED
eng "Foreign key clause is not yet supported in conjunction with partitioning"
ER_PARTITION_NO_TEMPORARY
eng "Cannot create temporary table with partitions"
ER_FULLTEXT_NOT_SUPPORTED_WITH_PARTITIONING
eng "FULLTEXT index is not supported for partitioned tables"
which is two error messages too many. So I'd just say
ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING
end "Partitioned tables do not support %s"
where %s could be CREATE TEMPORARY TABLE, FOREIGN KEY, FULLTEXT, WITHOUT OVERLAPS,
and whatever else partitioned tables don't or won't support.
Note that you cannot remove old error messages, so just rename the first one
to ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING, and keep the rest unused
but still present in the errmsg-utf8.txt file.
> +ER_PERIOD_WITHOUT_OVERLAPS_NON_UNIQUE
> + eng "Period WITHOUT OVERLAPS is only allowed for unique keys"
"Only UNIQUE or PRIMARY keys can be WITHOUT OVERLAPS"
> diff --git a/sql/table.cc b/sql/table.cc
> index 7ed5121a9c6..8b84fb3035d 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -1519,6 +1520,15 @@ bool read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields)
> size_t length= extra2_read_len(&extra2, e2end);
> if (!length)
> DBUG_RETURN(true);
> +
> + auto fill_extra2= [extra2, length](LEX_CUSTRING *section){
> + if (section->str)
> + return true;
> + *section= {extra2, length};
> + return false;
> + };
don't use a lambda here either, make it a proper function
> +
> + bool fail= false;
> switch (type) {
> case EXTRA2_TABLEDEF_VERSION:
> if (fields->version.str) // see init_from_sql_statement_string()
> @@ -1726,11 +1725,26 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
> keyinfo= &first_keyinfo;
> thd->mem_root= &share->mem_root;
>
> + auto err= [thd, share, &handler_file, &se_plugin, old_root](){
> + share->db_plugin= NULL;
> + share->error= OPEN_FRM_CORRUPTED;
> + share->open_errno= my_errno;
> + delete handler_file;
> + plugin_unlock(0, se_plugin);
> + my_hash_free(&share->name_hash);
> +
> + if (!thd->is_error())
> + open_table_error(share, OPEN_FRM_CORRUPTED, share->open_errno);
> +
> + thd->mem_root= old_root;
> + return HA_ERR_NOT_A_TABLE;
> + };
Revert that too
> +
> if (write && write_frm_image(frm_image, frm_length))
> - goto err;
> + DBUG_RETURN(err());
>
> if (frm_length < FRM_HEADER_SIZE + FRM_FORMINFO_SIZE)
> - goto err;
> + DBUG_RETURN(err());
>
> share->frm_version= frm_image[2];
> /*
> @@ -2251,14 +2265,30 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
> pos+= period.constr_name.length;
>
> if (init_period_from_extra2(&period, pos, end))
> - goto err;
> + DBUG_RETURN(err());
> status_var_increment(thd->status_var.feature_application_time_periods);
> }
>
> + if (extra2.without_overlaps.str)
> + {
> + const uchar *key_pos= extra2.without_overlaps.str;
> + period.unique_keys= read_frm_keyno(key_pos);
> + for (uint k= 0; k < period.unique_keys; k++)
> + {
> + key_pos+= frm_keyno_size;
> + uint key_nr= read_frm_keyno(key_pos);
> + key_info[key_nr].without_overlaps= true;
> + }
> +
> + if ((period.unique_keys + 1) * frm_keyno_size
> + != extra2.without_overlaps.length)
> + DBUG_RETURN(err());
you can also check here that extra2.application_period.str != NULL
otherwise it's OPEN_FRM_CORRUPTED too
> + }
> +
> if (extra2.field_data_type_info.length &&
> field_data_type_info_array.parse(old_root, share->fields,
> extra2.field_data_type_info))
> - goto err;
> + DBUG_RETURN(err());
>
> for (i=0 ; i < share->fields; i++, strpos+=field_pack_length, field_ptr++)
> {
> @@ -8600,6 +8616,15 @@ void TABLE::evaluate_update_default_function()
> DBUG_VOID_RETURN;
> }
>
a comment please. What does the function return?
> +int TABLE::check_period_overlaps(const KEY &lhs_key, const KEY &rhs_key,
> + const uchar *lhs, const uchar *rhs)
> +{
> + int cmp_res= key_period_compare_bases(lhs_key, rhs_key, lhs, rhs);
> + if (cmp_res)
> + return cmp_res;
> +
> + return key_period_compare_periods(lhs_key, rhs_key, lhs, rhs);
> +}
>
> void TABLE::vers_update_fields()
> {
> diff --git a/sql/table.h b/sql/table.h
> index 043341db608..e871471101f 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -1620,6 +1621,13 @@ struct TABLE
> int period_make_insert(Item *src, Field *dst);
> int insert_portion_of_time(THD *thd, const vers_select_conds_t &period_conds,
> ha_rows *rows_inserted);
> + /*
> + @return -1, lhs precedes rhs
> + 0, lhs overlaps rhs
> + 1, lhs succeeds rhs
> + */
ah, a comment, good :) but move it to the function definition, please
> + static int check_period_overlaps(const KEY &lhs_key, const KEY &rhs_key,
> + const uchar *lhs, const uchar *rhs);
> int delete_row();
> void vers_update_fields();
> void vers_update_end();
> @@ -1759,10 +1767,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 extra2_str_size(size_t len)
> +{ return (len > 255 ? 3 : 1) + len; }
why did you move that? it's still not used anywhere outside of unireg.cc
>
> class select_unit;
> class TMP_TABLE_PARAM;
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 7130b3e5d8a..ea5d739a97e 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -390,7 +385,9 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING &table,
>
> if (create_info->period_info.name)
> {
> - extra2_size+= 1 + extra2_str_size(period_info_len);
> + // two extra2 sections are taken after 10.5
This is a confusing comment, it suggests that both extra2 sections
were added in 10.5. Remove the comment, please, it's not worth it.
> + extra2_size+= 2 + extra2_str_size(period_info_len)
> + + extra2_str_size(without_overlaps_len);
> }
>
> bool has_extra2_field_flags_= has_extra2_field_flags(create_fields);
> diff --git a/sql/unireg.h b/sql/unireg.h
> index 419fbc4bd80..873d6f681fc 100644
> --- a/sql/unireg.h
> +++ b/sql/unireg.h
> @@ -177,7 +177,8 @@ enum extra2_frm_value_type {
>
> EXTRA2_ENGINE_TABLEOPTS=128,
> EXTRA2_FIELD_FLAGS=129,
> - EXTRA2_FIELD_DATA_TYPE_INFO=130
> + EXTRA2_FIELD_DATA_TYPE_INFO=130,
> + EXTRA2_PERIOD_WITHOUT_OVERLAPS=131,
Please, try to create a table that uses WITHOUT OVERLAPS
and open it in 10.4. Just as a test, to make sure it works as expected.
> };
>
> enum extra2_field_flags {
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx
Follow ups
