- developers - lists.mariadb.org

Re: [Maria-developers] 4af7a583802: Refactor MYSQL_BIN_LOG: extract Event_log ancestor
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 12, Nikita Malyavin wrote: > revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802) > parent(s): 2da3dc5d422 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-08 20:59:09 +1000 > message: > > Refactor MYSQL_BIN_LOG: extract Event_log ancestor Would be good to have some explanation here. I understand that you'll use this refactoring in following commits, but conceptually, what Event_log is? I mean, like Event_log is a log where Log_event_xxx events are written, but not _something else that MYSQL_BIN_LOG is and Event_log isn't_ or Event_log is a log where Log_event_xxx events are written. While MYSQL_BIN_LOG is Event_log that also does .... > diff --git a/sql/log.cc b/sql/log.cc > index 827a34c2883..6f4e3191588 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name, > write_file_name_to_index_file= 1; > } a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open > diff --git a/sql/log.h b/sql/log.h > index 73cd66d5a4a..74c409e1ac7 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -327,6 +327,7 @@ class MYSQL_LOG > bool strip_ext, char *buff); > virtual int generate_new_name(char *new_name, const char *log_name, > ulong next_log_number); > + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } > protected: > /* LOCK_log is inited by init_pthread_objects() */ > mysql_mutex_t LOCK_log; > @@ -376,6 +377,64 @@ struct Rows_event_factory > } > }; > > +class Event_log: public MYSQL_LOG > +{ > +public: > +#if !defined(MYSQL_CLIENT) > + Rows_log_event* > + prepare_pending_rows_event(THD *thd, TABLE* table, > + binlog_cache_data *cache_data, > + uint32 serv_id, size_t needed, > + bool is_transactional, > + Rows_event_factory event_factory); forgot to delete the same from MYSQL_BIN_LOG > +#endif > + > + bool flush_and_sync(bool *synced); same > + void cleanup() > + { > + if (inited) > + mysql_mutex_destroy(&LOCK_binlog_end_pos); > + > + MYSQL_LOG::cleanup(); > + } > + void init_pthread_objects() > + { > + MYSQL_LOG::init_pthread_objects(); > + > + mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos, > + MY_MUTEX_INIT_SLOW); > + } > + > + bool open( > + const char *log_name, > + enum_log_type log_type, > + const char *new_name, ulong next_file_number, > + enum cache_type io_cache_type_arg); > + IO_CACHE *get_log_file() { return &log_file; } > + > + int write_description_event(enum_binlog_checksum_alg checksum_alg, > + bool encrypt, bool dont_set_created); > + > + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); > +}; > + > /* Tell the io thread if we can delay the master info sync. */ > #define SEMI_SYNC_SLAVE_DELAY_SYNC 1 > /* Tell the io thread if the current event needs a ack. */ > @@ -451,7 +510,7 @@ class binlog_cache_data; > struct rpl_gtid; > struct wait_for_commit; > > -class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > +class MYSQL_BIN_LOG: public TC_LOG, private Event_log > { > /** The instrumentation key to use for @ LOCK_index. */ > PSI_mutex_key m_key_LOCK_index; > @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > > - bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); > + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file) > + { return Event_log::write_event(ev, data, file); } Perhaps using Event_log::write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); ? > bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); } > > bool write_event_buffer(uchar* buf,uint len); > @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > uint next_file_id(); > inline char* get_index_fname() { return index_file_name;} > inline char* get_log_fname() { return log_file_name; } > - inline char* get_name() { return name; } > inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } should remove that too, I reckon > inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; } > inline IO_CACHE* get_log_file() { return &log_file; } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f3603cbfaeb: report progress
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: f3603cbfaeb (mariadb-10.5.2-483-gf3603cbfaeb) > parent(s): 3b6038bee88 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > report progress Perhaps, squash this with your commit "test progress reporting" ? > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 29d0f487303..87dbe226c2e 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -11300,6 +11300,9 @@ static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi, > > IO_CACHE *log_file= log->flip(); > > + thd_progress_report(thd, 0, my_b_write_tell(log_file) > + / rgi->tables_to_lock->m_conv_table->s->reclength); 1. why do you do this "/ rgi->...->s->reclength" ? 2. How does it work with your "cache flipping"? While data is copied between tables, concurrent connections write into an IO_CACHE. Then you flip, they what, write into the second IO_CACHE? What file length will you use here in thd_progress_report()? > + > do > { > const auto *descr_event= rgi->rli->relay_log.description_event_for_exec; > @@ -11319,6 +11322,9 @@ static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi, > free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC)); > if (ev != rgi->rli->relay_log.description_event_for_exec) > delete ev; > + thd_progress_report(thd, my_b_tell(log_file) > + / rgi->tables_to_lock->m_conv_table->s->reclength, > + thd->progress.max_counter); > } > while(!error); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 9277b838aad: Forbid savepoints setup while ONLINE ALTER goes on
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 9277b838aad (mariadb-10.5.2-485-g9277b838aad) > parent(s): 37b6f5cb8ef > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > Forbid savepoints setup while ONLINE ALTER goes on Why is that? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 903ae1c03ae: Fix running without binlog
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 903ae1c03ae (mariadb-10.5.2-479-g903ae1c03ae) > parent(s): a5fca9a6e30 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-27 17:28:05 +1000 > message: > > Fix running without binlog Strange. With such a comment I'd expect you add a test for ALTER TABLE with disabled binlog. Instead you change an error to a warning in one of the existing tests. This test change doesn't match the commit comment. > diff --git a/mysql-test/suite/binlog/r/online_alter.result b/mysql-test/suite/binlog/r/online_alter.result > index 9bca94b8a87..bb3fa85e76c 100644 > --- a/mysql-test/suite/binlog/r/online_alter.result > +++ b/mysql-test/suite/binlog/r/online_alter.result > @@ -34,9 +34,9 @@ insert into t1 values (123), (456), (789); > set debug_sync= 'now SIGNAL end'; > connection default; > Warnings: > -Error 1364 Field 'b' doesn't have a default value > -Error 1364 Field 'b' doesn't have a default value > -Error 1364 Field 'b' doesn't have a default value > +Warning 1364 Field 'b' doesn't have a default value > +Warning 1364 Field 'b' doesn't have a default value > +Warning 1364 Field 'b' doesn't have a default value > select * from t1; > a b > 5 0 > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index f0d1167ad61..e348056646d 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -11308,9 +11308,12 @@ static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi, > break; > > ev->thd= thd; > + bool abort_on_warning= thd->abort_on_warning; > + thd->abort_on_warning= false; > thd->set_n_backup_active_arena(&event_arena, &backup_arena); > error= ev->apply_event(rgi); > thd->restore_active_arena(&event_arena, &backup_arena); > + thd->abort_on_warning= abort_on_warning; > > event_arena.free_items(); > free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC)); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Review for: MDEV-27036: Add asserts to detect duplicate JSON keys
by Sergey Petrunia 24 Nov '21

