- developers - lists.mariadb.org

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

28 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 24 Mar '21

24 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 22 Mar '21

22 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

Re: [Maria-developers] e5fc78f84e3: MDEV-20220: Merge 5.7 P_S replication table 'replication_applier_status_by_worker
by Sergei Golubchik 22 Mar '21

22 Mar '21

Hi, Sujatha! Could you split this patch, please? 1. Just add replication_applier_status_by_worker table. With CHANNEL_NAME column and (perhaps, not sure if it applies) with WORKER_ID column. Without extra columns and backup. 2. Add extra columns. 3. Add backup. With these three commits you'll have exactly the same diff as now, just split (and with CHANNEL_NAME column). But really, I wonder whether this backup functionality is needed at all? In MySQL there is persistent information about these workers, it doesn't go away when they're stopped, it's stored persistently in a table, indexed by worker_id. If we don't have anything persistent like that and all workers completely disappear into oblivion, then, may be, replication_applier_status_by_worker should not show anything when they aren't running? If you agree, then you'll only need two commits. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Mar 21, Sujatha wrote: > revision-id: e5fc78f84e3 (mariadb-10.5.2-303-ge5fc78f84e3) > parent(s): 8b8969929d7 > author: Sujatha <sujatha.sivakumar(a)mariadb.com> > committer: Sujatha <sujatha.sivakumar(a)mariadb.com> > timestamp: 2020-11-27 12:59:42 +0530 > message: > > MDEV-20220: Merge 5.7 P_S replication table 'replication_applier_status_by_worker > > Fix: > === > Iterate through rpl_parallel_thread_pool and display slave worker thread > specific information as part of 'replication_applier_status_by_worker' table. > > --------------------------------------------------------------------------------- > |Column Name: | Description: | > |-------------------------------------------------------------------------------| > | | | > |THREAD_ID | Thread_Id as displayed in 'performance_schema.threads'| > | | table for thread with name | > | | 'thread/sql/rpl_parallel_thread' | > | | | > | | THREAD_ID will be NULL when worker threads are stopped| > | | due to error/force stop | > | | | > |SERVICE_STATE | Thread is running or not | > | | | > |LAST_SEEN_TRANSACTION | Last GTID executed by worker | > | | | > |LAST_ERROR_NUMBER | Last Error that occurred on a particular worker | > | | | > |LAST_ERROR_MESSAGE | Last error specific message | > | | | > |LAST_ERROR_TIMESTAMP | Time stamp of last error | > | | | > |WORKER_IDLE_TIME | Total idle time in seconds that the worker thread has | > | | spent waiting for work from SQL thread | > | | | > |LAST_TRANS_RETRY_COUNT | Total number of retries attempted by last transaction | > --------------------------------------------------------------------------------- > > > In case STOP SLAVE is executed worker threads will be gone, hence worker > threads will be unavailable. Querying the table at this stage will give empty > rows. To address this case when worker threads are about to stop, due to an > error or forced stop, create a backup pool and preserve the data which is > relevant to populate performance schema table. Clear the backup pool upon > slave start.

2 4

Re: [Maria-developers] e5fc78f84e3: MDEV-20220: Merge 5.7 P_S replication table 'replication_applier_status_by_worker
by sujatha 22 Mar '21

22 Mar '21

