- developers - lists.mariadb.org

Re: [Maria-developers] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()
by Sergei Golubchik 01 Apr '21

01 Apr '21

Hi, Michael! On Mar 29, Michael Widenius wrote: > revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82) > parent(s): a194c5cfd41 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 15:53:35 +0200 > message: > > Add support for minimum field width for strings to my_vsnprintf() > > diff --git a/mysql-test/main/almost_full.result b/mysql-test/main/almost_full.result > index b2d7092aa51..4b5c8417412 100644 > --- a/mysql-test/main/almost_full.result > +++ b/mysql-test/main/almost_full.result > @@ -7,24 +7,24 @@ INSERT INTO t1 SET b=repeat('a',600); > ERROR HY000: The table 't1' is full > CHECK TABLE t1 EXTENDED; > Table Op Msg_type Msg_text > -test.t1 check warning Datafile is almost full, 65448 of 65534 used > +test.t1 check warning Datafile is almost full, 65448 of 65534 used This happens because mi_check.c has mi_check_print_warning(param, "Datafile is almost full, %10s of %10s used", ... could you please remove explicit width specification from these messages? this looks rather ugly in warnings. This applies both to MyISAM and Aria. > test.t1 check status OK > UPDATE t1 SET b=repeat('a', 800) where a=10; > ERROR HY000: The table 't1' is full > diff --git a/mysql-test/main/grant4.result b/mysql-test/main/grant4.result > index 981bbdeddf5..ba64010f1bb 100644 > --- a/mysql-test/main/grant4.result > +++ b/mysql-test/main/grant4.result > @@ -187,8 +187,8 @@ connection con1; > check table mysqltest_db1.t1; > Table Op Msg_type Msg_text > mysqltest_db1.t1 check warning 1 client is using or hasn't closed the table properly > -mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 > -mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7 > +mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 > +mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7 Same here. > mysqltest_db1.t1 check error Corrupt > # The below statement should fail before repairing t1. > # Otherwise info about such repair will be missing from its result-set. > diff --git a/mysql-test/main/myisam_recover.result b/mysql-test/main/myisam_recover.result > index da96682186c..d3c385bc49b 100644 > --- a/mysql-test/main/myisam_recover.result > +++ b/mysql-test/main/myisam_recover.result > @@ -69,9 +69,9 @@ flush table t1; > # check table is needed to mark the table as crashed. > check table t1; > Table Op Msg_type Msg_text > -test.t1 check warning Size of datafile is: 42 Should be: 21 > -test.t1 check error Record-count is not ok; is 6 Should be: 3 > -test.t1 check warning Found 6 key parts. Should be: 3 > +test.t1 check warning Size of datafile is: 42 Should be: 21 > +test.t1 check error Record-count is not ok; is 6 Should be: 3 > +test.t1 check warning Found 6 key parts. Should be: 3 Same. Must be in many messages in mi_check.c and ma_check.c > test.t1 check error Corrupt > # > diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c > index a2e3f9b738d..1621db2e412 100644 > --- a/strings/my_vsnprintf.c > +++ b/strings/my_vsnprintf.c > @@ -224,12 +224,27 @@ static char *backtick_string(CHARSET_INFO *cs, char *to, const char *end, > */ > > static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end, > - size_t width, char *par, uint print_type, > - my_bool nice_cut) > + longlong length_arg, size_t width, char *par, > + uint print_type, my_bool nice_cut) > { > int well_formed_error; > uint dots= 0; > size_t plen, left_len= (size_t) (end - to) + 1, slen=0; > + my_bool left_fill= 1; > + size_t length; > + > + /* > + The sign of the length argument specific the string should be right > + or left adjusted > + */ > + if (length_arg < 0) You're trying to support left alignment, but you have yourself added a test that shows that it doesn't work. So this code is dead, never used. See below. > + { > + length= -length_arg; > + left_fill= 0; > + } > + else > + length= (size_t) length_arg; > + > if (!par) > par = (char*) "(null)"; > > @@ -693,7 +720,8 @@ size_t my_vsnprintf_ex(CHARSET_INFO *cs, char *to, size_t n, > if (*fmt == 's' || *fmt == 'T') /* String parameter */ > { > reg2 char *par= va_arg(ap, char *); > - to= process_str_arg(cs, to, end, width, par, print_type, (*fmt == 'T')); > + to= process_str_arg(cs, to, end, (longlong) length, width, par, see? length is unsigned type size_t, the sign before length is ignored by the parser. Whether you cast length to a signed type or not and whether you checks for length < 0 or not, left alignment still won't work. That's why you have added this test: > + test1("Negative width is ignored for strings < x> < y>", > + "Negative width is ignored for strings <%-4s> <%-5s>", "x", "y"); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 1bbc852ef71: Changed field_index to use field_index_t instead of uint16
by Sergei Golubchik 01 Apr '21

01 Apr '21

Hi, Michael! On Mar 29, Michael Widenius wrote: > revision-id: 1bbc852ef71 (mariadb-10.5.2-525-g1bbc852ef71) > parent(s): 039f4a054bb > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:31:54 +0200 > message: > > Changed field_index to use field_index_t instead of uint16 Just curious. Why? > diff --git a/sql/unireg.cc b/sql/unireg.cc > index 51dd1fb6e8c..f1a0c5c5269 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -146,8 +146,8 @@ get_fieldno_by_name(HA_CREATE_INFO *create_info, List<Create_field> &create_fiel > { > if (field_name.streq(sql_field->field_name)) > { > - DBUG_ASSERT(field_no <= uint16(~0U)); > + DBUG_ASSERT(field_no < NO_CACHED_FIELD_INDEX); this look suspicios. NO_CACHED_FIELD_INDEX is ((uint)(-1)) which is always greater than anything you can put into field_index_t. You want to ensure that field_no is valid when casted into field_index_t. I'd suggest to redefine NO_CACHED_FIELD_INDEX as ((field_index_t)~0U) > - return uint16(field_no); > + return (field_index_t) field_no; why not to declare field_no to be field_index_t? then you won't need a cast. > } > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 1d577194654: Revert MDEV-16592 "Change Item::with_sum_func to a virtual method"
by Sergei Golubchik 01 Apr '21

01 Apr '21

Hi, Michael! On Mar 29, Michael Widenius wrote: > revision-id: 1d577194654 (mariadb-10.5.2-526-g1d577194654) > parent(s): 1bbc852ef71 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 15:03:04 +0200 > message: > > Revert MDEV-16592 "Change Item::with_sum_func to a virtual method" > > diff --git a/sql/item.cc b/sql/item.cc > index e5be8699d80..e0ba4ff2c89 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -975,9 +975,7 @@ class Item :public Value_source, > void share_name_with(const Item *item) > { > name= item->name; > - common_flags= static_cast<uint8> > - ((common_flags & ~IS_AUTO_GENERATED_NAME) | > - (item->common_flags & IS_AUTO_GENERATED_NAME)); > + is_autogenerated_name= item->is_autogenerated_name; this, apparently, should be in the commit 039f4a054bb "Removed Item::common_flags and replaced it with bit fields" > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 25fe7dbad5d: Reduce usage of strlen()
by Sergei Golubchik 01 Apr '21

01 Apr '21

Hi, Michael! On Mar 31, Michael Widenius wrote: > commit 25fe7dbad5d > Author: Michael Widenius <michael.widenius(a)gmail.com> > Date: Wed Aug 12 20:29:55 2020 +0300 > > Reduce usage of strlen() I'll only mention actual mistakes, no style comments or suggestions or question, just want to get it over with. > diff --git a/sql/item.h b/sql/item.h > index 9a6d0b4abfe..d6ff10bf3ba 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1093,7 +1093,7 @@ class Item :public Value_source, > void share_name_with(const Item *item) > { > name= item->name; > - copy_flags(item, item_base_t::IS_AUTOGENERATED_NAME); > + copy_flags(item, item_base_t::IS_EXPLICIT_NAME); belongs to a previous commit > } > virtual void cleanup(); > virtual void make_send_field(THD *thd, Send_field *field); > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc > index 262481d019b..a9e008b0b92 100644 > --- a/sql/item_windowfunc.cc > +++ b/sql/item_windowfunc.cc > @@ -99,13 +99,15 @@ Item_window_func::fix_fields(THD *thd, Item **ref) > > if (window_spec->window_frame && is_frame_prohibited()) > { > - my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0), window_func()->func_name()); > + my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0), > + window_func()->func_name()); This should be func_name().str, otherwise it'll crash or print garbage on Windows. Same below, in a couple of places. > return true; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] dbb62d20f36: Fixes that enables my_new.cc (new wrapper using my_malloc)
by Sergei Golubchik 29 Mar '21

29 Mar '21

Hi, Michael! On Mar 28, Michael Widenius wrote: > revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36) > parent(s): 0c37211c2a2 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:31:53 +0200 > message: > > Fixes that enables my_new.cc (new wrapper using my_malloc) > > This is not enabled by default as there are leaks in the > server that needs to be fixed first. One can compile > with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the > memory leaks from 'new' The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant. You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with -DHAVE_CXX_NEW=0 this works for me in 10.5 and fails to compile my_new.cc, as it should. Please, only keed your my_new.cc changes and say in the commit message "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'" As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it automatically? Like #ifdef USE_MYSYS_NEW #define SF_REMEMBER_FRAMES 14 #else #define SF_REMEMBER_FRAMES 8 #endif or may be even just always make it 14. It should be easy to enable this feature, right? > diff --git a/CMakeLists.txt b/CMakeLists.txt > index a0b540f12d2..c51fb98abdc 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -366,10 +366,8 @@ ENDIF() > # Run platform tests > INCLUDE(configure.cmake) > > -# force -DUSE_MYSYS_NEW unless already done by HAVE_CXX_NEW > -IF(NOT HAVE_CXX_NEW) > - ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > -ENDIF() > +# Always use USE_MYSYS_NEW > +ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > > # Find header files from the bundled libraries > # (wolfssl, readline, pcre2, etc) > diff --git a/include/my_global.h b/include/my_global.h > index e999e555bf7..6e3b48a00fd 100644 > --- a/include/my_global.h > +++ b/include/my_global.h > @@ -487,6 +487,9 @@ typedef unsigned short ushort; > duplicate declaration of __cxa_pure_virtual, solved by declaring it a > weak symbol. > */ > +#ifndef REALLY_USE_MYSYS_NEW > +#undef USE_MYSYS_NEW > +#endif > #if defined(USE_MYSYS_NEW) && ! defined(DONT_DECLARE_CXA_PURE_VIRTUAL) > C_MODE_START > int __cxa_pure_virtual () __attribute__ ((weak)); > diff --git a/mysys/my_static.c b/mysys/my_static.c > index 3c4b9efc1f8..96a23cdb283 100644 > --- a/mysys/my_static.c > +++ b/mysys/my_static.c > @@ -48,6 +48,7 @@ PSI_memory_key key_memory_my_err_head; > PSI_memory_key key_memory_my_file_info; > PSI_memory_key key_memory_pack_frm; > PSI_memory_key key_memory_charsets; > +PSI_memory_key key_memory_new; where do you initialize it? I'd suggest statically, like PSI_memory_key key_memory_new= PSI_INSTRUMENT_MEM; > > #ifdef _WIN32 > PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 0c37211c2a2: Fixed my_addr_resolve
by Sergei Golubchik 29 Mar '21

29 Mar '21

Hi, Michael! On Mar 28, Michael Widenius wrote: > revision-id: 0c37211c2a2 (mariadb-10.5.2-517-g0c37211c2a2) > parent(s): 555c4470130 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:31:53 +0200 > message: > > Fixed my_addr_resolve > > At least on openSUSE Leap 15.1 one should not use the dli_fbase offset > provided by dladdr() for addresses in the main program. > Without this patch the stack traces from sf_report_leaked_memory() > are not usable. This is very strange. First, the whole purpose of this code is to use dli_fbase offset for the main program. That's why it was added in the first place. PIE binaries can be loaded at any address in memory and the address you see with addr2line or nm or readelf is not the same address you'll see in gdb or in a strack trace. We have to subtract the base address first. But may be openSUSE is different? So, I built on openSUSE Leap and the stack trace was perfectly usable without your patch. I tried both `kill -ABRT` and an artificially introduced memory leak. So I suspect there was some misunderstanding and the problem was somewhere else (I cannot know what, of course, but just as an example - the build was bad and recompilation fixed it). Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] ee538938345: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 25 Mar '21

25 Mar '21

Hi, Andrei! On Mar 24, Andrei Elkin wrote: > >> Here it goes. > >> > >> As after master crash and failover to slave, the demoted-to-slave > >> ex-master must be ready to face and accept its own (generated by) > >> events, without generally necessary --replicate-same-server-id. > >> > >> So the acceptance conditions are refined/relaxed for the semisync > >> slave connected to master in the GTID mode which ensures (under > >> gtid_strict_mode ON) that there can't be any duplicate events for > >> execution on such an ex-master slave. Non-GTID "binlog-truncated" > >> ex-master semisync slave has to be configured with > >> --replicate-same-server-id. > > > > Why would you generally care about same server id in the gtid mode? > > Maybe you did not care in above to the 'connection' word in 'the gtid > connection mode' which is about > > Change-Master to ... master_use_gtid= slave_pos > > So the "legacy" pre-gtid one corresponds to master_use_gtid= no. > [Thinking into future, it may stay for some more time actually to > serve for manual recovery of the slave.] As discussed on slack, let's now only support fully automatic recovery for the case of master_use_gtid=slave_pos. If one uses master_use_gtid=none the failover is non-trivial anyway. We can always add flipping of replicate-same-server-id later, if needed. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ee538938345: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 23 Mar '21

23 Mar '21

Hi, Andrei! On Mar 22, Andrei Elkin wrote: > > Here it goes. > > As after master crash and failover to slave, the demoted-to-slave > ex-master must be ready to face and accept its own (generated by) > events, without generally necessary --replicate-same-server-id. > > So the acceptance conditions are refined/relaxed for the semisync slave > connected to master in the GTID mode which ensures (under gtid_strict_mode ON) > that there can't be any duplicate events for execution > on such an ex-master slave. > Non-GTID "binlog-truncated" ex-master semisync slave has to be > configured with --replicate-same-server-id. Why would you generally care about same server id in the gtid mode? If gtid is not present locally, then it should be applied, right? Why to look at server id at all? It's a thing of the past, from pre-gtid times. > >> >> +--let $query1 = INSERT INTO t VALUES (20) > >> >> +--let $query2 = DELETE FROM t2 WHERE f = 666 /* no such record */ > >> >> +--source binlog_truncate_active_log.inc > >> >> + > >> >> +--echo # Case B. > >> >> +# The inverted sequence ends up to truncate only $query2 > >> >> +--let $this_search_pattern = Successfully truncated.*to remove transactions starting from GTID 0-1-10 > >> >> +--let $query1 = DELETE FROM t2 WHERE f = 0 > >> > > >> > why not `= 666 /* no such record */` ? > >> > is `= 0` important here? > >> > >> Yes, there's a fine detail here. > >> > >> 'The inverted sequence ends up' to leave in binlog > >> > >> $query1 = DELETE FROM t2 WHERE f = 0 > >> > >> which is as ineffective as 666. The patch recognizes the ineffectiveness > >> through a special value of engines involved counter in Gtid event. > >> In principle the truncation could > >> start from it rather than the following $query2. > >> But that would mean more complicated coding. > >> So the semantics of the zero is to be good, as we're so used to :-)! > > > > Sorry, I didn't understand anything. What does the value in the WHERE > > clause has to do with engines and where the binlog is truncated? ok. after discussing the issue on slack, the answer appears to be "the value in WHERE has nothing to do with any of that, it doesn't matter whether it's 0 or 666 as long as it doesn't match any rows" then, I'd say, better change 0 to 667 in the test. We wouldn't be having this discussion at all if you'd have some number that appeared arbitrary (and "667" is fine). My confusion was exactly caused by the fact that it looked like =0 matters, that it was important to hav 0 there. > >> >> +--let $query2 = INSERT INTO t VALUES (20) > >> >> +--source binlog_truncate_active_log.inc > > ... > >> >> diff --git a/sql/sql_class.h b/sql/sql_class.h > >> >> index 140394fefc1..87fa88d4c89 100644 > >> >> --- a/sql/sql_class.h > >> >> +++ b/sql/sql_class.h > >> >> @@ -4632,7 +4632,7 @@ class THD :public Statement, > >> >> LF_PINS *tdc_hash_pins; > >> >> LF_PINS *xid_hash_pins; > >> >> bool fix_xid_hash_pins(); > >> >> - > >> >> + bool is_1pc_ro_trans; > >> > > >> > I don't like how it looks, it's in THD (for every single connection > >> > and more) and it's only used in some exotic cases. It's set in two > >> > places around ha_commit_one_phase(), when it would be better to set > >> > it inside ha_commit_one_phase(), in one place only. > >> > > >> > But I cannot suggest how to get rid of it yet as I don't understand > >> > what it's for, I've asked a question about it below, in > >> > Gtid_log_event::Gtid_log_event > >> > >> The flag is to help sorting out ineffective (no engine data change) > >> STATEMENT binlog-format logged transaction. I thought it's practical > >> enough to face and important to handle. > >> Through an example, > >> let the table `t' be initially empty, and > >> let binlog contain > >> > >> trx1: INSERT INTO t SET a = 1; /* slave acked, trx got committed in engine */ > >> trx2: INSERT INTO t SET a = 2; /* not acked, trx remains prepared in engine */ > >> trx3: DELETE FROM t WHERE a = 0; /* ineffective, does not have Xid event */ > >> > >> prior a crash with their states as commented. > >> The truncate trx should be trx2 as the trx3's status > >> suggests it's harmless for involving into > >> truncation. > >> However we have yet to distinguish it from unsafe (to truncate) > >> event group ("transaction") that also ends with COMMIT query not Xid event. > > > > Hmm. I see. But thd isn't a universal dumpster for passing > > arguments to the function, don't use it like that. > > Well, I had to resort to that. To explain, > > > Please, pass this "read-only transaction with no xid" flag to the > > Gtid_log_event constructor as an argument. Or recalculate it inside > > the constructor, it's not such an expensive thing to do. > > To pass a flag ensues adding an extra arg to > struct handlerton::commit > which I don't feel easy about considering the reason. yes, one more reason to remove binlog_hton. But for now, like with binlog_commit_by_xid, I'd say, call binlog_commit directly and make binlog_hton->commit a no-op. Like: --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1935,6 +1935,7 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, if (ha_info) { + binlog_commit(ht, thd, all); for (; ha_info; ha_info= ha_info_next) { int err; > >> >> + if (!truncate_validated) > >> >> + { > >> >> + DBUG_ASSERT(round <= 2); > >> >> + /* > >> >> + Either truncate was not set or was reset, else > >> >> + it gets incremented, otherwise it may only set to an earlier > >> >> + offset only at the turn out of the 1st round. > >> > > >> > sorry, I cannot parse that :( > >> > >> The comments are for the assert in a branch that acts when the truncate position > >> (gtid) is not yet finalized. > >> Let me annotate under each assert's line. > >> > >> > > >> >> + */ > >> >> + DBUG_ASSERT(truncate_gtid.seq_no == 0 || > >> > >> 'truncate was not set or was reset' ^, that is truncate_gtid. > >> The 'was reset' is actually not present in the assert - that'd be > >> reflected with > >> truncate_reset_done == true > >> > >> > >> >> + last_gtid_coord >= binlog_truncate_coord || > >> > >> 'it gets incremented' ^ > > > > why does that mean "it gets incremented" ? > > I think my English failed me. > > It is getting incremented I understand that "it gets incremented" means "it = it + 1", no problem. How this assert verifies that "it gets incremented" ? > meant to say (in my last reply as well). > The use case is that at this point there's a truncate candidate /* 1 */ > and it has not been settled yet /* 2 */, > > !truncate_validated == true /* 2 */ && truncate_gtid.seq_no > 0 /* 1 */ > > and the current gtid is still "better" to take over as the candidate in > this block. The candidate's offset must not decrease (unless... > > >> >> + (round == 2 && truncate_set_in_1st)); > >> > >> And if it's neither of the above there must've been truncate candidate > >> set in the 1st round. > > > > ... > ... the above ). > > >> >> switch (typ) > >> >> { > >> >> + case FORMAT_DESCRIPTION_EVENT: > >> >> + if (round > 1) > >> >> + { > >> >> + if (fdle != fdle_arg) > >> >> + delete fdle; > >> >> + fdle= (Format_description_log_event*) ev; > >> > > >> > why is it needed? all binlog files should have the same > >> > Format_description_log_event anyway. > >> > >> Actually contrary to what was stated (the patch removed that claim) in > >> the parent revision of this commit, the FD:s of the > >> recovery binlog files may have different checksums. I.e > >> > >> set @@global.binlog_checksum = value > >> > >> can be issued to rotate binlog to a new hot file having different checksum value > >> while the binlog *CheckPoint* file is behind. So this is possible to > >> have at recovery: > >> > >> B_1(CP, checksum = NONE), B_2(hot, checksum = CRC32) > >> > >> which also manifests a normal use case bug I have not reported any bug (though wanted to). > > > > okay, checksum can differ. But how does it affect > > Format_description_log_event ? > > FD holds knowledge of whether events in the file are with the checksum > info. In my example above B_1 events would be (are) considered as checksummed > by the base. Okay. Please, 1. add a comment about it 2. add a test case for it ideally, also, move it into a separate commit Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f1db957c5da: MDEV-21469: Implement crash-safe binary logging of the user XA
by Sergei Golubchik 23 Mar '21

23 Mar '21

Hi, Andrei! On Mar 16, Andrei Elkin wrote: > > On Mar 14, Sujatha wrote: > >> revision-id: f1db957c5da (mariadb-10.5.2-247-gf1db957c5da) > >> parent(s): dc92235f21e > >> author: Sujatha <sujatha.sivakumar(a)mariadb.com> > >> committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > >> timestamp: 2020-12-21 16:10:46 +0200 > >> message: > >> > >> MDEV-21469: Implement crash-safe binary logging of the user XA ... > >> diff --git a/sql/log.cc b/sql/log.cc > >> index 731bb3e98f0..c69b8518cf4 100644 > >> --- a/sql/log.cc > >> +++ b/sql/log.cc > >> @@ -2019,28 +2027,49 @@ static int binlog_xa_recover_dummy(handlerton *hton __attribute__((unused)), > >> return 0; > >> } > >> > >> - > >> +/* > >> + The function invokes binlog_commit() and returns its result > >> + when it has not yet called it already. > >> + binlog_cache_mngr::completed_by_xid remembers the fact of > >> + the 1st of maximum two subsequent calls. > >> +*/ > >> static int binlog_commit_by_xid(handlerton *hton, XID *xid) > >> { > >> + int rc= 0; > >> THD *thd= current_thd; > >> - > >> - (void) thd->binlog_setup_trx_data(); > >> + binlog_cache_mngr *cache_mngr= thd->binlog_setup_trx_data(); > >> > >> DBUG_ASSERT(thd->lex->sql_command == SQLCOM_XA_COMMIT); > >> > >> - return binlog_commit(hton, thd, TRUE); > >> -} > >> + if (!cache_mngr->completed_by_xid) > > > > On what code path can you have completed_by_xid == false here? > > Let me answer broadly, the purpose of `completed_by_xid' is to > make sure binlog_hton->commit_by_xid() does not invoke lower level > binlog_commit() for the 2nd time at the following function. I understand that. And I mean that if commit_by_xid is always true, then binlog_hton->commit_by_xid can be a noop, because you always invoke binlog_commit_by_xid (or binlog_rollback_by_xid) explicitly. > I leave only two essential statements prepared with explanatory comments: > > ha_commit_or_rollback_by_xid() > { > // binlog_commit() has to be called as first hton ... > > binlog_hton->commit_by_xid(binlog_hton, xid); > > // ... So the above raises the flag (sets up a fence) to block binlog_hton > // run for the 2nd time in the loop here: > > plugin_foreach(NULL, commit ? xacommit_handlerton : xarollback_handlerton, > MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); > > Now I am thinking a (much) better option exists to block that 2nd time invocation > of binlog_commit() with > merely testing the type of hton in > > static my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, void *arg) > { > handlerton *hton= plugin_hton(plugin); > - if (hton->recover) > + if (hton->recover && hton != binlog_hton) > { > hton->commit_by_xid(); > > and the same to xarollback_handlerton. > I am now committed to turn to it. No, see above. Ideally, binlog won't have a hton and it won't be in the list of engines at all. But if that's a too big step to make now, then just make binlog_hton->commit_by_xid noop (or NULL, if that works) and invoke binlog_commit_by_xid() explicitly by name. > >> + { > >> + rc= binlog_commit(hton, thd, TRUE); > >> + cache_mngr->completed_by_xid= true; > >> + } > >> > >> + return rc; > >> +} > >> > >> @@ -10333,14 +10386,108 @@ start_binlog_background_thread() ... > >> + > >> +/** > >> + Performs recovery based on transaction coordinator log for 2pc. At the > >> + time of crash, if the binary log was in active state, then recovery for > >> + "implicit" 'xid's and explicit 'XA' transactions is initiated, > >> + otherwise merely the gtid binlog state is updated. > >> + For 'xid' and 'XA' based recovery the following steps are performed. > >> + > >> + Identify the active binlog checkpoint file. > >> + Scan the binary log from the beginning. > >> + From GTID_LIST and GTID_EVENTs reconstruct the gtid binlog state. > >> + Prepare a list of 'xid's for recovery. > >> + Prepare a list of explicit 'XA' transactions for recovery. > >> + Recover the 'xid' transactions. > >> + The explicit 'XA' transaction recovery is initiated once all the server > >> + components are initialized. Please check 'execute_xa_for_recovery()'. > > > > why explicit XA recovery is delayed? > > It has to wait for at least until after > init_update_queries(); > in init_server_components() without which Better move init_update_queries() to happen before recovery. As far as I can see, init_update_queries() only sets flags in the array of commands, it does not depend on anything and can be done anytime. Ideally all the recovery could happen in one place, not split over two separate functions that are invoked at separate points in time during the startup. > >> @@ -10561,6 +10734,219 @@ int TC_LOG_BINLOG::recover(LOG_INFO *linfo, const char *last_log_name, > >> + { > >> + enable_apply_event= true; > >> + // partially engine-prepared XA is first cleaned out prior replay > >> + thd->lex->sql_command= SQLCOM_XA_ROLLBACK; > >> + ha_commit_or_rollback_by_xid(&gev->xid, 0); > >> + } > >> + else > >> + --recover_xa_count; > >> + } > >> + } > >> + else if (gev->flags2 & Gtid_log_event::FL_COMPLETED_XA) > >> + { > >> + if (member->state == XA_COMPLETE && > >> + member->in_engine_prepare > 0) > >> + enable_apply_event= true; > > > > why? you cannot replay a fully prepared and partially committed > > transaction > > We might make use of slave_exec_mode = IDEMPOTENT but only ROW format > events are subject to the mode. Yes, so why you're trying to replay it instead of just completing the commit? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 269328884e5: MDEV-21365: Check $MARIADB_HOME/my.cnf in addition to $MYSQL_HOME/my.cnf
by Sergei Golubchik 22 Mar '21

22 Mar '21

Hi, Rucha! On Mar 22, Rucha Deodhar wrote: > revision-id: 269328884e5 (mariadb-10.5.2-269-g269328884e5) > parent(s): 46fab5b32a8 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-03-22 16:18:18 +0530 > message: > > MDEV-21365: Check $MARIADB_HOME/my.cnf in addition to $MYSQL_HOME/my.cnf > > Currently for the server, we only check if $MYSQL_HOME is set. Added a check > if $MARIADB_HOME is set and try to read the configuration file from this > directory. > To keep behavior consistent with MariaDB Connector/C, checking $MARIADB_HOME > after $MYSQL_HOME. The patch is fine. But the commit comment says exactly the opposite of what you've implemented. You check MARIADB_HOME before MYSQL_HOME (which is correct), not after. Please fix the comment and ok to push. > diff --git a/mysys/my_default.c b/mysys/my_default.c > index c76ab8c6eb1..ec94aee290c 100644 > --- a/mysys/my_default.c > +++ b/mysys/my_default.c > @@ -1051,8 +1051,17 @@ static const char **init_default_directories(MEM_ROOT *alloc) > > #endif > > - if ((env= getenv("MYSQL_HOME"))) > + /* > + If value of $MARIADB_HOME environment variable name is NULL, check > + for $MYSQL_HOME > + */ > + if ((env= getenv("MARIADB_HOME"))) > errors += add_directory(alloc, env, dirs); > + else > + { > + if ((env= getenv("MYSQL_HOME"))) > + errors += add_directory(alloc, env, dirs); > + } > > /* Placeholder for --defaults-extra-file=<path> */ > errors += add_directory(alloc, "", dirs); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0