December 2019 - developers - lists.mariadb.org

Re: [Maria-developers] ef2519fee4e: MDEV-16546 System versioning setting to allow history modification
by Aleksey Midenkov 08 Jun '21

08 Jun '21

Hello, Sergei! On Fri, May 3, 2019 at 8:43 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On May 03, Aleksey Midenkov wrote: > > revision-id: ef2519fee4e (versioning-1.0.5-17-gef2519fee4e) > > parent(s): 56145be2951 > > author: Aleksey Midenkov <midenok(a)gmail.com> > > committer: Aleksey Midenkov <midenok(a)gmail.com> > > timestamp: 2018-06-28 13:42:09 +0300 > > message: > > > > MDEV-16546 System versioning setting to allow history modification > > > > 1. Add server variable system_versioning_modify_history which will > > allow to set values for row_start, row_end in DML operations. > > > > 2. If secure_timestamp is YES or REPLICATION, > > system_versioning_modify_history does not have effect. If > > secure_timestamp is SUPER, system_versioning_modify_history requires > > special privilege (same as for setting current timestamp). > > I thought more about this idea. We don't really want to have the history > editable, do we? Well, I'm thinking about rollback table data to specific point in time. That could be a useful feature. > But it's needed for replication, to keep the master and > slave identical. That's what secure_timestamp is for. > > The idea was that this new variable, system_versioning_modify_history, > will be just a convenience feature, it will not allow history editing > any more than one can do without it. > > But now I suspect that even with secure_timestamp=NO one cannot truly > edit history. One can only insert new rows with arbitrary timestamps. > For example, to insert a row with row_start=1000 and row_end=2000, one > needs to do (if secure_timestamp=NO): > > set timestamp=1000; > insert row; > set timestamp=2000; > delete row; > > But I don't see how one can update or delete a history row with > secure_timestamp=NO. > > Now, with a SUPER privilege and secure_timestamp=NO or SUPER, one can > use the BINLOG command and truly edit the history arbitrarily, by faking > row events. I don't really get it why this is so important: since there is some limitation by configuration and privilege, we are just fine. Everything can be changed at filesystem level after all. > > The conclusion, I believe, is that system_versioning_modify_history > should allow INSERTs when secure_timestamp=NO, and it should allow > UPDATE/DELETE only for a SUPER user when secure_timestamp=NO or SUPER. I don't see a reason to argue on that. The only thing that is not clear, why we don't allow INSERTs when secure_timestamp=SUPER? > > The second thing I don't like at all, is when a table is created like > > CREATE TABLE t1 (a int) WITH SYSTEM VERSIONING > > with row_start/row_end implicit. You don't have it in the test, but > anyway one should be able to load history into such a table, while the > table does not have row_start and row_end columns. From the user point > of view these columns don't exist, they're pseudo-columns, like ROWID. > They just cannot be insertable-into, conceptually. But a user will want > to restore the history, right? I don't have a solution for this yet :( > Any ideas? We don't have to follow the conception if it doesn't help us. Since we have physical row_start/row_end, we don't have to pretend they don't exist. Who will win from that? > > See below a couple of minor comments about the patch itself. > > ... These are going to be fixed. > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 15

Re: [Maria-developers] bd98ef106c8: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 20 Feb '20

20 Feb '20

Hi, Aleksey! 1. It should be possible to enable/disable auto-creation. For example, CREATE TABLE ... PARTITION BY SYSTEM_TIME ... PARTITIONS AUTO; this solves few problems at once: * a user explicitly tells when auto-creation should work * you don't need to worry "if this name is already occupied" * you can define that these partitions can never overflow (for INTERVAL) * if you know that AUTO partitions never overflow, you can keep the old behavior for ALTER TABLE ADD PARTITION. 2. Separate thread is an interesting solution. Nicely avoids lots of problems. Still: * it's asynchronous, all tests have to take it into account * it's racy. "low probability" or not, with our number of users they'll hit it and rightfully will report it as a bug * if LOCK TABLE prevents it, partitions can overflow But I think these problems are easier to solve than those we'll face if auto-creation will happen in the main connection thread. So, let's keep your approach with a thread. But instead of going through the parser and mysql_execute_command, create a function that takes a TABLE or a TABLE_SHARE. And adds a partition there. It'll fix the "racy" part. This function can be called from a new thread. As for the LOCK TABLES - if you're under LOCK TABLES, you can simply call that function (to add a partition) directly at the end of the main INSERT/UPDATE/DELETE statement. It'll solve the last problem, LOCK TABLES won't prevent auto-creation. A couple of style comments: 1. create a one-liner make_partition_name (or something) with: sprintf(move_ptr, "p%u", i) and use it both in create_default_partition_names() and in your new code. Just to make sure partition names are always generated uniformly by the same function and if we'll want to change it to "pp%u" it should be done only in one place. 2. don't overload operators, use, say, bool greater_than(size_t seconds) But overall it's pretty good. A nice idea with a separate thread. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Nov 07, Aleksey Midenkov wrote: > revision-id: bd98ef106c8 (mariadb-10.4.7-55-gbd98ef106c8) > parent(s): 6d1fa01e8f1 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:01:14 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > > When there are E empty partitions left, auto-create N new empty > partitions. > > This scheme must not allow partition overflow. I.e. E-fill time must > not exceed N-creation time. This means that low values for INTERVAL > and LIMIT must not be allowed for auto-creation. In case when overflow > is detected there is no need to do anything special: a warning will be > issued and the user will run manual rebuild to redistribute records > correctly. This is important because automatic ADD must be done fast, > without forced rebuild, by the obvious reason. > > Initial version implements hard-coded values of 1 for E and N. As well > as auto-creation threshold MinInterval = 1 hour, MinLimit = 1000. > > The name for newly added partition will be first chosen as "pX", where > X is partition number and "p" is hard-coded name prefix. If this name > is already occupied, the X will be incremented until the resulting > name will be free to use. > > Auto-creation mechanism is applied to every table having LIMIT or > INTERVAL clause. Note that there is no much sense in specifying > explicit partition list in this case and this is covered by > MDEV-19903. The syntax to explicitly turn it on/off as well as > user-defined values for E, N and name prefix is subject for further > discussion and feature requests. > > ALTER TABLE ADD PARTITION is now always fast. If there some history > partition overflow occurs manual ALTER TABLE REBUILD PARTITION is > needed.

