- developers - lists.mariadb.org

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

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

28 Nov '19

Hi, Julius! On Nov 28, Julius Goryavsky wrote: > Hi, Sergey, > > many thanks for the detailed review, I fixed all the flaws and left only > what raises questions or where I have explanations: > > >> + if ((list= curl_slist_append(list, token_header.c_str())) == NULL || > >> + (list= curl_slist_append(list, "Content-Type: application/json")) > == NULL || > > > > Why do you specify Content-Type header if you aren't sending any content? > > Seems redundant. > > I have already come across servers that respond with the wrong content-type > if the input request had a different content-type or if the input request > does not contain a content-type at all. Fortunately, the Hashicorp server > works without setting the correct content-type in the input, but may be > more reliable to set it explicitly? I don't understand. Content-Type in the request header does not mean that you *accept* application/json, it means you are *sending* application/json. And you are not, you are sending no body at all, just the header. > >> + (vault_ca != NULL && strlen(vault_ca) != 0 && (curl_res= > curl_easy_setopt(curl, CURLOPT_CAINFO , vault_ca)) != CURLE_OK) || > > > How can vault_ca == NULL here? > > I think it cannot, the check is redundant too. > > I don’t know, we have a guarantee that if this option is not set, then the > corresponding variable will always point to an empty zero-terminated > string, but will never just be NULL instead? (This is the path to the file > with the CA bundle. In principle, a Hashicorp Vault can work without it, it > is needed only if there is no root or intermediate certificates on the > other side.) Yes, I believe vault_ca can never be NULL. The default value is "", so it'll be either that or a string specified on the command line. > > > + std::string ver_str = std::string(ver, ver_len); > > > + return strtoul(ver_str.c_str(), NULL, 10); > > > Why do you need to do that? create std::string, copy/reallocate it? > > What's the point? Just do atol(ver) > > Does json_get_object_key return a new zero-terminated string? atol cannot > be applied to a fragment with a length (without trailing zero), as far as I > know. No, it doesn't return a zero-terminated string. It returns a pointer into a valid json string. So you can be sure that ver[ver_len] is a non-digit character (it'll be '\n' or, may be, '}'). Which is enough for atol(). > >> + return ENCRYPTION_KEY_VERSION_INVALID; > >> + return decode_data(key, key_len, dstbuf, buflen); > > > Why? I don't see that in hashicorp docs that the key is base64-encoded > > But after all, as far as I understand at this level, the key for us is > arbitrary binary data? What if it contains all kinds of quotation marks, > commas, or, UTF-8 escape codes and all kinds of unprintable characters like > a zero? I thought that it should be base64-endoded if it is binary data, so > that we can pass it through JSON encoding in the form of text that will not > cause a failure in the JSON parser. I believe that with the proper escaping json can handle any binary data just fine. With your base64 requirement you basically mean that one cannot use 3rd party tools to create/manage keys, unless the tool has an option to store the keys base64-encoded. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

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

27 Nov '19