24 Nov '21

Hi Sergei, Please find my input below. > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 0dfe95e81b0..fa07608177a 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -7910,28 +7910,27 @@ best_access_path(JOIN *join, > can make an adjustment is a special case of the criteria used > in ReuseRangeEstimateForRef-3. > */ > - if (table->opt_range_keys.is_set(key) && > + auto use_range_estimates= table->opt_range_keys.is_set(key) && > (const_part & What is the reason for this change? Please restore the old code. > (((key_part_map)1 << table->opt_range[key].key_parts)-1)) == > (((key_part_map)1 << table->opt_range[key].key_parts)-1) && > table->opt_range[key].ranges == 1 && > - records > (double) table->opt_range[key].rows) > + records > (double) table->opt_range[key].rows; > + if (use_range_estimates) > { > records= (double) table->opt_range[key].rows; > trace_access_idx.add("used_range_estimates", "clipped down"); > } > else > { > + trace_access_idx.add("used_range_estimates",false); > if (table->opt_range_keys.is_set(key)) > { > - trace_access_idx.add("used_range_estimates",false) > - .add("cause", > - "not better than ref estimates"); > + cause= "not better than ref estimates"; As we have already discussed: Do not assign this to "cause". To avoid duplicate names, change the name above from "cause" to e.g. "reason". > } > else > { > - trace_access_idx.add("used_range_estimates", false) > - .add("cause", "not available"); > + cause= "not available"; > } The same as above. > } > } > @@ -8217,9 +8215,10 @@ best_access_path(JOIN *join, > best_uses_jbuf= TRUE; > best_filter= 0; > best_type= JT_HASH; > + Json_writer_object trace_access_hash(thd); > trace_access_hash.add("type", "hash"); > trace_access_hash.add("index", "hj-key"); > - trace_access_hash.add("cost", rnd_records); > + trace_access_hash.add("est_records_cost", rnd_records); Please use "rnd_records" instead of "est_records_cost". > trace_access_hash.add("cost", best); > trace_access_hash.add("chosen", true); > } > commit 0e8b82dbcbd8ae96e0758114c87e48061d603c5f > Author: Sergei Krivonos <sergei.krivonos(a)mariadb.com> > Date: Fri Nov 12 03:36:10 2021 +0200 > > MDEV-27036: add assert to detect duplicated JSON keys > > diff --git a/sql/my_json_writer.cc b/sql/my_json_writer.cc > index 9470ba57855..501f0b72318 100644 > --- a/sql/my_json_writer.cc > +++ b/sql/my_json_writer.cc > @@ -39,6 +41,9 @@ inline void Json_writer::on_start_object() > #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST) > if(!fmt_helper.is_making_writer_calls()) > { > + if(got_name != named_item_expected()){ > + std::cout <<"got_name["<<got_name<<"] != named_item_expected["<<named_item_expected()<<']'<<std::endl; > + } First: as I've already mentioned, please don't write anything to to stderr or stdout directly. Use the sql_print_error() function. Second: the message is not informative. The above will print something like: got_name[1] != named_item_expected[0] which is not at all informative. What's "got_name[1]" ? Please change the printout to be something like: Json_writer: attempt to write invalid JSON. Last data in JSON: %s here %s should print last 256 chars in the JSON document. > VALIDITY_ASSERT(got_name == named_item_expected()); > named_items_expectation.push_back(true); > } > @@ -140,7 +150,19 @@ Json_writer& Json_writer::add_member(const char *name, size_t len) > } > #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST) > if (!fmt_helper.is_making_writer_calls()) > + { > + VALIDITY_ASSERT(!got_name); > got_name= true; > + VALIDITY_ASSERT(named_items.size()); > + auto& named_items_keys= named_items.top(); > + auto emplaced= named_items_keys.emplace(name, len); > + auto is_uniq_key= emplaced.second; > + if(!is_uniq_key) > + { > + std::cerr << "Duplicated key: " << *emplaced.first << std::endl; > + VALIDITY_ASSERT(is_uniq_key); Same input as above: use sql_print_error and make the error message helpful. > + } > + } > #endif > return *this; > } > commit 57e8f68955954ff96104c8356733bfa73feae849 (HEAD -> bb-10.8-MDEV-27036, origin/bb-10.8-MDEV-27036) > Author: Sergei Krivonos <sergei.krivonos(a)mariadb.com> > Date: Mon Nov 15 05:57:25 2021 +0200 > > MDEV-27036: unittest JSON object member name collision > > diff --git a/unittest/sql/my_json_writer-t.cc b/unittest/sql/my_json_writer-t.cc > index 6398a589c33..7d20802527d 100644 > --- a/unittest/sql/my_json_writer-t.cc > +++ b/unittest/sql/my_json_writer-t.cc > @@ -119,7 +119,14 @@ int main(int args, char **argv) > ok(w.invalid_json, "JSON array end of object"); > } > > - > + { > + Json_writer w; > + w.start_object(); > + w.add_member("name").add_ll(1); > + w.add_member("name").add_ll(2); > + w.end_object(); > + ok(w.invalid_json, "JSON object member name collision"); > + } Please add more unit test coverage. Check that this is accepted: { "foo": { "foo": {} } } Check that this is not accepted: { "foo": { "bar": {} }, "foo": "something else" } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 37b6f5cb8ef: decouple commit/rollback code from binlog hton
by Sergei Golubchik 23 Nov '21

