Hi, Nikita! On Nov 15, Nikita Malyavin wrote:
revision-id: 6ac19d09c66 (mariadb-10.4.4-280-g6ac19d09c66) parent(s): 67ddb6507d5 author: Nikita Malyavin <nikitamalyavin@gmail.com> committer: Nikita Malyavin <nikitamalyavin@gmail.com> 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@mariadb.org