2 5

Re: [Maria-developers] 10670133744: MDEV-16978 Application-time periods: WITHOUT OVERLAPS
by Sergei Golubchik 14 Jan '20

14 Jan '20

Hi, Nikita! On Dec 16, Nikita Malyavin wrote: > revision-id: 10670133744 (mariadb-10.4.4-492-g10670133744) > parent(s): 251c6e17269 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > 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(a)mariadb.org

2 2

Re: [Maria-developers] c91ec05e01b: MDEV-20865 Store foreign key info in TABLE_SHARE
by Sergei Golubchik 07 Jan '20

07 Jan '20

Hi, Aleksey! Just a question for now (and a couple of style comments, I didn't look at the logic yet) On Dec 04, Aleksey Midenkov wrote: > revision-id: c91ec05e01b (mariadb-10.4.4-427-gc91ec05e01b) > parent(s): e6d653b448d > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-11-26 13:04:07 +0300 > message: > > MDEV-20865 Store foreign key info in TABLE_SHARE > > 1. Access foreign keys via TABLE_SHARE::foreign_keys and > TABLE_SHARE::referenced_keys; > > 2. Remove handler FK interface: > > - get_foreign_key_list() > - get_parent_foreign_key_list() > - referenced_by_foreign_key() Good, that was the goal > 3. Invalidate referenced shares on: > > - RENAME TABLE > - DROP TABLE > - RENAME COLUMN > - CREATE TABLE > - ADD FOREIGN KEY > > When foreign table is created or altered by the above operations all > referenced shares are closed. This blocks the operation while any > referenced shares are used (when at least one its TABLE instance is > locked). And this is the main question of this email: Why do you close referenced tables? Minor comments below > diff --git a/sql/handler.h b/sql/handler.h > index e913af1d15d..10984505f70 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; > struct HA_CREATE_INFO; > struct st_foreign_key_info; > typedef struct st_foreign_key_info FOREIGN_KEY_INFO; > +class Table_ident; > +class FK_list : public List<FOREIGN_KEY_INFO> > +{ > +public: > + /* Get all referenced tables for foreign key fk_name. */ > + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); > + /* Get all referenced or foreign tables. */ > + bool get(THD *thd, std::set<Table_ident> &result, bool foreign); Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List. But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine. > +}; > typedef bool (stat_print_fn)(THD *thd, const char *type, size_t type_len, > const char *file, size_t file_len, > const char *status, size_t status_len); > diff --git a/sql/lex_string.h b/sql/lex_string.h > index a62609c6b60..769f4dcbf5e 100644 > --- a/sql/lex_string.h > +++ b/sql/lex_string.h > @@ -41,11 +41,47 @@ class Lex_cstring : public LEX_CSTRING > str= start; > length= end - start; > } > + Lex_cstring(const LEX_CSTRING &src) > + { > + str= src.str; > + length= src.length; > + } > void set(const char *_str, size_t _len) > { > str= _str; > length= _len; > } > + Lex_cstring *strdup_root(MEM_ROOT &mem_root) The way you use it, it looks like you really need a constructor, not a strdup. > + { > + Lex_cstring *dst= > + (Lex_cstring *) alloc_root(&mem_root, sizeof(Lex_cstring)); > + if (!dst) > + return NULL; > + if (!str) > + { > + dst->str= NULL; > + dst->length= 0; > + return dst; > + } > + dst->str= (const char *) memdup_root(&mem_root, str, length + 1); > + if (!dst->str) > + return NULL; > + dst->length= length; > + return dst; > + } > + bool operator< (const Lex_cstring& rhs) const > + { > + return length < rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) < 0); > + } > + bool operator== (const Lex_cstring& rhs) const > + { > + return length == rhs.length && 0 == memcmp(str, rhs.str, length); > + } > + bool operator> (const Lex_cstring& rhs) const > + { > + return length > rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) > 0); > + } Nope. We've been here before, haven't we? Don't overload operators. If you want a method to compare, call it cmp(), or greater_than(), or something btw your comparison has quite weird and unconventional semantics > + > }; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 15

Re: [Maria-developers] MDEV-21318: Wrong results with window functions and implicit grouping
by Sergey Petrunia 20 Dec '19

20 Dec '19

