- developers - lists.mariadb.org

Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 07 Dec '21

07 Dec '21

Hi, Aleksey! This is a review of diff b95c5105e2 2552bce607 This was quite good, thanks! Just a couple of questions/comments, see below. On Nov 23, Aleksey Midenkov wrote: > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index 006a65e1a16..a5bb8426ae4 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -1070,15 +1080,572 @@ alter table t1 add partition partitions 2; ... > +--let $datadir= `select @@datadir` > +--let $dummy= $datadir/test/t1#P#p1.ibd > +--write_file $dummy > +EOF > + > +set timestamp= unix_timestamp('2000-01-01 01:00:00'); > +--error ER_GET_ERRNO > +update t1 set x= x + 2; That's a good one! > +show warnings; > +--remove_file $dummy > + > diff --git a/sql/table.h b/sql/table.h > index 2e074abcea0..d861db261bf 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -910,6 +911,11 @@ struct TABLE_SHARE > vers_kind_t versioned; > period_info_t vers; > period_info_t period; > + /* > + Protect multiple threads from repeating partition auto-create over > + single share. could you please add here (to the comment) TODO remove it when partitioning metadata will be in TABLE_SHARE > + */ > + bool vers_skip_auto_create; > > bool init_period_from_extra2(period_info_t *period, const uchar *data, > const uchar *end); > @@ -2557,6 +2567,11 @@ struct TABLE_LIST > bool merged; > bool merged_for_insert; > bool sequence; /* Part of NEXTVAL/CURVAL/LASTVAL */ > + /* > + Protect single thread from repeating partition auto-create over > + multiple share instances (as the share is closed on backoff action). > + */ > + bool vers_skip_create; I don't understand it. What does it mean "multiple share instances"? Can there be multiple TABLE_SHARE objects for the same table? How? > > /* > Items created by create_view_field and collected to change them in case > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index d7bb02bbd4e..56d5a68efd0 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -2587,11 +2587,15 @@ char *generate_partition_syntax(THD *thd, partition_info *part_info, > err+= str.append(ctime, ctime_len); > err+= str.append('\''); > } > + if (vers_info->auto_hist) > + err+= str.append(STRING_WITH_LEN(" AUTO")); > } > - if (vers_info->limit) > + else if (vers_info->limit) > { > err+= str.append(STRING_WITH_LEN("LIMIT ")); > err+= str.append_ulonglong(vers_info->limit); > + if (vers_info->auto_hist) > + err+= str.append(STRING_WITH_LEN(" AUTO")); Can you do this str.appent("AUTO") once, after the if()? Not in both branches? > } > } > else if (part_info->part_expr) > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index fd92e437cac..ddcc6f413ba 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -865,9 +870,167 @@ void partition_info::vers_set_hist_part(THD *thd) > { > vers_info->hist_part= next; > if (next->range_value > thd->query_start()) > - return; > + { > + error= false; > + break; > + } > + } > + if (error) > + { > + if (auto_hist) > + { > + *create_count= 0; > + const my_time_t hist_end= (my_time_t) vers_info->hist_part->range_value; > + DBUG_ASSERT(thd->query_start() >= hist_end); > + MYSQL_TIME h0, q0; > + my_tz_OFFSET0->gmt_sec_to_TIME(&h0, hist_end); > + my_tz_OFFSET0->gmt_sec_to_TIME(&q0, thd->query_start()); > + longlong q= pack_time(&q0); > + longlong h= pack_time(&h0); > + while (h <= q) > + { > + if (date_add_interval(thd, &h0, vers_info->interval.type, > + vers_info->interval.step)) > + return true; > + h= pack_time(&h0); > + ++*create_count; > + if (*create_count == MAX_PARTITIONS - 2) > + { > + my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(ME_WARNING)); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(0), > + table->s->db.str, table->s->table_name.str); > + return true; > + } > + } > + } > + else > + { > + DBUG_ASSERT(!vers_info->auto_hist); ^^^ This assert fails for me in the update-big test > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "INTERVAL"); > + } > + } > + } > + > + return false; > +} > + > + > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index ac22a63bdba..459edb614f7 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -4203,6 +4444,15 @@ bool open_tables(THD *thd, const DDL_options_st &options, > } > > thd->current_tablenr= 0; > + > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (!thd->stmt_arena->is_conventional()) > + { > + for (tables= *start; tables; tables= tables->next_global) > + tables->vers_skip_create= false; > + } This looks a bit like an overkill, you do it for every prepare, while auto-adding a partition is a rare event, as you said. May be you can do instead in TABLE::vers_switch_partition uint *create_count= !thd->stmt_arena->is_conventional() && table_list->vers_skip_create ? > +#endif > + > restart: > /* > Close HANDLER tables which are marked for flush or against which there Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

[Maria-developers] some questions for the community
by 杨凡 26 Nov '21

26 Nov '21