Hello Andrei, Good Morning. On 22/03/21 3:50 pm, Andrei Elkin wrote: > Sujatha, howdy. > > Just one note from me at this point, > >> ==> https://dev.mysql.com/doc/refman/5.7/en/replication-channels.html >> >> A multi-source replica can also be set up as a multi-threaded >> replica, by setting the slave_parallel_workers system variable to a >> value greater than 0. When you do this on a multi-source replica, each >> channel on the replica has the specified number of applier >> threads, plus a coordinator thread to manage them. You cannot >> configure the number of applier threads for individual channels. >> >> In case of MySQL, worker threads are dedicated to particular channel. >> >> In MariaDB "The pool of replication worker threads is shared among all >> multi-source master connections, and among all replication domains >> that can replicate in parallel using out-of-order". >> >> Because of this I didn't include CHANNEL_NAME. In MariaDB Slave_worker >> thread's don't have WORKER_ID they only have 'thread_id'. > Would it make sense to report > > LAST_CHANNEL (or LAST_SOURCE) similar to other 'LAST_' like > > |LAST_SEEN_TRANSACTION | Last GTID executed by worker | > > ? Yes, as 'LAST_SOURCE', we can display 'mi->connection_name'. In MariaDB documentation 'CONNECTION_NAME' is used to identify a source, so I used 'CONNECTION_NAME' instead of 'CHANNEL_NAME'(MySQL'). Thank you. S.Sujatha > >>>> Fix: >>>> === >>>> Iterate through rpl_parallel_thread_pool and display slave worker thread >>>> specific information as part of 'replication_applier_status_by_worker' table. >>>> >>>> --------------------------------------------------------------------------------- >>>> |Column Name: | Description: | >>>> |-------------------------------------------------------------------------------| >>>> | | | >>>> |THREAD_ID | Thread_Id as displayed in 'performance_schema.threads'| >>>> | | table for thread with name | >>>> | | 'thread/sql/rpl_parallel_thread' | >>>> | | | >>>> | | THREAD_ID will be NULL when worker threads are stopped| >>>> | | due to error/force stop | >>>> | | | >>>> |SERVICE_STATE | Thread is running or not | >>>> | | | >>>> |LAST_SEEN_TRANSACTION | Last GTID executed by worker | >>>> | | | >>>> |LAST_ERROR_NUMBER | Last Error that occurred on a particular worker | >>>> | | | >>>> |LAST_ERROR_MESSAGE | Last error specific message | >>>> | | | >>>> |LAST_ERROR_TIMESTAMP | Time stamp of last error | >>>> | | | >>>> |WORKER_IDLE_TIME | Total idle time in seconds that the worker thread has | >>>> | | spent waiting for work from SQL thread | >>>> | | | >>>> |LAST_TRANS_RETRY_COUNT | Total number of retries attempted by last transaction | >>>> --------------------------------------------------------------------------------- > Cheers, > > Andrei

1 0

Re: [Maria-developers] d86fe72c027: MDEV-22010: use executables MariaDB named in scripts
by Sergei Golubchik 21 Mar '21

21 Mar '21