Hi Varun, > revision-id: fc34657511a9aa08dd92f7363dc53f58934f9673 (mariadb-10.2.29-62-gfc34657511a) > parent(s): f0aa073f2bf3d8d85b3d028df89cdb4cdfc4002d > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-12-17 16:40:06 +0530 > message: > > MDEV-21318: Wrong results with window functions and implicit grouping > > The issue here is for degenerate joins we should execute the window > function but it is not getting executed in all the cases. > > To get the window function values window function needs to be executed > always. This currently does not happen in few cases > where the join would return 0 or 1 row like > 1) IMPOSSIBLE WHERE > 2) IMPOSSIBLE HAVING > 3) MIN/MAX optimization > 4) EMPTY CONST TABLE > 5) ZERO LIMIT > > The fix is to make sure that window functions get executed > and the temporary table is setup for the execution of window functions > > --- > mysql-test/r/win.result | 51 ++++++++++++++++++++++++++++++++++++++++++++ > mysql-test/t/win.test | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > sql/sql_select.cc | 33 ++++++++++++++++++++++++++++- > sql/sql_select.h | 1 + > 4 files changed, 140 insertions(+), 1 deletion(-) ... > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index c9cb533aa33..f6943b18cee 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1447,6 +1447,7 @@ JOIN::optimize_inner() > zero_result_cause= "Zero limit"; > } > table_count= top_join_tab_count= 0; > + implicit_grouping_with_window_funcs(); not needed + redundant (I'll elaborate below) > error= 0; > goto setup_subq_exit; > } > @@ -1502,6 +1503,7 @@ JOIN::optimize_inner() > zero_result_cause= "No matching min/max row"; > table_count= top_join_tab_count= 0; > error=0; > + implicit_grouping_with_window_funcs(); not needed + redundant (I'll elaborate below) > goto setup_subq_exit; > } > if (res > 1) > @@ -1517,6 +1519,7 @@ JOIN::optimize_inner() > tables_list= 0; // All tables resolved > select_lex->min_max_opt_list.empty(); > const_tables= top_join_tab_count= table_count; > + implicit_grouping_with_window_funcs(); This doesn't seem to be needed? > /* > Extract all table-independent conditions and replace the WHERE > clause with them. All other conditions were computed by opt_sum_query > @@ -1615,6 +1618,7 @@ JOIN::optimize_inner() > zero_result_cause= "no matching row in const table"; > DBUG_PRINT("error",("Error: %s", zero_result_cause)); > error= 0; > + implicit_grouping_with_window_funcs(); OK > goto setup_subq_exit; > } > if (!(thd->variables.option_bits & OPTION_BIG_SELECTS) && > @@ -1639,6 +1643,7 @@ JOIN::optimize_inner() > zero_result_cause= > "Impossible WHERE noticed after reading const tables"; > select_lex->mark_const_derived(zero_result_cause); > + implicit_grouping_with_window_funcs(); Ok > goto setup_subq_exit; > } > > @@ -1781,6 +1786,7 @@ JOIN::optimize_inner() > zero_result_cause= > "Impossible WHERE noticed after reading const tables"; > select_lex->mark_const_derived(zero_result_cause); > + implicit_grouping_with_window_funcs(); > goto setup_subq_exit; There is no test coverage for this case. Please add. > } > > @@ -18225,7 +18231,8 @@ void set_postjoin_aggr_write_func(JOIN_TAB *tab) > } > } > else if (join->sort_and_group && !tmp_tbl->precomputed_group_by && > - !join->sort_and_group_aggr_tab && join->tables_list) > + !join->sort_and_group_aggr_tab && join->tables_list && > + join->top_join_tab_count) > { > DBUG_PRINT("info",("Using end_write_group")); > aggr->set_write_func(end_write_group); > @@ -26924,6 +26931,30 @@ Item *remove_pushed_top_conjuncts(THD *thd, Item *cond) > return cond; > } > > +/* > + There are 5 cases in which we shortcut the join optimization process as we > + conclude that the join would be a degenerate one > + 1) IMPOSSIBLE WHERE > + 2) IMPOSSIBLE HAVING > + 3) MIN/MAX optimization (@see opt_sum_query) > + 4) EMPTY CONST TABLE > + 5) ZERO LIMIT > + If a window function is present in any of the above cases then to get the > + result of the window function, we need to execute it. So we need to Do we really need to do this for cases #2 and #5? In these case, no ouptut is produced, why compute the window functions? > + create a temporary table for its execution. Here we need to take in mind > + that aggregate functions and non-aggregate function need not be executed. > + > +*/ > + > + > +void JOIN::implicit_grouping_with_window_funcs() The name of the function doesn't look right. It's a noun ("subject") while most functions start with verb (e.g. JOIN::optimize_...) handle_implicit_grouping_with_window_funcs() isn't ideal either but would be better I think. > +{ > + if (select_lex->have_window_funcs() && send_row_on_empty_set()) > + { > + const_tables= top_join_tab_count= table_count= 0; > + } > +} > + > /** > @} (end of group Query_Optimizer) > */ BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [MariaDB/server] Proper locking for mysql.gtid_slave_pos truncation (84b437d)
by Kristian Nielsen 18 Dec '19

18 Dec '19

Sergey Vojtovich <notifications(a)github.com> writes: > ATTN @dr-m, @andrelkin, @SachinSetiya, @knielsen So IIUC, this is about incorrect usage of ha_truncate() in rpl_slave_state::truncate_state_table(). This is used only for SET GLOBAL gtid_slave_pos = "..." when all slave threads are stopped and nothing else is accessing the gtid_pos table. So it's fine to use ha_truncate() if that can be done easily (and correctly). But it would also be fine just to loop and delete all rows one by one in a normal transaction, if that is simpler. gtid_slave_pos is a small table, there are normally only a few rows per active replication domain. I'm not myself very familiar with details of metadata locking etc. around ha_truncate(). But looking at the code now, I don't understand why it only truncates one table? If --gtid-pos-auto-engines is in effect, there could be multiple tables... shouldn't they all be cleared when setting the gtid_slave_pos variable? If so, maybe the delete-rows-one-by-one approach is in any case preferable over ha_truncate, since it can then be done transactionally, to not leave an inconsistent gtid_slave_pos state if one truncate fails and another succeeds? Hope this helps, - Kristian.