Hello, I am a student of Xi'an University of Posts and Telecommunications. My name is yangfan.I want to ask something about community code sharing and GSOC. I am now in my sophomore year, and I am very interested in databases, especially your marria database. I am now familiar with the c/c++ language and master the basic Linux system programming and network programming. Currently learning C++ multi-threaded programming. I can use basic SQL statements, databases, basic Linux commands, and some knowledge about operating systems and networks. I joined the xiyou linux group. Now, I want to learn quickly and contribute code to your community. Can you tell me what to do? And I want to participate in GSOC in 2022. If you can give me some help, I will be very happy. I only learned about open source culture from university. After a long period of study and thinking, I feel that open source is a very mea ningful thing. It not only helps personal growth but also helps a product iteration. The slogan of our group is free sharing and openness. At the same time, I continue to promote open source culture. I am fascinated by this culture. I also want to join a community and make my contribution to you. Although my current ability is not very strong, but I will continue to work hard. I am very interested in programming, and I want to focus on distributed and database work now and in the future. Maybe my English is not very good, I hope you can be more tolerant.

1 0

Re: [Maria-developers] a5fca9a6e30: MENT-651 [6/8] store cache managers in a list
by Sergei Golubchik 26 Nov '21

26 Nov '21

Hi, Nikita! On Nov 14, Nikita Malyavin wrote: > revision-id: a5fca9a6e30 (mariadb-10.5.2-478-ga5fca9a6e30) > parent(s): ad6e7b87107 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-27 17:28:05 +1000 > message: > > MENT-651 [6/8] store cache managers in a list again, a couple of lines with a description would be good here > diff --git a/mysql-test/suite/binlog/t/online_alter.test b/mysql-test/suite/binlog/t/online_alter.test > index 804e847d008..4ed2db74bd6 100644 > --- a/mysql-test/suite/binlog/t/online_alter.test > +++ b/mysql-test/suite/binlog/t/online_alter.test > @@ -218,7 +219,7 @@ set autocommit = 0; > start transaction; > update t1 set b= 7007 where a = 7; > --error ER_DUP_ENTRY > -update t1 set a= 8 where a = 8 or a = 9; > +update t1 set a= 8, b= 8008 where a = 8 or a = 9 order by a; why did you change tests for what is, as far as I understand, just an optimization? > commit; > set autocommit = 1; > > diff --git a/sql/handler.cc b/sql/handler.cc > index 8b95653e69f..8b22167e729 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6660,7 +6660,16 @@ static int binlog_log_row_online_alter(TABLE* table, > THD *thd= table->in_use; > > if (!table->online_alter_cache) > - table->online_alter_cache= thd->binlog_setup_cache_data(); > + { > + auto *cache_mngr= online_alter_binlog_get_cache_mngr(thd, table); > + // Use transaction cache directly, if it is not multi-transaction mode > + table->online_alter_cache= binlog_get_cache_data(cache_mngr, > + !thd->in_multi_stmt_transaction_mode()); > + > + trans_register_ha(thd, false, binlog_hton, 0); > + if (thd->in_multi_stmt_transaction_mode()) > + trans_register_ha(thd, true, binlog_hton, 0); I still don't understand that. Why do you need a handlerton, why do you fake a transaction? And why do you need to search a list in THD, instead of storing cache_mngr in TABLE_SHARE? > + } > > // We need to log all columns for the case if alter table changes primary key. > table->use_all_columns(); > diff --git a/sql/log.cc b/sql/log.cc > index 6c678624230..c58096e6d9a 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2258,14 +2298,18 @@ static int binlog_rollback(handlerton *hton, THD *thd, bool all) > { > DBUG_ENTER("binlog_rollback"); > > - for (TABLE *table= thd->open_tables; table; table= table->next) > - { > - if (!table->online_alter_cache) > - continue; > - table->online_alter_cache->reset(); > - delete table->online_alter_cache; > - table->online_alter_cache= NULL; > - } > + bool is_ending_trans= ending_trans(thd, all); > + > + /* > + This is a crucial moment that we are running through > + thd->online_alter_cache_list, and not through thd->open_tables to cleanup > + stmt cache, though both have it. The reason is that the tables can be closed > + to that moment in case of an error. > + The same reason applies to the fact we don't store cache_mngr in the table > + itself -- because it can happen to be not existing. > + Still in case if tables are left opened > + */ > + binlog_online_alter_cleanup(thd->online_alter_cache_list, is_ending_trans); still don't understand that comment either :( Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Squash aaed3436 MENT-651 [4/8] ALTER ONLINE TABLE
by Nikita Malyavin 25 Nov '21

25 Nov '21

Sergei, I think all the commits starting from [4/8] ALTER ONLINE TABLE should be squashed. I will also add a better description in the commit message covering our latest discussions. I would also embed a reference to the original unsquashed branch. It seems to me as a good idea in sense of helping the reader investigate the code -- Yours truly, Nikita Malyavin

2 5

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