Hi, Julius! On Nov 27, 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..6de1d9d1f3a > --- /dev/null > +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc > @@ -0,0 +1,330 @@ > +/* 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.h> This is redundant, because plugin_encryption.h includes plugin.h But it does not harm either, so if you want to include it explictly for clarity - sure, do it > +#include <mysql/plugin_encryption.h> > +#include <mysqld_error.h> > +#include <string.h> > +#include <stdlib.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 > + std::ostringstream *read_data = static_cast<std::ostringstream*>(userp); > + size_t ss_pos = read_data->tellp(); > + read_data->seekp(0, std::ios::end); > + size_t number_of_read_bytes = read_data->tellp(); > + read_data->seekp(ss_pos); > + if (number_of_read_bytes + 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 setup_curl_session (CURL *curl, char *curl_errbuf, > + curl_slist **list_ptr, > + std::ostringstream &read_data_stream) > +{ > + struct curl_slist *list = NULL; > + std::string token_header = std::string("X-Vault-Token:") + > + std::string(token); > + CURLcode curl_res = CURLE_OK; > + read_data_stream.str(""); > + read_data_stream.clear(); Seems redundant, the stream was just created > + curl_errbuf[0] = '\0'; > + if ((list= curl_slist_append(list, token_header.c_str())) == NULL || > + (list= curl_slist_append(list, "Content-Type: application/json")) == NULL || Why do you specify Content-Type header if you aren't sending any content? Seems redundant. > + (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, static_cast<void *>(&read_data_stream))) != CURLE_OK || Do you need to cast here? Seems redundant. > + (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) != CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1)) != CURLE_OK || > + (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L)) != CURLE_OK || This is redundant, CURLOPT_SSL_VERIFYPEER is 1 by default, CURLOPT_SSL_VERIFYHOST is 2 by default. But again, if you want to be explicit for the sake of clarity - feel free to > + (vault_ca != NULL && strlen(vault_ca) != 0 && (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO, vault_ca)) != CURLE_OK) || How can vault_ca == NULL here? I think it cannot, the check is redundant too. > + (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 || > + (curl_res = curl_easy_setopt(curl, CURLOPT_HTTP_VERSION, (long)CURL_HTTP_VERSION_1_1)) != CURLE_OK) Why? > + { > + *list_ptr = list; > + my_printf_error(ER_UNKNOWN_ERROR, > + get_error_from_curl(curl_res, curl_errbuf).c_str(), > + ME_ERROR_LOG_ONLY); > + return true; > + } > + *list_ptr = list; > + return false; > +} > + > +static bool curl_run (char * url, const char * * 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; > + } > + if (setup_curl_session(curl, curl_errbuf, &list, read_data_stream) || your setup_curl_session doesn't make much sense now. It could've been useful if one could prepare the session once and then reuse it. but the way you do it now, it's just draws a rather arbitrary line where some one of the curl setup happens inside setup_curl_session function and the rest in the caller. better remove setup_curl_session and put that big if() here. > + (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) > + { > + if (list != NULL) > + { > + 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; > + } > + if (list != NULL) > + { > + curl_slist_free_all(list); > + } > + if (http_code == 404) > + { > + *response = NULL; > + return false; > + } > + *response = read_data_stream.str().c_str(); Really? Who owns this string? Will read_data_stream free it automatically when it gets destructed? > + return http_code < 200 || http_code >= 300; > +} > + > +static unsigned int get_latest_version (uint key_id) > +{ > + char url[2048]; > + 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)) { > + my_printf_error(ER_UNKNOWN_ERROR, "Unable to get key data", 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response_len = strlen(response); > + 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; > + } > + std::string ver_str = std::string(ver, ver_len); > + return strtoul(ver_str.c_str(), NULL, 10); Why do you need to do that? create std::string, copy/reallocate it? What's the point? Just do atol(ver) > +} > + > +static int decode_data (const char *src, int src_len, > + unsigned char* dstbuf, unsigned *buflen) > +{ > + int length_of_memory_needed_for_decode = > + my_base64_needed_decoded_length(src_len); > + char* data= (char *)malloc( > + length_of_memory_needed_for_decode * sizeof(char)); > + int decoded_length= > + my_base64_decode(src, src_len, data, NULL, 0); > + if (decoded_length <= 0) > + { > + memset(data, 0, length_of_memory_needed_for_decode); > + free(data); > + return ENCRYPTION_KEY_BUFFER_TOO_SMALL; > + } > + if (*buflen < (unsigned) decoded_length) { > + *buflen= decoded_length; > + free(data); > + return ENCRYPTION_KEY_BUFFER_TOO_SMALL; > + } > + *buflen= decoded_length; > + memcpy(dstbuf, data, decoded_length); > + free(data); > + return 0; > +} > + > +static unsigned int get_key_from_vault (unsigned int key_id, > + unsigned int key_version, > + unsigned char* dstbuf, > + unsigned int *buflen) > +{ > + char url[2048]; > + 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)) > + { > + my_printf_error(ER_UNKNOWN_ERROR, "Unable to get key data", 0); > + return ENCRYPTION_KEY_VERSION_INVALID; > + } > + response_len = strlen(response); > + 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; > + } > +#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: > + */ > + std::string ver_str = std::string(ver, ver_len); same redundant std::string > + if (strtoul(ver_str.c_str(), NULL, 10) != key_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; > + return decode_data(key, key_len, dstbuf, buflen); Why? I don't see that in hashicorp docs that the key is base64-encoded > +} > + > +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