23 Nov '21

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 37b6f5cb8ef (mariadb-10.5.2-484-g37b6f5cb8ef) > parent(s): f3603cbfaeb > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > decouple commit/rollback code from binlog hton let's squash it with "[6/8] store cache managers in a list" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] ad6e7b87107: introduce cache flipping
by Sergei Golubchik 23 Nov '21

23 Nov '21

Hi, Nikita! Interesting trick. On Nov 13, Nikita Malyavin wrote: > revision-id: ad6e7b87107 (mariadb-10.5.2-477-gad6e7b87107) > parent(s): 85fbd867b4c > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-27 14:17:39 +1000 > message: > > introduce cache flipping Could you add few lines from the Cache_flip_event_log comment here too please? > diff --git a/sql/log.h b/sql/log.h > index 74c409e1ac7..1e422ae4524 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -424,10 +424,9 @@ class Event_log: public MYSQL_LOG > > bool open( > const char *log_name, > - enum_log_type log_type, > const char *new_name, ulong next_file_number, > enum cache_type io_cache_type_arg); > - IO_CACHE *get_log_file() { return &log_file; } > + virtual IO_CACHE *get_log_file() { return &log_file; } > > int write_description_event(enum_binlog_checksum_alg checksum_alg, > bool encrypt, bool dont_set_created); > @@ -435,6 +434,73 @@ class Event_log: public MYSQL_LOG > bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); > }; > > +/** > + A single-reader, multiple-writer non-blocking layer for Event_log. > + Provides IO_CACHE for writing and IO_CACHE for reading.\ > + > + Writers use an overrided get_log_file version for their writes, while readers > + should use flip() to initiate reading. > + flip() swaps pointers to allow non-blocking reads. > + > + Writers can block other writers and a reader with a mutex, but a reader only > + swaps two pointers under a lock, so it won't block writers. > + > + TODO should be unnecessary after MDEV-24676 is done > + */ > +class Cache_flip_event_log: public Event_log { > + IO_CACHE alt_buf; > + IO_CACHE *current, *alt; > +public: > + > + Cache_flip_event_log() : Event_log(), alt_buf{}, > + current(&log_file), alt(&alt_buf) {} > + bool open(enum cache_type io_cache_type_arg) > + { > + log_file.dir= mysql_tmpdir; > + alt_buf.dir= log_file.dir; > + bool res= Event_log::open(NULL, NULL, 0, io_cache_type_arg); > + if (res) > + return res; > + > + name= my_strdup(key_memory_MYSQL_LOG_name, "online-alter-binlog", > + MYF(MY_WME)); > + if (!name) > + return false; > + > + res= init_io_cache(&alt_buf, -1, LOG_BIN_IO_SIZE, io_cache_type_arg, 0, 0, > + MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL)) != 0; > + return res; > + } > + > + /** > + Swaps current and alt_log. Can be called only from the reader thread. > + @return a new IO_CACHE pointer to read from. > + */ > + IO_CACHE *flip() > + { > + IO_CACHE *tmp= current; > + reinit_io_cache(alt, WRITE_CACHE, 0, 0, 0); > + mysql_mutex_lock(get_log_lock()); > + reinit_io_cache(current, READ_CACHE, 0, 0, 0); > + current= alt; > + mysql_mutex_unlock(get_log_lock()); > + alt= tmp; > + > + return alt; > + } > + > + IO_CACHE *get_log_file() override > + { > + return current; may be mysql_mutex_assert_owner(get_log_lock()) ? > + } > + > + void cleanup() > + { > + end_io_cache(&alt_buf); > + Event_log::cleanup(); > + } > +}; > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
by Sergei Golubchik 21 Nov '21

