May 2023 - developers - lists.mariadb.org

[Maria-developers] START ALTER replication test cases
by Kristian Nielsen 10 May '23

10 May '23

Hi Sachin, Andrei, I'm looking at test cases for START ALTER / Lag-free Alter on Slave. And I happened to notice something in test case rpl.rpl_start_alter_bugs: --connection master set global binlog_alter_two_phase=true; # ... ALTER TABLE t1 DROP PRIMARY KEY; ALTER TABLE t1 ADD UNIQUE KEY ui (i); ALTER TABLE t1 ADD PRIMARY KEY (i); Note the set *global* binlog_alter_two_phase. This will not affect the testcase at all, as the ALTER TABLE queries are using the *session* value (which defaults to false). This can be verified by checking the binlog after running the testcase, mysql-test/var/mysqld.1/data/master-bin.000001 . It has only normal ALTER TABLE query events, no START ALTER / COMMIT ALTER. So it looks strange that a start_alter test doesn't use START ALTER, and I just wanted to point this out to you in case it is a mistake and maybe the test coverage of START ALTER is less than intended? A quick grep shows similar SET GLOBAL in other start_alter tests, but I did not investigate if they have the same issue. Hope this helps, - Kristian.

2 2

Re: [Maria-developers] da5a72f32d4: MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
by Sergei Golubchik 10 May '23

10 May '23

Hi, Nikita, On May 05, Nikita Malyavin wrote: > revision-id: da5a72f32d4 (mariadb-11.0.1-114-gda5a72f32d4) > parent(s): 0d6584c019c > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-01 01:15:41 +0300 > message: > > MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table > > The row events were applied "twice": once for the ha_partition, and > one more time for the underlying storage engine. > > There's no such problem in binlog/rpl, because > ha_partiton::row_logging is normally set to false. s/ha_partiton/ha_partition/ > The proposed fix adds table->skip_online_logging field, preventing the > underlying engines from replicating online events. Only upper layer > engine should do this. > > table->skip_online_logging is initialized to 0 together with the table > itself. Normally it is always 0, except for the intermediate > overriding part, once it is checked in handler::binlog_log_row. > > Note on MDEV-31040 (Server crashes in MYSQL_LOG::is_open upon ALTER on > partitioned table): > First the solution was to override table->s->online_alter_binlog, and > set it to NULL during the partitioning operation, which was > ridiculously wrong: no-one is supposed to modify that field outside > ALTER! That field write access is protected by EXCLUSIVE metadata > lock. I don't think you need to list your different (non-working) attemps to fix the bug in the commit comment. Just explain the fix that works, not the fix that doesn't. > In comparison, table->skip_online_logging is a connection-local field. > > The test is a non-deterministic one. It wasn't convenient to construct > a deterministic test here, besides for the case that will never be > repeated. Is it still the case? Your test in alter_table_online_debug.test with debug_sync - is it non-deterministic? > diff --git a/sql/handler.cc b/sql/handler.cc > index 3fcbb5171d6..a7e229e8f8d 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7280,7 +7280,8 @@ int handler::binlog_log_row(const uchar *before_record, > log_func, row_logging_has_trans); > > #ifdef HAVE_REPLICATION > - if (unlikely(!error && table->s->online_alter_binlog)) > + if (unlikely(!error && table->s->online_alter_binlog && > + !table->skip_online_logging)) this doesn't need any juggling with skip_online_logging. This problem is already solved for, say, long uniques. You can use this == table->file condition to filter out individual partitions and have your binlog_log_row_online_alter called only for the main ha_partition handler. > error= binlog_log_row_online_alter(table, before_record, after_record, > log_func); > #endif // HAVE_REPLICATION Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 24c653be25a: unpack_row: unpack a correct number of fields
by Sergei Golubchik 06 May '23

06 May '23