2 2

Re: [Maria-developers] f72427f463d: MDEV-20923:UBSAN: member access within address Б─╕ which does not point to an object of type 'xid_count_per_binlog'
by andrei.elkin＠pp.inet.fi 17 Dec '19

17 Dec '19

Sujatha, Kristian, howdy. Sorry, I managed to sent my view without actually cc-ing Kristian, as claimed. X-From-Line: nobody Wed Nov 27 19:27:51 2019 From: Andrei Elkin <andrei.elkin(a)mariadb.com> To: sujatha <sujatha.sivakumar(a)mariadb.com> Cc: <commits(a)mariadb.org>, <maria-developers(a)lists.launchpad.net> Subject: Re: f72427f463d: MDEV-20923:UBSAN: member access within address Let me try that again now. Kristian, the question was about `xid_count_per_binlog' struct comment -- .. struct xid_count_per_binlog : public ilink { char *binlog_name; uint binlog_name_len; ulong binlog_id; /* Total prepared XIDs and pending checkpoint requests in this binlog. */ long xid_count; /* For linking in requests to the binlog background thread. */ xid_count_per_binlog *next_in_queue; xid_count_per_binlog(); /* Give link error if constructor used. */ .. -----------------------------^ }; Now I am guessing a constructor was attempted to fail out.. Was that related to de-POD-ing of the object ('cos of the constructor)? It's pretty obscure. We would appreciate your explanation. The former review mail is quoted (except one more [@]-marked view note, Sujatha) further. Cheers, Andrei ------------------------------------------------------------------------------- Sujatha, hello. The patch looks quite good. I have two suggestions though. The first is to match the destructor's free() with actual allocation (of what is to be freed) in the new parameterized constructor. And 2nd-ly, I suggest to leave the explicit zero argument constructor intact. Kristian is cc-d to help us to decide. My comments are below. Cheers, Andrei sujatha <sujatha.sivakumar(a)mariadb.com> writes: > revision-id: f72427f463d316a54ebf87c2e84c73947e3c5fe4 (mariadb-10.1.43-5-gf72427f463d) > parent(s): 13db50fc03e7312e6c01b06c7e4af69f69ba5382 > author: Sujatha > committer: Sujatha > timestamp: 2019-11-12 16:11:16 +0530 > message: > > MDEV-20923:UBSAN: member access within address … which does not point to an object of type 'xid_count_per_binlog' > > Problem: > ------- > Accessing a member within 'xid_count_per_binlog' structure results in > following error when 'UBSAN' is enabled. > > member access within address 0xXXX which does not point to an object of type > 'xid_count_per_binlog' > > Analysis: > --------- > The problem appears to be that no constructor for 'xid_count_per_binlog' is > being called, and thus the vtable will not be initialized. > > Fix: > --- > Defined a parameterized constructor for 'xid_count_per_binlog' class. > > --- > sql/log.cc | 28 ++++++++++++++-------------- > sql/log.h | 9 ++++++++- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/sql/log.cc b/sql/log.cc > index acf1f8f8a9c..2b8b67febef 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -3216,7 +3216,7 @@ void MYSQL_BIN_LOG::cleanup() > DBUG_ASSERT(!binlog_xid_count_list.head()); > WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::cleanup(): Removing xid_list_entry " > "for %s (%lu)", b); > - my_free(b); > + delete b; > } > > mysql_mutex_destroy(&LOCK_log); > @@ -3580,17 +3580,17 @@ bool MYSQL_BIN_LOG::open(const char *log_name, > */ > uint off= dirname_length(log_file_name); > uint len= strlen(log_file_name) - off; > - char *entry_mem, *name_mem; > - if (!(new_xid_list_entry = (xid_count_per_binlog *) > - my_multi_malloc(MYF(MY_WME), > - &entry_mem, sizeof(xid_count_per_binlog), > - &name_mem, len, > - NULL))) > + char *name_mem; > + name_mem= (char *) my_malloc(len, MYF(MY_ZEROFILL)); > + if (!name_mem) > goto err; > memcpy(name_mem, log_file_name+off, len); That is both my_malloc and memcpy to go into the new constructor to match [*] (find the label below). > - new_xid_list_entry->binlog_name= name_mem; > - new_xid_list_entry->binlog_name_len= len; > - new_xid_list_entry->xid_count= 0; > + new_xid_list_entry= new xid_count_per_binlog(name_mem, (int)len); > + if (!new_xid_list_entry) > + { > + my_free(name_mem); > + goto err; > + } > > /* > Find the name for the Initial binlog checkpoint. > @@ -3711,7 +3711,7 @@ bool MYSQL_BIN_LOG::open(const char *log_name, > { > WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::open(): Removing xid_list_entry for " > "%s (%lu)", b); > - my_free(binlog_xid_count_list.get()); > + delete binlog_xid_count_list.get(); > } > mysql_cond_broadcast(&COND_xid_list); > WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::open(): Adding new xid_list_entry for " > @@ -3758,7 +3758,7 @@ Turning logging off for the whole duration of the MySQL server process. \ > To turn it on again: fix the cause, \ > shutdown the MySQL server and restart it.", name, errno); > if (new_xid_list_entry) > - my_free(new_xid_list_entry); > + delete new_xid_list_entry; [@] With `delete` we don't need the non-null check: `if()` to remove. > if (file >= 0) > mysql_file_close(file, MYF(0)); > close(LOG_CLOSE_INDEX); > @@ -4252,7 +4252,7 @@ bool MYSQL_BIN_LOG::reset_logs(THD *thd, bool create_new_log, > DBUG_ASSERT(b->xid_count == 0); > WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::reset_logs(): Removing " > "xid_list_entry for %s (%lu)", b); > - my_free(binlog_xid_count_list.get()); > + delete binlog_xid_count_list.get(); > } > mysql_cond_broadcast(&COND_xid_list); > reset_master_pending--; > @@ -9736,7 +9736,7 @@ TC_LOG_BINLOG::mark_xid_done(ulong binlog_id, bool write_checkpoint) > break; > WSREP_XID_LIST_ENTRY("TC_LOG_BINLOG::mark_xid_done(): Removing " > "xid_list_entry for %s (%lu)", b); > - my_free(binlog_xid_count_list.get()); > + delete binlog_xid_count_list.get(); > } > > mysql_mutex_unlock(&LOCK_xid_list); > diff --git a/sql/log.h b/sql/log.h > index b4c9b24a3a9..30a55e577a4 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -587,7 +587,14 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > long xid_count; > /* For linking in requests to the binlog background thread. */ > xid_count_per_binlog *next_in_queue; > - xid_count_per_binlog(); /* Give link error if constructor used. */ Maybe we should leave it as it seems to be for catching inadvertent object constructions. I'm cc-ing Kristian to clear out. > + xid_count_per_binlog(char *log_file_name, uint log_file_name_len) > + :binlog_name(log_file_name), binlog_name_len(log_file_name_len), > + binlog_id(0), xid_count(0) > + {} > + ~xid_count_per_binlog() > + { > + my_free(binlog_name); [*] > + } > }; > I_List<xid_count_per_binlog> binlog_xid_count_list; > mysql_mutex_t LOCK_binlog_background_thread; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

Re: [Maria-developers] 6ac19d09c66: MDEV-16978 Application-time periods: WITHOUT OVERLAPS
by Sergei Golubchik 17 Dec '19

17 Dec '19

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

2 20

[Maria-developers] MariaDB for CentOS 8
by Thomas Stephen Lee 12 Dec '19

12 Dec '19

Hi, Please release MariaDB 10.4 for CentOS 8. https://downloads.mariadb.org/mariadb/repositories/ does not show CentOS 8. thanks -- Thomas Stephen Lee

3 5

Re: [Maria-developers] 387839e44cc: Plugin for Hashicorp Vault Key Management System
by Sergei Golubchik 04 Dec '19

04 Dec '19

Hi, Julius! Just a couple of comments, see below: On Dec 04, Julius Goryavsky wrote: > revision-id: 387839e44cc (mariadb-10.4.10-179-g387839e44cc) > parent(s): 8115a02283d > author: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > committer: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > timestamp: 2019-11-15 13:37:35 +0100 > message: > > Plugin for Hashicorp Vault Key Management System > diff --git a/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > new file mode 100644 > index 00000000000..32c0e6417a3 > --- /dev/null > +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc ... > + if (tolen_len > max_token_size) > + { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Maximum allowed token length exceeded", > + 0); > + return true; > + } > + size_t buf_len = x_vault_token_len + tolen_len + 1; typo: "token_len" I think > +#ifdef _WIN32 > + char *token_header = (char *) _alloca(sizeof(char) * buf_len); > +#else > + char *token_header = (char *) alloca(sizeof(char) * buf_len); > +#endif Please, remove all these ifdefs and add one at the very top of the file: #ifdef _WIN32 #define alloca _alloca #endif Here I presume that you've tested it in buildbot and alloca is indeed present on all builders and we don't need a fallback here. Also remove sizeof(char) everywhere, it's always 1. C99 standard says 6.5.3.4 The sizeof operator ... When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1. > + snprintf(token_header, buf_len, "%s%s", x_vault_token, token); > + curl_errbuf[0] = '\0'; > + if ((list= curl_slist_append(list, token_header)) == NULL || > + (curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, > + write_response_memory)) != CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA, > + &read_data_stream)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) != > + CURLE_OK || ... > + /* > + If the result of the conversion does not fit into an > + unsigned integer or if an error (for example, overflow) > + occurred during the conversion, then we need to return > + an error: > + */ > +#if ULONG_MAX > UINT_MAX > + if (version > UINT_MAX || errno) > +#else > + if (errno) > +#endif did you verify that otherwise it makes compilation to fail? better to keep the number of ifdefs in the code to a minimum, so don't add this one unless absolutely necessary > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + return (unsigned int) version; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 3a11b49bb5b: MDEV-20900: IN predicate to IN subquery conversion causes performance regression
by Sergey Petrunia 03 Dec '19

03 Dec '19

Hi Varun, As far as I understand, the intent of the patch is that this check: + for (uint i=0; i < n; i++) + { + if (item1->element_index(i)->cmp_type() != + item2->element_index(i)->cmp_type()) matches this check in subquery_types_allow_materialization() if (!inner->type_handler()->subquery_type_allows_materialization(inner, outer)) which has implementations like so: bool Type_handler_real_result:: subquery_type_allows_materialization(const Item *inner, const Item *outer) const { DBUG_ASSERT(inner->cmp_type() == REAL_RESULT); return outer->cmp_type() == REAL_RESULT; } // .. and the same for other datatypes ... I would like to see comments in both places (cmp_row_types and subquery_types_allow_materialization()) explaining that. Note that some Type_handler::subquery_types_allow_materialization() have a more complex implementation, for example Type_handler_string_result::subquery_types_allow_materialization() compares collations. Is there any reasons why cmp_row_types() doesn't use the same call as subquery_types_allow_materialization does: if (!inner->type_handler()->subquery_type_allows_materialization(inner, outer)) ? On Mon, Nov 04, 2019 at 04:38:02PM +0530, Varun wrote: > revision-id: 3a11b49bb5b716538f98c6a212bbbfa6fc9b7a88 (mariadb-10.3.17-145-g3a11b49bb5b) > parent(s): 162f475c4be81dfbceed093ad03d114b4c69a3c0 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-11-04 16:28:25 +0530 > message: > > MDEV-20900: IN predicate to IN subquery conversion causes performance regression > > Disable the IN predicate to IN subquery conversion when the types on the left and > right hand side of the IN predicate are not of comparable type. > > --- > mysql-test/main/opt_tvc.result | 53 ++++++++++++++++++++++++++++++++++++++---- > mysql-test/main/opt_tvc.test | 31 ++++++++++++++++++++++++ > sql/sql_tvc.cc | 27 ++++++++++++++++++++- > 3 files changed, 106 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/main/opt_tvc.result b/mysql-test/main/opt_tvc.result > index 5329a9f64be..a68e70e8a25 100644 > --- a/mysql-test/main/opt_tvc.result > +++ b/mysql-test/main/opt_tvc.result > @@ -629,11 +629,9 @@ SELECT * FROM t1 WHERE i IN (NULL, NULL, NULL, NULL, NULL); > i > EXPLAIN EXTENDED SELECT * FROM t1 WHERE i IN (NULL, NULL, NULL, NULL, NULL); > id select_type table type possible_keys key key_len ref rows filtered Extra > -1 PRIMARY t1 ALL NULL NULL NULL NULL 3 100.00 > -1 PRIMARY <derived3> ALL NULL NULL NULL NULL 5 100.00 Using where; FirstMatch(t1); Using join buffer (flat, BNL join) > -3 DERIVED NULL NULL NULL NULL NULL NULL NULL NULL No tables used > +1 SIMPLE t1 ALL NULL NULL NULL NULL 3 100.00 Using where > Warnings: > -Note 1003 /* select#1 */ select `test`.`t1`.`i` AS `i` from `test`.`t1` semi join ((values (NULL),(NULL),(NULL),(NULL),(NULL)) `tvc_0`) where `test`.`t1`.`i` = `tvc_0`.`_col_1` > +Note 1003 select `test`.`t1`.`i` AS `i` from `test`.`t1` where `test`.`t1`.`i` in (NULL,NULL,NULL,NULL,NULL) > SET in_predicate_conversion_threshold= default; > DROP TABLE t1; > # > @@ -687,3 +685,50 @@ f1 f2 > 1 1 > DROP TABLE t1,t2,t3; > SET @@in_predicate_conversion_threshold= default; > +# > +# MDEV-20900: IN predicate to IN subquery conversion causes performance regression > +# > +create table t1(a int, b int); > +insert into t1 select seq-1, seq-1 from seq_1_to_10; > +set in_predicate_conversion_threshold=2; > +explain select * from t1 where t1.a IN ("1","2","3","4"); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where t1.a IN ("1","2","3","4"); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=0; > +explain select * from t1 where t1.a IN ("1","2","3","4"); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where t1.a IN ("1","2","3","4"); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=2; > +explain select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +set in_predicate_conversion_threshold=0; > +explain select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 Using where > +select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +a b > +1 1 > +2 2 > +3 3 > +4 4 > +drop table t1; > +SET @@in_predicate_conversion_threshold= default; > diff --git a/mysql-test/main/opt_tvc.test b/mysql-test/main/opt_tvc.test > index 7319dbdc9e8..e4e8c6d7919 100644 > --- a/mysql-test/main/opt_tvc.test > +++ b/mysql-test/main/opt_tvc.test > @@ -3,6 +3,7 @@ > # > source include/have_debug.inc; > source include/default_optimizer_switch.inc; > +source include/have_sequence.inc; > > create table t1 (a int, b int); > > @@ -397,3 +398,33 @@ SELECT * FROM t3 WHERE (f1,f2) IN ((2, 2), (1, 2), (3, 5), (1, 1)); > DROP TABLE t1,t2,t3; > > SET @@in_predicate_conversion_threshold= default; > + > +--echo # > +--echo # MDEV-20900: IN predicate to IN subquery conversion causes performance regression > +--echo # > + > +create table t1(a int, b int); > +insert into t1 select seq-1, seq-1 from seq_1_to_10; > + > +set in_predicate_conversion_threshold=2; > + > +let $query= select * from t1 where t1.a IN ("1","2","3","4"); > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=0; > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=2; > +let $query= select * from t1 where (t1.a,t1.b) in (("1","1"),(2,2),(3,3),(4,4)); > +eval explain $query; > +eval $query; > + > +set in_predicate_conversion_threshold=0; > +eval explain $query; > +eval $query; > + > +drop table t1; > +SET @@in_predicate_conversion_threshold= default; > + > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > index 816c6fe1089..78c7c34a81a 100644 > --- a/sql/sql_tvc.cc > +++ b/sql/sql_tvc.cc > @@ -796,6 +796,31 @@ bool Item_subselect::wrap_tvc_into_select(THD *thd, st_select_lex *tvc_sl) > } > > > +/* > + @brief > + Check whether the items are of comparable type or not > + > + @retval > + 0 comparable > + 1 not comparable > +*/ > + > +static bool cmp_row_types(Item* item1, Item* item2) > +{ > + uint n= item1->cols(); > + if (item2->check_cols(n)) > + return true; > + > + for (uint i=0; i < n; i++) > + { > + if (item1->element_index(i)->cmp_type() != > + item2->element_index(i)->cmp_type()) > + return true; > + } > + return false; > +} > + > + > /** > @brief > Transform IN predicate into IN subquery > @@ -843,7 +868,7 @@ Item *Item_func_in::in_predicate_to_in_subs_transformer(THD *thd, > > for (uint i=1; i < arg_count; i++) > { > - if (!args[i]->const_item()) > + if (!args[i]->const_item() || cmp_row_types(args[0], args[i])) > return this; > } > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 387839e44cc: Plugin for Hashicorp Vault Key Management System
by Sergei Golubchik 02 Dec '19

02 Dec '19

Hi, Julius! Looks good! A couple of minor comments, see below On Dec 02, Julius Goryavsky wrote: > revision-id: 387839e44cc (mariadb-10.4.10-179-g387839e44cc) > parent(s): 8115a02283d > author: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > committer: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > timestamp: 2019-11-15 13:37:35 +0100 > message: > > Plugin for Hashicorp Vault Key Management System > > diff --git a/plugin/hashicorp_key_management/CMakeLists.txt b/plugin/hashicorp_key_management/CMakeLists.txt > new file mode 100644 > index 00000000000..4b5f79afa66 > --- /dev/null > +++ b/plugin/hashicorp_key_management/CMakeLists.txt > @@ -0,0 +1,11 @@ > +INCLUDE(FindCURL) > +IF(NOT CURL_FOUND) > + # Can't build plugin > + RETURN() > +ENDIF() > + > +INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/sql ${CURL_INCLUDE_DIR}) > + > +MYSQL_ADD_PLUGIN(HASHICORP_KEY_MANAGEMENT > + hashicorp_key_management_plugin.cc > + LINK_LIBRARIES ${CURL_LIBRARIES}) > diff --git a/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > new file mode 100644 > index 00000000000..26ee7b7cd3d > --- /dev/null > +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > @@ -0,0 +1,325 @@ > +/* Copyright (C) 2019 MariaDB Corporation > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#include <mysql/plugin_encryption.h> > +#include <mysqld_error.h> > +#include <string.h> > +#include <stdlib.h> > +#include <limits.h> > +#include <errno.h> > +#include <string> > +#include <sstream> > +#include <curl/curl.h> > + > +static char* vault_url; > +static char* token; > +static char* vault_ca; > + > +static MYSQL_SYSVAR_STR(vault_ca, vault_ca, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY, > + "Path to Certificate Authority (CA) bundle (is a file " > + "that contains root and intermediate certificates)", > + NULL, NULL, ""); > + > +static MYSQL_SYSVAR_STR(vault_url, vault_url, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY, > + "HTTP[s] URL that is used to connect to the Hashicorp Vault server", > + NULL, NULL, "https://127.0.0.1:8200/v1/secret"); > + > +static MYSQL_SYSVAR_STR(token, token, > + PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY | PLUGIN_VAR_NOSYSVAR, > + "Authentication token that passed to the Hashicorp Vault " > + "in the request header", > + NULL, NULL, ""); > + > +static struct st_mysql_sys_var* settings[] = { > + MYSQL_SYSVAR(vault_url), > + MYSQL_SYSVAR(token), > + MYSQL_SYSVAR(vault_ca), > + NULL > +}; > + > +static std::string get_error_from_curl (CURLcode curl_code, char *curl_errbuf) > +{ > + size_t len = strlen(curl_errbuf); > + std::ostringstream stream; > + if (curl_code != CURLE_OK) > + { > + stream << "CURL returned this error code: " << curl_code; > + stream << " with error message : "; > + if (len) > + stream << curl_errbuf; > + else > + stream << curl_easy_strerror(curl_code); > + } > + return stream.str(); > +} > + > +#define max_response_size 65536 > + > +static size_t write_response_memory (void *contents, size_t size, size_t nmemb, > + void *userp) > +{ > + size_t realsize = size * nmemb; > + if (size != 0 && realsize / size != nmemb) > + return 0; // overflow detected > + std::ostringstream *read_data = static_cast<std::ostringstream*>(userp); > + size_t current_length = read_data->tellp(); > + if (current_length + realsize > max_response_size) > + return 0; // response size limit exceeded > + read_data->write(static_cast<char*>(contents), realsize); > + if (!read_data->good()) > + return 0; > + return realsize; > +} > + > +#define timeout 300 > + > +static bool curl_run (char *url, std::string *response) > +{ > + char curl_errbuf [CURL_ERROR_SIZE]; > + struct curl_slist *list = NULL; > + std::ostringstream read_data_stream; > + CURLcode curl_res = CURLE_OK; > + long http_code = 0; > + CURL *curl = curl_easy_init(); > + if (curl == NULL) > + { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Cannot initialize curl session", > + ME_ERROR_LOG_ONLY); > + return true; > + } > + std::string token_header = std::string("X-Vault-Token:") + > + std::string(token); > + curl_errbuf[0] = '\0'; > + if ((list= curl_slist_append(list, token_header.c_str())) == NULL || > + (curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, > + write_response_memory)) != CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA, > + &read_data_stream)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) != > + CURLE_OK || > + /* > + The options CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST are > + set explicitly to withstand possible future changes in curl defaults: > + */ > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L)) != > + CURLE_OK || > + (vault_ca != NULL && strlen(vault_ca) != 0 && vault_ca still cannot be NULL :) > + (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO, vault_ca)) != > + CURLE_OK) || > + (curl_res= curl_easy_setopt(curl, CURLOPT_USE_SSL, CURLUSESSL_ALL)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, timeout)) != > + CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_TIMEOUT, timeout)) != > + CURLE_OK) > + { if you get an error here you won't free list, so it'll be a memory leak. In fact, I don't understand why you need two if() here, just combine them in one, the error message is the same anyway, and the second if() already has curl_slist_free_all(). > + my_printf_error(ER_UNKNOWN_ERROR, > + get_error_from_curl(curl_res, curl_errbuf).c_str(), > + ME_ERROR_LOG_ONLY); > + return true; > + } > + if ((curl_res = curl_easy_setopt(curl, CURLOPT_URL, url)) != CURLE_OK || > + (curl_res = curl_easy_perform(curl)) != CURLE_OK || > + (curl_res = curl_easy_getinfo (curl, CURLINFO_RESPONSE_CODE, > + &http_code)) != CURLE_OK) > + { > + curl_slist_free_all(list); > + my_printf_error(ER_UNKNOWN_ERROR, > + get_error_from_curl(curl_res, curl_errbuf).c_str(), > + ME_ERROR_LOG_ONLY); > + return true; > + } > + curl_slist_free_all(list); > + if (http_code == 404) > + { > + *response = std::string(""); > + return false; > + } > + *response = read_data_stream.str(); > + return http_code < 200 || http_code >= 300; > +} > + > +static unsigned int get_latest_version (uint key_id) > +{ > + char url[2048]; > + std::string response_str; > + const char *response; > + const char *js, *ver; > + int js_len, ver_len; > + size_t response_len; > + snprintf(url, sizeof(url), "%s/%u", vault_url, key_id); > + if (curl_run(url, &response_str)) { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Unable to get key data", > + 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response = response_str.c_str(); > + response_len = response_str.size(); > + if (json_get_object_key(response, response + response_len, > + "metadata", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js+js_len,"version", > + &ver, &ver_len) != JSV_NUMBER) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + /* > + Here we may use atoi, but it has undefined overflow behavior > + and does not work with unsigned integers, so use strtoul: > + */ > + unsigned long version = strtoul(ver, NULL, 10); > + /* > + If the result of the conversion does not fit into an > + unsigned integer or if an error (for example, overflow) > + occurred during the conversion, then we need to return > + an error: > + */ > + if (version > UINT_MAX || errno) this might be an error with gcc, "condition is always true" double check this please, no need to fix anything if there's no error > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + return version; this might be an error with VS, because of the implicit ulong->uint cast. double check this please, no need to fix anything if there's no error > +} > + > +static unsigned int get_key_from_vault (unsigned int key_id, > + unsigned int key_version, > + unsigned char *dstbuf, > + unsigned int *buflen) > +{ > + char url[2048]; > + std::string response_str; > + const char *response; > + const char *js, *ver, *key; > + int js_len, key_len, ver_len; > + size_t response_len; > + if (key_version != ENCRYPTION_KEY_VERSION_INVALID) > + snprintf(url, sizeof(url), "%s/%u?%u", vault_url, key_id, key_version); > + else > + snprintf(url, sizeof(url), "%s/%u", vault_url, key_id); > + if (curl_run(url, &response_str)) > + { > + my_printf_error(ER_UNKNOWN_ERROR, > + "Unable to get key data", > + 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response = response_str.c_str(); > + response_len = response_str.size(); > +#ifndef NDEBUG > + /* > + An internal check that is needed only for debugging the plugin > + operation - in order to ensure that we get from the Hashicorp Vault > + server exactly the version of the key that is needed: > + */ > + if (key_version != ENCRYPTION_KEY_VERSION_INVALID) > + { > + if (json_get_object_key(response, response + response_len, > + "metadata", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js+js_len,"version", > + &ver, &ver_len) != JSV_NUMBER) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + /* > + Here we may use atoi, but it has undefined overflow behavior > + and does not work with unsigned integers, so use strtoul: > + */ > + unsigned long version = strtoul(ver, NULL, 10); > + /* > + If the result of the conversion does not fit into an > + unsigned integer or if an error (for example, overflow) > + occurred during the conversion, then we need to return > + an error: > + */ > + if (errno || key_version != version) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + } > +#endif > + if (json_get_object_key(response, response + response_len, > + "data", &js, &js_len) != JSV_OBJECT) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (json_get_object_key(js, js + js_len, "data", > + &key, &key_len) != JSV_STRING) > + { > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + if (*buflen < (unsigned) key_len) { > + *buflen= key_len; > + return ENCRYPTION_KEY_BUFFER_TOO_SMALL; > + } > + *buflen= key_len; > + memcpy(dstbuf, key, key_len); > + return 0; > +} > + > +struct st_mariadb_encryption hashicorp_key_management_plugin= { > + MariaDB_ENCRYPTION_INTERFACE_VERSION, > + get_latest_version, > + get_key_from_vault, > + 0, 0, 0, 0, 0 > +}; > + > +static int hashicorp_key_management_plugin_init(void *p) > +{ > + curl_global_init(CURL_GLOBAL_ALL); > + return 0; > +} > + > +static int hashicorp_key_management_plugin_deinit(void *p) > +{ > + curl_global_cleanup(); > + return 0; > +} > + > +/* > + Plugin library descriptor > +*/ > +maria_declare_plugin(hashicorp_key_management) > +{ > + MariaDB_ENCRYPTION_PLUGIN, > + &hashicorp_key_management_plugin, > + "hashicorp_key_management", > + "MariaDB Corporation", > + "HashiCorp Vault key management plugin", > + PLUGIN_LICENSE_GPL, > + hashicorp_key_management_plugin_init, > + hashicorp_key_management_plugin_deinit, > + 0x0100 /* 1.0 */, > + NULL, /* status variables */ > + settings, > + "1.0", > + MariaDB_PLUGIN_MATURITY_ALPHA > +} > +maria_declare_plugin_end; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0