21 Nov '21

Hi, Vladislav! few questions below: On Nov 18, Vladislav Vaintroub wrote: > revision-id: c80991c79f7 (mariadb-10.6.1-213-gc80991c79f7) > parent(s): cee33f1ab7c > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2021-11-09 02:04:22 +0100 > message: > > MDEV-25785 Add support for OpenSSL 3.0 > > Summary of changes > > - MD_CTX_SIZE is increased > > - EVP_CIPHER_CTX_buf_noconst(ctx) does not work anymore, points > to nobody knows where. The assumption made previously was that > (since the function does not seem to be documented) > was that it points to the last partial source block. > Add own partial block buffer for NOPAD encryption instead > > - SECLEVEL in CipherString in openssl.cnf > had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible The definition of SECLEVEL is: Level 0 Everything is permitted. This retains compatibility with previous versions of OpenSSL. Level 1 The security level corresponds to a minimum of 80 bits of security. Any parameters offering below 80 bits of security are excluded. As a result RSA, DSA and DH keys shorter than 1024 bits and ECC keys shorter than 160 bits are prohibited. All export cipher suites are prohibited since they all offer less than 80 bits of security. SSL version 2 is prohibited. Any cipher suite using MD5 for the MAC is also prohibited. Note that signatures using SHA1 and MD5 are also forbidden at this level as they have less than 80 security bits. Only at level 3 does it say "TLS versions below 1.1 are not permitted." I don't see why you had to change from 1 to 0. Do we have "signatures using SHA1 and MD5"? > - ctx_buf buffer now must be aligned to 16 bytes with openssl( > previously with WolfSSL only), ot crashes will happen > > - updated aes-t , to be better debuggable > using function, rather than a huge multiline macro > added test that does "nopad" encryption piece-wise, to test > replacement of EVP_CIPHER_CTX_buf_noconst > > diff --git a/cmake/ssl.cmake b/cmake/ssl.cmake > index 7c2488be8bd..64c93ff9b4f 100644 > --- a/cmake/ssl.cmake > +++ b/cmake/ssl.cmake > @@ -139,9 +139,20 @@ MACRO (MYSQL_CHECK_SSL) > SET(SSL_INTERNAL_INCLUDE_DIRS "") > SET(SSL_DEFINES "-DHAVE_OPENSSL") > > + FOREACH(x INCLUDES LIBRARIES DEFINITIONS) > + SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}}) > + ENDFOREACH() why do you set/restore them? > + # Silence "deprecated in OpenSSL 3.0" > + IF((NOT OPENSSL_VERSION) # 3.0 not determined by older cmake > + OR NOT(OPENSSL_VERSION VERSION_LESS "3.0.0")) > + SET(SSL_DEFINES "${SSL_DEFINES} -DOPENSSL_API_COMPAT=0x10100000L") > + SET(CMAKE_REQUIRED_DEFINITIONS -DOPENSSL_API_COMPAT=0x10100000L) > + ENDIF() This is just postponing the inevitable. They'll drop the old API eventually. As far as I understand the internals were changed in a way that doesn't fit the old API. Is there some safe subset of OpenSSL API that works both in 1.0 and in 3.0 ? It might be more future proof to use only that. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 4