Hi, Nikita, On May 05, Nikita Malyavin wrote: > revision-id: 24c653be25a (mariadb-11.0.1-123-g24c653be25a) > parent(s): c1cbda5a2c8 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-04 20:49:43 +0300 > message: > > unpack_row: unpack a correct number of fields > > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index 908d2c39d97..30bd80bdfc7 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -228,24 +228,30 @@ int unpack_row(const rpl_group_info *rgi, TABLE *table, uint const colcnt, > const TABLE *conv_table= rpl_data.conv_table; > DBUG_PRINT("debug", ("Table data: tabldef: %p, conv_table: %p", > tabledef, conv_table)); > - > + bool is_online_alter= rpl_data.is_online_alter(); > DBUG_ASSERT(rgi); > > - for (field_ptr= begin_ptr; field_ptr < end_ptr && *field_ptr; ++field_ptr) > + for (field_ptr= begin_ptr; field_ptr < end_ptr > + /* In Online Alter conv_table can be wider than > + original table, but we need to unpack it all. */ > + && (*field_ptr || is_online_alter); I failed to understand this, could you please explain? The comment didn't help much, unfortunately. The test case didn't either. > + ++field_ptr) > { > + intptr fidx= field_ptr - begin_ptr; intptr is a variant of int with sizeof(intptr) == sizeof(void*). it's needed to cast a pointer to int, do some math, cast back. here you need my_ptrdiff_t or ptrdiff_t > /* > If there is a conversion table, we pick up the field pointer to > the conversion table. If the conversion table or the field > pointer is NULL, no conversions are necessary. > */ > - Field *conv_field= conv_table ? conv_table->field[field_ptr - begin_ptr] : NULL; > + Field *conv_field= conv_table ? conv_table->field[fidx] : NULL; > Field *const f= conv_field ? conv_field : *field_ptr; > +#ifdef DBUG_TRACE > + Field *dbg= is_online_alter ? f : *field_ptr; > +#endif > DBUG_PRINT("debug", ("Conversion %srequired for field '%s' (#%ld)", > conv_field ? "" : "not ", > - (*field_ptr)->field_name.str, > - (long) (field_ptr - begin_ptr))); > + dbg->field_name.str, > + (long) ())); eh? you surely meant `(long) fidx` ? how did it even compile? > DBUG_ASSERT(f != NULL); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] c5ce597f06a: MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction
by Sergei Golubchik 06 May '23

06 May '23

Hi, Nikita, On May 03, Nikita Malyavin wrote: > revision-id: c5ce597f06a (mariadb-11.0.1-115-gc5ce597f06a) > parent(s): da5a72f32d4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-01 01:15:41 +0300 > message: > > MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction > > Non-transactional engines changes are visible immediately the row operation > succeeds, so we should apply the changes to the online buffer immediately > after the statement ends. > > diff --git a/sql/log.cc b/sql/log.cc > index f30ce3bcbd1..1d98455c2b3 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -7701,23 +7701,35 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > auto *binlog= cache.sink_log; > DBUG_ASSERT(binlog); > > - bool do_commit= commit || !cache.hton->rollback || > - cache.hton->flags & HTON_NO_ROLLBACK; // Aria > + bool do_commit= (commit && is_ending_transaction) > + || cache.hton->flags & HTON_NO_ROLLBACK // Aria > + || !cache.hton->rollback; > + > + error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache); you've lost the useful comment about STMT_END. and you flush events for rollback too that seems like a waste. I personally would've kept the old structure of if()'s. Like - bool do_commit= commit || !cache.hton->rollback || + bool non_trans= !cache.hton->rollback || and - if (do_commit) + if (commit || non_trans) with - if (is_ending_transaction) + if (is_ending_transaction || non_trans) That would've called binlog flush only where it needs to be. > + > if (do_commit) > { > - // do not set STMT_END for last event to leave table open in altering thd > - error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache); > - if (is_ending_transaction) > + /* > + If the cache wasn't reinited to write, then it remains empty after > + the last write. > + */ > + if (cache.cache_log.type != READ_CACHE && !error) this is a confusing new condition. are you trying to avoid locking a mutex for an empty cache? If yes, you can check my_b_bytes_in_cache(), that'd be more clear. > { > mysql_mutex_lock(binlog->get_log_lock()); > error= binlog->write_cache(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > } > - else > - cache.store_prev_position(); > } > - else if (!is_ending_transaction) > + else if (!commit) // rollback > + { > cache.restore_prev_position(); > + } > + else add // trans engine, end of statement > + { > + DBUG_ASSERT(!is_ending_transaction); > + cache.store_prev_position(); > + } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] d25439f1cbb: MDEV-31140: FLUSH BINARY LOGS DELETE_DOMAIN_ID=(D) can errorneously delete active domains
by Kristian Nielsen 02 May '23

02 May '23

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> Want to review a small patch for https://jira.mariadb.org/browse/MDEV-31140 ? > Thanks for a very good piece of analysis and the refinement! > I agree with both. Great, thanks for review! Do you have an opinion into which branch I should push it? The fix should be low-risk. I'm thinking maybe 11.0 or 10.11 is fine, since the bug does not affect normal operation. But I'm open to other suggestions, whatever is the current policy for this kind of bug? > The existed test was more than a bit unlucky, to confess, > to let the problem stay unnoticed. Yes, there is a good test coverage, so a bit unlucky, agree. - Kristian. > On the occasion of the International workers' day, let me wish you more > contribution, and inspiration for how to make not just this server > better alone :-)!!! Thanks, and all the best to you too :-) - Kristian. >> Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: >> >>> revision-id: d25439f1cbb79e5467b4249792130bfe524e3f10 (mariadb-10.11.2-21-gd25439f1cbb) >>> parent(s): 656c2e18b1e9ea5d0314745f3988d126eedbc22a >>> author: Kristian Nielsen >>> committer: Kristian Nielsen >>> timestamp: 2023-04-27 12:16:07 +0200 >>> message: >>> >>> MDEV-31140: FLUSH BINARY LOGS DELETE_DOMAIN_ID=(D) can errorneously delete active domains >>> >>> Fix the code in rpl_binlog_state::drop_domain(), so that _all_ entries for >>> the domain in the binlog state must match an entry in the initial GTID_LIST, >>> not just one entry match. >>> >>> Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> >>> >>> --- >>> .../r/binlog_flush_binlogs_delete_domain.result | 14 ++++++++++--- >>> .../t/binlog_flush_binlogs_delete_domain.test | 13 +++++++++++- >>> sql/rpl_gtid.cc | 24 ++++++++++++---------- >>> 3 files changed, 36 insertions(+), 15 deletions(-) >>> >>> diff --git a/mysql-test/suite/binlog/r/binlog_flush_binlogs_delete_domain.result b/mysql-test/suite/binlog/r/binlog_flush_binlogs_delete_domain.result >>> index fdcfb4bfa01..1c11191802f 100644 >>> --- a/mysql-test/suite/binlog/r/binlog_flush_binlogs_delete_domain.result >>> +++ b/mysql-test/suite/binlog/r/binlog_flush_binlogs_delete_domain.result >>> @@ -46,15 +46,23 @@ Warning 1076 The current gtid binlog state is incompatible with a former one mis >>> Warning 1076 The gtid domain being deleted ('1') is not in the current binlog state >>> FLUSH BINARY LOGS DELETE_DOMAIN_ID = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 0); >>> ERROR HY000: Could not delete gtid domain. Reason: binlog files may contain gtids from the domain ('1') being deleted. Make sure to first purge those files. >>> +MDEV-31140: Missing error from DELETE_DOMAIN_ID when gtid_binlog_state partially matches GTID_LIST. >>> FLUSH BINARY LOGS; >>> PURGE BINARY LOGS TO 'master-bin.000005'; >>> +SET @@SESSION.gtid_domain_id=8; >>> +SET @@SESSION.server_id=10*8 + 1; >>> +INSERT INTO t SELECT 1+MAX(a) FROM t; >>> +FLUSH BINARY LOGS DELETE_DOMAIN_ID = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 0); >>> +ERROR HY000: Could not delete gtid domain. Reason: binlog files may contain gtids from the domain ('8') being deleted. Make sure to first purge those files. >>> +FLUSH BINARY LOGS; >>> +PURGE BINARY LOGS TO 'master-bin.000006'; >>> FLUSH BINARY LOGS DELETE_DOMAIN_ID = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 0); >>> Warnings: >>> Warning 1076 The gtid domain being deleted ('0') is not in the current binlog state >>> Gtid_list of the current binlog does not contain 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 0: >>> -show binlog events in 'master-bin.000006' limit 1,1; >>> +show binlog events in 'master-bin.000007' limit 1,1; >>> Log_name Pos Event_type Server_id End_log_pos Info >>> -master-bin.000006 # Gtid_list 1 # [] >>> +master-bin.000007 # Gtid_list 1 # [] >>> SET @@SESSION.gtid_domain_id=1;; >>> SET @@SESSION.server_id=1; >>> SET @@SESSION.gtid_seq_no=1; >>> @@ -75,7 +83,7 @@ INSERT INTO t SET a=1; >>> SELECT @gtid_binlog_state_saved "as original state", @@GLOBAL.gtid_binlog_state as "out of order for 11 domain state"; >>> as original state out of order for 11 domain state >>> 1-1-1,1-2-2,11-11-11 1-1-1,1-2-2,11-11-1 >>> -PURGE BINARY LOGS TO 'master-bin.000007'; >>> +PURGE BINARY LOGS TO 'master-bin.000008'; >>> the following command succeeds with warnings >>> FLUSH BINARY LOGS DELETE_DOMAIN_ID = (1); >>> Warnings: >>> diff --git a/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test b/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test >>> index 8311f4bd800..1643ecff72d 100644 >>> --- a/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test >>> +++ b/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test >>> @@ -21,7 +21,6 @@ FLUSH BINARY LOGS DELETE_DOMAIN_ID = (); >>> --echo but with a warning >>> --let $binlog_pre_flush=query_get_value(SHOW MASTER STATUS, Position, 1) >>> FLUSH BINARY LOGS DELETE_DOMAIN_ID = (99); >>> ---let $binlog_start=$binlog_pre_flush >>> --source include/show_binary_logs.inc >>> >>> # Log one event in a specified domain and try to delete the domain >>> @@ -62,6 +61,8 @@ FLUSH BINARY LOGS DELETE_DOMAIN_ID = (1); >>> # expected overrun of the static buffers of underlying dynamic arrays is doing. >>> --let $domain_cnt=17 >>> --let $server_in_domain_cnt=3 >>> +--let $err_domain_id=`SELECT FLOOR($domain_cnt/2)` >>> +--let $err_server_id=`SELECT FLOOR($server_in_domain_cnt/2)` >>> --let $domain_list= >>> --disable_query_log >>> while ($domain_cnt) >>> @@ -86,6 +87,16 @@ while ($domain_cnt) >>> --error ER_BINLOG_CANT_DELETE_GTID_DOMAIN >>> --eval FLUSH BINARY LOGS DELETE_DOMAIN_ID = ($domain_list) >>> >>> +--echo MDEV-31140: Missing error from DELETE_DOMAIN_ID when gtid_binlog_state partially matches GTID_LIST. >>> +FLUSH BINARY LOGS; >>> +--let $purge_to_binlog= query_get_value(SHOW MASTER STATUS, File, 1) >>> +--eval PURGE BINARY LOGS TO '$purge_to_binlog' >>> +--eval SET @@SESSION.gtid_domain_id=$err_domain_id >>> +--eval SET @@SESSION.server_id=10*$err_domain_id + $err_server_id >>> +eval INSERT INTO t SELECT 1+MAX(a) FROM t; >>> +--error ER_BINLOG_CANT_DELETE_GTID_DOMAIN >>> +--eval FLUSH BINARY LOGS DELETE_DOMAIN_ID = ($domain_list) >>> + >>> # Now satisfy the safety condtion to purge log files containing $domain list >>> FLUSH BINARY LOGS; >>> --let $purge_to_binlog= query_get_value(SHOW MASTER STATUS, File, 1) >>> diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc >>> index c4e5c75b10a..7b67a83b3dd 100644 >>> --- a/sql/rpl_gtid.cc >>> +++ b/sql/rpl_gtid.cc >>> @@ -2209,18 +2209,16 @@ rpl_binlog_state::drop_domain(DYNAMIC_ARRAY *ids, >>> >>> /* >>> For each domain_id from ids >>> - when no such domain in binlog state >>> - warn && continue >>> - For each domain.server's last gtid >>> - when not locate the last gtid in glev.list >>> - error out binlog state can't change >>> - otherwise continue >>> + If the domain is already absent from the binlog state >>> + Warn && continue >>> + If any GTID with that domain in binlog state is missing from glev.list >>> + Error out binlog state can't change >>> */ >>> for (ulong i= 0; i < ids->elements; i++) >>> { >>> rpl_binlog_state::element *elem= NULL; >>> uint32 *ptr_domain_id; >>> - bool not_match; >>> + bool all_found; >>> >>> ptr_domain_id= (uint32*) dynamic_array_ptr(ids, i); >>> elem= (rpl_binlog_state::element *) >>> @@ -2235,14 +2233,18 @@ rpl_binlog_state::drop_domain(DYNAMIC_ARRAY *ids, >>> continue; >>> } >>> >>> - for (not_match= true, k= 0; k < elem->hash.records; k++) >>> + all_found= true; >>> + for (k= 0; k < elem->hash.records && all_found; k++) >>> { >>> rpl_gtid *d_gtid= (rpl_gtid *)my_hash_element(&elem->hash, k); >>> - for (ulong l= 0; l < glev->count && not_match; l++) >>> - not_match= !(*d_gtid == glev->list[l]); >>> + bool match_found= false; >>> + for (ulong l= 0; l < glev->count && !match_found; l++) >>> + match_found= match_found || (*d_gtid == glev->list[l]); >>> + if (!match_found) >>> + all_found= false; >>> } >>> >>> - if (not_match) >>> + if (!all_found) >>> { >>> sprintf(errbuf, "binlog files may contain gtids from the domain ('%u') " >>> "being deleted. Make sure to first purge those files", > > modified sql/rpl_gtid.cc > @@ -2220,7 +2220,7 @@ rpl_binlog_state::drop_domain(DYNAMIC_ARRAY *ids, > { > rpl_binlog_state::element *elem= NULL; > uint32 *ptr_domain_id; > - bool not_match; > + ulong match; // count-down for matches found > > ptr_domain_id= (uint32*) dynamic_array_ptr(ids, i); > elem= (rpl_binlog_state::element *) > @@ -2235,14 +2235,15 @@ rpl_binlog_state::drop_domain(DYNAMIC_ARRAY *ids, > continue; > } > > - for (not_match= true, k= 0; k < elem->hash.records; k++) > + for (match= elem->hash.records, k= 0; > + k < elem->hash.records && match + k == elem->hash.records; k++) > { > rpl_gtid *d_gtid= (rpl_gtid *)my_hash_element(&elem->hash, k); > - for (ulong l= 0; l < glev->count && not_match; l++) > - not_match= !(*d_gtid == glev->list[l]); > + for (ulong l= 0, same= 0; l < glev->count && !same; l++) > + match-= (same= (*d_gtid == glev->list[l])); > } > > - if (not_match) > + if (match) > { > sprintf(errbuf, "binlog files may contain gtids from the domain ('%u') " > "being deleted. Make sure to first purge those files",

1 0