Hi, Rucha! On Mar 21, Rucha Deodhar wrote: > revision-id: d86fe72c027 (mariadb-10.5.2-386-gd86fe72c027) > parent(s): de407e7cb4d > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-03-18 00:58:43 +0530 > message: > > MDEV-22010: use executables MariaDB named in scripts > > As a part of this MDEV following changes were made: > 1) Mariadb named executables used instead of mysql named executables in scripts > 2) renamed mysql-test-run and mysql-stress-test to mariadb-test-run and > mariadb-stress-test and created a symlink. > > diff --git a/.gitignore b/.gitignore > index d58dd2aa2b8..1a840d1a51d 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -600,3 +600,7 @@ tests/mariadb-client-test > versioninfo_dll.rc > versioninfo_exe.rc > win/packaging/ca/symlinks.cc > +mysql-test/mysql-stress-test.pl > +mysql-test/mysql-test-run > +mysql-test/mysql-test-run.pl > +mysql-test/mtr Please, add these lines together with other mysql-test/ lines in the file. Then you'll see that mysql-test-run and mtr are already ignored. > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh > index 5f183afe8fc..147bd7351bc 100644 > --- a/scripts/mysql_install_db.sh > +++ b/scripts/mysql_install_db.sh > @@ -338,11 +338,11 @@ then > cannot_find_file resolveip @resolveip_locations@ > exit 1 > fi > - mysqld=`find_in_dirs mysqld @mysqld_locations@` > + mysqld=`find_in_dirs mariadbd @mysqld_locations@` > if test -z "$mysqld" > then > - cannot_find_file mysqld @mysqld_locations@ > - exit 1 > + cannot_find_file mysqld @mysqld_locations@ you wanted to say "cannot_find_file mariadbd @mysqld_locations@" > + exit 1 > fi > langdir=`find_in_dirs --dir errmsg.sys @errmsg_locations@` > if test -z "$langdir" > @@ -579,7 +579,7 @@ else > echo > echo " shell> $mysqld --skip-grant-tables --general-log &" > echo > - echo "and use the command line tool $bindir/mysql" > + echo "and use the command line tool $bindir/mariadb or $bindir/mysql" I don't think you need any "or mysql" here. > echo "to connect to the mysql database and look at the grant tables:" > echo > echo " shell> $bindir/mysql -u root mysql" > @@ -613,7 +613,8 @@ then > echo "PLEASE REMEMBER TO SET A PASSWORD FOR THE MariaDB root USER !" > echo "To do so, start the server, then issue the following commands:" > echo > - echo "'$bindir/mysqladmin' -u root password 'new-password'" > + echo "'$bindir/mariadb-admin' -u root password 'new-password' or" > + echo "'$bindir/mariadb-admin' -u root -h $hostname password 'new-password' or" and not here. > echo "'$bindir/mysqladmin' -u root -h $hostname password 'new-password'" > echo > echo "Alternatively you can run:" > diff --git a/scripts/mysql_secure_installation.sh b/scripts/mysql_secure_installation.sh > index b2a9edf4953..6098e0148b5 100644 > --- a/scripts/mysql_secure_installation.sh > +++ b/scripts/mysql_secure_installation.sh > @@ -159,15 +159,15 @@ then > cannot_find_file my_print_defaults $basedir/bin $basedir/extra > exit 1 > fi > - mysql_command=`find_in_basedir mysql bin` > + mysql_command=`find_in_basedir mariadb bin` > if test -z "$mysql_command" > then > - cannot_find_file mysql $basedir/bin > - exit 1 > + cannot_find_file mysql $basedir/bin should be cannot_find_file mariadb $basedir/bin" > + exit 1 > fi > else > print_defaults="@bindir@/my_print_defaults" > - mysql_command="@bindir@/mysql" > + mysql_command="@bindir@/mariadb" > fi > > if test ! -x "$print_defaults" > diff --git a/scripts/mysqld_multi.sh b/scripts/mysqld_multi.sh > index 0b33f61710e..d83663e03fc 100644 > --- a/scripts/mysqld_multi.sh > +++ b/scripts/mysqld_multi.sh > @@ -728,7 +728,7 @@ sub example > > [mysqld_multi] > mysqld = @bindir@/mysqld_safe mariadbd-safe ? (here and everywhere in this file) Also, you've missed mysqld_safe in FIND_PROC set in scripts/CMakeLists.txt > -mysqladmin = @bindir@/mysqladmin > +mysqladmin = @bindir@/mariadb-admin > user = multi_admin > password = my_password > > diff --git a/scripts/mysqld_safe.sh b/scripts/mysqld_safe.sh > index 126eb924814..7763fb9db91 100644 > --- a/scripts/mysqld_safe.sh > +++ b/scripts/mysqld_safe.sh > @@ -531,7 +531,11 @@ else > ledir='@libexecdir@' > fi > > -helper=`find_in_bin mysqld_safe_helper` > +helper=`find_in_bin mariadbd-safe-helper` > +if test -x helper > +then > + helper=`find_in_bin mysqld_safe_helper` > +fi no, remove the if, no need to fallback (and you've got the condition wrong anyway) > print_defaults=`find_in_bin my_print_defaults` > # Check if helper exists > command -v $helper --help >/dev/null 2>&1 > diff --git a/scripts/wsrep_sst_common.sh b/scripts/wsrep_sst_common.sh > index 5e134570881..b8621a76d6f 100644 > --- a/scripts/wsrep_sst_common.sh > +++ b/scripts/wsrep_sst_common.sh > @@ -273,16 +273,16 @@ SCRIPTS_DIR="$(cd $(dirname "$0"); pwd -P)" > EXTRA_DIR="$SCRIPTS_DIR/../extra" > CLIENT_DIR="$SCRIPTS_DIR/../client" > > -if [ -x "$CLIENT_DIR/mysql" ]; then > - MYSQL_CLIENT="$CLIENT_DIR/mysql" > +if [-x "$CLIENT_DIR/mariadb"]; then "[" is a name of an executable, always leave a space after it. there's no [-x executable. > + MYSQL_CLIENT="$CLIENT_DIR/mariadb" > else > - MYSQL_CLIENT=mysql > + MYSQL_CLIENT=mariadb > fi Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2858f9fb7e0: MDEV-22189: Change error messages inside code to have mariadb instead of
by Sergei Golubchik 21 Mar '21

21 Mar '21

Hi, Rucha! First, please check the buildbot and make sure all tests pass. You've changed MySQL->MariaDB in result files, but without corresponding changes in the code your result files now differ from the actual test output. One more comment below On Mar 21, Rucha Deodhar wrote: > revision-id: 2858f9fb7e0 (mariadb-10.5.2-274-g2858f9fb7e0) > parent(s): 41485551c58 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-03-18 13:58:34 +0530 > message: > > MDEV-22189: Change error messages inside code to have mariadb instead of > mysql > > Fix: Changed error messages, rerecorded results and changed other relevant > files. > > diff --git a/debian/additions/innotop/innotop b/debian/additions/innotop/innotop > index 19229a57a82..a045a479be9 100644 > --- a/debian/additions/innotop/innotop > +++ b/debian/additions/innotop/innotop > @@ -987,12 +987,12 @@ sub parse_tx_text { > # Parsing the line that begins 'MySQL thread id' is complicated. The only > # thing always in the line is the thread and query id. See function > # innobase_mysql_print_thd in InnoDB source file sql/ha_innodb.cc. > - my ( $thread_line ) = $txn =~ m/^(MySQL thread id .*)$/m; > + my ( $thread_line ) = $txn =~ m/^(MariaDB thread id .*)$/m; > my ( $mysql_thread_id, $query_id, $hostname, $ip, $user, $query_status ); > > if ( $thread_line ) { > # These parts can always be gotten. > - ( $mysql_thread_id, $query_id ) = $thread_line =~ m/^MySQL thread id $d, .*?query id $d/m; > + ( $mysql_thread_id, $query_id ) = $thread_line =~ m/^MariaDB thread id $d, .*?query id $d/m; > > # If it's a master/slave thread, "Has (read|sent) all" may be the thread's > # proc_info. In these cases, there won't be any host/ip/user info > @@ -4144,15 +4144,15 @@ my $innodb_parser = InnoDBParser->new; > > my $nonfatal_errs = join('|', > 'Access denied for user', > - 'Unknown MySQL server host', > + 'Unknown MariaDB server host', > 'Unknown database', > - 'Can\'t connect to local MySQL server through socket', > - 'Can\'t connect to MySQL server on', > + 'Can\'t connect to local MariaDB server through socket', > + 'Can\'t connect to MariaDB server on', > 'MySQL server has gone away', > 'Cannot call SHOW INNODB STATUS', > 'Access denied', > 'AutoCommit', > - 'Lost connection to MySQL server', > + 'Lost connection to MariaDB server', > 'Too many connections', > ); innotop is generally a separate tool that technically could work both with MariaDB and MySQL. So, better use (?:MariaDB|MySQL) in all regexp patterns above. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-13467 review.
by Alexey Botchkov 20 Mar '21

20 Mar '21

One thing i'd still like you to do differently. + if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint) + { + const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1 for the type + char point_temp[point_size]; + memset(point_temp+4, Geometry::wkb_point, 1); + memcpy(point_temp+5, static_cast<const Gis_multi_point *>(g)->get_data_ptr()+5, 4); + memcpy(point_temp+4+WKB_HEADER_SIZE, g->get_data_ptr()+4+WKB_ HEADER_SIZE, + POINT_DATA_SIZE); + point_temp[point_size-1]= '\0'; + Geometry_buffer gbuff; + Geometry *gg= Geometry::construct(&gbuff, point_temp, point_size-1); > Gis_multi_point::geometry_n(uint32 num, String *result) stores the point in String *, > but I need Geometry* to use further, so still would need some recalculation. You could call the ::construct with the string->prr() instead of point_temp. Like str->q_append(0); //SRID g->geometry_n(str, 1); gg= Geometry::construct(&gbuff, str->ptr(), str->length()); But I propose an even simpler method. { Gis_pont p; p.set_data_ptr(g->get_data_ptr()+4+WKB_HEADER_SIZE, POINT_DATA_SIZE); calculate_haversine(&p, radius); } > Regarding radius, Daniel has suggestion: when r=NULL, result= NULL, but r=0, > how should that be treated? > Here is MySQL example (no error, but warning) There i see that MySQL returns the error when the radius is 0 or below. Right? I think it's ok for us to do same. > In a single function we are calculating radian for x,y, I think it is faster than to call function 2 times for x and y. I doubt so. Really would like to compare the speed here. Still i'm ok with that single function if you like it better. Best regards. HF On Tue, Mar 2, 2021 at 1:16 PM Anel Husakovic <anel(a)mariadb.org> wrote: > Hi Alexey, > thanks for your review, hope everything is ok now. > Here is the new patch 1f618e36577a2a322807 > <https://github.com/MariaDB/server/commit/1f618e36577a2a32280705c640d6f9a748…> according > to your review and comments below. > > On Sat, Jan 9, 2021 at 2:09 PM Alexey Botchkov <holyfoot(a)mariadb.com> > wrote: > >> Hi, Anel! >> >> Sorry for that long delay with this review. Had real difficult weeks of >> my life :( >> Now it seems to be over, so getting back to normal. >> Below is my opinions of the code i see in the branch >> bb-10.1-anel-MDEV-13467-gis-feature-st_sphere-v2 >> Hope it's the recent version. >> >> + Adapter for functions that takes two or three arguments. >> +*/ >> + >> +class Create_func_arg2_or_3 : public Create_func >> +{ >> Do you think we need the separate Create_fund_arg2_or_3 class? >> If you don't see any other class to inherit from it, >> i'd recommend to leave the Create_func_distance_sphere alone. >> No need to have two constructors in the Item_func_sphere_distance >> too. Just use the list argument as it's done for the >> Item_func_json_contains (in 10.2). >> > > Ack ^ droped the Create_func_arg2_or_3 and inherited from > Create_native_function which can use a variable number of parameters. > Thanks > > >> Anyway i belive we should get rid of duplicating code >> about the 'param1' and 'param2'. They can be popped/checked >> with no if (arg_count == 2) condition. >> >> + double distance= 0.0; >> + null_value= (args[0]->null_value || args[1]->null_value); >> + if (null_value) >> + { >> + // Returned item must be set to Null >> + DBUG_ASSERT(maybe_null); >> don't think it's needed. >> + return 0; >> Should be return 0.0 i belive. Or goto handle_errors. >> Thet goes to all but one 'return' statements in this function. >> >> + } >> >> > ... >> > if (!arg1 || !arg2) >> >> This last condition should never happen since we have not-null_value. >> So i'd remove that 'if' section completely. >> And anyway it should do the 'return' in that branch. >> >> + null_value= args[2]->null_value; >> + if (null_value) >> + { >> + return 0; >> + } >> >> seems nicer to me this way >> if (args[2]->null_value) >> { >> null_value= true; >> goto handle_errors; >> } >> >> > Ack. > Regarding radius, Daniel has suggestion > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…>: > when r=NULL, result= NULL, but r=0, how should that be treated? > Here is MySQL > <https://dbfiddle.uk/?rdbms=mysql_8.0&fiddle=d467903dce23464df0ebf396220da886>example > (no error, but warning) > > > >> + if (!(g1= Geometry::construct(&buffer1, arg1->ptr(), arg1->length())) >> || >> + !(g2= Geometry::construct(&buffer2, arg2->ptr(), arg2->length()))) >> + { >> + goto handle_errors; >> >> we should launch the ER_GIS_INVALID_DATA here. >> >> > ER_GIS_INVALID_DATA is introduced in 10.2, I will change this (and test > case) when pushing to 10.2, but I would also push to 10.1 since there may > be users using it in 10.1. > Also when inspecting this error in 10.2 noted that lot tests have > commented this error, so will remove that comment in the same commit. > > mysql-test/suite/innodb_gis/t/gis.test > > mysql-test/suite/innodb_gis/t/1.test > > mysql-test/suite/innodb_gis/t/bug16236208.test > > mysql-test/suite/innodb_gis/t/precise.test > > mysql-test/suite/innodb_gis/t/rtree.test > > > + // Generate error message in case different geometry is used? >> Yes, i think the explainig message here is good to have. >> >> +double Item_func_sphere_distance::spherical_distance(Geometry *g1, >> + Geometry *g2, >> + const double >> sphere_radius) >> +double Item_func_sphere_distance::spherical_distance_points(Geometry *g1, >> + Geometry *g2, >> + const double >> r) >> Why these two are member static functions, not simply static? >> If you plan any 'external' use for them, please add comments about >> what exacly they do. >> >> > Changed function to be member function only. > > > >> +double Item_func_sphere_distance::spherical_distance(Geometry *g1, >> + Geometry *g2, >> + const double >> sphere_radius) >> +{ >> + double res= 0.0; >> + switch (g1->get_class_info()->m_type_id) >> + { >> + case Geometry::wkb_point: >> + res= spherical_distance_points(g1, g2, sphere_radius); >> + break; >> + >> + case Geometry::wkb_multipoint: >> + res= spherical_distance_points(g1, g2, sphere_radius); >> + break; >> >> Why do we need this function? >> We confirmed already that the type is either wkb_point or >> wkb_multipolygon, >> so just can call spherical_distance_points. No? >> >> > Indeed, left from earlier implementation (v1). > > >> >>>>> >> >> >> +class Item_func_sphere_distance: public Item_real_func >> +{ >> + double sphere_radius= 6370986.0; // Default radius equals Earth radius >> >> Some compilers don't like this syntax, and we didn't use it yet, so >> i'd recomment to move this to the constructor or even better to the >> ::val_real(). >> >> +double Gis_point::calculate_haversine(const Geometry *g, >> + const double sphere_radius) >> +{ >> + DBUG_ASSERT(sphere_radius > 0); >> + double x1r= 0.0; >> + double x2r= 0.0; >> + double y1r= 0.0; >> + double y2r= 0.0; >> That lot of unnecessary initializations seems excessive. >> >> Ack > > >> + if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint) >> + { >> + const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1 >> for the type >> + char point_temp[point_size]; >> + memset(point_temp+4, Geometry::wkb_point, 1); >> + memcpy(point_temp+5, static_cast<const Gis_multi_point >> *>(g)->get_data_ptr()+5, 4); >> + memcpy(point_temp+4+WKB_HEADER_SIZE, >> g->get_data_ptr()+4+WKB_HEADER_SIZE, >> + POINT_DATA_SIZE); >> + point_temp[point_size-1]= '\0'; >> + Geometry_buffer gbuff; >> + Geometry *gg= Geometry::construct(&gbuff, point_temp, point_size-1); >> >> There is the Gis_multi_point::geometry_n() function, should be used here >> to get the point. >> >> Gis_multi_point::geometry_n(uint32 num, String *result) stores the point > in String *, but I need Geometry* to use further, so still would need some > recalculation. > > I didn’t find a multipoint API to convert String * to Geometry* so I did > my own calculation after debugging from row data. > > > I don't like that lot of static_cast-s. >> Isn't it nicer to have just the function of >> double calculate_harvesine_points(Geometry *p1, Geometry *p2, double >> radius) >> double calculate_harvesine_multipoint_point(Geometry *mp, Geometry *p, >> double radius) >> double calculate_harvesine_multipoint_multipoint(Geometry *mp1, Geometry >> *mp2, double radius) >> which would call each other? >> >> > I would prefer to have 2 static member functions and use static cast where > needed, if you don't mind. > The same could be done with 3 functions but with the code repeated. > > >> The ::get_xy_radian() method looks unneeded to me. >> It's enough to have the to_radian(double d) function. >> >> > In a single function we are calculating radian for x,y, I think it is > faster than to call function 2 times for x and y. > > >> BTW it's possible to do the multipoint/multipoint calculations faster >> than n^2. >> Not necessary to do it right now, just not to remember. >> >> > Regarding this there are some implementations that don't use Multipoints > <https://postgis.net/docs/manual-1.4/ST_Distance_Sphere.html> as both > arguments, MySQL > <https://dev.mysql.com/doc/refman/8.0/en/spatial-convenience-functions.html>also > use only Multipoints,Points combination, not MultiPoints,MultiPoints, so we > need to decide what to support. > However, there are algorithms to calculate max > <http://citeseerx.ist.psu.edu/viewdoc/download;jsessionid=CA2AE9B932194B2970…> > ,min <https://core.ac.uk/download/pdf/82507217.pdf> distance and > O(nlong) using convex hull <https://en.wikipedia.org/wiki/Convex_hull> > (which we have implemented <https://mariadb.com/kb/en/st_convexhull/>) > and using rotating calipers > <https://en.wikipedia.org/wiki/Rotating_calipers> from N points from > convex hull it could be O(n). > > Also there is suggested usage of iteration > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…>from > Daniel. > > > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…> > > > >> Best regards. >> HF >> >> > > -- > *Anel Husaković, * > *Software developer* > *MariaDB Foundation* >

1 0