Re: [Maria-developers] 4492c869f31: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
by Sergei Golubchik 18 Nov '21

18 Nov '21

Hi, Rucha! Thanks! Looks good now. One comment about tests: On Nov 18, Rucha Deodhar wrote: > revision-id: 4492c869f31 (mariadb-10.6.1-212-g4492c869f31) > parent(s): 3f3ec40c91b > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-11-08 16:51:33 +0530 > message: > > MDEV-26238: Remove inconsistent behaviour of --default-* options > in my_print_defaults ... > diff --git a/mysql-test/main/my_print_defaults.test b/mysql-test/main/my_print_defaults.test > index bfd4e563826..b52deaf09ff 100644 > --- a/mysql-test/main/my_print_defaults.test > +++ b/mysql-test/main/my_print_defaults.test > @@ -87,20 +85,74 @@ read_buffer_size=1M ... > +--echo # checking that --defaults* option only works when mentioned at beginning > + > +--echo # Testing --defaults-file at beginning only > +--exec $MYSQL_MY_PRINT_DEFAULTS --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld > +--remove_file $MYSQLTEST_VARDIR/tmp/tmp3.cnf How does it test that "--defaults* option only works at beginning"? You test that --defaults-file works at the beginning all right. This is redundant, as there are lots of tests in this file that have --defaults-file, but it doesn't hurt. But you don't test the "only" part. You don't test anything that you've changed - I suspect your new test would happily pass without any changes in my_print_defaults.c To test that --defaults-file works *only* at the beginning, you need to verify that it fails not at the beginning. Like error 1 exec $MYSQL_MY_PRINT_DEFAULTS --myslqd --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf also with -c error 1 exec $MYSQL_MY_PRINT_DEFAULTS -c $MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld This should fail even at the beginning. The point is - you need to test the behavior that you've changed. As your test has introduced failures - you need to test for these failures. But all your tests succeed. > +--echo # Testing --defaults-extra-file works at beginning only > +--exec $MYSQL_MY_PRINT_DEFAULTS --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf --defaults-extra-file=$MYSQLTEST_VARDIR/tmp/tmp2.cnf --mysqld Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-27036: do not output empty subqueries in traces
by Sergey Petrunia 16 Nov '21

16 Nov '21

Hi Sergei, > commit eb7d6304a2aa168e09d48f6884dadf8c9f868feb > Author: Sergei Krivonos <sergei.krivonos(a)mariadb.com> > Date: Mon Nov 15 01:37:04 2021 +0200 > > do not output empty subqueries in traces This reads as if the trace can have an empty clause describing a subquery. This is very confusing. Actually, the patch removes potential empty "subqueries" arrays. Do you have a testcase for this? I'm trying to find one with grep -rn -A1 '"subqueries"' mysql-test/{main,suite} and I can't. > diff --git a/sql/sql_explain.cc b/sql/sql_explain.cc > index 499b6dbc97a..9a9bf786d62 100644 > --- a/sql/sql_explain.cc > +++ b/sql/sql_explain.cc > @@ -25,6 +25,8 @@ > #include "opt_range.h" > #include "sql_expression_cache.h" > > +#include <algorithm> > + Is this needed? It seems to work without it. > + bool Explain_node::has_json_printable_connection_objects(Explain_query *query) const > + { > + auto n= children.elements(); > + if (n) > + { > + for (; n--;) > + { Why such a convoluted loop inside an if statement? Can you make the code simpler by using for (n=0; n < children.elements(); n++) { if (is_connection_printable_in_json(children.at(i)->connection_type)) return true; } return false; ? > + auto node= query->get_node(children.at(n)); > + /* Derived tables are printed inside Explain_table_access objects */ > + > + if (!is_connection_printable_in_json(node->connection_type)) > + continue; > + > + return true; > + } > + } > + return false; > + } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0