Re: [Maria-developers] 6daf451415f: Let "FTWRL " use extra(HA_EXTRA_FLUSH)
Sergei, On Thu, Apr 02, 2020 at 05:13:27PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Apr 02, Sergey Vojtovich wrote: > > On Thu, Apr 02, 2020 at 11:43:14AM +0200, Sergei Golubchik wrote: > > > Hi, Sergey! > > > > > > On Apr 01, Sergey Vojtovich wrote: > > > > revision-id: 6daf451415f (mariadb-10.5.0-69-g6daf451415f) > > > > parent(s): c24253d0fa3 > > > > author: Sergey Vojtovich > > > > committer: Sergey Vojtovich > > > > timestamp: 2019-12-25 20:24:24 +0400 > > > > message: > > > > > > > > Let "FTWRL " use extra(HA_EXTRA_FLUSH) > > > > > > > > Rather than flushing caches with tdc_remove_table(TDC_RT_REMOVE_UNUSED) > > > > flush them with extra(HA_EXTRA_FLUSH) instead. This goes inline with > > > > regular FTWRL. > > > > > > Not quite. FTWRL calls flush_tables(thd, FLUSH_ALL), and flush_tables() > > > does extra(HA_EXTRA_FLUSH) + tc_release_table() + tdc_release_share(share) > > > > > > So FTWRL still closes all tables properly. > > Sure it does. Because it acquired table/share previously - they have to be > > released eventually. > > > > "FTWRL table_name" will do the same a few lines below in > > close_thread_tables(). > > You mean after flush_tables_with_read_lock(), during the normal cleanup > at the end of the statement? Apparently close_thread_tables() at the end of flush_tables_with_read_lock(), which I was thinking about, is only called on error. So, yes, normal cleanup at the end of the satatement. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] eeba5b2b158: Fixed close_cached_connection_tables() flushing
Sergei,
On Thu, Apr 02, 2020 at 11:44:04AM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Apr 02, Sergey Vojtovich wrote:
> > revision-id: eeba5b2b158 (mariadb-10.5.0-72-geeba5b2b158)
> > parent(s): a737b71295e
> > author: Sergey Vojtovich
> > committer: Sergey Vojtovich
> > timestamp: 2019-12-25 20:24:26 +0400
> > message:
> >
> > Fixed close_cached_connection_tables() flushing
> >
> > Let DROP SERVER and ALTER SERVER perform fair affected tables flushing.
> > That is acquire MDL_EXCLUSIVE and do tdc_remove_table(TDC_RT_REMOVE_ALL).
> >
> > Aim of this patch is elimination of another inconsistent use of
> > TDC_RT_REMOVE_UNUSED. It fixes (to some extent) a problem described in the
> > beginning of sql_server.cc, when close_cached_connection_tables()
> > interferes with concurrent transaction.
> >
> > A better fix should probably introduce proper MDL locks for server
> > objects?
> >
> > Part of MDEV-17882 - Cleanup refresh version
> >
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 9494c0b7bd2..5c6d366ce6f 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -689,33 +689,21 @@ static my_bool
> > close_cached_connection_tables_callback(
> >Close cached connections
>
> Could you, perhaps, move this function to sql_server.cc ?
> Also it could be made static there.
>
> And please clarify the comment, "Close cached connections' was not
> helpful at all. May be "Close all tables with a given CONNECTION= value"
Ok.
>
> >@return false ok
> > - @return true If there was an error from
> > closed_cached_connection_tables or
> > - if there was any open connections that we had to force
> > closed
> > + @return true error, some tables may keep using old server info
> > */
> >
> > bool close_cached_connection_tables(THD *thd, LEX_CSTRING *connection)
> > {
> > - bool res= false;
> > - close_cached_connection_tables_arg argument;
> > + close_cached_connection_tables_arg argument= { thd, connection, 0 };
> >DBUG_ENTER("close_cached_connections");
> > - DBUG_ASSERT(thd);
> > -
> > - argument.thd= thd;
> > - argument.connection= connection;
> > - argument.tables= NULL;
> >
> >if (tdc_iterate(thd,
> >(my_hash_walk_action)
> > close_cached_connection_tables_callback,
> >))
> > DBUG_RETURN(true);
> >
> > - for (TABLE_LIST *table= argument.tables; table; table= table->next_local)
> > -res|= tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED,
> > - table->db.str,
> > - table->table_name.str);
> > -
> > - /* Return true if we found any open connections */
> > - DBUG_RETURN(res);
> > + DBUG_RETURN(close_cached_tables(thd, argument.tables, true,
> > + thd->variables.lock_wait_timeout));
>
> are you sure you want to do it when argument.tables is empty? It'll do
> purge_tables() which seems to be a bit extreme for DROP SERVER
You're right. I'll get this fixed.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5c26e91f755: Cleanup close_all_tables_for_name()
Sergei, On Thu, Apr 02, 2020 at 11:43:26AM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Apr 01, Sergey Vojtovich wrote: > > revision-id: 5c26e91f755 (mariadb-10.5.0-70-g5c26e91f755) > > parent(s): 6daf451415f > > author: Sergey Vojtovich > > committer: Sergey Vojtovich > > timestamp: 2019-12-25 20:24:25 +0400 > > message: > > > > Cleanup close_all_tables_for_name() > > > > close_all_tables_for_name() is always preceded by > > wait_while_table_is_used(), which makes tdc_remove_table() redundant. > > The only (now fixed) exception was close_cached_tables(). > > > > Part of MDEV-17882 - Cleanup refresh version > > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > > index 60e2b9957a4..9494c0b7bd2 100644 > > --- a/sql/sql_base.cc > > +++ b/sql/sql_base.cc > > @@ -391,13 +391,12 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables, > >if (! table) > > continue; > > > > - if (thd->mdl_context.upgrade_shared_lock(table->mdl_ticket, > > MDL_EXCLUSIVE, > > - timeout)) > > + if (wait_while_table_is_used(thd, table, > > + HA_EXTRA_PREPARE_FOR_FORCED_CLOSE)) > > the comment just above this block says > > /* > If we are under LOCK TABLES, we need to reopen the tables without > opening a door for any concurrent threads to sneak in and get > lock on our tables. To achieve this we use exclusive metadata > locks. > */ > > how do you achieve it now without the MDL_EXCLUSIVE ? Well, I didn't remove upgrade to MDL_EXCLUSIVE. It is now done by wait_while_table_is_used(). Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 6daf451415f: Let "FTWRL " use extra(HA_EXTRA_FLUSH)
Sergei, On Thu, Apr 02, 2020 at 11:43:14AM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Apr 01, Sergey Vojtovich wrote: > > revision-id: 6daf451415f (mariadb-10.5.0-69-g6daf451415f) > > parent(s): c24253d0fa3 > > author: Sergey Vojtovich > > committer: Sergey Vojtovich > > timestamp: 2019-12-25 20:24:24 +0400 > > message: > > > > Let "FTWRL " use extra(HA_EXTRA_FLUSH) > > > > Rather than flushing caches with tdc_remove_table(TDC_RT_REMOVE_UNUSED) > > flush them with extra(HA_EXTRA_FLUSH) instead. This goes inline with > > regular FTWRL. > > Not quite. FTWRL calls flush_tables(thd, FLUSH_ALL), and flush_tables() > does extra(HA_EXTRA_FLUSH) + tc_release_table() + tdc_release_share(share) > > So FTWRL still closes all tables properly. Sure it does. Because it acquired table/share previously - they have to be released eventually. "FTWRL table_name" will do the same a few lines below in close_thread_tables(). Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 7740cb24572: Don't use plugin->data for storage engine plugins
Hi Sergei, On Tue, Mar 31, 2020 at 02:15:34PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Mar 31, Sergey Vojtovich wrote: > > revision-id: 7740cb24572 (mariadb-10.4.4-472-g7740cb24572) > > parent(s): da6d7f72b0a > > author: Sergey Vojtovich > > committer: Sergey Vojtovich > > timestamp: 2019-11-15 15:23:42 +0400 > > message: > > > > Don't use plugin->data for storage engine plugins > > > > Use plugin->plugin->info->hton instead. > > plugin_data() replaced with plugin_hton(). Because plugin->data is becoming redundant with declarative approach introduced in the parent revision. > > plugin_hton() must never return NULL anymore and is only good to be called > > against plugins in PLUGIN_IS_READY state. Because there was just one use case when it wasn't enforced: iter_schema_engines(). And it was trivially fixed. > > Why is that? > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [MariaDB/server] Proper locking for mysql.gtid_slave_pos truncation (84b437d)
Kristian, Thanks for your prompt reply! On Wed, Dec 18, 2019 at 08:38:31AM +0100, Kristian Nielsen wrote: > Sergey Vojtovich writes: > > > ATTN @dr-m, @andrelkin, @SachinSetiya, @knielsen > > So IIUC, this is about incorrect usage of ha_truncate() in > rpl_slave_state::truncate_state_table(). > > This is used only for > > SET GLOBAL gtid_slave_pos = "..." > > when all slave threads are stopped and nothing else is accessing the > gtid_pos table. It is a table, so any client connection can be accessing it any time? > > So it's fine to use ha_truncate() if that can be done easily (and > correctly). But it would also be fine just to loop and delete all rows one > by one in a normal transaction, if that is simpler. gtid_slave_pos is a > small table, there are normally only a few rows per active replication > domain. It is good to have this alternative with less strict locking. I'll leave it up to Andrei to decide if he wants to implement it. > > I'm not myself very familiar with details of metadata locking etc. around > ha_truncate(). I think original code worked well initially. Then we got some InnoDB improvement, which made original code not valid. > > But looking at the code now, I don't understand why it only truncates one > table? If --gtid-pos-auto-engines is in effect, there could be multiple > tables... shouldn't they all be cleared when setting the gtid_slave_pos > variable? If so, maybe the delete-rows-one-by-one approach is in any case > preferable over ha_truncate, since it can then be done transactionally, to > not leave an inconsistent gtid_slave_pos state if one truncate fails and > another succeeds? Andrei? Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] b3473961810: MDEV-11094: Blackhole table updates on slave fail when row annotation is enabled
Hi Sujatha,
On Wed, May 29, 2019 at 04:37:27PM +0530, sujatha wrote:
> revision-id: b347396181018cedc946450cb49891f1a0aa4575
> (mariadb-10.1.39-48-gb3473961810)
> parent(s): 8358c6f03edb941be488f009c2bd0eb9df47e8c5
> author: Sujatha
> committer: Sujatha
> timestamp: 2019-05-29 15:18:52 +0530
> message:
>
> MDEV-11094: Blackhole table updates on slave fail when row annotation is
> enabled
...skip...
> diff --git a/storage/blackhole/ha_blackhole.cc
> b/storage/blackhole/ha_blackhole.cc
> index 01aaa9ea15f..43bcdc541a1 100644
> --- a/storage/blackhole/ha_blackhole.cc
> +++ b/storage/blackhole/ha_blackhole.cc
> @@ -25,6 +25,16 @@
> #include "ha_blackhole.h"
> #include "sql_class.h" // THD,
> SYSTEM_THREAD_SLAVE_SQL
>
> +static bool is_row_based_replication(THD *thd)
> +{
> + /*
> +A row event which has its thd->query() == NULL or a row event which has
> +replicate_annotate_row_events enabled. In the later case the thd->query()
> +will be pointing to the query, received through replicated annotate event
> +from master.
> + */
> + return ((thd->query() == NULL) ||
> thd->variables.binlog_annotate_row_events);
Any reason to have so many redundant parenthesis?
Why code comment rather than function comment?
May be move "thd->system_thread == SYSTEM_THREAD_SLAVE_SQL" inside as well?
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] cf9bafc: MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings
Hi Sergei,
On Fri, Oct 19, 2018 at 04:13:16PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Oct 19, Sergey Vojtovich wrote:
> > revision-id: cf9bafc8cb0943c914d118525888140ea15e6b55
> > (mariadb-10.0.36-50-gcf9bafc)
> > parent(s): 8e716138cef2d9be603cdf71701d82bcb72dfd69
> > author: Sergey Vojtovich
> > committer: Sergey Vojtovich
> > timestamp: 2018-10-18 23:50:50 +0400
> > message:
> >
> > MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings
> > in thr_lock / has_old_lock upon FLUSH TABLES
> >
> > Explicit partition access of partitioned MEMORY table under LOCK TABLES
> > may cause subsequent statements to crash the server, deadlock, trigger
> > valgrind warnings or ASAN errors. Freed memory was being used due to
> > incorrect cleanup.
> >
> > At least MyISAM and InnoDB don't seem to be affected, since their
> > THR_LOCK structures don't survive FLUSH TABLES. MEMORY keeps table shared
> > data (including THR_LOCK) even if there're no open instances.
> >
> > There's partition_info::lock_partitions bitmap, which holds bits of
> > partitions allowed to be accessed after pruning. This bitmap is
> > updated for each individual statement.
> >
> > This bitmap was abused in ha_partition::store_lock() such that when we
> > need to unlock a table, locked by LOCK TABLES, only locks for partitions
> > that were accessed by previous statement were released.
> >
> > Eventually FLUSH TABLES frees THR_LOCK_DATA objects, which are still
> > linked into THR_LOCK lists. When such THR_LOCK gets reused we end up with
> > freed memory access.
> >
> > Fixed by using ha_partition::m_locked_partitions bitmap similarly to
> > ha_partition::external_lock().
>
> Thanks, good comment.
>
> > Some unused code removed.
>
> Generally, it's better to do that in a separate commit.
>
> It's very easy to do with git citool, and doesn't requite any advance
> planning, you just edit whatever you like and then run 'git citool'
> twice, selecting what lines you want to commit each time.
Agree. I'll move this cleanup to a separate commit. Thanks for citool
explanation, very useful.
>
> > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > index 0488ebf..02de92a 100644
> > --- a/sql/ha_partition.cc
> > +++ b/sql/ha_partition.cc
> > @@ -3907,9 +3907,14 @@ THR_LOCK_DATA **ha_partition::store_lock(THD *thd,
> >}
> >else
> >{
> > -for (i= bitmap_get_first_set(&(m_part_info->lock_partitions));
> > +MY_BITMAP *used_partitions= lock_type == TL_UNLOCK ||
> > +lock_type == TL_IGNORE ?
>
> why TL_IGNORE too? external_lock only checks for TL_UNLOCK.
external_lock() checks for F_UNLCK, which is different namespace. TL_IGNORE
is something very hard to follow. In this particular case it is called
exactly by FLUSH TABLES, when it collects list of locks for tables to be
re-locked. FWICT we can't catch TL_IGNORE in this else branch otherwise.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b88e98df9da: C++11 experiment: check if all builders support it
Hi, Sergei!
On Tue, Jul 03, 2018 at 07:53:39PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> > Current status is: all staging builders pass, except for Ubuntu Precise.
>
> But we no longer build on precise, do we?
Not anymore. Though, as discussed before, it also fails to build on labrador
and my push to bb-10.4-release was built only on few platforms.
>
> Any followup MDEVs after this one is pushed?
> Like migrate atomics ops to C++11 or something?
No MDEVs, but it should happen eventually. I'd like to wait a bit with
implementing this (may be till first release) so that we're more or less
certain we won't have to revert it.
>
> > revision-id: b88e98df9daa586bd6ed1f5908ededaa65a6e464
> > (mariadb-10.3.6-16-gb88e98df9da)
> > parent(s): 9988a423d229730b252942b7fa066d1591b7eca9
> > author: Sergey Vojtovich
> > committer: Sergey Vojtovich
> > timestamp: 2018-06-04 12:41:18 +0400
> > message:
> >
> > C++11 experiment: check if all builders support it
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 76b0817f8c7..efeae3d1ca5 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -89,6 +89,14 @@ ELSE()
> > ENDIF()
> > PROJECT(${MYSQL_PROJECT_NAME})
> >
> > +IF(CMAKE_VERSION VERSION_LESS "3.1")
> > + IF(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
> > +SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=gnu++11")
>
> Why gnu++11 and not c++11 ?
I don't recall if there was a reason for this, most probably I just copied it
from somewhere.
> And why not for clang?
No special reason, I just didn't see any problems with clang so far. Though I
can't say I did try hard.
According to manual, clang enables many c++11 features by default and atomic
operations seem to be eneabled as well (if I read it correctly).
>
> > + ENDIF()
> > +ELSE()
> > + SET(CMAKE_CXX_STANDARD 11)
>
> I'd expect this to mean c++11, not gnu++11.
I see your point. We should change the above to c++11 as well to be consistent.
>
> > +ENDIF()
> > +
> > SET(CPACK_PACKAGE_NAME "MariaDB")
> > SET(CPACK_PACKAGE_DESCRIPTION_SUMMARY "MariaDB: a very fast and robust SQL
> > database server")
> > SET(CPACK_PACKAGE_URL "http://mariadb.org;)
> > diff --git a/storage/tokudb/PerconaFT/portability/toku_crash.cc
> > b/storage/tokudb/PerconaFT/portability/toku_crash.cc
> > index 0af85342a99..297cc29d9ca 100644
> > --- a/storage/tokudb/PerconaFT/portability/toku_crash.cc
> > +++ b/storage/tokudb/PerconaFT/portability/toku_crash.cc
> > @@ -70,7 +70,7 @@ run_gdb(pid_t parent_pid, const char *gdb_path) {
> > "-ex", "thread apply all bt",
> > "-ex", "thread apply all bt full",
> > exe_buf, pid_buf,
> > - NULL);
> > + (char*) NULL);
>
> Why's that?
storage/tokudb/PerconaFT/portability/toku_crash.cc: In function ‘void
run_gdb(pid_t, const char*)’:
storage/tokudb/PerconaFT/portability/toku_crash.cc:73:16: error: missing
sentinel in function call [-Werror=format=]
NULL);
^
And then man execlp specifies this sentinel as "/* (char *) NULL */".
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] innobase/sync0policy.h converts opaque pthread_t to ULINT
Hi Sergei, If you disable just DebugMutex but leave UNIV_DEBUG enabled, how do you make assertions like this work properly: `ut_ad(mutex_own())`? Regards, Sergey On Mon, Apr 16, 2018 at 01:30:33PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Apr 16, Sergey Vojtovich wrote: > > > > Something similar (HAVE_IB_ATOMIC_PTHREAD_T_GCC) was removed in > > 2b47f8ff03845f7ffe2fa3bd583dd4123dae2b61. IIRC the reason was: relevant code > > didn't follow this macro properly and this code was still functional. > > > > You may use os_thread_pf(os_thread_get_curr_id()) to cast it to ulint. Yes, > > it > > is unsafe. But I'd leave it this way assuming it works. This code is > > subject for > > refactoring and I would avoid further spread of hacks. > > Exactly. It's DebugMutex and I'd say it's not worth fixing for cases > when pthread_t doesn't fit in ulint. DebugMutex is completely disabled > in non-debug builds anyway. > > If we'll ever have a bug that would need DebugMutex to analyze and > cannot be repeated on a platform where pthread_t fits in ulint... in > that case it might make sense to get back to this. Until then I'd just > wouldn't compile DebugMutex where it doesn't work. > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] innobase/sync0policy.h converts opaque pthread_t to ULINT
Hi!
Something similar (HAVE_IB_ATOMIC_PTHREAD_T_GCC) was removed in
2b47f8ff03845f7ffe2fa3bd583dd4123dae2b61. IIRC the reason was: relevant code
didn't follow this macro properly and this code was still functional.
You may use os_thread_pf(os_thread_get_curr_id()) to cast it to ulint. Yes, it
is unsafe. But I'd leave it this way assuming it works. This code is subject for
refactoring and I would avoid further spread of hacks.
Real fix options I could immediately think of:
- let all InnoDB threads have id based on some global counter, something like
THD::thread_id
- if all threads would have been guaranteed to have some per-thread variable
(like my_thread_var), we could have used it's address as thread id
- SYS_gettid, but it isn't portable (see toku_os_gettid())
- add some mutex and access m_thread_id under mutex protection
The bigger problem is: this is not the only code that is accessing pthread_t
concurrently. In fact this code is probably least important of these.
Regards,
Sergey
On Mon, Apr 16, 2018 at 11:35:44AM +0200, Sergei Golubchik wrote:
> Hi, Teodor!
>
> I'd just say that this MutexDebug thing relies on pthread_t being
> ulint.
>
> So, I'd add a test to innodb.cmake, like, PTHREAD_T_IS_ULINT or detected
> that by the compiler in sync0policy.h. And only compiled MutexDebug is
> pthread_t is ulint. Or, at least, sizeof(pthread_t) == sizeof(ulint).
>
> On Apr 16, Teodor Mircea Ionita wrote:
> > Hi,
> >
> > While trying to build on macOS 10.13.4 with clang 9.1.0 I originally came
> > across this error:
> > --
> > .../storage/innobase/include/sync0policy.h:79:4: error: cannot initialize a
> > parameter of type 'int64' (aka 'long long') with
> > an rvalue of type 'os_thread_id_t' (aka '_opaque_pthread_t *')
> >my_atomic_storelint(_thread_id,
> > os_thread_get_curr_id());
> > ---
> > This problem was originally reported in MDEV-15778. Since pthread_t on
> > macOS is a pointer to a struct named _opaque_pthread_t, the "obvious"
> > solution for this was to explicitly cast to ULINT which in turn is defined
> > as size_t, fine so far. Solution was tested in PR#691.
> >
> > However, upon testing my patch with buildbot, on slave kvm-fulltest2, which
> > is 64bit qemu vm with Ubuntu precise-i386 (32bit) running and gcc compiling
> > in -march=i686. The above "fix" now yields:
> > --
> > /home/buildbot/buildbot/build/mariadb-10.3.6/storage/innobase/include/sync0policy.h:79:4:
> > error: invalid cast from type ‘os_thread_id_t {aka long unsigned int}’ to
> > type ‘ulint {aka unsigned int}’
> > ---
> >
> > Now reconsidering more carefully, this looks more serious then initially
> > thought, posix pthread_t(3) is defined as:
> > --
> > POSIX.1 allows an implementation wide freedom in choosing the type used to
> > represent a thread ID; for example, representation using either an
> > arithmetic type or a structure is permitted. Therefore, variables of type
> > pthread_t can't portably be compared using the C equality operator (==);
> > use pthread_equal(3)instead.
> > ---
> >
> > So in this particular platform spec, where pthread_t on Linux is defined as
> > unsigned long int integral type and if LP64 model is used, this implies
> > pthread_t always being 64bit in either both 32 and 64 arch and trying to
> > convert to size_t on a 32bit arch will fail.
> >
> > Now given that the standard particularly states pthread_t as being opaque
> > and no assumptions should be made in regards to the actual type, not even
> > an initializer type (which ULINT_UNDEFINED defines as -1), trying to cast
> > m_thread_id to any other type ( (ULINT) ) either implicitly or explicit is
> > not portable. Is this a bug that needs fixing or just impossible to handle
> > pthread id atomically in any other fashion? How about a test case I can run
> > to confirm/infirm erratic behavior?
> >
> > Since I don't have access to the particular bb slave instance to check with
> > sizeof() maybe somebody else can help me here to confirm this? Relevant
> > links bellow:
> >
> > https://jira.mariadb.org/browse/MDEV-15778
> > https://buildbot.askmonty.org/buildbot/builders/kvm-fulltest2/builds/12317/steps/compile/logs/stdio
> > https://linux.die.net/man/3/pthread_self
> > https://github.com/MariaDB/server/pull/691
> >
> > Cheers,
> > Teodor
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and secur...@mariadb.org
>
> ___
> Mailing list: https://launchpad.net/~maria-developers
> Post to : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help : https://help.launchpad.net/ListHelp
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-14929 - AddressSanitizer: memcpy-param-overlap in Field_longstr::compress
Sergei,
On Sat, Mar 31, 2018 at 01:15:02PM +0200, Sergei Golubchik wrote:
> Hi, Sergey,
>
> > From 6de643a580b4e706e0201927e43a033f5cb98970 Mon Sep 17 00:00:00 2001
> > From: Sergey Vojtovich <s...@mariadb.org>
> > Date: Wed, 21 Mar 2018 13:54:15 +0400
> > Subject: [PATCH] MDEV-14929 - AddressSanitizer: memcpy-param-overlap in
> > Field_longstr::compress
> >
> > Handle overlaping "from" and Field_blob_compressed::value for compressed
> > blobs similarily to regular blobs.
> > ---
> > mysql-test/r/column_compression.result | 13 +
> > mysql-test/t/column_compression.test | 12
> > sql/field.cc | 16 +++-
> > 3 files changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/sql/field.cc b/sql/field.cc
> > index 9da65526e145..1b044ec740e2 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -8718,17 +8718,23 @@ int Field_blob_compressed::store(const char *from,
> > size_t length,
> > {
> >ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED;
> >uint to_length= (uint)MY_MIN(max_data_length(), field_charset->mbmaxlen
> > * length + 1);
> > + String tmp(from, length, cs);
> >int rc;
> >
> > + if (from >= value.ptr() && from <= value.ptr() + value.length() &&
> > + tmp.copy(from, length, cs))
>
> minor style comment: this could be
>
> if (from >= value.ptr() && from <= value.end() && tmp.copy())
Sure, thanks!
>
> Bigger question: Field_blob::store() also checks whether charset
> conversion is necessary. Why is it not applicable here?
Character set conversion is done in Field_longstr::compress().
But generally this optimisation is not applicable indeed, we can't just
return bmove()'d string here.
>
> And, it'd be good to have a test case where from > value.ptr()
> (with a substring() function).
I'll try to come up with something.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 665402a: MDEV-14265 - RPMLint warning: shared-lib-calls-exit
Hi Sergei, Thanks for looking into this patch! On Mon, Dec 18, 2017 at 09:34:30PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Dec 18, Sergey Vojtovich wrote: > > revision-id: 665402aa9b83565c1cef3caa06cd998d3808f0d7 > > (mariadb-10.1.29-31-g665402a) > > parent(s): ef9e78c9d41a5ac644f08068e3dabad948b0e30a > > committer: Sergey Vojtovich > > timestamp: 2017-12-18 20:23:36 +0400 > > message: > > > > MDEV-14265 - RPMLint warning: shared-lib-calls-exit > > > > find_type_with_warning() client helper did exit(1) on error, exit(1) moved > > to > > clients. > > It was find_type_or_exit(), and you could've had less changes if you'd > replaced it with an inline function. Basically, copied the function into > the header and made it static inline. That's what I did initially. Then I got undeclared exit(), then I included my_global.h, then I got huge mysql.h.pp diff. Or did you have some way to workaround it on your mind? I'll fix the name, thanks. > > > mysql_read_default_options() did exit(1) on error, error is passed through > > and > > handled now. > > ok > > > my_str_malloc_default() did exit(1) on error, replaced my_str_ allocator > > functions with normal my_malloc()/my_realloc()/my_free(). > > I think you need to replace my_str_malloc_default() with > my_malloc(..., MYF(MY_FAE)), not my_malloc(..., MYF(0)) I did it this way in ctype-tis620.c and my_vsnprintf.c. But xml.c handles allocation failure, why should we bother then? > > > sql_connect.cc did many exit(1) on hash initialisation failure. Removed > > error > > check since my_hash_init() never fails. > > > > my_malloc() did exit(1) on error. Replaced with abort(). > > > > my_load_defaults() did exit(1) on error, replaced with abort(). > > I'd say, return 2 instead of exit/abort. my_load_defaults already does > it, because my_search_option_files returns 2 for OOM. Ok. > > > my_load_defaults() still does exit(0) when invoked with --print-defaults. > > dunno, return -1, for example, and exit in the caller? There're ~30 callers to be fixed. :( I'll try to come up with some alternative. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9145b7d: MDEV-11153 - Introduce status variables for table cache monitoring and tuning
Hi Sergei, The only reason is there were minor requests to make it tunable. It is easier to convert read-only var to command line option. If we know for sure that we won't do it, I can make it status var instead. Thanks, Sergey On Thu, Oct 26, 2017 at 07:15:42PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > Looks ok, thanks. Just one question: > > On Aug 10, Sergey Vojtovich wrote: > > revision-id: 9145b7dd11a0e95016304e53acf306daac323580 > > (mariadb-10.3.0-49-g9145b7d) > > parent(s): 63ad4fe5bbe77fe17ab441fcce0106bdee9b618d > > committer: Sergey Vojtovich > > timestamp: 2017-08-10 15:45:03 +0400 > > message: > > > > MDEV-11153 - Introduce status variables for table cache monitoring and > > tuning > > > > Status variables added: Table_open_cache_hits, Table_open_cache_misses, > > Table_open_cache_overflows. > > > > System variables added: table_open_cache_active_instances (read-only). > > > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > > index e89dd47..92721b6 100644 > > --- a/sql/sys_vars.cc > > +++ b/sql/sys_vars.cc > > @@ -3219,6 +3219,12 @@ static Sys_var_ulong Sys_table_cache_size( > > BLOCK_SIZE(1), NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), > > ON_UPDATE(fix_table_open_cache)); > > > > +static Sys_var_uint Sys_table_cache_active_instances( > > + "table_open_cache_active_instances", > > + "Number of active table cache instances", > > + READ_ONLY GLOBAL_VAR(tc_active_instances), NO_CMD_LINE, > > + VALID_RANGE(1, 64), DEFAULT(1), BLOCK_SIZE(1)); > > Why did you do it as a sysvar, not as a status variable? > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 62021f3: MDEV-12070 - Introduce thd_query_safe() from MySQL 5.7
Hi Sergei,
On Fri, Jun 23, 2017 at 12:47:46PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Jun 22, Sergey Vojtovich wrote:
> > revision-id: 62021f391a42c5577190aa43cb8ad91e56235b46
> > (mariadb-10.2.6-57-g62021f3)
> > parent(s): 557e1bd472612848a42e772c1fb6f8ed32ab33b4
> > committer: Sergey Vojtovich
> > timestamp: 2017-06-22 17:15:10 +0400
> > message:
> >
> > MDEV-12070 - Introduce thd_query_safe() from MySQL 5.7
> >
> > Merged relevant part of MySQL revision:
> > https://github.com/mysql/mysql-server/commit/565d20b44f24fcc855dc616164d87b03cfad10bc
> >
> > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > index f8cf829..db65eb3 100644
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -4529,6 +4529,32 @@ extern "C" LEX_STRING * thd_query_string (MYSQL_THD
> > thd)
> >return(>query_string.string);
> > }
> >
> > +
> > +/**
> > + Get the current query string for the thread.
> > +
> > + @param thd The MySQL internal thread pointer
> > + @param buf Buffer where the query string will be copied
> > + @param buflen Length of the buffer
> > +
> > + @return Length of the query
> > +
> > + @note This function is thread safe as the query string is
> > +accessed under mutex protection and the string is copied
> > +into the provided buffer. @see thd_query_string().
> > +*/
> > +
> > +extern "C" size_t thd_query_safe(MYSQL_THD thd, char *buf, size_t buflen)
> > +{
> > + mysql_mutex_lock(>LOCK_thd_data);
> > + size_t len= MY_MIN(buflen - 1, thd->query_length());
> > + memcpy(buf, thd->query(), len);
> > + mysql_mutex_unlock(>LOCK_thd_data);
> > + buf[len]= '\0';
> > + return len;
> > +}
>
> Okay. But as neither thd_query_string() nor thd_query() are part of the
> plugin API, I'd suggest to remove them, they're inherently unsafe and
> should not be used. Few other engines use thd_query_string() need to be
> fixed too (but they bypass the plugin API and we don't promise stability
> for internal functions).
thd_query() - yes, this looks fairly broken.
thd_query_string() - I'd say this one should stay, because in most cases
we need to access query_string from the same thread; in this case we better
avoid mutex for performance reasons.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 922f970: MDEV-12882 - Assertion failed in MDL_context::upgrade_shared_lock
Sergei,
On Thu, Jun 22, 2017 at 07:57:21PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Jun 22, Sergey Vojtovich wrote:
> > revision-id: 922f9700a7fb084e3f7d6cff56e27e24fc603ed7
> > (mariadb-10.2.6-58-g922f970)
> > parent(s): 62021f391a42c5577190aa43cb8ad91e56235b46
> > committer: Sergey Vojtovich
> > timestamp: 2017-06-22 17:35:36 +0400
> > message:
> >
> > MDEV-12882 - Assertion failed in MDL_context::upgrade_shared_lock
> >
> > Relaxed assertion (in MySQL it was removed).
> > For "LOCK TABLES t1 WRITE CONCURRENT, t1 READ" upgrade lock to weakest
> > existing suitable lock, which is MDL_SHARED_NO_READ_WRITE.
> >
> > diff --git a/mysql-test/t/mdl.test b/mysql-test/t/mdl.test
> > new file mode 100644
> > index 000..5a74ae5
> > --- /dev/null
> > +++ b/mysql-test/t/mdl.test
> > @@ -0,0 +1,15 @@
>
> Hm. I'd totally expect main.mdl test to exist for years :)
Me too. I guess in most cases tests for MDL need debug_sync, thus most tests
went into mdl_sync.test.
>
> > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > index 412cd1d..7c910e8 100644
> > --- a/sql/sql_parse.cc
> > +++ b/sql/sql_parse.cc
> > @@ -2787,6 +2787,7 @@ static bool lock_tables_open_and_lock_tables(THD
> > *thd, TABLE_LIST *tables)
> > ! table->prelocking_placeholder &&
> > table->table->file->lock_count() == 0)
> >{
> > +enum enum_mdl_type lock_type;
> > /*
> >In case when LOCK TABLE ... READ LOCAL was issued for table with
> >storage engine which doesn't support READ LOCAL option and
> > doesn't
> > @@ -2799,9 +2800,12 @@ static bool lock_tables_open_and_lock_tables(THD
> > *thd, TABLE_LIST *tables)
> > deadlock_handler.init();
> > thd->push_internal_handler(_handler);
> >
> > +lock_type= table->table->mdl_ticket->get_type() ==
> > MDL_SHARED_WRITE ?
> > + MDL_SHARED_NO_READ_WRITE : MDL_SHARED_READ_ONLY;
> > +
> > bool result= thd->mdl_context.upgrade_shared_lock(
> > table->table->mdl_ticket,
> > -MDL_SHARED_READ_ONLY,
> > +lock_type,
> > thd->variables.lock_wait_timeout);
>
> This only works if you lock WRITE, then READ. If you do it the other way
> around, you'll get an error. Isn't it inconsistent?
Well, it works the other way around too. But in this case connection holds two
locks: READ_ONLY and WRITE. Which is even better than NO_READ_WRITE, but I
thought connection never acquires more than one lock for a table.
Apparently number of locks acquired may depends on the oreder they were
requested. I added "reverse" test to my patch.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-12620 - set lock_wait_timeout = 1; flush tables with read lock;
Hi Sergei, On Fri, Jun 16, 2017 at 02:28:55PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Jun 16, Sergey Vojtovich wrote: > > revision-id: 056bab0880544d91ea67d18fe8db65b4f6625482 > > (mariadb-10.1.24-22-g056bab0) > > parent(s): 58f87a41bd8de7370cc05c41977fadc685826c9e > > committer: Sergey Vojtovich > > timestamp: 2017-06-16 15:47:46 +0400 > > message: > > > > MDEV-12620 - set lock_wait_timeout = 1;flush tables with read lock; > > lock not released after timeout > > > > Release GRL if FLUSH TABLES phase failed. > > > > diff --git a/mysql-test/t/mdl_sync.test b/mysql-test/t/mdl_sync.test > > index 8e80978..c479c2a 100644 > > --- a/mysql-test/t/mdl_sync.test > > +++ b/mysql-test/t/mdl_sync.test > > @@ -4806,6 +4806,30 @@ disconnect con2; > > disconnect con3; > > > > +--echo # > > +--echo # MDEV-12620 - set lock_wait_timeout = 1;flush tables with read > > lock; > > +--echo # lock not released after timeout > > +--echo # > > +CREATE TABLE t1(a INT) ENGINE=InnoDB; > > +SET debug_sync='open_tables_after_open_and_process_table SIGNAL ready > > WAIT_FOR go'; > > +send SELECT * FROM t1; > > + > > +connect (con1,localhost,root,,); > > +SET debug_sync='now WAIT_FOR ready'; > > +# lock_wait_timeout should be 0 in 10.3, so that we don't have to wait at > > all > > what's special in 10.3? Rev. 8026cd6202bd252dc9baac31d6c8bc39653bd637: ... Valid range of lock_wait_timeout and innodb_lock_wait_timeout was extended so that 0 is acceptable value (means no wait). ... > > > +SET lock_wait_timeout=1; > > +--error ER_LOCK_WAIT_TIMEOUT > > +FLUSH TABLES WITH READ LOCK; > > +SET debug_sync='now SIGNAL go'; > > + > > +connection default; > > +reap; > > +SET debug_sync='RESET'; > > +DROP TABLE t1; > > What should happen without a bug fix? > DROP TABLE hangs? Yes, DROP TABLE hangs. > > > + > > +disconnect con1; > > + > > # Check that all connections opened by test cases in this file are really > > # gone so execution of other tests won't be affected by their presence. > > --source include/wait_until_count_sessions.inc > > diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc > > index 376dfeb..d68ce96 100644 > > --- a/sql/sql_reload.cc > > +++ b/sql/sql_reload.cc > > @@ -253,7 +253,8 @@ bool reload_acl_and_cache(THD *thd, unsigned long long > > options, > >NOTE: my_error() has been already called by reopen_tables() > > within > >close_cached_tables(). > > */ > > -result= 1; > > +thd->global_read_lock.unlock_global_read_lock(thd); > > +return 1; > >} > > Sure. Looks correct. > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 00c30e5: MDEV-11645: archive.archive fails in buildbot with valgrind (Use of uninitialised value)
Hi Varun,
On Wed, Mar 01, 2017 at 07:50:01AM +0530, Varun wrote:
> revision-id: 00c30e5a234fbd01484cd069c01e97c075025323
> (mariadb-10.2.3-283-g00c30e5)
> parent(s): b13cee83f9cfcffa98d227c492411a9acec85f42
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2017-03-01 07:46:26 +0530
> message:
>
> MDEV-11645: archive.archive fails in buildbot with valgrind (Use of
> uninitialised value)
>
> Set null values to record_buffer->buffer for the VARCHAR fields
It would be nice to elaborate where uninitialized data comes from. Am I right
that it comes from record? It would be nice to explain why NULL values and CHAR
type are not affected. As well as why 10.1 is unaffected, ideally point to
revision that caused this.
>
> ---
> storage/archive/ha_archive.cc | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/storage/archive/ha_archive.cc b/storage/archive/ha_archive.cc
> index 9d6d100..5599b43 100644
> --- a/storage/archive/ha_archive.cc
> +++ b/storage/archive/ha_archive.cc
> @@ -372,10 +372,25 @@ int Archive_share::write_v1_metafile()
>
> unsigned int ha_archive::pack_row_v1(uchar *record)
> {
> + uint offset;
Why you declare offset here...
>uint *blob, *end;
>uchar *pos;
>DBUG_ENTER("pack_row_v1");
>memcpy(record_buffer->buffer, record, table->s->reclength);
> + for(Field** field= table->field; (*field) ; field++)
> + {
> +Field *fld= *field;
> +if (fld->type() == MYSQL_TYPE_VARCHAR)
> +{
> + if (!(fld->is_real_null(record - table->record[0])))
> + {
> +ptrdiff_t start= (fld->ptr - table->record[0]);
> +Field_varstring *const field_var= (Field_varstring *)fld;
> +offset= field_var->data_length() + field_var->length_size();
And use it here? Why not declare it inline with start and field_var?
> +bzero(record_buffer->buffer + start + offset, fld->field_length -
> offset + 1);
According to man bzero:
This function is deprecated (marked as LEGACY in POSIX.1-2001): use memset(3)
in new programs. POSIX.1-2008 removes the specification of bzero().
Please use memset() instead.
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
Hi Sergei,
On Thu, Dec 15, 2016 at 04:38:04PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 15, Sergey Vojtovich wrote:
> > > > + /*
> > > > An upgradable shared metadata lock which blocks all attempts to
> > > > update
> > > > table data, allowing reads.
> > > > A connection holding this kind of lock can read table metadata and
> > > > read
> > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > > > index 1ca7e9c..d3f4517 100644
> > > > --- a/sql/sql_base.cc
> > > > +++ b/sql/sql_base.cc
> > > > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list,
> > > > Open_table_context *ot_ctx)
> > > > to general/slow log would be disabled in read only transactions.
> > > >*/
> > > >if (table_list->mdl_request.type >= MDL_SHARED_WRITE &&
> > > > + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
> > >
> > > if you're going to keep read and write locks mixed,
> > > then I'd suggest to move this condition into a small inline helper,
> > > like table_list->mdl_request.is_write_lock(). it's used few times
> > > already in this very file and there will be only one place to change
> > > if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
> > I can merge this from MySQL, it is called is_write_lock_request() there. In
> > fact
> > MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
>
> I think it'd be good. There were three or four places in sql_base.cc
> where it should replace the >= check.
Ok.
>
> > > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > > > index 6dc9192..0d740bb 100644
> > > > --- a/sql/sql_parse.cc
> > > > +++ b/sql/sql_parse.cc
> ...
> > > > +thd->pop_internal_handler();
> > > > +
> > > > +if (deadlock_handler.need_reopen())
> > > > +{
> > > > + /*
> > > > +Deadlock occurred during upgrade of metadata lock.
> > > > +Let us restart acquring and opening tables for LOCK TABLES.
> > >
> > > will you use MDL_SHARED_READ_ONLY right away when retrying?
> > > or it'll do the upgrade thing again?
> > Upgrade thing again, which is farily stupid. That's how it was in original
> > code
> > and I didn't change it in a hurry.
>
> Can you do it? It's probably just updating the lock in the
> corresponding table_list entry.
>
> Still, better do it as a separate change, after you push this MDEV-11227
> commit, so that 10.2.3 wouldn't have to wait for it.
Ok. Should I consider this patch is approved?
It is possible to push it now, but taking into account complexity and bb state:
should we postpone it until bb is greener?
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
Hi Sergei,
Thanks for your feedback. Answers inline.
On Thu, Dec 15, 2016 at 02:23:03PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 13, Sergey Vojtovich wrote:
> > revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c
> > (mariadb-10.2.2-225-g5f02c66)
> > parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d
> > committer: Sergey Vojtovich
> > timestamp: 2016-12-13 16:17:49 +0400
> > message:
> >
> > MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
> >
> > Implementation of MDEV-7660 introduced unwanted incompatible change:
> > modifications under LOCK TABLES with autocommit enabled are rolled back on
> > disconnect. Previously everything was committed, because LOCK TABLES didn't
> > adjust autocommit setting.
>
> Thanks, very helpful comment (that I didn't quote in my reply for
> brevity)!
>
> Few questions below:
>
> > diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test
> > index dea16c8..30fb694 100644
> > --- a/mysql-test/t/delayed.test
> > +++ b/mysql-test/t/delayed.test
> > @@ -319,7 +319,7 @@ drop table if exists t1;
> > --enable_warnings
> > create table t1 (a int, b int);
> > insert into t1 values (1,1);
> > -lock table t1 read;
> > +lock table t1 read local;
>
> why?
This is remnant of intermediate patch, when MDL_SHARED_READ_ONLY was acquired
for
MyISAM too. It'd block subsequent "insert delayed" much earlier, making this
test
useless.
I reverted this change. Thanks for spotting!
>
> > connect (update,localhost,root,,);
> > connection update;
> > --send insert delayed into t1 values (2,2);
> > diff --git a/sql/mdl.h b/sql/mdl.h
> > index 7d659af..d77c07c 100644
> > --- a/sql/mdl.h
> > +++ b/sql/mdl.h
> > @@ -196,6 +196,12 @@ enum enum_mdl_type {
> >*/
> >MDL_SHARED_UPGRADABLE,
> >/*
> > +A shared metadata lock for cases when we need to read data from table
> > +and block all concurrent modifications to it (for both data and
> > metadata).
> > +Used by LOCK TABLES READ statement.
> > + */
> > + MDL_SHARED_READ_ONLY,
>
> Hmm, stronger than MDL_SHARED_WRITE? All read locks were
> before write locks until now. Why is that changed?
I don't think your statement is completely valid: there's MDL_SHARED_NO_WRITE
(which is data read lock) after MDL_SHARED_READ_ONLY.
Also I don't completely understand how to evalutate strength of MDL_SHARED_*
locks by their position. They all do different things and have rather complex
compatibility and weight matrices.
This position was chosen by MySQL. I can only guess that they want to keep locks
up to SW to be used by DML and locks starting with SU used by DDL or LOCK
TABLES.
>
> > + /*
> > An upgradable shared metadata lock which blocks all attempts to update
> > table data, allowing reads.
> > A connection holding this kind of lock can read table metadata and read
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 1ca7e9c..d3f4517 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list,
> > Open_table_context *ot_ctx)
> > to general/slow log would be disabled in read only transactions.
> >*/
> >if (table_list->mdl_request.type >= MDL_SHARED_WRITE &&
> > + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
>
> if you're going to keep read and write locks mixed,
> then I'd suggest to move this condition into a small inline helper,
> like table_list->mdl_request.is_write_lock(). it's used few times
> already in this very file and there will be only one place to change
> if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
I can merge this from MySQL, it is called is_write_lock_request() there. In fact
MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
>
> >thd->tx_read_only &&
> >!(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK)))
> >{
> > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > index 6dc9192..0d740bb 100644
> > --- a/sql/sql_parse.cc
> > +++ b/sql/sql_parse.cc
> > @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd)
> > static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables)
> > {
> >Lock_tables_prelocking_strategy lock_tables_prelocking_strategy;
> > + MDL_deadlock_and_lock_abort_error_handler deadlock_handler;
> > + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint();
> >uint counter;
> >TABLE_LIST *table;
Re: [Maria-developers] Deleting unused branches on github
Hi! ALL BRANCH OWNERS: PLEASE REVIEW AND CLEANUP YOUR STALE BRANCHES AT https://github.com/MariaDB/server/branches/yours IT SHOULDN'T TAKE MORE THAN A FEW MINUTES! On Mon, Nov 07, 2016 at 04:59:44PM +0100, Kristian Nielsen wrote: ...skip... > Right, that was the intention of my mail, Cc:'ing the top committer in each > branch (sorry I seem to have forgotten to Cc: you). > > Or do you have a better way to determine who is the branch owner of a > particular branch? Top committer is fine. > > Of course, if a branch should somehow end up being wrongly removed, I will > just re-push it from a backup I will take before deleting. But better to > avoid errorneous removal in the first place. I'm more worried that this email will be largerly ignored. I suggest we wait 1-2 weeks and then start catching people on IRC with the same request. If there's no action, we may consider we did our best and aren't obligated to keep these branches anymore. > > > At least the following branches may have valuable revisions: > >> origin/10.1-spider 1 year, 1 month ago Michael Widenius > >> <mo...@mariadb.org> > >> origin/10.0-power 1 year, 7 months ago Sergey Vojtovich > >> <s...@mariadb.org> > > Ok, thanks, I've removed them from the lists. > > Updated list with branches to be removed below. > > - Kristian. > > --- > > These branches have not been updated for a long time, and looked like they > were not used any more. They are annotated with the date and the committer > of the last commit: > > origin/bb-10.2-decimal5 months ago Monty > <mo...@mariadb.org> > origin/bb-10.1-jan2 8 months ago Jan > Lindström <jan.lindst...@mariadb.com> > origin/bb-10.1-xtrabackup 8 months ago > Vladislav Vaintroub <w...@mariadb.com> > origin/10.2-window_simple 9 months ago Sergei > Petrunia <pser...@askmonty.org> > origin/bb-10.2-vicentiu-create10 months ago Vicențiu > Ciorbaru <vicen...@mariadb.org> > origin/10.2-travis-ci 12 months ago Otto > Kekäläinen <o...@mariadb.org> > origin/bb-10.1-jan-encryption 1 year, 1 month ago Jan > Lindström <jan.lindst...@mariadb.com> > origin/bb-10.1-default1 year, 3 months ago Monty > <mo...@mariadb.org> > origin/10.0-FusionIO-Galera 1 year, 4 months ago Jan > Lindström <jan.lindst...@mariadb.com> > origin/10.0-custombld 1 year, 4 months ago Kristian > Nielsen <kniel...@knielsen-hq.org> > origin/10.1-window1 year, 4 months ago Vicentiu > Ciorbaru <vicen...@mariadb.org> > origin/10.0-FusionIO 1 year, 5 months ago Jan > Lindström <jan.lindst...@mariadb.com> > origin/bb-10.1-binlog_row_image 1 year, 5 months ago Vicențiu > Ciorbaru <vicen...@mariadb.org> > origin/sanja-old-10.0-batch 1 year, 10 months ago > Oleksandr Byelkin <sa...@mariadb.com> > origin/bb-10.1-eperi 1 year, 11 months ago Michael > Widenius <mo...@mariadb.org> > > > These branches all contain an MDEV in their name that has been closed: > > origin/10.1-MDEV-6877-binlog_row_image MDEV-6877 Closed > origin/10.2-MDEV-8348 MDEV-8348 Closed > origin/10.2-MDEV-9114 MDEV-9114 Closed > origin/bb-10.0.22-mdev8989 MDEV-8989 Closed > origin/bb-10.0-galera-mdev8496-hf MDEV-8496 Closed > origin/bb-10.0-mdev-10341 MDEV-10341 Closed > origin/bb-10.1-mdev6657-r2 MDEV-6657 Closed > origin/bb-10.1-mdev8646 MDEV-8646 Closed > origin/bb-10.1-mdev8989 MDEV-8989 Closed > origin/bb-10.1-mdev9362 MDEV-9362 Closed > origin/bb-10.1-mdev-9468MDEV-9468 Closed > origin/bb-10.2-mdev8646 MDEV-8646 Closed > origin/bb-10.2-mdev8789 MDEV-8789 Closed > origin/bb-10.2-mdev9864 MDEV-9864 Closed > origin/bb-5.5-mdev6735 MDEV-6735 Closed > origin/bb-5.5-mdev-9304 MDEV-9304 Closed > origin/bb-vicentiu-mdev7978 MDEV-7978 Closed > origin/hf-10.1-mdev9021 MDEV-9021 Closed > origin/hf-10.1-mdev9853 MDEV-9853 Closed > origin/MDEV-8909
Re: [Maria-developers] Deleting unused branches on github
Hi Kristian, Thanks for raising this issue. Amount of branches is painful indeed. A few comments from my side. First, your list seem to be quite outdated. Did you run git remote prune origin? At the moment of writing https://github.com/MariaDB/server says there're 124 branches. At least the following branches don't seem to exist anymore: > origin/bb-10.1-systemd1 yearago Sergey > Vojtovich <s...@mariadb.org> > origin/bb-svoj1 year, 1 month ago Sergey > Vojtovich <s...@mariadb.org> > origin/bb-10.1-atomics1 year, 10 months ago Sergei > Golubchik <s...@mariadb.org> > origin/bb-lf-no-oom 1 year, 10 months ago Sergei > Golubchik <s...@mariadb.org> > origin/bb-lf-iterator 1 year, 11 months ago Sergey > Vojtovich <s...@mariadb.org> > origin/bb-no-ndb 2 years, 3 months ago Sergei > Golubchik <s...@mariadb.org> > origin/bb-10.2-mdev10813MDEV-10813 Closed > origin/bb-10.2-mdev5492 MDEV-5492 Closed > origin/bb-10.2-MDEV-6720MDEV-6720 Closed > origin/bb-10.2-mdev9857 MDEV-9857 Closed > origin/bb-10.2-mdev7660 MDEV-7660 Closed > origin/bb-mdev6089 MDEV-6089 Closed > origin/bb-mdev7715 MDEV-7715 Closed > origin/bb-mdev7728 MDEV-7728 Closed > origin/bb-mdev7793 MDEV-7793 Closed > origin/bb-mdev7894 MDEV-7894 Closed > origin/bb-mdev7895 MDEV-7895 Closed > origin/bb-mdev7922 MDEV-7922 Closed > origin/mdev-8380 MDEV-8380 Closed > origin/svoj-gittest 1 year, 6 months ago Sergey Vojtovich > <s...@mariadb.org> Second, I strongly suggest not to remove anything without branch owner approval. Potential data loss overweights clean-up benefits. At least the following branches may have valuable revisions: > origin/10.1-spider1 year, 1 month ago Michael > Widenius <mo...@mariadb.org> > origin/10.0-power 1 year, 7 months ago Sergey > Vojtovich <s...@mariadb.org> On Mon, Nov 07, 2016 at 01:26:07PM +0100, Kristian Nielsen wrote: > We currently have more than 200 branches in the MariaDB Server github repo, > most of which look like they are no longer used. I would like to remove the > unused ones to remove clutter. Eg. in `git branch -la` or the drop-downs on > the github webpage. > > Below is a list of a little less than two-thirds of the branches that I > determined to be probably unused. I was fairly conservative in my criteria, > ie. if there was reasonable doubt, I omitted a branch from the list to > delete. > > Please take a look at the list and see if there is any branch that should > _not_ be deleted from the github repository. Or if someone objects to > deleting anything on principle grounds, let me know as well, of course. > > (I Cc'ed people that were the last to commit on a branch to be deleted). > > Note that if one of these branches should become needed again later, > eg. some bb-XXX branch not used for a long time, there is no problem > re-creating it. The only problem with removing a branch should be if it > contains contents that will be needed in the future, and which is not > available anywhere else (and I tried to avoid putting such branches on the > list in the first place). > > Also note that once these are deleted, a simple `git pull` will not remove > them from the local clone. The `git fetch --prune` can be used to effect > this. > > If there are no objections, I will do the deletions in two weeks. > > - Kristian. ...skip... Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] new PLUGIN API.
Hi Alexey,
Just soem thought from my side...
> +const char *ap_get_database(MYSQL_THD thd)
> +{
> + if (!thd)
> +return "";
> +
> + return thd->db;
> +}
I think this should be benchmarked with perf. Things like this ain't good
performance wise (we already fixed a bunch of those). That is when function call
convention is more expensive than payload. Especially if you call this many
times per query.
Check for thd != NULL is not performance wise either (because payload is really
small). I believe that thd must be set in all cases: auditing something that
doesn't have context sounds a bit strange.
> + uint audit_event_code;
> + const void *audit_event_data;
Why? Just to avoid passing this as an argument? Though these are normally just
12 bytes, (FWICS) you only need them for a very short duration. All other time
it's just waste of memory. It's not a problem alone, but THD is already way too
overloaded by things like this. This point was originally risen by Vicentiu
back in Amsterdam.
In a nutshell, performance wise it is best if there's just one function call per
event.
Regards,
Sergey
On Sun, Oct 23, 2016 at 01:16:07PM +0400, Alexey Botchkov wrote:
> Hi, Sergei.
>
> I'd like to draw your attention to this old issue:
> https://jira.mariadb.org/browse/MDEV-7389
>
> The idea was to make a bigger thing - to modify the plugin API
> so it is easier to use and let user to do more. Particularly to
> notify warnings to the audit plugins for this 7389 task.
> That was done with this task: https://jira.mariadb.org/browse/MDEV-5313
>
> Just to refresh our memory:
> I proposed to get rid off the API versions and version-dependent
> memory structures that are used to transfer data to and from the plugin.
> All we need to do is adding new 'audit_plugin_service'. Which is
> just the normal service that offer methods to the auditing plugin to
> send commands to the server and get the server data. You can look at the
> patch http://lists.askmonty.org/pipermail/commits/2016-February/009025.html
>
> So, Serg, do you have anything to say on that subject?
>
> Best regards.
> HF
>
>
> ___
> Mailing list: https://launchpad.net/~maria-developers
> Post to : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help : https://help.launchpad.net/ListHelp
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
Hi Sergei!
On Sat, Oct 15, 2016 at 11:13:24AM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Oct 15, Sergey Vojtovich wrote:
> > Hi Sergei,
> >
> > Sorry to say this, but you reviewed outdated commits. There's just one in
> > bb-10.2-mdev7660 now. Since value of those cleanups was rather questionable
> > and I got conflicts on rebase I discarded them.
>
> Oh, okay. I know that feeling, just yesterday I did two patches of
> various cleanups in mysql.cc and later discarded them too...
>
> Below are few replies and a review of the new patch
>
> > Still some answers inline.
> >
> > On Fri, Oct 14, 2016 at 08:12:52PM +0200, Sergei Golubchik wrote:
> > > Hi, Sergey!
> > >
> > > Here's a review of the last two commits in bb-10.2-mdev7660
> > > (I've quickly looked through other, cleanup, commits, didn't have any
> > > comments, so I've excluded them from the diff to make it smaller)
> > >
> > > > diff --git a/mysql-test/t/innodb_mysql_lock.test
> > > > b/mysql-test/t/innodb_mysql_lock.test
> > > > index cb57c09..85ba418 100644
> > > > --- a/mysql-test/t/innodb_mysql_lock.test
> > > > +++ b/mysql-test/t/innodb_mysql_lock.test
> > > > @@ -150,14 +150,16 @@ let $wait_condition=
> > > > --source include/wait_condition.inc
> > > > LOCK TABLES t1 READ;
> > > > SELECT release_lock('bug42147_lock');
> > > > +let $wait_condition=
> > > > + SELECT COUNT(*) > 0 FROM information_schema.processlist
> > > > + WHERE state = 'executing'
> > > > + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)';
> > > > +--source include/wait_condition.inc
> > > > +UNLOCK TABLES;
> > This change is in new patch too.
> >
> > >
> > > 1. What did this test do?
> > Attempts to trigger a warning in thr_lock.c code, see:
> > http://lists.mysql.com/commits/82819
> >
> > Since InnoDB doesn't use thr_lock, this test doesn't cover problematic code
> > anymore.
>
> Do you need to remove the "temporary suppression" from
> mysql-test-run.pl?
Removed.
>
> > > 2. What does this 'executing' mean? Really executing or waiting
> > >inside InnoDB on a lock?
> > IIRC my intent was to make sure INSERT goes through THR_LOCK stage before
> > UNLOCK,
> > which is supposed to trigger this warning.
>
> So, is it waiting on InnoDB for a lock?
Yes.
> Because if it's really executing, you cannot reliably catch it "in the
> fly". I assume it's waiting on the lock, you test couldn't have reliably
> worked otherwise.
I just attempted to trigger this warning on a vanilla 10.2 tree with no luck.
Also I don't completely understand value of this test. Should we remove it?
>
> > > > diff --git a/mysql-test/r/lock_tables_lost_commit.result
> > > > b/mysql-test/r/lock_tables_lost_commit.result
> > > > index 769e973..394ef0a 100644
> > > > --- a/mysql-test/r/lock_tables_lost_commit.result
> > > > +++ b/mysql-test/r/lock_tables_lost_commit.result
> > > > @@ -9,7 +9,6 @@ disconnect con1;
> > > > connection con2;
> > > > SELECT * FROM t1;
> > > > a
> > > > -10
> > >
> > > This test doesn't make sense after your fix. and the original bug#578
> > > reappears. The problem is that mysqlimport issues LOCK TABLES,
> > > but does not issue UNLOCK TABLES at the end. I suspect
> > > this has to be fixed now.
> > >
> > > Alternatively, you can make LOCK TABLE to commit (not rollback)
> > > a transaction at disconnect. But that would be weird, wouldn't it?
> > This change is in new patch too.
> >
> > You're right, we should fix mysqlimport. I just wonder why this trivial fix
> > wasn't introduced before?
>
> Will you do it as a part of MDEV-7660?
Better not.
>
> > > > diff --git a/mysql-test/suite/handler/handler.inc
> > > > b/mysql-test/suite/handler/handler.inc
> > > > index b1e881f..8cad6a5 100644
> > > > --- a/mysql-test/suite/handler/handler.inc
> > > > +++ b/mysql-test/suite/handler/handler.inc
> > > > @@ -1091,6 +1091,12 @@ connection default;
> > > > --reap
> > > > drop table t2;
> > > >
> > > > +# This test expects "handler t1 read a next" to get blocked on table
> > > > level
> > > > +# lock so that further "drop table t1" can break the lock and close
> > > > handler.
&g
Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
Hi Sergei,
Sorry to say this, but you reviewed outdated commits. There's just one in
bb-10.2-mdev7660 now. Since value of those cleanups was rather questionable
and I got conflicts on rebase I discarded them.
Still some answers inline.
On Fri, Oct 14, 2016 at 08:12:52PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> Here's a review of the last two commits in bb-10.2-mdev7660
> (I've quickly looked through other, cleanup, commits, didn't have any
> comments, so I've excluded them from the diff to make it smaller)
>
> > diff --git a/mysql-test/t/innodb_mysql_lock.test
> > b/mysql-test/t/innodb_mysql_lock.test
> > index cb57c09..85ba418 100644
> > --- a/mysql-test/t/innodb_mysql_lock.test
> > +++ b/mysql-test/t/innodb_mysql_lock.test
> > @@ -150,14 +150,16 @@ let $wait_condition=
> > --source include/wait_condition.inc
> > LOCK TABLES t1 READ;
> > SELECT release_lock('bug42147_lock');
> > +let $wait_condition=
> > + SELECT COUNT(*) > 0 FROM information_schema.processlist
> > + WHERE state = 'executing'
> > + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)';
> > +--source include/wait_condition.inc
> > +UNLOCK TABLES;
This change is in new patch too.
>
> 1. What did this test do?
Attempts to trigger a warning in thr_lock.c code, see:
http://lists.mysql.com/commits/82819
Since InnoDB doesn't use thr_lock, this test doesn't cover problematic code
anymore.
> 2. What does this 'executing' mean? Really executing or waiting
>inside InnoDB on a lock?
IIRC my intent was to make sure INSERT goes through THR_LOCK stage before
UNLOCK,
which is supposed to trigger this warning.
>
> > connection default;
> > --reap
> >
> > -connection con2;
> > -UNLOCK TABLES;
> > -
> > -connection default;
> > disconnect con2;
> > DROP TABLE t1;
> >
> > diff --git a/mysql-test/t/lock_sync.test b/mysql-test/t/lock_sync.test
> > index c090e3a..07c16ac 100644
> > --- a/mysql-test/t/lock_sync.test
> > +++ b/mysql-test/t/lock_sync.test
> > @@ -873,116 +879,6 @@ set @@global.concurrent_insert=
> > @old_concurrent_insert;
> >
> >
> > --echo #
> > ---echo # Test for bug #45143 "All connections hang on concurrent ALTER
> > TABLE".
> > ---echo #
> > ---echo # Concurrent execution of statements which required weak write lock
> > ---echo # (TL_WRITE_ALLOW_WRITE) on several instances of the same table and
> > ---echo # statements which tried to acquire stronger write lock (TL_WRITE,
> > ---echo # TL_WRITE_ALLOW_READ) on this table might have led to deadlock.
>
> Is this test no longer applicable? Why?
This change is in new patch too.
>From comment:
Removed tests for BUG#45143 and BUG#55930 which cover InnoDB + THR_LOCK. To
operate properly these tests require code flow to go through THR_LOCK debug
sync points, which is not the case after this patch. These tests are removed
by WL#6671 as well. An alternative is to port them to different storage
engine.
>
> > -#
> > -# Suppress warnings for INSERTs that use get_lock().
> > -#
> > -disable_query_log;
> > -call mtr.add_suppression("Unsafe statement written to the binary log using
> > statement format since BINLOG_FORMAT = STATEMENT");
> > -enable_query_log;
> > -
> > ---disable_warnings
> > -drop table if exists t1;
> > -drop view if exists v1;
> > ---enable_warnings
> > ---echo # Create auxiliary connections used through the test.
> > -connect (con_bug45143_1,localhost,root,,test,,);
> > -connect (con_bug45143_3,localhost,root,,test,,);
> > -connect (con_bug45143_2,localhost,root,,test,,);
> > -connection default;
> > ---echo # Reset DEBUG_SYNC facility before using it.
> > -set debug_sync= 'RESET';
> > ---echo # Turn off logging so calls to locking subsystem performed
> > ---echo # for general_log table won't interfere with our test.
> > -set @old_general_log = @@global.general_log;
> > -set @@global.general_log= OFF;
> > -
> > -create table t1 (i int) engine=InnoDB;
> > ---echo # We have to use view in order to make LOCK TABLES avoid
> > ---echo # acquiring SNRW metadata lock on table.
> > -create view v1 as select * from t1;
> > -insert into t1 values (1);
> > ---echo # Prepare user lock which will be used for resuming execution of
> > ---echo # the first statement after it acquires TL_WRITE_ALLOW_WRITE lock.
> > -select get_lock("lock_bug45143_wait", 0);
> > -
> > -connection con_bug45143_1;
> > ---echo # Sending:
> > ---send insert into t1 values (get_lock("lock_bug45143_wait", 100));
> > -
> > -connection con_bug45143_2;
> > ---echo # Wait until the above INSERT takes TL_WRITE_ALLOW_WRITE lock on
> > 't1'
> > ---echo # and then gets blocked on user lock 'lock_bug45143_wait'.
> > -let $wait_condition= select count(*)= 1 from information_schema.processlist
> > - where state= 'User lock' and
> > - info='insert into t1 values
> > (get_lock("lock_bug45143_wait", 100))';
> > ---source include/wait_condition.inc
> > ---echo # Ensure that upcoming SELECT waits after acquiring
Re: [Maria-developers] [Commits] 9f7e77b: MDEV-10296 - Multi-instance table cache
Hi Sergei, On Fri, Sep 09, 2016 at 09:03:23AM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Sep 07, Sergey Vojtovich wrote: > > revision-id: 9f7e77b5414b5b2ef9b5f50f131966821fefb9c7 > > (mariadb-10.1.8-229-g9f7e77b) > > parent(s): 58634f6e50b40b28533a03a1afcb68f139937351 > > committer: Sergey Vojtovich > > timestamp: 2016-09-07 12:47:43 +0400 > > message: > > > > MDEV-10296 - Multi-instance table cache > > > > - simplified access to per-share free tables list > > - explain paddings > > > > @@ -43,7 +52,9 @@ struct TDC_element > > for this share. > >*/ > >All_share_tables_list all_tables; > > - char pad[CPU_LEVEL1_DCACHE_LINESIZE]; // free_tables follows this > > immediately > > + /** Avoid false sharing between TDC_element and free_tables */ > > + char pad[CPU_LEVEL1_DCACHE_LINESIZE]; > > + Share_free_tables free_tables[0]; > > }; > > I vaguely remember that zero-length arrays are not supported > by some compilers (they're gcc extension, after all). > > So I generally use [1] in cases like this, not [0]. Ok, it's fairly easy to fix. I'll change it. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 97d212a: MDEV-10545: Server crashed in my_copy_fix_mb on querying I_S and P_S tables
Hi Nirbhay, On Fri, Sep 02, 2016 at 09:00:27AM -0400, Nirbhay Choubey wrote: > > > diff --git a/sql/log_event.cc b/sql/log_event.cc > > > index afa58af..66e7c60 100644 > > > --- a/sql/log_event.cc > > > +++ b/sql/log_event.cc > > > @@ -6022,7 +6022,7 @@ int Load_log_event::do_apply_event(NET* net, > > rpl_group_info *rgi, > > >new_db.str= (char *) rpl_filter->get_rewrite_db(db, _db.length); > > >thd->set_db(new_db.str, new_db.length); > > >DBUG_ASSERT(thd->query() == 0); > > > - thd->reset_query_inner();// Should not be needed > > > + if (thd->query()) thd->reset_query(); // Should not be needed > > Why if? Original code didn't have it. > > > > Now that we call reset_query(), the check has been added to avoid it and an > extra mutex. > And from the above assert, I think it will be ever better have this check > within unlikely(). Taking into account this assert I'd say there's no point in reset_query at all. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 48d1ba6: MDEV-10296 - Multi-instance table cache
Hi Sergei, On Wed, Aug 31, 2016 at 04:47:09PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On Aug 03, Sergey Vojtovich wrote: > > > On Jun 29, Sergey Vojtovich wrote: > > > > revision-id: 48d1ba6097939efe5efd28dd3ed9d281cc2bc2f4 > > > > (mariadb-10.2.0-93-g48d1ba6) > > > > parent(s): 8bec9746f0d5b363f385713035ca3f2daff34e1c > > > > committer: Sergey Vojtovich > > > > timestamp: 2016-06-29 16:33:08 +0400 > > > > message: > > > > > > > > MDEV-10296 - Multi-instance table cache > > > > > > > > Improve scalability by implementing multi-instance table cache. > > > > > > > > diff --git > > > > a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > > > > b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > > > > index e075c64..22b12a9 100644 > > > > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > > > > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > > > > @@ -4559,6 +4559,20 @@ NUMERIC_BLOCK_SIZE 1 > > > > ENUM_VALUE_LISTNULL > > > > READ_ONLY NO > > > > COMMAND_LINE_ARGUMENT REQUIRED > > > > +VARIABLE_NAME TABLE_OPEN_CACHE_INSTANCES > > > > +SESSION_VALUE NULL > > > > +GLOBAL_VALUE 1 > > > > +GLOBAL_VALUE_ORIGINCOMPILE-TIME > > > > > > Default value 1 and it's not auto-tuned on startup? > > > You think multi-instance table cache is better stay unused? :) > > > If not, either use a reasonable default or, may be, choose it on > > > startup based on my_gencpus() > > Yes, I think multi-instance table cache is better stay unused probably > > in 99% of installations. E.g. 2 CPU / 20 cores / 40 threads Broadwell > > is 95% satisfied by single instance. > > Well... If you add a feature that is "better stay unused probably in 99% > of installations" and disable it by default, it will not be used at all. One very valid "internal" use case is benchmarking. At least Intel and IBM were interested in improved single table load scalability. There're also numerous writings that blame MariaDB for bad scalability compared to MySQL. This patch aims to solve one of the bottlenecks. > > Let's think how we can make this feature used, is autosizing the only > option? Used among those estimated 1% who has powerful enough hardware? Not sure, probably explain this in manual? Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
[Maria-developers] Commit emails reply-to maria-developers@lists.launchpad.net
Hi! Why do commit emails have "Reply-To: maria-developers@lists.launchpad.net"? I believe it is wrong at least for the following reasons: - I must be careful if I want to reply privately, e.g. something like "hold on, I'll review it in a few hours" doesn't deserve to hit maria-developers@ - I want a person, I'm replying to, to be primary recepient, but Reply-To substitutes his email with maria-devlopers@. Makes it easy to miss reviews. Sergei mentioned that one of reasons might be so that we want to keep comm...@mariadb.org. I believe it doesn't work because: - if I do group reply, I end up with email that has CC: comm...@mariadb.org Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 294961c: MDEV-10538: MariaDB fails to start without galera_recovery in systemd mode
Hi Nirbhay, Ok to push. I suggest that we avoid such overcomplicated constructs in our default systemd configuration. Consider integrating recovery into the server (I know it's not trivial) or creating "included" config for galera. Thanks, Sergey On Mon, Aug 22, 2016 at 06:38:07PM -0400, Nirbhay Choubey wrote: > revision-id: 294961cc4d4d55730e807b3f0a7f93a3dd4d4f7b > (mariadb-10.1.16-14-g294961c) > parent(s): d0d99dec914d409d7889fcec2efd126edad10bde > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-08-22 18:38:06 -0400 > message: > > MDEV-10538: MariaDB fails to start without galera_recovery in systemd mode > > Update ExecStartPre scripts to not fail if 'galera_recovery' > script is not available. > > --- > support-files/mariadb.service.in | 6 -- > support-files/mari...@.service.in | 15 +-- > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/support-files/mariadb.service.in > b/support-files/mariadb.service.in > index 6b8b2ba..9b97172 100644 > --- a/support-files/mariadb.service.in > +++ b/support-files/mariadb.service.in > @@ -63,9 +63,11 @@ PermissionsStartOnly=true > # galera_recovery simply returns an empty string. In any case, however, > # the script is not expected to return with a non-zero status. > # It is always safe to unset _WSREP_START_POSITION environment variable. > +# Do not panic if galera_recovery script is not available. (MDEV-10538) > ExecStartPre=/bin/sh -c "systemctl unset-environment _WSREP_START_POSITION" > -ExecStartPre=/bin/sh -c "VAR=`/usr/bin/galera_recovery`; [ $? -eq 0 ] && \ > - systemctl set-environment _WSREP_START_POSITION=$VAR || exit 1" > +ExecStartPre=/bin/sh -c "[ ! -e /usr/bin/galera_recovery ] && VAR= || \ > + VAR=`/usr/bin/galera_recovery`; [ $? -eq 0 ] \ > + && systemctl set-environment _WSREP_START_POSITION=$VAR || exit 1" > > # Needed to create system tables etc. > # ExecStartPre=/usr/bin/mysql_install_db -u mysql > diff --git a/support-files/mari...@.service.in > b/support-files/mari...@.service.in > index 965e852..e941f21 100644 > --- a/support-files/mari...@.service.in > +++ b/support-files/mari...@.service.in > @@ -70,16 +70,19 @@ PermissionsStartOnly=true > # galera_recovery simply returns an empty string. In any case, however, > # the script is not expected to return with a non-zero status. > # It is always safe to unset _WSREP_START_POSITION%I environment variable. > +# Do not panic if galera_recovery script is not available. (MDEV-10538) > ExecStartPre=/bin/sh -c "systemctl unset-environment _WSREP_START_POSITION%I" > -ExecStartPre=/bin/sh -c "VAR=`/usr/bin/galera_recovery \ > - --defaults-file=@INSTALL_SYSCONF2DIR@/my%I.cnf`; [ $? -eq 0 ] && \ > - systemctl set-environment _WSREP_START_POSITION%I=$VAR || exit 1" > + > +ExecStartPre=/bin/sh -c "[ ! -e /usr/bin/galera_recovery ] && VAR= || \ > + VAR=`/usr/bin/galera_recovery > --defaults-file=@INSTALL_SYSCONF2DIR@/my%I.cnf`; [ $? -eq 0 ] \ > + && systemctl set-environment _WSREP_START_POSITION%I=$VAR || exit 1" > + > # Alternate: (remove ConditionPathExists above) > # use [mysqld.INSTANCENAME] as sections in my.cnf > # > -#ExecStartPre=/bin/sh -c "VAR=`/usr/bin/galera_recovery \ > -# --defaults-group-suffix=%I`; [ $? -eq 0 ] && \ > -# systemctl set-environment _WSREP_START_POSITION%I=$VAR || exit 1" > +#ExecStartPre=/bin/sh -c "[ ! -e /usr/bin/galera_recovery ] && VAR= || \ > +# VAR=`/usr/bin/galera_recovery --defaults-group-suffix=%I`; [ $? -eq 0 ] \ > +# && systemctl set-environment _WSREP_START_POSITION%I=$VAR || exit 1" > > # Needed to create system tables etc. > # ExecStartPre=/usr/bin/mysql_install_db -u mysql > ___ > commits mailing list > comm...@mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-10604 Disable unstable MTR tests by default
Hi Elene, In another thread you mentioned that there's alternative solution used by Otto to maintain this list. I'm generally fine with either solution. This patch as such looks Ok. How do we qualify tests unstable? Failed since the beginning of the year, modified in last version, failed in Debian? Who shall maintain this list? What is the procedure of removing test case from unstable? I'm not completely happy that 10.0 is GA, TokuDB is "stable", but (almost) all TokuDB are disabled. This is rather a bad message to our users. Regards, Sergey On Mon, Aug 22, 2016 at 03:04:26PM +0300, Elena Stepanova wrote: > Hi Sergey (Vojtovich), > > Please review the patch that we've discussed at length. > Serg, FYI. > > The length of the "unstable" list shouldn't scare anyone. At the end, this > task should cause no change in our current procedure, the tests will still > be run in buildbot and hopefully on developers' local machines (and every > external user who wants to run them can do it easily). The change should > mainly help those who run tests as a part of their build process. > > The lists can be tuned later, now they have been created generously, to > solve the most urgent issue in shortest time possible. > > Please note that it's still not a 100% guarantee that nothing will fail > sporadically, I've already got an absolutely new random failure after > disabling all known ones, so it will still require some iterations. > > Regards, > Elena > > Forwarded Message > Subject: [Commits] 5ac5050: MDEV-10604 Disable unstable MTR tests by default > Date: Mon, 22 Aug 2016 14:44:30 +0300 (EEST) > From: ele...@montyprogram.com > Reply-To: maria-developers@lists.launchpad.net > To: comm...@mariadb.org > > revision-id: 5ac505064a75508213d4dbce388ff5ee62d1ad56 > (mariadb-10.0.26-92-g5ac5050) > parent(s): 05f61ba46046ca835071a73b9255e787dcce9255 > author: Elena Stepanova > committer: Elena Stepanova > timestamp: 2016-08-22 14:38:27 +0300 > message: > > MDEV-10604 Disable unstable MTR tests by default > > - add new syntax to disabled.def in MTR to distinguish "unstable" > tests from disabled tests; > - allow wildcards in test names in disabled.def; > - add a new option enable-unstable and env option MTR_ENABLE_DISABLED; > - mark "unstable" tests which have failed since the beginning > of the year in buildbot on main 10.0 branch; > - mark "unstable" tests which fail in Debian builds; > - mark "unstable" tests which have been changed during the last > two releases ...skip... Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 48d1ba6: MDEV-10296 - Multi-instance table cache
Hi Sergei,
On Tue, Aug 02, 2016 at 06:54:16PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> Ok, see comments below.
>
> But why is this supposed to improve scalability?
> Are you talking about the use case of one table being access by multiple
> threads? Or this helps in other cases too?
Aim of this patch is to improve scalability when single table is being accessed
by multiple threads.
> And what is the effect of the more expensive tc_remove_all_unused_tables() ?
Not sure I understand this question, could you elaborate?
>
> On Jun 29, Sergey Vojtovich wrote:
> > revision-id: 48d1ba6097939efe5efd28dd3ed9d281cc2bc2f4
> > (mariadb-10.2.0-93-g48d1ba6)
> > parent(s): 8bec9746f0d5b363f385713035ca3f2daff34e1c
> > committer: Sergey Vojtovich
> > timestamp: 2016-06-29 16:33:08 +0400
> > message:
> >
> > MDEV-10296 - Multi-instance table cache
> >
> > Improve scalability by implementing multi-instance table cache.
> >
> > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> > b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> > index e075c64..22b12a9 100644
> > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> > @@ -4559,6 +4559,20 @@ NUMERIC_BLOCK_SIZE 1
> > ENUM_VALUE_LISTNULL
> > READ_ONLY NO
> > COMMAND_LINE_ARGUMENT REQUIRED
> > +VARIABLE_NAME TABLE_OPEN_CACHE_INSTANCES
> > +SESSION_VALUE NULL
> > +GLOBAL_VALUE 1
> > +GLOBAL_VALUE_ORIGINCOMPILE-TIME
>
> Default value 1 and it's not auto-tuned on startup?
> You think multi-instance table cache is better stay unused? :)
> If not, either use a reasonable default or, may be, choose it on
> startup based on my_gencpus()
Yes, I think multi-instance table cache is better stay unused probably in 99%
of installations. E.g. 2 CPU / 20 cores / 40 threads Broadwell is 95% satisfied
by single instance.
If we want to autosize table_open_cache_instances, we should base on the
following (most important to less important):
- expected database load (we can hardly guess that)
- number of physical CPU's or NUMA nodes
- architecture (newer architectures have cheaper cache cohernece and need less
instances)
- number of cores
- number of threads
- frequency
IIRC my_getncpus() returns number of HW threads, which alone is not good for
autosizing.
>
> > +DEFAULT_VALUE 1
> > +VARIABLE_SCOPE GLOBAL
> > +VARIABLE_TYPE BIGINT UNSIGNED
> > +VARIABLE_COMMENT The number of table cache instances
> > +NUMERIC_MIN_VALUE 1
> > +NUMERIC_MAX_VALUE 64
> > +NUMERIC_BLOCK_SIZE 1
> > +ENUM_VALUE_LISTNULL
> > +READ_ONLY YES
> > +COMMAND_LINE_ARGUMENT REQUIRED
> > VARIABLE_NAME THREAD_CACHE_SIZE
> > SESSION_VALUE NULL
> > GLOBAL_VALUE 151
> > diff --git a/sql/table_cache.cc b/sql/table_cache.cc
> > index bfe51df..f1eb006 100644
> > --- a/sql/table_cache.cc
> > +++ b/sql/table_cache.cc
> > @@ -125,67 +114,101 @@ static int fix_thd_pins(THD *thd)
> >part of table definition cache.
> > */
> >
> > +struct Share_free_tables
> > {
> > + typedef I_P_List <TABLE, TABLE_share> List;
> > + List list;
> > + char pad[CPU_LEVEL1_DCACHE_LINESIZE];
>
> a comment, please, explaining the padding.
Ok.
> Or see a suggestion at the end.
>
> > +};
> >
> > +
> > +struct Table_cache_instance
> > +{
> > + /**
> > +Protects free_tables (TABLE::global_free_next and
> > TABLE::global_free_prev),
> > +records, Share_free_tables::List (TABLE::prev and TABLE::next),
> > +TABLE::in_use.
> > + */
> > + mysql_mutex_t LOCK_table_cache;
> > + I_P_List <TABLE, I_P_List_adapter<TABLE, ::global_free_next,
> > +::global_free_prev>,
> > +I_P_List_null_counter, I_P_List_fast_push_back >
> > +free_tables;
> > + ulong records;
> > + char pad[CPU_LEVEL1_DCACHE_LINESIZE];
>
> why is it padded?
To avoid false sharing between instances (global free_tables lists).
>
> > +
> > + Table_cache_instance(): records(0)
> >{
> > +mysql_mutex_init(key_LOCK_table_cache, _table_cache,
> > + MY_MUTEX_INIT_FAST);
> >}
> > +
> > + ~Table_cache_instance()
> > + {
> > +mysql_mutex_destroy(_table_cache);
> > +DBUG_ASSERT(free_tables.is_empty());
> > +DBUG_ASSERT(records == 0);
> > + }
> > +};
> >
> >
> &g
Re: [Maria-developers] e252dae: MDEV-9872 - Add common optimized CRC32 function interface
Hi Sergei, On Tue, Aug 02, 2016 at 07:22:14PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > So, what did you do here? Did you change InnoDB's version of crc32 or > just moved it to extra/ ? Just moved it to extra, no changes to crc32-vpmsum code. Also let server use it on PPC64LE. > > You removed mysys/my_crc32.c - where will crc32 come from on x86? That was a deadcode. > > Why did you name new function crc32_vpmsum instead of just naming it > crc32 and avoding #ifdef's elsewhere? Well, I didn't name it. The name was chosen by library authors. I can see only one ifdef that can be avoided (mysys/checksum.c). InnoDB never calls crc32(), so it has to have this mess. I can change it, but my preference is to keep it and make my_checksum() static inline instead. > > On Jul 05, Sergey Vojtovich wrote: > > revision-id: e252dae86392a2b50d647a380db36fb8b01bf405 > > (mariadb-10.2.1-5-ge252dae) > > parent(s): 72c94872835f27b536f617978dbc664ddb57da06 > > committer: Sergey Vojtovich > > timestamp: 2016-07-05 18:09:23 +0400 > > message: > > > > MDEV-9872 - Add common optimized CRC32 function interface > > > > Move crc32-vpmsum to extra. > > Compile static crc32-vpmsum instead of adding sources directly. > > Make use of crc32-vpmsum via my_checksum(). > > > > Based on contribution by Daniel Black. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 5486f34: MDEV-10341: InnoDB: Failing assertion: mutex_own(mutex) - mutex_exit_func
Hi Vicentiu, Ok to push. This patch is against 10.2, do you plan to fix previous versions separately? On Mon, Aug 01, 2016 at 02:46:03PM +0300, vicentiu wrote: > revision-id: 5486f3458859fa4bd161150b9ac0e9c0f633a0e3 > (mariadb-10.2.1-9-g5486f34) > parent(s): 08683a726773f8cdf16a4a3dfb3920e5f7842481 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-08-01 14:43:41 +0300 > message: > > MDEV-10341: InnoDB: Failing assertion: mutex_own(mutex) - mutex_exit_func > > Fix memory barrier issues on releasing mutexes. We must have Looks like a cutted comment. > > --- > storage/innobase/include/os0sync.h| 17 - > storage/innobase/include/sync0sync.ic | 2 ++ > storage/xtradb/include/os0sync.h | 16 > storage/xtradb/include/sync0sync.ic | 2 ++ > 4 files changed, 20 insertions(+), 17 deletions(-) Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 5486f34: MDEV-10341: InnoDB: Failing assertion: mutex_own(mutex) - mutex_exit_func
Hi Vicentiu,
I'm back from vacation, but quite busy with paperwork etc. I'll review your
patch tomorrow.
Regards,
Sergey
On Mon, Aug 01, 2016 at 02:46:03PM +0300, vicentiu wrote:
> revision-id: 5486f3458859fa4bd161150b9ac0e9c0f633a0e3
> (mariadb-10.2.1-9-g5486f34)
> parent(s): 08683a726773f8cdf16a4a3dfb3920e5f7842481
> author: Vicențiu Ciorbaru
> committer: Vicențiu Ciorbaru
> timestamp: 2016-08-01 14:43:41 +0300
> message:
>
> MDEV-10341: InnoDB: Failing assertion: mutex_own(mutex) - mutex_exit_func
>
> Fix memory barrier issues on releasing mutexes. We must have
>
> ---
> storage/innobase/include/os0sync.h| 17 -
> storage/innobase/include/sync0sync.ic | 2 ++
> storage/xtradb/include/os0sync.h | 16
> storage/xtradb/include/sync0sync.ic | 2 ++
> 4 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/storage/innobase/include/os0sync.h
> b/storage/innobase/include/os0sync.h
> index 1cf4e9c..08911e7 100644
> --- a/storage/innobase/include/os0sync.h
> +++ b/storage/innobase/include/os0sync.h
> @@ -479,20 +479,13 @@ os_atomic_test_and_set(volatile lock_word_t* ptr)
> }
>
> /** Do an atomic release.
> -
> -In theory __sync_lock_release should be used to release the lock.
> -Unfortunately, it does not work properly alone. The workaround is
> -that more conservative __sync_lock_test_and_set is used instead.
> -
> -Performance regression was observed at some conditions for Intel
> -architecture. Disable release barrier on Intel architecture for now.
> @param[in,out] ptr Memory location to write to
> @return the previous value */
> inline
> -lock_word_t
> +void
> os_atomic_clear(volatile lock_word_t* ptr)
> {
> - return(__sync_lock_test_and_set(ptr, 0));
> + __sync_lock_release(ptr);
> }
>
> # elif defined(HAVE_IB_GCC_ATOMIC_TEST_AND_SET)
> @@ -861,6 +854,12 @@ for synchronization */
> architecture. Disable memory barrier for Intel architecture for now. */
> # define os_rmb do { } while(0)
> # define os_wmb do { } while(0)
> +# ifndef __WIN__
> +# define os_mb __sync_synchronize()
> +# else
> + /* Windows atomic primitives imply a full memory barrier themselves. */
> +# define os_mb do { } while(0)
> +# endif
> # define os_isync do { } while(0)
> # define IB_MEMORY_BARRIER_STARTUP_MSG \
> "Memory barrier is not used"
> diff --git a/storage/innobase/include/sync0sync.ic
> b/storage/innobase/include/sync0sync.ic
> index 85c7f75..19474e7 100644
> --- a/storage/innobase/include/sync0sync.ic
> +++ b/storage/innobase/include/sync0sync.ic
> @@ -178,6 +178,8 @@ mutex_exit_func(
> to wake up possible hanging threads if
> they are missed in mutex_signal_object. */
>
> + os_mb;
> +
> if (mutex_get_waiters(mutex) != 0) {
>
> mutex_signal_object(mutex);
> diff --git a/storage/xtradb/include/os0sync.h
> b/storage/xtradb/include/os0sync.h
> index 0f93f3f..f06aa9d 100644
> --- a/storage/xtradb/include/os0sync.h
> +++ b/storage/xtradb/include/os0sync.h
> @@ -531,19 +531,13 @@ os_atomic_test_and_set(volatile lock_word_t* ptr)
>
> /** Do an atomic release.
>
> -In theory __sync_lock_release should be used to release the lock.
> -Unfortunately, it does not work properly alone. The workaround is
> -that more conservative __sync_lock_test_and_set is used instead.
> -
> -Performance regression was observed at some conditions for Intel
> -architecture. Disable release barrier on Intel architecture for now.
> @param[in,out] ptr Memory location to write to
> @return the previous value */
> inline
> -lock_word_t
> +void
> os_atomic_clear(volatile lock_word_t* ptr)
> {
> - return(__sync_lock_test_and_set(ptr, 0));
> + __sync_lock_release(ptr);
> }
>
> # elif defined(HAVE_IB_GCC_ATOMIC_TEST_AND_SET)
> @@ -912,6 +906,12 @@ for synchronization */
> architecture. Disable memory barrier for Intel architecture for now. */
> # define os_rmb do { } while(0)
> # define os_wmb do { } while(0)
> +# ifndef __WIN__
> +# define os_mb __sync_synchronize()
> +# else
> + /* Windows atomic primitives imply a full memory barrier themselves. */
> +# define os_mb do { } while(0)
> +# endif
> # define os_isync do { } while(0)
> # define IB_MEMORY_BARRIER_STARTUP_MSG \
> "Memory barrier is not used"
> diff --git a/storage/xtradb/include/sync0sync.ic
> b/storage/xtradb/include/sync0sync.ic
> index 83f28bf..dbecc79 100644
> --- a/storage/xtradb/include/sync0sync.ic
> +++ b/storage/xtradb/include/sync0sync.ic
> @@ -181,6 +181,8 @@ mutex_exit_func(
> to wake up possible hanging threads if
> they are missed in mutex_signal_object. */
>
> + os_mb;
> +
> if (mutex_get_waiters(mutex) != 0) {
>
> mutex_signal_object(mutex);
> ___
> commits mailing list
> comm...@mariadb.org
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Re: [Maria-developers] [Commits] 3bbf3d2: MDEV-10216: Assertion `strcmp(share->unique_file_name, filename) ||
Hi Nirbhay, Looks good, thanks! On Fri, Jul 01, 2016 at 02:47:29PM -0400, Nirbhay Choubey wrote: > revision-id: 3bbf3d2e246da1a2fd458b634f30cbe35afb5668 > (mariadb-10.2.0-142-g3bbf3d2) > parent(s): 76f492e26d8d45c678c570be2cf4ca5d238edceb > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-07-01 14:47:27 -0400 > message: > > MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) || > > .. share->last_version' failed in myisam/mi_open.c:67: test_if_reopen > > During the RENAME operation since the renamed temporary table is also > opened and added to myisam_open_list/maria_open_list, resetting the > last_version at the end of operation (HA_EXTRA_PREPARE_FOR_RENAME) > will cause an assertion failure when a subsequent query tries to open > an additional temporary table instance and thus attempts to reuse it > from the open table list. > > This commit fixes the issue by skipping flush/close operations executed > toward the end of ALTER for temporary tables. It also enables a shortcut > for simple ALTERs (like rename, disable/enable keys) on temporary > tables. > > As safety checks, added some assertions at code points that should not > be hit for temporary tables. ... Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [MariaDB/server] MDEV-10298: Systemd hardening (#195)
Hi Kristian, I did watch this thread and I seen that GPLv2 is acceptable and probably even desired license. What I was waiting for is feedback from Rasmus to confirm he's fine about this change (since he's original author of this template). Rasmus, could you approve this template change? Thanks, Sergey On Wed, Jun 29, 2016 at 02:19:00PM +0200, Kristian Nielsen wrote: > Sergey Vojtovich <notificati...@github.com> writes: > > > Similar to other open source projects, the MariaDB Foundation needs to > > have shared ownership of all code that is included in the MariaDB > > distribution. The easiest way to achieve this is by submitting your > > code under the BSD-new license. > > > > The other alternative is to sign the code contribution agreement which > > can be found here: https://mariadb.com/kb/en/mariadb/mca/ > > As was concluded in this mailing thread, > > https://lists.launchpad.net/maria-developers/msg09606.html > > the third option is to contribute the code under GPLv2. Personally, I would > strongly recommend to contribute under GPLv2 only. There are good and > important reason why the GPL exists and why it is used by many of the most > successful free software projects. Most of the code by core developers is > contributed to the project under GPLv2 only, as is most of the major > external contributions. > > Svoj, please update your "contributer request template" to mention > prominently the GPLv2 option, and use it in all future cases. eg: > > --- > Similar to other open source projects, the MariaDB Foundation needs to have > the right to include and re-distribute under the GPL all code that is > included in the MariaDB distribution. The easiest way to achieve this is by > submitting your code under the same license as the MariaDB source code, > GPLv2. > > The other alternatives are to submit under the BSD-new license, or to sign > the code contribution agreement which can be found here: > https://mariadb.com/kb/en/mariadb/mca/. These alternatives both provide > further rights for the MariaDB Foundation and its members to use the code in > your contribution, including the rights to submit it for inclusion in Oracle > MySQL or to use it commercially in proprietary products. > > Please indicate in a comment below that you are contributing your new code > of the whole pull request, including one or several files that are either > new files or modified ones, under the GPLv2 license, under the BSD-new > license, or that you have filled out the contribution agreement and sent it. > --- > > Thanks, > > - Kristian. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 511bd2c: MDEV-10216: Assertion `strcmp(share->unique_file_name, filename) ||
Hi Nirbhay, last_version of MyISAM is used for debugging only and has no functional effect. last_version of Aria has some functional effect. Effect of HA_EXTRA_PREPARE_FOR_RENAME for MyISAM is flush key blocks and reset last_version. Effect of HA_EXTRA_PREPARE_FOR_RENAME for Aria is more complex. Other engines ignore HA_EXTRA_PREPARE_FOR_RENAME. I don't completely understand why "ALTER TABLE ... RENAME TO ..." has to copy data between tables. But it is probably subject of another bug (or even task). copy_data_between_tables() seem to be the only function that does HA_EXTRA_PREPARE_FOR_RENAME for temporary table (intermediate in this case). I don't think the above HA_EXTRA_PREPARE_FOR_RENAME is reasonable for MyISAM. There's no point in resetting last_version and flushing key blocks, since we're going to reuse this itermediate table anyway. I don't think the above can be reasonable for any engine. Said the above, I'd vote to remove this call. But please check with some Aria expert. Your fix is mostly alright, but I guess we shouldn't reset last_version in case of HA_EXTRA_PREPARE_FOR_DROP. Regards, Sergey On Wed, Jun 15, 2016 at 08:57:06AM -0400, Nirbhay Choubey wrote: > revision-id: 511bd2ca3269bc7bf80e30cceeab534f7f3e5666 > (mariadb-10.2.0-83-g511bd2c) > parent(s): b2ae32aafdd2787ad456f38833f630182ded25e8 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-06-15 08:57:04 -0400 > message: > > MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) || > > .. share->last_version' failed in myisam/mi_open.c:67: test_if_reopen > > During the RENAME operation since the renamed temporary table is also > opened and added to myisam_open_list/maria_open_list, resetting the > last_version at the end of operation (HA_EXTRA_PREPARE_FOR_RENAME) > will cause an assertion failure when a subsequent query tries to open > an additional temporary table instance and thus attempts to reuse it > from the open table list. > > Fixed by not resetting share->last_version for temporary tables so that > the share gets reused when needed. > > --- > mysql-test/r/reopen_temp_table.result | 18 ++ > mysql-test/t/reopen_temp_table.test | 16 > storage/maria/ma_extra.c | 7 ++- > storage/myisam/mi_extra.c | 7 ++- > 4 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/reopen_temp_table.result > b/mysql-test/r/reopen_temp_table.result > index 08affaa..e63a21e 100644 > --- a/mysql-test/r/reopen_temp_table.result > +++ b/mysql-test/r/reopen_temp_table.result > @@ -164,5 +164,23 @@ SELECT COUNT(*)=4 FROM t6; > COUNT(*)=4 > 1 > DROP TABLE t5, t6; > +# > +# MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) || > +# share->last_version' failed in myisam/mi_open.c:67: test_if_reopen > +# > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=MYISAM; > +INSERT INTO t7 VALUES(1); > +ALTER TABLE t7 RENAME TO t; > +SELECT * FROM t a, t b; > +ii > +11 > +DROP TABLE t; > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=ARIA; > +INSERT INTO t7 VALUES(1); > +ALTER TABLE t7 RENAME TO t; > +SELECT * FROM t a, t b; > +ii > +11 > +DROP TABLE t; > # Cleanup > DROP DATABASE temp_db; > diff --git a/mysql-test/t/reopen_temp_table.test > b/mysql-test/t/reopen_temp_table.test > index 98de983..83a5bbc 100644 > --- a/mysql-test/t/reopen_temp_table.test > +++ b/mysql-test/t/reopen_temp_table.test > @@ -159,5 +159,21 @@ SELECT COUNT(*)=6 FROM t5; > SELECT COUNT(*)=4 FROM t6; > DROP TABLE t5, t6; > > +--echo # > +--echo # MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) || > +--echo # share->last_version' failed in myisam/mi_open.c:67: test_if_reopen > +--echo # > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=MYISAM; > +INSERT INTO t7 VALUES(1); > +ALTER TABLE t7 RENAME TO t; > +SELECT * FROM t a, t b; > +DROP TABLE t; > + > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=ARIA; > +INSERT INTO t7 VALUES(1); > +ALTER TABLE t7 RENAME TO t; > +SELECT * FROM t a, t b; > +DROP TABLE t; > + > --echo # Cleanup > DROP DATABASE temp_db; > diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c > index fd21d28..5b33ad6 100644 > --- a/storage/maria/ma_extra.c > +++ b/storage/maria/ma_extra.c > @@ -399,7 +399,12 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function > function, >share->bitmap.changed_not_flushed= 0; > } > /* last_version must be protected by intern_lock; See collect_tables() */ > -share->last_version= 0L; /* Impossible version */ > +/* > + Temporary table has already been added to maria_open_list, so > + lets not reset the last_version. > +*/ > +if (!share->temporary) > + share->last_version= 0L; /* Impossible version */ > mysql_mutex_unlock(>intern_lock); > break; >} > diff --git a/storage/myisam/mi_extra.c b/storage/myisam/mi_extra.c > index a47c198..bc5705a 100644 > ---
Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
Hi Sergei,
On Wed, Jun 01, 2016 at 08:39:36AM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Jun 01, Sergey Vojtovich wrote:
> > On Tue, May 31, 2016 at 06:48:31PM +0200, Sergei Golubchik wrote:
> > >
> > > I think it's fine. I had a few questions though, see below:
> > Thanks for positive feedback. As I mentioned before, I'm less
> > optimistic about this patch due to many behavior changes. I can try to
> > make a list if you like.
>
> Yes, please!
- InnoDB lock wait timeouts are now honored which are much shorter by default
than server lock wait timeouts (1 year vs 50 seconds)
- with @@autocommit= 1 LOCK TABLES disables autocommit implicitely, though
user still sees @@autocommt= 1
- the above starts implicit transaction
- transactions started by LOCK TABLES are now rolled back on disconnect
(previously everything was committed due to autocommit)
- transactions started by LOCK TABLES are now rolled back by ROLLBACK
(previously everything was committed due to autocommit)
- it is now impossible to change BINLOG_FORMAT under LOCK TABLES (at least
to statement) due to running transaction
- LOCK TABLES WRITE is additionally handled by MDL, except for HANDLER READ
(which causes deadlock)
- the above makes impossible to break HANDLER READ lock
- ...in contrast LOCK TABLES READ protection against DML is pure InnoDB
- combining transactional and non-transactional tables under LOCK TABLES
may cause rolled back changes in transactional table and "committed"
changes in non-transactional table
- user may disable innodb_table_locks, which will cause LOCK TABLES to be
noop basically
There're likely more minor behavior changes that I missed.
> > > > commit 5645626
> > > > Author: Sergey Vojtovich <s...@mariadb.org>
> > > > Date: Tue May 24 12:25:56 2016 +0400
> > > >
> > > > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using
> > > > thr_lock.c locks
> > > > for InnoDB tables"
> > > >
> > > > - InnoDB now acquires shared lock for HANDLER ... READ
> > >
> > > Why?
> > It is about change in ha_innobase::init_table_handle_for_HANDLER().
> > To isolate HANDLER READ from LOCK TABLES WRITE.
>
> But should it be done? SELECT works without locks, why should HANDLER be
> different?
> On the other hand, I don't have strong preference for either case.
> Do whatever you think is better.
LOCK TABLES WRITE acquires MDL_SHARED_NO_READ_WRITE
SELECT acquires MDL_SHARED read (incompatible with MDL_SHARED_NO_READ_WRITE)
HANDLER READ acquires no MDL lock
In other words SELECT is isolated from LOCK TABLES WRITE on MDL level, while
HANDLER READ was isolated on THR_LOCK level. Now HANDLER READ is isolated on
InnoDB level.
> > > > diff --git a/mysql-test/t/innodb_mysql_lock.test
> > > > b/mysql-test/t/innodb_mysql_lock.test
> > > > index cb57c09..85ba418 100644
> > > > --- a/mysql-test/t/innodb_mysql_lock.test
> > > > +++ b/mysql-test/t/innodb_mysql_lock.test
> > > > @@ -150,14 +150,16 @@ let $wait_condition=
> > > > --source include/wait_condition.inc
> > > > LOCK TABLES t1 READ;
> > > > SELECT release_lock('bug42147_lock');
> > > > +let $wait_condition=
> > > > + SELECT COUNT(*) > 0 FROM information_schema.processlist
> > > > + WHERE state = 'executing'
> > > > + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)';
> > > > +--source include/wait_condition.inc
> > > > +UNLOCK TABLES;
> > >
> > > I don't understand the original test case. But after your changes it
> > > actually makes sense :)
> > Previously "INSERT INTO t1 SELECT get_lock('bug42147_lock', 60)"
> > was able to complete while concurrent thread was holding "LOCK TABLES t1
> > READ".
>
> Exactly! This didn't make much sense, it actually looked like a bug.
>
> > Now it's blocked and I had to move "UNLOCK TABLES" before reap.
>
> Which is good.
I supposed there's a bug in THR_LOCK still.
>
> > > > diff --git a/mysql-test/suite/handler/handler.inc
> > > > b/mysql-test/suite/handler/handler.inc
> > > > index b1e881f..8cad6a5 100644
> > > > --- a/mysql-test/suite/handler/handler.inc
> > > > +++ b/mysql-test/suite/handler/handler.inc
> > > > @@ -1091,6 +1091,12 @@ connection default;
> > > > --reap
> > > > drop table t2;
> > > >
> > > > +# This test expects "handler t1 read a next" to get blocked on table
> > > > level
> >
Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
Hi Sergei,
On Tue, May 31, 2016 at 06:48:31PM +0200, Sergei Golubchik wrote:
> Hi, Sergey,
>
> I think it's fine. I had a few questions though, see below:
Thanks for positive feedback. As I mentioned before, I'm less optimistic about
this patch due to many behavior changes. I can try to make a list if you like.
>
> > commit 47aa5f9
> > Author: Sergey Vojtovich <s...@mariadb.org>
> > Date: Fri May 6 13:44:07 2016 +0400
> >
> > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c
> > locks
> > for InnoDB tables"
> >
> > Don't use thr_lock.c locks for InnoDB tables.
> >
> > Let HANDLER READ call external_lock() even if SE is not going to be
> > locked by
> > THR_LOCK. This fixes at least main.implicit_commit failure.
> >
> > Removed tests for BUG#45143 and BUG#55930 which cover InnoDB +
> > THR_LOCK. To
> > operate properly these tests require code flow to go through THR_LOCK
> > debug
> > sync points, which is not the case after this patch. These tests are
> > removed
> > by WL#6671 as well. An alternative is to port them to different storage
> > engine.
> >
> > For the very same reason partition_debug_sync test was adjusted to use
> > MyISAM.
> >
> > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc
> > index e8ade81..76107ae 100644
> > --- a/sql/sql_handler.cc
> > +++ b/sql/sql_handler.cc
> > @@ -752,11 +752,12 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables,
> >tables->table= table; // This is used by
> > fix_fields
> >table->pos_in_table_list= tables;
> >
> > - if (handler->lock->lock_count > 0)
> > + if (handler->lock->table_count > 0)
> >{
> > int lock_error;
> >
> > -handler->lock->locks[0]->type= handler->lock->locks[0]->org_type;
> > +if (handler->lock->lock_count > 0)
> > + handler->lock->locks[0]->type= handler->lock->locks[0]->org_type;
>
> I don't understand this code in mysql_ha_read() at all :(
> even before your changes
I guess you're referring to org_type. I'm not sure I understand it either:
looks like it is needed to let HANDLER READ use the same lock type as it got
during HANDLER OPEN. I have no idea why is it needed.
This particular change was needed so that mysql_lock_tables()/external_lock()
is called even if we got 0 from lock_count().
>
> > /* save open_tables state */
> > TABLE* backup_open_tables= thd->open_tables;
> > diff --git a/storage/xtradb/handler/ha_innodb.h
> > b/storage/xtradb/handler/ha_innodb.h
> > index 2027a59..efb8120 100644
> > --- a/storage/xtradb/handler/ha_innodb.h
> > +++ b/storage/xtradb/handler/ha_innodb.h
> > @@ -218,6 +218,7 @@ class ha_innobase: public handler
> > bool can_switch_engines();
> > uint referenced_by_foreign_key();
> > void free_foreign_key_create_info(char* str);
> > + uint lock_count(void) const;
> > THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to,
> > enum thr_lock_type lock_type);
> > void init_table_handle_for_HANDLER();
> >
> > commit 5645626
> > Author: Sergey Vojtovich <s...@mariadb.org>
> > Date: Tue May 24 12:25:56 2016 +0400
> >
> > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c
> > locks
> > for InnoDB tables"
> >
> > - InnoDB now acquires shared lock for HANDLER ... READ
>
> Why?
It is about change in ha_innobase::init_table_handle_for_HANDLER().
To isolate HANDLER READ from LOCK TABLES WRITE.
It is InnoDB-side implementation of MDEV-7895.
>
> > - LOCK TABLES now disables autocommit implicitely
> > - UNLOCK TABLES now re-enables autocommit implicitely if it was
> > disabled by
> > LOCK TABLES
> > - adjusted test cases to this new behavior
> >
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 3091bd6..7d39484 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -2824,6 +2824,8 @@ Locked_tables_list::unlock_locked_tables(THD *thd)
> > request for metadata locks and TABLE_LIST elements.
> >*/
> >reset();
> > + if (thd->variables.option_bits & OPTION_AUTOCOMMIT)
> > +thd->variables.option_bits&= ~(OPTION_NOT_AUTOCOMMIT);
>
> 1. Was it possible - before your change - for OPTION_AUTOCOMMIT and
> OPTION_NOT_A
Re: [Maria-developers] [Commits] 53696a6: MDEV-10004: Galera's pc.recovery process fails in 10.1 with systemd
Hi Nirbhay, Looks good to push. On Thu, May 26, 2016 at 11:52:04PM -0400, Nirbhay Choubey wrote: > revision-id: 53696a63a2a517e04bf27382184162da50994ecb > (mariadb-10.1.14-4-g53696a6) > parent(s): 9a1c4e900b98fdb9940aab57c895753f175c2bd8 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-05-26 23:52:04 -0400 > message: > > MDEV-10004: Galera's pc.recovery process fails in 10.1 with systemd ...skip... > +# Safety checks > +if [ -n "$log_file" -a -f "$log_file" ]; then > + [ "$euid" = "0" ] && chown $user $log_file > + chmod 600 $log_file > +else > + echo "WSREP: mktemp failed" Should this be "log" instead of "echo"? Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 454a9dc: MDEV-10004: Galera's pc.recovery process fails in 10.1 with systemd
Hi Nirbhay,
I believe this patch is acceptable. Still I didn't completely understand the
need for all this complexity and your answer to Daniel's question.
Some minor comments inline.
On Fri, May 20, 2016 at 11:04:24PM -0400, Nirbhay Choubey wrote:
> revision-id: 454a9dc53a5b02af316dd713c577082eb8cf79c4
> (mariadb-10.1.14-4-g454a9dc)
> parent(s): 9a1c4e900b98fdb9940aab57c895753f175c2bd8
> author: Nirbhay Choubey
> committer: Nirbhay Choubey
> timestamp: 2016-05-20 23:04:20 -0400
> message:
>
> MDEV-10004: Galera's pc.recovery process fails in 10.1 with systemd
>
> Galera recovery process works in two phases. In the first
> phase, mysqld is started as non-daemon with --wsrep-recover
> to recover and fetch the last logged global transaction ID.
> This ID is then used in second phase as the start position
> (--wsrep-start-position=XX) to start mysqld as daemon.
>
> As this process was implemented in mysqld_safe script, the
> recovery did not work when server was started using systemd.
>
> Fixed by introducing a shell script (wsrep_recovery.sh) that
> mimics the first phase of the recovery process.
>
> ---
> cmake/systemd.cmake | 3 +-
> scripts/galera_recovery.sh | 94
>
> support-files/mariadb.service.in | 11 -
> 3 files changed, 106 insertions(+), 2 deletions(-)
>
> diff --git a/cmake/systemd.cmake b/cmake/systemd.cmake
> index b0161cf..17f44f7 100644
> --- a/cmake/systemd.cmake
> +++ b/cmake/systemd.cmake
> @@ -55,9 +55,10 @@ MACRO(CHECK_SYSTEMD)
>IF(HAVE_SYSTEMD AND HAVE_SYSTEMD_SD_DAEMON_H AND
> HAVE_SYSTEMD_SD_LISTEN_FDS
> AND HAVE_SYSTEMD_SD_NOTIFY AND HAVE_SYSTEMD_SD_NOTIFYF)
> ADD_DEFINITIONS(-DHAVE_SYSTEMD)
> -SET(SYSTEMD_SCRIPTS mariadb-service-convert galera_new_cluster)
> +SET(SYSTEMD_SCRIPTS mariadb-service-convert galera_new_cluster
> galera_recovery)
> SET(SYSTEMD_DEB_FILES "usr/bin/mariadb-service-convert
> usr/bin/galera_new_cluster
> + usr/bin/galera_recovery
> ${INSTALL_SYSTEMD_UNITDIR}/mariadb.service
> ${INSTALL_SYSTEMD_UNITDIR}/mariadb@.service
>
> ${INSTALL_SYSTEMD_UNITDIR}/mariadb@bootstrap.service.d/use_galera_new_cluster.conf")
> diff --git a/scripts/galera_recovery.sh b/scripts/galera_recovery.sh
> new file mode 100755
> index 000..6d4f1d5
> --- /dev/null
> +++ b/scripts/galera_recovery.sh
> @@ -0,0 +1,94 @@
> +#!/bin/sh
> +
> +# Copyright (c) 2016 MariaDB Corporation
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
> +
> +
> +# This script is intended to be executed by systemd. It starts mysqld with
> +# --wsrep-recover to recover from a non-graceful shutdown, determines the
> +# last stored global transaction ID and echoes it in
> --wsrep-start-position=XX
> +# format. The output is then captured and used by systemd to start mysqld.
> +# If the server was configured to start without wsrep, nothing is echoed.
> +
> +user='@MYSQLD_USER@'
> +log_file=$(mktemp /tmp/wsrep_recovery.XX)
> +euid=$(id -u)
> +recovered_pos=""
> +skipped=""
> +start_pos=""
> +start_pos_opt=""
> +ret=0
> +
> +log ()
> +{
> + local msg="$1"
> + # Print all messages to stderr as we reserve stdout for printing
> + # --wsrep-start-position=.
> + echo "$msg" >&2
> +}
> +
> +finish()
> +{
> + rm -f "$log_file"
> +}
> +
> +trap finish EXIT
> +
> +# Safety checks
> +if [ -z "$log_file" ]; then
> + log "WSREP: mktemp failed"
> + exit 1
> +fi
> +
> +if [ -f "$log_file" ]; then
> + [ "$euid" = "0" ] && chown $user $log_file
> +chmod 600 $log_file
> +else
> + log "WSREP: Failed to change the permissions of $log_file."
I'd say it's rather mktemp failed than change permissions failed.
> + exit 1
> +fi
> +
> +# Redirect server's error log to the log file.
> +eval /usr/sbin/mysqld --user=$user --wsrep_recover 2> "$log_file"
> +ret=$?
> +if [ $ret -ne 0 ]; then
> + # Something went wrong, let us also print the error log so that it
> + # shows up in systemctl status output as a hint to the user.
> + log "WSREP: Failed to start mysqld for wsrep recovery: '`cat $log_file`'"
> + exit 1
> +fi
Since this script is executed unconditionally, what shall happen if wsrep is
not
[Maria-developers] Effect of gcc optimization levels
Hi Wlad, Last week I promised to benchmark effect of various gcc optimization levels. Here're results: -O0 418TPS -Os 663TPS -O1 759TPS -O2 807TPS -O3 812TPS -O3 -unroll2 -ip 812TPS (default) -Ofast 819TPS Benchmark was done on a Sandy Bridge system running Ubuntu 12.10. Single thread OLTP RO, performance schema off. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Joinable threads
Hi Monty, On Fri, May 06, 2016 at 12:52:02PM +0300, Michael Widenius wrote: > Hi! > > On 5 May 2016 17:42, "Sergey Vojtovich" <s...@mariadb.org> wrote: > > > > Hi Monty, > > > > Thanks for you reply. Please allow me to disagree with some points. :) > > > > Attached please find 2 patches, both fix this memory leak. One approach is > > for detached threads, another one is for joinable threads. > > Sorry, I missed the part about service threads. For these I don't mind to > use joinable threads. Ok, mystery solved now! Thanks for confirming you're not against them in general. > > Just one question: > How to handle hanged threads? > With the old code we could, if this would ever be a problem, just change > cond_wait to cond_timed_wait. > Don't know how to do this with joinable threads, except using alarms which > is a bit of code (20 lines) I don't think pthread_join() is good for hanged threads. But threads should never hang either. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Joinable threads
Hi Monty,
Thanks for you reply. Please allow me to disagree with some points. :)
Attached please find 2 patches, both fix this memory leak. One approach is
for detached threads, another one is for joinable threads.
On Thu, May 05, 2016 at 02:12:17PM +0300, Michael Widenius wrote:
> Hi!
>
> On Wed, May 4, 2016 at 11:13 AM, Sergey Vojtovich <s...@mariadb.org> wrote:
> > Hi Monty,
> >
> > I vaguely recall you told me once that you like detached threads more than
> > joinable, specifically for service threads. Could you remind me what were
> > the reasons behind it?
> >
> > I need this to fix https://jira.mariadb.org/browse/MDEV-9994 properly.
>
> The main reason is just that with joinable threads you have more work
> to manage the threads as you need to remember to join them as
> otherwise you will get a memory loss.
I believe joinable.patch prooves the opposite, that is joinable threads are
easier to manage.
With joinable.patch we have 32 lines less, while with detached.patch we have 4
lines more.
>
> The only time the server has a benefit of join able threads, at least
> as far as I am aware of it, is at shutdown, which hasn't been a real
> problem as in most cases
> my_thread_end() will ensure that all threads has been calling
> my_thread_end() before the server dies.
How can my_thread_end() ensure my_thread_end() is called before server dies?
I believe detached threads are only beneficial if we don't care about clean
thread shutdown (that is main thread may exit any time independenly of other
threads state). Otherwise we have to add code synchronizing thread shutdown
like for checkpoint thread.
>
> Referring to the valgrind issue, I would just ignore the memory loss
> for tls, as this isn't a real error. We are already ignoring this on
> other platforms.
I agree it's minor. I would agree to suppress the warning if it was generated
by third party library and we could do nothing about it. But this memory leak
is a consequence of API violation on MariaDB side.
I'd suggest to remove all these suppressions. Even if we get warnings from
other threads - it's trivial to fix.
Thanks,
Sergey
diff --git a/storage/maria/ma_checkpoint.c b/storage/maria/ma_checkpoint.c
index de8a9610..2bbbfb0 100644
--- a/storage/maria/ma_checkpoint.c
+++ b/storage/maria/ma_checkpoint.c
@@ -46,7 +46,7 @@ static mysql_mutex_t LOCK_checkpoint;
static mysql_cond_t COND_checkpoint;
/** @brief control structure for checkpoint background thread */
static MA_SERVICE_THREAD_CONTROL checkpoint_control=
- {THREAD_DEAD, FALSE, _checkpoint, _checkpoint};
+ {0, FALSE, FALSE, _checkpoint, _checkpoint};
/* is ulong like pagecache->blocks_changed */
static ulong pages_to_flush_before_next_checkpoint;
static PAGECACHE_FILE *dfiles, /**< data files to flush in background */
@@ -326,7 +326,6 @@ static int really_execute_checkpoint(void)
int ma_checkpoint_init(ulong interval)
{
- pthread_t th;
int res= 0;
DBUG_ENTER("ma_checkpoint_init");
if (ma_service_thread_control_init(_control))
@@ -335,11 +334,12 @@ int ma_checkpoint_init(ulong interval)
{
compile_time_assert(sizeof(void *) >= sizeof(ulong));
if (!(res= mysql_thread_create(key_thread_checkpoint,
- , NULL, ma_checkpoint_background,
+ _control.thread, NULL,
+ ma_checkpoint_background,
(void *)interval)))
{
/* thread lives, will have to be killed */
- checkpoint_control.status= THREAD_RUNNING;
+ checkpoint_control.alive= TRUE;
}
}
DBUG_RETURN(res);
@@ -721,7 +721,6 @@ pthread_handler_t ma_checkpoint_background(void *arg)
DBUG_EXECUTE_IF("maria_checkpoint_indirect", level= CHECKPOINT_INDIRECT;);
ma_checkpoint_execute(level, FALSE);
}
- my_service_thread_signal_end(_control);
my_thread_end();
return 0;
}
diff --git a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c
index 2538dee..295487a 100644
--- a/storage/maria/ma_loghandler.c
+++ b/storage/maria/ma_loghandler.c
@@ -54,7 +54,7 @@ static mysql_mutex_t LOCK_soft_sync;
static mysql_cond_t COND_soft_sync;
/** @brief control structure for checkpoint background thread */
static MA_SERVICE_THREAD_CONTROL soft_sync_control=
- {THREAD_DEAD, FALSE, _soft_sync, _soft_sync};
+ {0, FALSE, FALSE, _soft_sync, _soft_sync};
/* transaction log file descriptor */
@@ -8819,7 +8819,6 @@ ma_soft_sync_background( void *arg
__attribute__((unused)))
if (my_service_thread_sleep(_sync_control, sleep))
break;
}
-my_service_thread_signal_end(_sync_control);
my_thread_end();
DBUG_RETURN(0);
}
@@ -8832,7 +8831,6 @@ ma_soft_sync_background( void *arg
__attribute__((unused)))
int translog_soft_sync_start(void)
{
- pthread_t th;
int res= 0;
uin
Re: [Maria-developers] 745b522: MDEV-8889 - Assertion `next_insert_id == 0' failed in handler::ha_external_lock
Hi Sergei, Thanks for you review! On Fri, Apr 22, 2016 at 06:07:19PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > I believe this fix is ok... > > On Apr 21, Sergey Vojtovich wrote: > > revision-id: 745b5226e68d376eed709d930f72c3fbc7f07d2a > > (mariadb-10.0.24-26-g745b522) > > parent(s): 072ca71d26487817d025ee97955e6360c3c5c086 > > committer: Sergey Vojtovich > > timestamp: 2016-04-21 16:51:00 +0400 > > message: > > > > MDEV-8889 - Assertion `next_insert_id == 0' failed in > > handler::ha_external_lock > > > > There was a race condition between delayed insert thread and connection > > thread > > actually performing INSERT/REPLACE DELAYED. It was triggered by concurrent > > INSERT/REPLACE DELAYED and statements that flush the same table either > > explicitely or implicitely (like FLUSH TABLE, ALTER TABLE, ...). > > > > This race condition was caused by a gap in delayed thread shutdown logic, > > which allowed concurrent connection running INSERT/REPLACE DELAYED to change > > essential data consequently leaving table in semi-consistent state. > > > > Specifically query thread could decrease "tables_in_use" reference counter > > in > > this gap, causing delayed insert thread to shutdown without releasing auto > > increment and table lock. > > > > Fixed by extending condition so that delayed insert thread won't shutdown > > until there're locked tables. > > s/until/if/ I believe "until" is fine here, because it describes things that happen inside a loop. > > > Also removed volatile qualifier from tables_in_use and stacked_inserts since > > they're supposed to be protected by mutexes. > > It looks like stacked_inserts is accessed without a mutex at the end of > Delayed_insert::handle_inserts(). That's why I was careful enough to write "supposed to be protected". :) Along with "stacked_inserts" it updates shared "rows" list without protection. To me it looks like a sure sign of another, though I didn't come up with a test case yet. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 21265d5: MDEV-9943 - TokuDB fails to compile with gcc 5.2.1
Hi Sergei,
I don't mind, but it needs to be tested. It have to wait till tomorrow.
Regards,
Sergey
On Tue, Apr 19, 2016 at 06:30:43PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Apr 19, Sergey Vojtovich wrote:
> > revision-id: 21265d58c1ea65f5c21a56cf669321f39066d7b5
> > (mariadb-5.5.48-19-g21265d5)
> > parent(s): 6c0e231c0282b43d6a46a4983f5971e960d3b8ca
> > committer: Sergey Vojtovich
> > timestamp: 2016-04-19 16:16:13 +0400
> > message:
> >
> > MDEV-9943 - TokuDB fails to compile with gcc 5.2.1
> >
> > For some reason check_cxx_compiler_flag() passes result variable name down
> > to
> > compiler:
> > https://github.com/Kitware/CMake/blob/master/Modules/CheckCXXSourceCompiles.cmake#L57
> >
> > But compiler doesn't permit dashes in macro name, like in
> > -DHAVE_CXX_-fimplicit-templates.
> >
> > Workarounded by renaming HAVE_CXX_-fimplicit-templates to
> > HAVE_CXX_IMPLICIT_TEMPLAES.
> >
> > diff --git a/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> > b/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> > index 99629e4..4ae7b63 100644
> > --- a/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> > +++ b/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> > @@ -103,8 +103,8 @@ set_cflags_if_supported(
> >
> > if (CMAKE_CXX_FLAGS MATCHES -fno-implicit-templates)
> ># must append this because mysql sets -fno-implicit-templates and we
> > need to override it
> > - check_cxx_compiler_flag(-fimplicit-templates
> > HAVE_CXX_-fimplicit-templates)
> > - if (HAVE_CXX_-fimplicit-templates)
> > + check_cxx_compiler_flag(-fimplicit-templates HAVE_CXX_IMPLICIT_TEMPLATES)
> > + if (HAVE_CXX_IMPLICIT_TEMPLATES)
> > set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fimplicit-templates")
> >endif ()
> > endif()
>
> I'd rather fix other similar places too. If you agree, ok to push with
> the patch below.
>
> --- a/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> +++ b/storage/tokudb/ft-index/cmake_modules/TokuSetupCompiler.cmake
> @@ -61,12 +61,13 @@ endmacro(set_cflags_if_supported_named)
> ## adds a compiler flag if the compiler supports it
> macro(set_cflags_if_supported)
>foreach(flag ${ARGN})
> -check_c_compiler_flag(${flag} HAVE_C_${flag})
> -if (HAVE_C_${flag})
> +STRING(REGEX REPLACE "[-,= ]" "_" res ${flag})
> +check_c_compiler_flag(${flag} HAVE_C_${res})
> +if (HAVE_C_${res})
>set(CMAKE_C_FLAGS "${flag} ${CMAKE_C_FLAGS}")
> endif ()
> -check_cxx_compiler_flag(${flag} HAVE_CXX_${flag})
> -if (HAVE_CXX_${flag})
> +check_cxx_compiler_flag(${flag} HAVE_CXX_${res})
> +if (HAVE_CXX_${res})
>set(CMAKE_CXX_FLAGS "${flag} ${CMAKE_CXX_FLAGS}")
> endif ()
>endforeach(flag)
> @@ -75,8 +76,9 @@ endmacro(set_cflags_if_supported)
> ## adds a linker flag if the compiler supports it
> macro(set_ldflags_if_supported)
>foreach(flag ${ARGN})
> -check_cxx_compiler_flag(${flag} HAVE_${flag})
> -if (HAVE_${flag})
> +STRING(REGEX REPLACE "[-,= ]" "_" res ${flag})
> +check_cxx_compiler_flag(${flag} HAVE_${res})
> +if (HAVE_${res})
>set(CMAKE_EXE_LINKER_FLAGS "${flag} ${CMAKE_EXE_LINKER_FLAGS}")
>set(CMAKE_SHARED_LINKER_FLAGS "${flag} ${CMAKE_SHARED_LINKER_FLAGS}")
> endif ()
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MTR related community contribution
Hi! IIUC one of limitations might be caused by mtr reserving 20 TCP ports per instance, first reserved port is 16000. This leaves (65535 - 16000) / 20= 2476.75 max possible mysqld instances. Regards, Sergey On Sun, Feb 07, 2016 at 12:52:01PM +0100, Vladislav Vaintroub wrote: > > > On 2/6/2016 1:02 PM, Vicențiu Ciorbaru wrote: > >Hi Vlad, > > > >I'm looking at the community pull request > >https://github.com/grooverdan/mariadb-server/commit/a9dfee63bad7393002621f92c15f1debd535ee1a > > > >I have a question regarding your commit: > >2943d2b7e99b4221ef481890fc9a56ad569f3985 > > > >The contribution aims to remove the hard limit of 50 unique ids as it > >seems to cause problems for some use cases with a lot of proceeses. It > >does this by either leaving it at 50 or to be equal to the number of > >spawned processes. > > > >I'm thinking that even setting the limit of unique ids to num_processes > >might cause problems if there is another mtr running in parallel and both > >have > 50 processes. This is probably minor but why set this limit so low? > >Wouldn't a larger number (say 10k) be ok? Or why not generate a random > >number and lock a file with that number? (if the random number is invalid, > >generate a new one. On average it should finish quite quickly) > Hi Vicentiu, > The 50 ports range limit was there before my patch, I just moved the start > of the the range up from 250 to 300, so that the old mtr and "new"(i.e new > in 2008) mtr have non-overlapping port range- because these 2 mtrs have > different conflicting ideas of when a port is free. > > 10K or so limit should be fine. The reason why the limit is so small is > unknown to me. Perhaps back in the day 50 mysqlds on the same machine were > considered ought to be enough for anybody:) > > I'm not sure randomization needs implementing, unless a measurement would > show that it takes noticeable amount of time during startup. Randomization > could be a minor annoyance to some people if they are used to have > particular port numbers when mysqld is started via mtr, so if it does not > bring much , maybe better not to do it. > > > ___ > Mailing list: https://launchpad.net/~maria-developers > Post to : maria-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~maria-developers > More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] b904ca0: MDEV-9297 - mysql_install_db launches mysqld 3x
Hi Sergei,
On Tue, Dec 29, 2015 at 01:51:02PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 23, Sergey Vojtovich wrote:
> > revision-id: b904ca0c2e9e35ffecb314f1082e45bfb42038f3
> > (mariadb-10.1.8-99-gb904ca0)
> > parent(s): 29290a3d53a05bd7177bcf7de05e1c8fff6e9cea
> > committer: Sergey Vojtovich
> > timestamp: 2015-12-23 15:52:34 +0400
> > message:
> >
> > MDEV-9297 - mysql_install_db launches mysqld 3x
> >
> > Perform database installation in one mysqld run. This makes mysql_install_db
> > 3x faster.
> >
> > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh
> > index f8f36bf..9dcd23a 100644
> > --- a/scripts/mysql_install_db.sh
> > +++ b/scripts/mysql_install_db.sh
> > @@ -427,7 +427,7 @@ mysqld_install_cmd_line()
> >
> > # Create the system and help tables by passing them to "mysqld --bootstrap"
> > s_echo "Installing MariaDB/MySQL system tables in '$ldata' ..."
> > -if { echo "use mysql;"; cat "$create_system_tables"
> > "$create_system_tables2" "$fill_system_tables"; } | eval "$filter_cmd_line"
> > | mysqld_install_cmd_line > /dev/null
> > +if { echo "use mysql;"; cat "$create_system_tables"
> > "$create_system_tables2" "$fill_system_tables" "$fill_help_tables"
> > "$maria_add_gis_sp"; } | eval "$filter_cmd_line" | mysqld_install_cmd_line
> > > /dev/null
> > then
> >s_echo "OK"
> > else
> > @@ -462,26 +462,6 @@ else
> >exit 1
> > fi
> >
> > -s_echo "Filling help tables..."
> > -if { echo "use mysql;"; cat "$fill_help_tables"; } |
> > mysqld_install_cmd_line > /dev/null
> > -then
> > - s_echo "OK"
> > -else
> > - echo
> > - echo "WARNING: HELP FILES ARE NOT COMPLETELY INSTALLED!"
> > - echo "The \"HELP\" command might not work properly."
> > -fi
> > -
> > -s_echo "Creating OpenGIS required SP-s..."
> > -if { echo "use test;"; cat "$maria_add_gis_sp"; } |
> > mysqld_install_cmd_line > /dev/null
> > -then
> > - s_echo "OK"
> > -else
> > - echo
> > - echo "WARNING: OPENGIS REQUIRED SP-S WERE NOT COMPLETELY INSTALLED!"
> > - echo "GIS extentions might not work properly."
> > -fi
>
> Uhm, I don't know. I believe the point was not to treat
> fill_help_tables failure as fatal.
>
> To keep this logic you can merge maria_add_gis_sp and fill_help_tables
> in one mysqld_install_cmd_line invocation. But that's all.
>
> On the other hand, perhaps it's not important to preserve this logic?
> Then your patch is fine...
Yes, I noticed that fill_help_tables failure wasn't considered fatal same for
GIS extension. But how can they possibly fail? Wrong SQL? Then we should fix it.
Generic errors will most probably be caught earlier.
I failed to find good reason to preserve that logic.
Now why I did this... for me it usually takes 10 seconds to complete. 10 seconds
is too few to switch to something else, but too much to just waste them. Back
then I had to do it dozens of times, so I wasted a few minutes. I like wasting
minutes, but not this way.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] a1ab431: MDEV-7261 - Backport the default autosized value of
Hi Sergei, On Wed, Nov 25, 2015 at 02:11:21PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 25, Sergey Vojtovich wrote: > > On Mon, Nov 23, 2015 at 10:44:39AM +0100, Sergei Golubchik wrote: > > > On Nov 23, Sergey Vojtovich wrote: > > > > On Mon, Nov 23, 2015 at 10:00:33AM +0100, Sergei Golubchik wrote: > > > > > On Nov 23, Sergey Vojtovich wrote: > > > > > > > > > > Could there be any other fix for P_S autosizing besides moving all its > > > > > dependencies to early options? > > > > No simple solution on my mind. May be initialize PFS with defaults and > > > > then reinitialize with real values? > > > > > > perfschema creates fixed-size (*) arrays based on these values. And > > > allocates data mutexes, rwlocks, conditions there. > > > > > > if perfschema data structures are re-initialized, all mutexes/etc have > > > to be. That's why Marc has created these "early options" in the > > > first place. To reinitialize only those few mutexes that my_getopt > > > needs, not half of the server. > > > > > All I can see now is 19 PFS variables depending on table_definition_cache && > > table_open_cache && max_connections && open_files_limit values. I couldn't > > easily track down further dependencies, at least number of allocated arrays > > basing on these variables is far over 19. > > > > Could you confirm that: > > 1. we want to fix PFS autosizing along with this patch > > No. > > > 2. we want to avoid early initialization of these 4 server variables and > >instead reinitialize PFS when they're autosized > > No. > > 1. PFS autosizing can be fixed later. But assuming it will be fixed, I > wouldn't want you to do a patch now that will be completely reverted > when fixing PFS autosizing. > > 2. No, why? I'm lost now: - we don't want to fix PFS autosizing along with this patch (that is we don't want to make these 4 variables early options right now) - and we don't want this patch because it will be reverted later (though I'd say not reverted, but moved because this logic will stay) This makes me think that: - either we don't want to fix this bug until PFS autosizing is fixed first - or there's another options which I can't think of What's your thinking? Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 2b3bffb: MDEV-8491 - On shutdown, report the user and the host executed that.
Hi Sergei,
On Thu, Nov 26, 2015 at 02:55:49PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Nov 26, Sergey Vojtovich wrote:
> > > > +
> > > > + if ((user= my_strdup(user_host_buff, MYF(0))) &&
> > > > + !my_atomic_casptr((void **) _user,
> > > > +(void **) _shutdown_user, user))
> > > > +my_free(user);
> > > > +}
> > >
> > > Interesting. Why is that?
> > It supposed to be safe concurrent shutdown.
>
> Okay, but please add a comment about it.
Ok.
>
> > > > @@ -1888,7 +1912,13 @@ static void __cdecl kill_server(int sig_ptr)
> > > >if (sig != 0) // 0 is not a valid signal number
> > > > my_sigset(sig, SIG_IGN);/* purify inspected */
> > > >if (sig == MYSQL_KILL_SIGNAL || sig == 0)
> > > > -sql_print_information(ER_DEFAULT(ER_NORMAL_SHUTDOWN),my_progname);
> > > > + {
> > > > +char *user= (char *) my_atomic_loadptr((void**) _user);
> > > > +sql_print_information(ER_DEFAULT(ER_NORMAL_SHUTDOWN), my_progname,
> > > > + user ? user : "unknown");
> > >
> > > can it, really, be "unknown" here? when?
> > Yes, in a few cases. E.g. killed by signal.
>
> Oh, right, sorry. I thought that if() filters that out (because signals
> have a special message ER_DEFAULT(ER_GOT_SIGNAL) below).
>
> Any other cases? Otherwise I'd suggest
>
> - user ? user : "unknown");
> + user ? user : STRINGIFY_ARG(MYSQL_KILL_SIGNAL))
No user info in following cases:
- if listening socket is closed and SIGNALS_DONT_BREAK_READ is defined (which
is never seem to be defined), kill_server is called with MYSQL_KILL_SIGNAL
- if signal SIG{TERM|QUIT|KILL} received and USE_ONE_SIGNAL_HAND defined (is it
guaranteed to be defined properly, kill_server is called with 0
- special handling of SIGINT
- wsrep applier thread may call kill_server() without thd, but that's probably
fixable
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] ed5ec30: MDEV-7780 - Support for faking server version
Hi Sergei,
On Thu, Dec 24, 2015 at 01:24:09PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 24, Sergey Vojtovich wrote:
> > revision-id: ed5ec30f9ca448086db5acbfd5f484a534119810
> > (mariadb-10.1.8-101-ged5ec30)
> > parent(s): 1f19c6aa93de730cd10c9fcbb337f9ec5c30ef9d
> > committer: Sergey Vojtovich
> > timestamp: 2015-12-24 15:46:18 +0400
> > message:
> >
> > MDEV-7780 - Support for faking server version
> >
> > Embedded now supports "--version=", while "--version" is still
> > silently
> > ignored. Also only run protocol check in non-embedded mode.
>
> As I said on irc, I'd simply use not_embedded.inc, but ok, if you want
> it to work in embedded, sure, do it.
>
> One question below.
>
> > ---
> > mysql-test/suite/sys_vars/t/version.test | 3 ++-
> > sql/mysqld.cc| 4 ++--
> > 2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/mysql-test/suite/sys_vars/t/version.test
> > b/mysql-test/suite/sys_vars/t/version.test
> > index 7955bf8..daa9538 100644
> > --- a/mysql-test/suite/sys_vars/t/version.test
> > +++ b/mysql-test/suite/sys_vars/t/version.test
> > @@ -1,5 +1,6 @@
> > SELECT @@version;
> > perl;
> > - $cnt= grep /my_favorite_version/, `$ENV{MYSQL} -e status`;
> > + $cnt= $ENV{MYSQL_TEST} =~ /mysqltest_embedded / ? 1 :
> > +grep /my_favorite_version/, `$ENV{MYSQL} -e status`;
>
> Why wouldn't you use mysql_embedded in this case?
> Like
>
> $mysql= $ENV{MYSQL_TEST} =~ /mysqltest_embedded / ? 'MYSQL_EMBEDDED' :
> 'MYSQL';
> $cnt= grep /my_favorite_version/, `$ENV{$mysql} -e status`;
- mtr is looking for this binary in $basedir/libmysqld, while the correct path
is something like $bindir/libmysqld/examples$opt_vs_config => it's broken
- mtr doesn't add arguments to $ENV{MYSQL_EMBEDDED}, like it does for
$ENV{MYSQL} => it's dead broken
- we don't seem to install this binary => extra logic to handle installed tree
- it won't connect to original server, so it needs another version arg
- what's the point of testing protocol when there's no protocol whatsoever?
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 370ab48: MDEV-9209 - [PATCH] scripts: Do not prepend the prefix to absolute paths
Hi Sergei,
On Tue, Dec 22, 2015 at 01:27:48PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 18, Sergey Vojtovich wrote:
> > revision-id: 370ab48e9fcf4bc15ffa5e313fc34aa97981a832
> > (mariadb-10.1.9-24-g370ab48)
> > parent(s): 428e09a789a17211de9b4f0a2c4c1226d5dcf993
> > committer: Sergey Vojtovich
> > timestamp: 2015-12-18 16:50:03 +0400
> > message:
> >
> > MDEV-9209 - [PATCH] scripts: Do not prepend the prefix to absolute paths
> >
> > Allow absolute paths for INSTALL_*DIR.
>
> Okay, I agree now. Let's use your approach :)
> See below, one minor issue in mariadb.pc file.
>
> > diff --git a/scripts/CMakeLists.txt b/scripts/CMakeLists.txt
> > index eb78016..54289c0 100644
> > --- a/scripts/CMakeLists.txt
> > +++ b/scripts/CMakeLists.txt
> >
> > IF(UNIX)
> > CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/mysql_install_db.sh
> >${CMAKE_CURRENT_BINARY_DIR}/mysql_install_db ESCAPE_QUOTES @ONLY)
> > - SET(DEST ${INSTALL_SCRIPTDIR})
> >SET(EXT)
>
> you can remove EXT as well, it's apparently not used anymore.
Removed.
>
> >EXECUTE_PROCESS(
> >COMMAND chmod +x ${CMAKE_CURRENT_BINARY_DIR}/mysql_install_db
> > diff --git a/support-files/mariadb.pc.in b/support-files/mariadb.pc.in
> > index bc84a5c..0605d86 100644
> > --- a/support-files/mariadb.pc.in
> > +++ b/support-files/mariadb.pc.in
> > @@ -1,18 +1,18 @@
> > # these four variables are present in almost every .pc file
> > prefix=@CMAKE_INSTALL_PREFIX@
> > exec_prefix=${prefix}
> > -libdir=${prefix}/@INSTALL_LIBDIR@
> > -includedir=${prefix}/@INSTALL_INCLUDEDIR@
> > +libdir=@INSTALL_LIBDIRABS@
> > +includedir=@INSTALL_INCLUDEDIRABS@
>
> This is, strictly speaking, incorrect. In the old version one can change
> prefix to any other value and all other paths will auto-adjust. See:
>
> $ pkg-config --libs openrc
> -L/lib64 -lrc
> $ pkg-config --libs openrc --define-variable=prefix=/opt
> -L/opt/lib64 -lrc
Nice, I didn't know it's useful. I reverted all changes to mariadb.pc.in.
I assume this patch is ok to push now:
https://github.com/MariaDB/server/commit/d464a47d0cfaf8486444edfa78180167d9f6b73a
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 063967b: MDEV-9081 - Debian: insecure debian-sys-maint password handling
Hi Sergei, On Mon, Dec 21, 2015 at 01:45:27PM +0100, Sergei Golubchik wrote: ...skip... > > > besides, what the plan for moving to unix_socket auth? > > Strictly speaking there is no plan. If you're asking for my opinion: I like > > it. > > I'd avoid such massive changes to not very well tested scripts in GA > > versions. > > 10.2 seem to be reasonable target version. > > > > I could probably do that over the next week, or some time in February/March. > > my point was - if unix_socket comes soon, why bother fixing these > issues? and it not, then "will be solved by unix_socket" is not an > excuse we can use. I'm fine if we agree to port unix socket to 10.2 and leave previous versions unfixed. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 063967b: MDEV-9081 - Debian: insecure debian-sys-maint password handling
Hi Sergei,
On Mon, Dec 21, 2015 at 12:50:05PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Dec 21, Sergey Vojtovich wrote:
> > revision-id: 063967b1e447f0fc908a1ec0224fd7c1d268bf17
> > (mariadb-10.1.9-25-g063967b)
> > parent(s): 370ab48e9fcf4bc15ffa5e313fc34aa97981a832
> > committer: Sergey Vojtovich
> > timestamp: 2015-12-21 14:40:41 +0400
> > message:
> >
> > MDEV-9081 - Debian: insecure debian-sys-maint password handling
> >
> > Set file permission before password is actually written. This is a quick fix
> > to close security gap. To be replaced by MDEV-8375 - passwordless root
> > login.
> >
> > diff --git a/debian/mariadb-server-10.1.postinst
> > b/debian/mariadb-server-10.1.postinst
> > index 0f35802..069c25e 100644
> > --- a/debian/mariadb-server-10.1.postinst
> > +++ b/debian/mariadb-server-10.1.postinst
> > @@ -158,6 +158,8 @@ EOF
> > pass=`perl -e 'print
> > map{("a".."z","A".."Z",0..9)[int(rand(62))]}(1..16)'`;
> > if [ ! -d "$mysql_cfgdir" ]; then install -o 0 -g 0 -m 0755 -d
> > $mysql_cfgdir; fi
> > cat /dev/null > $dc
> > +chown 0:0 $dc
>
> is that needed?
> (not that it hurts, but still...)
Not needed because this script is always executed by root? Not sure, I just
copied this from a few lines below.
>
> > +chmod 0600 $dc
>
> ok. what about other issues you've mentioned in this MDEV-9081?
echo, while it has bad reputation seem to be more or less alright. At least I
couldn't intercept it.
REPLACE is not solved, but will be solved by unix_socket auth.
> besides, what the plan for moving to unix_socket auth?
Strictly speaking there is no plan. If you're asking for my opinion: I like it.
I'd avoid such massive changes to not very well tested scripts in GA versions.
10.2 seem to be reasonable target version.
I could probably do that over the next week, or some time in February/March.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 732d45e: MDEV-8378 - Debian: the Lintian complains about many "shlib-calls-exit" in many
Hi Sergei, On Tue, Dec 15, 2015 at 10:49:37PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Dec 14, Sergey Vojtovich wrote: > > revision-id: 732d45e93b81a104e3f3931e4908e22167a54622 > > (mariadb-10.0.22-57-g732d45e) > > parent(s): 3e206a518dec400e084451165f633b78eb2e7fee > > committer: Sergey Vojtovich > > timestamp: 2015-12-14 15:27:09 +0400 > > message: > > > > MDEV-8378 - Debian: the Lintian complains about many "shlib-calls-exit" in > > many > > of the plugins > > > > Fixed exit/_exit calls from libdaemon_example.so, libmysqlclient.so, > > libmysqlclient_r.so, libmysqld.so. > > > > Note: this is just rough prototype, not to be pushed. libmysqld.so still has > > a bunch of exit/_exit calls from InnoDB. ...skip... > Are you sure we want to fix that at all? Definitely daemon_example. The rest is nice to have (e.g. I wouldn't want my app to exit if embedded gets --print-defaults). But the fix should be different: change of abort to exit makes little sense. Either we should pass through error code, or disable code which is never executed by embedded/libmysqlclient with ifdefs. > And if yes - is it something we need to bother fixing now? The right fix as described above may become complex, so I'd rather postpone it for 10.2. But Debian is suffering from this now with 10.0. I don't know. > How comes it's "critical"? I think the reason behind "critical" is our willing to please Debian. If you don't mind I'll push daemon_example part of this fix and remove my_abort_hook: one line less in lintian report is a progress still. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 9d8ad0d: Add --persistent option for mysqlcheck
Hi Vicentu,
this is contribution by Daniel. He should get some credit for this. Assuming you
didn't change this code much, this should be committed on Daniel's behalf, so
that he gets credit on github. Or at the very least mention in a comment
contribution author.
Thanks,
Sergey
On Tue, Dec 15, 2015 at 11:11:50AM +0200, Vicentiu Ciorbaru wrote:
> revision-id: 9d8ad0d79e3c3a1a9bf7c8cc6477ee4e69fba633
> (mariadb-10.0.22-63-g9d8ad0d)
> parent(s): e9b4a041af5122dffd59012493c565e6e3db2664
> committer: Vicențiu Ciorbaru
> timestamp: 2015-12-15 11:11:28 +0200
> message:
>
> Add --persistent option for mysqlcheck
>
> 10.0 has an "analyze table .. persistent for all" syntax. This adds
> --persistent to mysqlcheck(mysqlanalyize) to perform this extended
> analyze table option.
>
> ---
> client/mysqlcheck.c | 6 +-
> man/mysqlcheck.1| 18 +-
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c
> index ead4127..b9e5478 100644
> --- a/client/mysqlcheck.c
> +++ b/client/mysqlcheck.c
> @@ -43,7 +43,7 @@ static my_bool opt_alldbs = 0, opt_check_only_changed = 0,
> opt_extended = 0,
> opt_silent = 0, opt_auto_repair = 0, ignore_errors = 0,
> tty_password= 0, opt_frm= 0, debug_info_flag= 0,
> debug_check_flag= 0,
> opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0,
> - opt_do_tables= 1;
> + opt_persistent_all= 0, opt_do_tables= 1;
> static my_bool opt_write_binlog= 1, opt_flush_tables= 0;
> static uint verbose = 0, opt_mysql_port=0;
> static int my_end_arg;
> @@ -160,6 +160,9 @@ static struct my_option my_long_options[] =
>{"password", 'p',
> "Password to use when connecting to server. If password is not given,
> it's solicited on the tty.",
> 0, 0, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},
> + {"persistent", 'Z', "When using ANALYZE TABLE use the PERSISTENT FOR ALL
> option.",
> + _persistent_all, _persistent_all, 0, GET_BOOL, NO_ARG, 0, 0,
> + 0, 0, 0, 0},
> #ifdef __WIN__
>{"pipe", 'W', "Use named pipes to connect to server.", 0, 0, 0, GET_NO_ARG,
> NO_ARG, 0, 0, 0, 0, 0, 0},
> @@ -909,6 +912,7 @@ static int handle_request_for_tables(char *tables, size_t
> length, my_bool view)
>case DO_ANALYZE:
> DBUG_ASSERT(!view);
> op= (opt_write_binlog) ? "ANALYZE" : "ANALYZE NO_WRITE_TO_BINLOG";
> +if (opt_persistent_all) end = strmov(end, " PERSISTENT FOR ALL");
> break;
>case DO_OPTIMIZE:
> DBUG_ASSERT(!view);
> diff --git a/man/mysqlcheck.1 b/man/mysqlcheck.1
> index c175483..e05bbf8 100644
> --- a/man/mysqlcheck.1
> +++ b/man/mysqlcheck.1
> @@ -1,6 +1,6 @@
> '\" t
> .\"
> -.TH "\FBMYSQLCHECK\FR" "1" "04/08/2015" "MariaDB 10\&.0" "MariaDB Database
> System"
> +.TH "\FBMYSQLCHECK\FR" "1" "27/12/2015" "MariaDB 10\&.0" "MariaDB Database
> System"
> .\" -
> .\" * set default formatting
> .\" -
> @@ -677,6 +677,22 @@ Specifying a password on the command line should be
> considered insecure\&. You c
> .sp -1
> .IP \(bu 2.3
> .\}
> +.\" mysqlcheck: persisent option
> +.\" persistent option: mysql
> +\fB\-\-persistent\fR,
> +\fB\-Z\fR
> +.sp
> +Used with ANALYZE TABLE to append the option PERSISENT FOR ALL.
> +.RE
> +.sp
> +.RS 4
> +.ie n \{\
> +\h'-04'\(bu\h'+03'\c
> +.\}
> +.el \{\
> +.sp -1
> +.IP \(bu 2.3
> +.\}
> .\" mysqlcheck: pipe option
> .\" pipe option: mysql
> \fB\-\-pipe\fR,
> ___
> commits mailing list
> comm...@mariadb.org
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 6c19f0b: MDEV-9172 - Analyze patches for IBM System z
Hi Sergei,
On Wed, Dec 09, 2015 at 01:17:31PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Nov 27, Sergey Vojtovich wrote:
> > revision-id: 6c19f0b0eb0600c276f8681673dc768d542d3bd3
> > (mariadb-10.1.9-8-g6c19f0b)
> > parent(s): 3c0e9d31b3e6494931deb09f5c93b05a96df8471
> > committer: Sergey Vojtovich
> > timestamp: 2015-11-27 13:58:30 +0400
> > message:
> >
> > MDEV-9172 - Analyze patches for IBM System z
> >
> > Extended my_timer_cycles() to support s390.
> > Some compiler tunings for RHEL/SLES RPM packages on s390.
> >
> > diff --git a/cmake/build_configurations/mysql_release.cmake
> > b/cmake/build_configurations/mysql_release.cmake
> > index 8c79ec2..75ce57e 100644
> > --- a/cmake/build_configurations/mysql_release.cmake
> > +++ b/cmake/build_configurations/mysql_release.cmake
> > @@ -154,6 +154,25 @@ IF(UNIX)
> > SET(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O3 ${COMMON_CXX_FLAGS}")
> >ENDIF()
> >
> > + # IBM Z flags
> > + IF(CMAKE_SYSTEM_PROCESSOR MATCHES "s390x")
> > +IF(RPM MATCHES "(rhel|centos)6" OR RPM MATCHES "(suse|sles)11")
> > + SET(z_flags "-funroll-loops -march=z9-109 -mtune=z10 ")
> > +ELSEIF(RPM MATCHES "(rhel|centos)7" OR RPM MATCHES "(suse|sles)12")
> > + SET(z_flags "-funroll-loops -march=z196 -mtune=zEC12 ")
> > +ELSE()
> > + SET(z_flags "")
> > +ENDIF()
>
> Does MariaDB compile and work without these flags?
Yes, it is performance optimization that I was talking about in a comment
to JIRA task.
>
> > diff --git a/mysys/my_rdtsc.c b/mysys/my_rdtsc.c
> > index 82e5609..4228973 100644
> > --- a/mysys/my_rdtsc.c
> > +++ b/mysys/my_rdtsc.c
> > @@ -249,6 +249,13 @@ ulonglong my_timer_cycles(void)
> > clock_gettime(CLOCK_SGI_CYCLE, );
> > return (ulonglong) tp.tv_sec * 10 + (ulonglong) tp.tv_nsec;
> >}
> > +#elif defined(__GNUC__) && defined(__s390__)
> > + /* covers both s390 and s390x */
> > + {
> > +ulonglong result;
> > +__asm__ __volatile__ ("stck %0" : "=Q" (result) : : "cc");
>
> I cannot really review that :)
> If you've checked the documentation and tested it and whatever,
> then let's push it.
Manual read, tested.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] The compile errors on Linux on Power
Hi Lixun,
you likely have quite outdated compiler on that host. Anyway, there is a patch
for similar issue on IBM Z, which should solve this problem:
http://lists.askmonty.org/pipermail/commits/2015-November/008648.html
Regards,
Segrey
On Tue, Dec 01, 2015 at 09:14:55PM +0800, Lixun Peng wrote:
> Hi everyone,
>
>
> Recently I want to test MariaDB 10.1.9 on Linux on Power8.
>
> *But GCC will report following errors:*
>
> [ 52%] Building CXX object
> > storage/innobase/CMakeFiles/innobase.dir/api/api0api.cc.o
> > In file included from
> > /root/mariadb-10.1.9/storage/innobase/include/ut0ut.h:36,
> > from
> > /root/mariadb-10.1.9/storage/innobase/include/univ.i:636,
> > from
> > /root/mariadb-10.1.9/storage/innobase/api/api0api.cc:27:
> > /root/mariadb-10.1.9/storage/innobase/include/os0sync.h:521:4: error:
> > #error "Unsupported platform"
> > [ 52%] Building CXX object
> > storage/archive/CMakeFiles/archive.dir/ha_archive.cc.o
> > In file included from
> > /root/mariadb-10.1.9/storage/innobase/include/sync0sync.h:837,
> > from
> > /root/mariadb-10.1.9/storage/innobase/include/mem0mem.h:34,
> > from
> > /root/mariadb-10.1.9/storage/innobase/include/data0data.h:33,
> > from
> > /root/mariadb-10.1.9/storage/innobase/include/que0que.h:30,
> > from
> > /root/mariadb-10.1.9/storage/innobase/include/api0misc.h:32,
> > from
> > /root/mariadb-10.1.9/storage/innobase/api/api0api.cc:38:
> > /root/mariadb-10.1.9/storage/innobase/include/sync0sync.ic: In function
> > ‘lock_word_t ib_mutex_test_and_set(ib_mutex_t*)’:
> > /root/mariadb-10.1.9/storage/innobase/include/sync0sync.ic:85: error:
> > ‘os_atomic_test_and_set’ was not declared in this scope
> > /root/mariadb-10.1.9/storage/innobase/include/sync0sync.ic: In function
> > ‘void mutex_reset_lock_word(ib_mutex_t*)’:
> > /root/mariadb-10.1.9/storage/innobase/include/sync0sync.ic:113: error:
> > ‘os_atomic_clear’ was not declared in this scope
> > make[2]: *** [storage/innobase/CMakeFiles/innobase.dir/api/api0api.cc.o]
> > Error 1
> > make[1]: *** [storage/innobase/CMakeFiles/innobase.dir/all] Error 2
> > make[1]: *** Waiting for unfinished jobs
>
>
>
> I found the problem is:
>
> *storage/innobase/include/sync0sync.ic*
>
> > ib_mutex_test_and_set(
> > /*==*/
> > ib_mutex_t* mutex) /*!< in: mutex */
> > {
> > #if defined(HAVE_ATOMIC_BUILTINS)
> > return(os_atomic_test_and_set(>lock_word));
> > #else
>
>
> *storage/innobase/include/os0sync.h*
>
> > #if defined(__powerpc__) || defined(__aarch64__)
> > /*
> > os_atomic_test_and_set_byte_release() should imply a release barrier
> > before
> > setting, and a full barrier after. But __sync_lock_test_and_set() is only
> > documented as an aquire barrier. So on PowerPC we need to add the full
> > barrier explicitly. */
> > # define os_atomic_test_and_set_byte_release(ptr, new_val) \
> > do { __sync_lock_release(ptr); \
> > __sync_synchronize(); } while (0)
> > #else
>
>
> Only HAVE_IB_GCC_ATOMIC_TEST_AND_SET or IB_STRONG_MEMORY_MODEL will enable
> codes about os_atomic_test_and_set() and os_atomic_clear().
>
> But on my Power8 environment, HAVE_ATOMIC_BUILTINS is defined,
> HAVE_IB_GCC_ATOMIC_TEST_AND_SET and IB_STRONG_MEMORY_MODEL are not defined.
>
> So in sync0sync.ic, os_atomic_test_and_set() is used, but in os0sync.h,
> os_atomic_test_and_set() is not defined.
>
>
> Does anybody know how to solve this problem?
> I'm not familiar with Power.
>
>
> Thanks,
> Lixun
>
> --
> Staff Database Engineer @ Alibaba Cloud Computing
> Oracle ACE for MySQL
> Phone: +86 18658156856 (Hangzhou)
> Blog: http://www.penglixun.com
> ___
> Mailing list: https://launchpad.net/~maria-developers
> Post to : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help : https://help.launchpad.net/ListHelp
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 2b3bffb: MDEV-8491 - On shutdown, report the user and the host executed that.
Hi Sergei,
On Thu, Nov 26, 2015 at 10:09:19AM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Nov 25, Sergey Vojtovich wrote:
> > revision-id: 2b3bffb42cc4adbec2d91c1b9c4028374b63a51a
> > (mariadb-10.1.8-75-g2b3bffb)
> > parent(s): 6019fee7d84e8ec7d64337ad080a04f9c106bb68
> > committer: Sergey Vojtovich
> > timestamp: 2015-11-25 18:12:19 +0400
> > message:
> >
> > MDEV-8491 - On shutdown, report the user and the host executed that.
>
> First, a test would be nice. I suppose you can add
> it to the main.shutdown test.
Ok.
>
> > diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> > index 0502841..0694cb0 100644
> > --- a/sql/mysqld.cc
> > +++ b/sql/mysqld.cc
> > @@ -1806,10 +1806,34 @@ static void close_server_sock()
> > #endif /*EMBEDDED_LIBRARY*/
> >
> >
> > -void kill_mysql(void)
> > +static volatile char *shutdown_user;
> > +static void set_shutdown_user(THD *thd)
> > +{
> > + char user_host_buff[MAX_USER_HOST_SIZE + 1];
> > + char *user, *expected_shutdown_user= 0;
> > + Security_context *sctx= thd->security_ctx;
> > +
> > + strxnmov(user_host_buff, MAX_USER_HOST_SIZE,
> > + sctx->priv_user ? sctx->priv_user : "", "[",
> > + sctx->user ? sctx->user : (thd->slave_thread ? "SQL_SLAVE" :
> > ""),
> > + "] @ ",
> > + sctx->host ? sctx->host : "", " [",
> > + sctx->ip ? sctx->ip : "", "]", NullS);
>
> this will be the fifth time this expression shows up in the source code.
> turn it into a function, perhaps? Like, an inline method of THD?
>
> also, sometimes it's "SQL_SLAVE", sometimes it's "", sometimes it's
> "unauthenticated", etc. I hope these differences can be removed and
> there will be one method that works identically for all occasions.
>
> Btw, you can use safe_str() helper here.
Ok.
>
> > +
> > + if ((user= my_strdup(user_host_buff, MYF(0))) &&
> > + !my_atomic_casptr((void **) _user,
> > +(void **) _shutdown_user, user))
> > +my_free(user);
> > +}
>
> Interesting. Why is that?
It supposed to be safe concurrent shutdown.
>
> > +
> > +
> > +void kill_mysql(THD *thd)
> > {
> >DBUG_ENTER("kill_mysql");
> >
> > + if (thd)
> > +set_shutdown_user(thd);
> > +
> > #if defined(SIGNALS_DONT_BREAK_READ) && !defined(EMBEDDED_LIBRARY)
> >abort_loop=1;// Break connection
> > loops
> >close_server_sock(); // Force accept to wake
> > up
> > @@ -1888,7 +1912,13 @@ static void __cdecl kill_server(int sig_ptr)
> >if (sig != 0) // 0 is not a valid signal number
> > my_sigset(sig, SIG_IGN);/* purify inspected */
> >if (sig == MYSQL_KILL_SIGNAL || sig == 0)
> > -sql_print_information(ER_DEFAULT(ER_NORMAL_SHUTDOWN),my_progname);
> > + {
> > +char *user= (char *) my_atomic_loadptr((void**) _user);
> > +sql_print_information(ER_DEFAULT(ER_NORMAL_SHUTDOWN), my_progname,
> > + user ? user : "unknown");
>
> can it, really, be "unknown" here? when?
Yes, in a few cases. E.g. killed by signal.
>
> > +if (user)
> > + my_free(user);
> > + }
> >else
> > sql_print_error(ER_DEFAULT(ER_GOT_SIGNAL),my_progname,sig); /*
> > purecov: inspected */
> >
> > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> > index 59908dc..f65d466 100644
> > --- a/sql/share/errmsg-utf8.txt
> > +++ b/sql/share/errmsg-utf8.txt
> > @@ -1735,29 +1735,29 @@ ER_WRONG_AUTO_KEY 42000 S1009
> > ER_UNUSED_9
> > eng "You should never see it"
> > ER_NORMAL_SHUTDOWN
> > -cze "%s: normální ukončení\n"
> > -dan "%s: Normal nedlukning\n"
> > -nla "%s: Normaal afgesloten \n"
> > -eng "%s: Normal shutdown\n"
> > -est "%s: MariaDB lõpetas\n"
> > -fre "%s: Arrêt normal du serveur\n"
> > -ger "%s: Normal heruntergefahren\n"
> > -greek "%s: Φυσιολογική διαδικασία shutdown\n"
> > -hun "%s: Normal leallitas\n"
> > -ita "%s: Shutdown normale\n"
> > -jpn "%s: 通常シャットダウン\n"
> > -kor
Re: [Maria-developers] [Commits] 9f3eab9: MDEV-7780 - Support for faking server version
Hi Sergei,
we have 2 "exec perl" and many pipes. Not much, so I can change as you suggest.
Thanks,
Sergey
On Thu, Nov 26, 2015 at 10:32:52AM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Nov 26, Sergey Vojtovich wrote:
> > revision-id: 9f3eab98b93ae289c8c7fba6b4781ca632ec3c1d
> > (mariadb-10.1.8-80-g9f3eab9)
> > parent(s): 3e69db83a792e7de8a7e7dc501a75f9f22bb6196
> > committer: Sergey Vojtovich
> > timestamp: 2015-11-26 13:23:18 +0400
> > message:
> >
> > MDEV-7780 - Support for faking server version
> >
> > Added --version=str (optional argument sets server version string).
> >
> > diff --git a/mysql-test/suite/sys_vars/t/version.test
> > b/mysql-test/suite/sys_vars/t/version.test
> > new file mode 100644
> > index 000..08eae97
> > --- /dev/null
> > +++ b/mysql-test/suite/sys_vars/t/version.test
> > @@ -0,0 +1,2 @@
> > +SELECT @@version;
> > +--exec $MYSQL -e "status" | perl -ne "print \$_ if /^Server version/"
>
> Is that portable enough? Do we have other tests that do that?
> Normally for perl you should use 'perl' command of mysqltest.
> Like
>
> perl;
> print grep /Server version/, `$ENV{MYSQL} -e status`
> EOF
>
> Regards,
> Sergei
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] a1ab431: MDEV-7261 - Backport the default autosized value of
Hi Sergei, On Mon, Nov 23, 2015 at 10:44:39AM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 23, Sergey Vojtovich wrote: > > On Mon, Nov 23, 2015 at 10:00:33AM +0100, Sergei Golubchik wrote: > > > On Nov 23, Sergey Vojtovich wrote: > > > > > > Could there be any other fix for P_S autosizing besides moving all its > > > dependencies to early options? > > No simple solution on my mind. May be initialize PFS with defaults and > > then reinitialize with real values? > > perfschema creates fixed-size (*) arrays based on these values. And > allocates data mutexes, rwlocks, conditions there. > > if perfschema data structures are re-initialized, all mutexes/etc have > to be. That's why Marc has created these "early options" in the > first place. To reinitialize only those few mutexes that my_getopt > needs, not half of the server. > > Regards, > Sergei > > (*) I have a vague recollection that this could've been changed (may be > partially) in 5.7. All I can see now is 19 PFS variables depending on table_definition_cache && table_open_cache && max_connections && open_files_limit values. I couldn't easily track down further dependencies, at least number of allocated arrays basing on these variables is far over 19. Could you confirm that: 1. we want to fix PFS autosizing along with this patch 2. we want to avoid early initialization of these 4 server variables and instead reinitialize PFS when they're autosized Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 6019fee: MDEV-7780 - Support for faking server version
Hi Sergei, On Wed, Nov 25, 2015 at 02:35:59PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 25, Sergey Vojtovich wrote: > > revision-id: 6019fee7d84e8ec7d64337ad080a04f9c106bb68 > > (mariadb-10.1.8-74-g6019fee) > > parent(s): 2c29e85eafbf5bafe9c431accea9c25db7699eeb > > committer: Sergey Vojtovich > > timestamp: 2015-11-25 13:58:15 +0400 > > message: > > > > MDEV-7780 - Support for faking server version > > > > Added --version=str (optional argument sets server version string). > > > diff --git a/mysql-test/r/mysqld--help.result > > b/mysql-test/r/mysqld--help.result > > index 7a6541b..c109ad2 100644 > > --- a/mysql-test/r/mysqld--help.result > > +++ b/mysql-test/r/mysqld--help.result > > @@ -1124,7 +1124,8 @@ The following options may be given as the first > > argument: > > CLIENT_STATISTICS, INDEX_STATISTICS and TABLE_STATISTICS > > tables in the INFORMATION_SCHEMA > > -v, --verbose Used with --help option for detailed help. > > - -V, --version Output version information and exit. > > + -V, --version[=name] > > + Output version information and exit. > > This is pretty weird. That --version and --version=str have completely > different behavior. > > On the other hand, the server variable is @@version, so --version=str > is kind of expected. So --set-version=str or --fake-version=str aren't > very intuitive either. > > I don't know a good solution to this. Use --version? Use --set-version > or --fake-version? I'm not sure either, I'm just following: "We can simply make @@version writable (from the command-line, not run-time)". Originally it looked weird to me too, but then I got used to it. > > > --wait-timeout=#The number of seconds the server waits for activity > > on a > > connection before closing it > > > > diff --git a/mysql-test/suite/sys_vars/t/version.test > > b/mysql-test/suite/sys_vars/t/version.test > > new file mode 100644 > > index 000..3bc3f2b > > --- /dev/null > > +++ b/mysql-test/suite/sys_vars/t/version.test > > @@ -0,0 +1 @@ > > +SELECT @@version; > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > > would be nice to have a test of the version as sent in the protocol. > You can use, for example, mysql client to show that version. Ok. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] a1ab431: MDEV-7261 - Backport the default autosized value of
Hi Sergei, On Mon, Nov 23, 2015 at 08:39:23AM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 20, Sergey Vojtovich wrote: > > revision-id: a1ab4314d1f88fa954a774c322709822d7b95344 > > (mariadb-10.1.8-58-ga1ab431) > > parent(s): cfc135b645db5fa78b9f63b1959ea3f2be137a26 > > committer: Sergey Vojtovich > > timestamp: 2015-11-20 17:32:26 +0400 > > message: > > > > MDEV-7261 - Backport the default autosized value of > > "table_definition_cache" from MySQL 5.6.8+ > > > > Fixed table_definition_cache auto-sizing. Note that this patch doesn't > > address broken PFS auto-sizing. > > Do you plan to address it after you've broken it? How? PFS autosizing relies on table_definition_cache, table_open_cache, max_connections, open_files_limit. The only options that is autosized prior to PFS autosizing was table_defintion_cache. Even this option was autosized incorrectly. So I didn't break it, it was broken even prior to this patch. In 5.7 all these options are marked EARLY and are autosized before PFS autosizing. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 334323b: MDEV-8542 - The "aria_recover" variable should be renamed "aria_recover_options"
Hi Sergei, On Mon, Nov 23, 2015 at 09:00:45AM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 20, Sergey Vojtovich wrote: > > revision-id: 334323bb3e5ca973cf239dce21a7d5510407f230 > > (mariadb-10.1.8-59-g334323b) > > parent(s): a1ab4314d1f88fa954a774c322709822d7b95344 > > committer: Sergey Vojtovich > > timestamp: 2015-11-20 17:36:23 +0400 > > message: > > > > MDEV-8542 - The "aria_recover" variable should be renamed > > "aria_recover_options" > > to match MyISAM > > > > Added aria_recover_options, marked aria_recover as deprecated. > > > > diff --git a/mysql-test/suite/maria/maria-recover-master.opt > > b/mysql-test/suite/maria/maria-recover-master.opt > > index 7582a38..976c388 100644 > > --- a/mysql-test/suite/maria/maria-recover-master.opt > > +++ b/mysql-test/suite/maria/maria-recover-master.opt > > @@ -1 +1 @@ > > ---loose-aria-recover=backup --loose-aria-log-dir-path=$MYSQLTEST_VARDIR/tmp > > +--loose-aria-recover-options=backup > > --loose-aria-log-dir-path=$MYSQLTEST_VARDIR/tmp > > I'm not going to grep the code to see whether you've renamed all > instances of 'aria[-_]recover' - I assume you did that and run tests > too. Yes, I did it all. > > > diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc > > index 2e2aa71..064d759 100644 > > --- a/storage/maria/ha_maria.cc > > +++ b/storage/maria/ha_maria.cc > > @@ -256,6 +256,11 @@ static MYSQL_SYSVAR_ULONG(pagecache_file_hash_size, > > pagecache_file_hash_size, > > 512, 128, 16384, 1); > > > > static MYSQL_SYSVAR_SET(recover, maria_recover_options, > > PLUGIN_VAR_OPCMDARG, > > + "Deprecated and will be removed in a future release. Please use " > > + "--aria-recover-options instead.", > > + NULL, NULL, HA_RECOVER_DEFAULT, _recover_typelib); > > + > > Why? my_getopt does unambigous prefix matching, so --aria-recover will > contnue to work after the rename without any explicit deprecated > variables. So this one doesn't do any good. > > In fact, this variable has bad effects, without it 'aria-recov' (or any > other unambigous prefix of aria-recover) would continue to work, but > with this new deprecated variable 'aria-recov' will no longer be > unambigous. You're right. Two questions: - I think MySQL is getting rid of unambiguous prefix matching. I just wonder if we plan to do the same? - If some day we decide to add --aria-recover-something, won't it break --aria-recover? If the above points aren't worthy, then we definitely should just rename this option. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] a1ab431: MDEV-7261 - Backport the default autosized value of
Hi Sergei, On Mon, Nov 23, 2015 at 09:21:44AM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 23, Sergey Vojtovich wrote: > > On Mon, Nov 23, 2015 at 08:39:23AM +0100, Sergei Golubchik wrote: > > > On Nov 20, Sergey Vojtovich wrote: > > > > revision-id: a1ab4314d1f88fa954a774c322709822d7b95344 > > > > (mariadb-10.1.8-58-ga1ab431) > > > > parent(s): cfc135b645db5fa78b9f63b1959ea3f2be137a26 > > > > committer: Sergey Vojtovich > > > > timestamp: 2015-11-20 17:32:26 +0400 > > > > message: > > > > > > > > MDEV-7261 - Backport the default autosized value of > > > > "table_definition_cache" from MySQL 5.6.8+ > > > > > > > > Fixed table_definition_cache auto-sizing. Note that this patch doesn't > > > > address broken PFS auto-sizing. > > > > > > Do you plan to address it after you've broken it? How? > > PFS autosizing relies on table_definition_cache, table_open_cache, > > max_connections, open_files_limit. > > > > The only options that is autosized prior to PFS autosizing was > > table_defintion_cache. Even this option was autosized incorrectly. > > > > So I didn't break it, it was broken even prior to this patch. In 5.7 all > > these > > options are marked EARLY and are autosized before PFS autosizing. > > That's what I was getting at, yes :) > > Perhaps you fix should've been to move table_definition_cache to "early > options" instead of moving autosizing after normal options are > initialized. At least, that's the only fix that I see, because you > cannot auto-size P_S variables late. It was moved because table_definition_cache depends on table_open_cache and they further depend on max_connections and open_files_limit. So I'll have to move all of the above to "early options". It shouldn't be that complex, but it's a bit out of the scope of this particular bug. And I initially attempted to keep it simple. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] a1ab431: MDEV-7261 - Backport the default autosized value of
Hi Sergei, On Mon, Nov 23, 2015 at 10:00:33AM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 23, Sergey Vojtovich wrote: > > > > > > > > So I didn't break it, it was broken even prior to this patch. In > > > > 5.7 all these options are marked EARLY and are autosized before > > > > PFS autosizing. > > > > > > That's what I was getting at, yes :) > > > > > > Perhaps you fix should've been to move table_definition_cache to > > > "early options" instead of moving autosizing after normal options > > > are initialized. At least, that's the only fix that I see, because > > > you cannot auto-size P_S variables late. > > It was moved because table_definition_cache depends on > > table_open_cache and they further depend on max_connections and > > open_files_limit. So I'll have to move all of the above to "early > > options". > > > > It shouldn't be that complex, but it's a bit out of the scope of this > > particular bug. And I initially attempted to keep it simple. > > I'm all for keeping it simple. But it's "as simple as possible, but not > simpler than that" :) > > Could there be any other fix for P_S autosizing besides moving all its > dependencies to early options? No simple solution on my mind. May be initialize PFS with defaults and then reinitialize with real values? Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 2709165: MDEV-9128 - Compiling on IBM System Z fails
Hi Jan, yes, for some reason XtraDB doesn't have this code and compiles well on Z. Regards, Sergey On Wed, Nov 18, 2015 at 02:57:19PM +0200, Jan Lindström wrote: > Hi Sergey, > > Is this true only for innodb_plugin ? > > R: Jan > > On Wed, Nov 18, 2015 at 2:07 PM, Sergey Vojtovich <s...@mariadb.org> wrote: > > > revision-id: 2709165caed4691d1203f0b91d22d0e7927b > > (mariadb-10.1.8-67-g2709165) > > parent(s): bff6780806f4afed8faaf4c79b1593964564097e > > committer: Sergey Vojtovich > > timestamp: 2015-11-18 15:51:20 +0400 > > message: > > > > MDEV-9128 - Compiling on IBM System Z fails > > > > Provided IBM System Z have outdated compiler version, which supports gcc > > sync > > builtins but not gcc atomic builtins. It also has weak memory model. > > > > InnoDB attempted to verify if __sync_lock_test_and_set() is available by > > checking IB_STRONG_MEMORY_MODEL. This macro has nothing to do with > > availability > > of __sync_lock_test_and_set(), the right one is HAVE_ATOMIC_BUILTINS. > > > > --- > > storage/innobase/include/os0sync.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/storage/innobase/include/os0sync.h > > b/storage/innobase/include/os0sync.h > > index 8e4b4f4..e192a3a 100644 > > --- a/storage/innobase/include/os0sync.h > > +++ b/storage/innobase/include/os0sync.h > > @@ -487,7 +487,7 @@ os_atomic_clear(volatile lock_word_t* ptr) > > __atomic_clear(ptr, __ATOMIC_RELEASE); > > } > > > > -# elif defined(IB_STRONG_MEMORY_MODEL) > > +# elif defined(HAVE_ATOMIC_BUILTINS) > > > > /** Do an atomic test and set. > > @param[in,out] ptr Memory location to set to non-zero > > ___ > > commits mailing list > > comm...@mariadb.org > > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] f21705d: MDEV-7806 - thread_pool_size is not auto-sized
Hi Sergei, On Tue, Nov 17, 2015 at 03:22:23PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 17, Sergey Vojtovich wrote: > > On Tue, Nov 17, 2015 at 02:23:56PM +0100, Sergei Golubchik wrote: > > > On Nov 17, Sergey Vojtovich wrote: > > > > revision-id: f21705dfbfcd87e46937222eb860727beb21a89b > > > > (mariadb-10.1.8-66-gf21705d) > > > > parent(s): c0216f1d02e63686f986fa8f352fdf6de61249a7 > > > > committer: Sergey Vojtovich > > > > timestamp: 2015-11-17 13:13:47 +0400 > > > > message: > > > > > > > > MDEV-7806 - thread_pool_size is not auto-sized > > > > > > > > thread_pool_size is auto-sized before my_getopt(). But my_getopt starts > > > > from > > > > resetting all options to their default values. So the auto-sized value > > > > is lost. > > > > > > > > Fixed by autosizing default value directly. > > > > > > Strange solution. Why wouldn't you move autosizing after > > > config variables are read? > > > > Well, it's not a precedent: we already have similar solution for > > tc_log_size. > > > > Wouldn't autosizing after config variables are read overwrite > > configured value? If so, we'll have to create special value for > > thread_pool_size, e.g. if value is 0 then do autosizing, preserve > > original value otherwise. > > I think the preferred way is the one for tdc_size: > > ... read the options ... > if (IS_SYSVAR_AUTOSIZE(_size)) > SYSVAR_AUTOSIZE(tdc_size, MY_MIN(400 + tdc_size / 2, 2000)); > > if you'd like you can fix tc_log_size too :) Oh, you're right. I still have pre-origin image of sys_vars in my mind. Though I wonder what's the role of default value for autosized vars now? Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] f21705d: MDEV-7806 - thread_pool_size is not auto-sized
Hi Sergei, On Tue, Nov 17, 2015 at 02:23:56PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 17, Sergey Vojtovich wrote: > > revision-id: f21705dfbfcd87e46937222eb860727beb21a89b > > (mariadb-10.1.8-66-gf21705d) > > parent(s): c0216f1d02e63686f986fa8f352fdf6de61249a7 > > committer: Sergey Vojtovich > > timestamp: 2015-11-17 13:13:47 +0400 > > message: > > > > MDEV-7806 - thread_pool_size is not auto-sized > > > > thread_pool_size is auto-sized before my_getopt(). But my_getopt starts from > > resetting all options to their default values. So the auto-sized value is > > lost. > > > > Fixed by autosizing default value directly. > > > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > > index 682d803..e65a939 100644 > > --- a/sql/sys_vars.cc > > +++ b/sql/sys_vars.cc > > @@ -3270,7 +3270,7 @@ static Sys_var_uint Sys_threadpool_size( > > "This parameter is roughly equivalent to maximum number of concurrently " > > "executing threads (threads in a waiting state do not count as > > executing).", > >GLOBAL_VAR(threadpool_size), CMD_LINE(REQUIRED_ARG), > > - VALID_RANGE(1, MAX_THREAD_GROUPS), DEFAULT(8), BLOCK_SIZE(1), > > + VALID_RANGE(1, MAX_THREAD_GROUPS), DEFAULT(my_getncpus()), BLOCK_SIZE(1), > >NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_threadpool_size), > >ON_UPDATE(fix_threadpool_size) > > ); > > Strange solution. Why wouldn't you move autosizing after > config variables are read? Well, it's not a precedent: we already have similar solution for tc_log_size. Wouldn't autosizing after config variables are read overwrite configured value? If so, we'll have to create special value for thread_pool_size, e.g. if value is 0 then do autosizing, preserve original value otherwise. I wouldn't mind if the above is preferred way. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-8140 patch
Hi Kentoku, Cheng, alright, I will apply as soon as I have license clarification from Cheng: https://github.com/MariaDB/server/pull/61 Thanks, Sergey On Tue, Nov 10, 2015 at 01:06:36PM +0900, kentoku wrote: > Hi Sergey, Cheng! > > Thanks to Cheng for making the patch! > > > This patch looks good. Kentoku, will you take care of it and apply to your > > tree? > > Or should we apply it directly to 10.0? > > Please apply it directly to 10.0 this time. > > Thanks, > Kentoku > > > 2015-11-09 19:15 GMT+09:00 Sergey Vojtovich <s...@mariadb.org>: > > Hi Kentoku, Cheng! > > > > This patch looks good. Kentoku, will you take care of it and apply to your > > tree? > > Or should we apply it directly to 10.0? > > > > Thanks, > > Sergey > > > > On Sun, Nov 08, 2015 at 07:58:41PM +0800, chengxiaoz wrote: > >> Hi kentoku and Sergey, > >> > >> Please review mypatch for MDEV-8140. > >> https://github.com/MariaDB/server/pull/61 > >> > >> Thanks. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-8140 patch
Hi Kentoku, Cheng! This patch looks good. Kentoku, will you take care of it and apply to your tree? Or should we apply it directly to 10.0? Thanks, Sergey On Sun, Nov 08, 2015 at 07:58:41PM +0800, chengxiaoz wrote: > Hi kentoku and Sergey, > > Please review mypatch for MDEV-8140. https://github.com/MariaDB/server/pull/61 > > Thanks. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] dc40d73: MDEV-8437 - plugin variables conflict with bootstrap
Hi Sergei,
On Thu, Nov 05, 2015 at 03:23:30PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Nov 03, Sergey Vojtovich wrote:
> > revision-id: dc40d737d4af3cb0e272c58507ad54e4eb24be28
> > (mariadb-10.1.8-22-gdc40d73)
> > parent(s): a574407444fc3ea93ca88afa93dc18154251bf74
> > committer: Sergey Vojtovich
> > timestamp: 2015-11-03 12:08:23 +0400
> > message:
> >
> > MDEV-8437 - plugin variables conflict with bootstrap
>
> ok to push
>
> > Removed redundant attempt to create mysql.plugin table:
> > - original code was supposed to INSTALL some plugins:
> > INSERT INTO plugin VALUES ('innodb', 'ha_innodb.so'),
> > ('federated', 'ha_federated.so'), ('blackhole', 'ha_blackhole.so'),
> > ('archive', 'ha_archive.so');
>
> I didn't see any INSERT's in the postinst script.
Exactly! I guess these are leftovers from mysql-server scripts. I can see them
in mysql-server-5.1/mysql-server-5.5/mysql-server-5.6 debian packages at least.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 01bd373: MDEV-8437 - plugin variables conflict with bootstrap
Hi Sergei, On Thu, Nov 05, 2015 at 03:30:53PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Nov 03, Sergey Vojtovich wrote: > > revision-id: 01bd373f8f0cf2e1737d01a48033a7615b2a > > (mariadb-10.1.8-23-g01bd373) > > parent(s): dc40d737d4af3cb0e272c58507ad54e4eb24be28 > > committer: Sergey Vojtovich > > timestamp: 2015-11-03 14:02:49 +0400 > > message: > > > > MDEV-8437 - plugin variables conflict with bootstrap > These patches are ok, but as far as I can see they don't fix the reported > issue. This particular patch fixes the problem: this is the only invocation of mysqld --bootstrap that didn't ignore errors. The rest are surrounded with set +e/set -e. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 01bd373: MDEV-8437 - plugin variables conflict with bootstrap
Hi Sergei, On Thu, Nov 05, 2015 at 03:49:08PM +0100, Sergei Golubchik wrote: > > > On Nov 03, Sergey Vojtovich wrote: > > > > revision-id: 01bd373f8f0cf2e1737d01a48033a7615b2a > > > > (mariadb-10.1.8-23-g01bd373) > > > > parent(s): dc40d737d4af3cb0e272c58507ad54e4eb24be28 > > > > committer: Sergey Vojtovich > > > > timestamp: 2015-11-03 14:02:49 +0400 > > > > message: > > > > > > > > MDEV-8437 - plugin variables conflict with bootstrap > > > These patches are ok, but as far as I can see they don't fix the reported > > > issue. > > This particular patch fixes the problem: this is the only invocation of > > mysqld --bootstrap that didn't ignore errors. The rest are surrounded with > > set +e/set -e. > > But 'mysqld --bootstrap' was supposed to succeed and execute statements > that the script is feeding into it. If it fails and the error is > suppressed I don't think it's called "fixed". That's a fair point, but as discussed before there're two angles to view this problem: one is that --bootstrap fails to recognize plugin options and another one is that postinst does things which it is not supposed to do. I'm fixing the latter. Strictly speaking upgrade should pass with just this patch applied. But since I agree that ignoring failure of other commands is not good, I did additional patches. There were 5 --bootstrap invocations: - mysql_install_db - fixed by calling on install (and not on upgrade) - change password column - fixed by removing - change root password - not fixed (though it's not that big problem, since it is mostly relevant for install and not upgrades) - addition of debian-sys-maint - not fixed (MDEV-9081) - create plugin table - fixed by removing Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 4685c90: mari...@.service.in should be configured for all systemd platforms.
Hi Nirbhay, On Wed, Nov 04, 2015 at 09:52:15PM -0500, Nirbhay Choubey wrote: > Hi Svoj! > > On Wed, Nov 4, 2015 at 1:54 AM, Sergey Vojtovich <s...@mariadb.org> wrote: > > > Hi Nirbhay, > > > > taking into account that CPackRPM won't be able to install it, > > > I think it does not apply to all rpm platforms, but ones with cmake > affected by > http://public.kitware.com/Bug/view.php?id=14782. It applied to all rpm platforms before we get this condition a few lines below: IF(NOT CMAKE_VERSION VERSION_LESS 3.3.0 OR NOT RPM) I'd rather adjust the first "IF(NOT RPM)" accordingly, because it is a good idea to have use_galera_new_cluster.conf installed to INSTALL_SUPPORTFILESDIR. Regards, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 4685c90: mari...@.service.in should be configured for all systemd platforms.
Hi Nirbhay,
taking into account that CPackRPM won't be able to install it, what's the
purpose of having this configured for all systemd platforms?
Regards,
Sergey
On Tue, Nov 03, 2015 at 05:38:12PM -0500, Nirbhay Choubey wrote:
> revision-id: 4685c903bc822596bcf12cbfdb5bcc4460ed0105
> parent(s): 95289e5b665d6a3e37f938d0db978d0d0166e493
> committer: Nirbhay Choubey
> branch nick: 10.1
> timestamp: 2015-11-03 17:38:11 -0500
> message:
>
> mari...@.service.in should be configured for all systemd platforms.
>
> ---
> support-files/CMakeLists.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/support-files/CMakeLists.txt b/support-files/CMakeLists.txt
> index 2ab4350..2929db1 100644
> --- a/support-files/CMakeLists.txt
> +++ b/support-files/CMakeLists.txt
> @@ -79,9 +79,9 @@ IF(UNIX)
>IF(HAVE_SYSTEMD)
> CONFIGURE_FILE(mariadb.service.in
> ${CMAKE_CURRENT_BINARY_DIR}/mariadb.service @ONLY)
> +CONFIGURE_FILE(mari...@.service.in
> + ${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service @ONLY)
> IF(NOT RPM)
> - CONFIGURE_FILE(mari...@.service.in
> - ${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service @ONLY)
>INSTALL(FILES use_galera_new_cluster.conf
>${CMAKE_CURRENT_BINARY_DIR}/mariadb.service
>${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service
> ___
> commits mailing list
> comm...@mariadb.org
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 0a1b60a: MDEV-8450: PATCH] Wrong macro expansion in Query_cache::send_result_to_client()
Hi Sanja,
ok to push, a few minor notes JFYI inline. I found only one real bug, but
generally I agree that we should fix these fragile macros.
On Tue, Sep 01, 2015 at 11:47:09AM +0200, sa...@mariadb.com wrote:
> revision-id: 0a1b60a8aee4bce3235fd5a1d8cb2231b51ff5cc
> (mariadb-10.0.21-7-g0a1b60a)
> parent(s): 4b41e3c7f33714186c97a6cc2e6d3bb93b050c61
> committer: Oleksandr Byelkin
> timestamp: 2015-09-01 11:47:06 +0200
> message:
>
> MDEV-8450: PATCH] Wrong macro expansion in
> Query_cache::send_result_to_client()
>
> Expression in macro protected by ()
>
> diff --git a/include/maria.h b/include/maria.h
> index 16a9bea..cdaeade 100644
> --- a/include/maria.h
> +++ b/include/maria.h
> @@ -43,7 +43,7 @@ extern "C" {
> #define MARIA_NAME_IEXT ".MAI"
> #define MARIA_NAME_DEXT ".MAD"
> /* Max extra space to use when sorting keys */
> -#define MARIA_MAX_TEMP_LENGTH2*1024L*1024L*1024L
> +#define MARIA_MAX_TEMP_LENGTH(2*1024L*1024L*1024L)
> /* Possible values for maria_block_size (must be power of 2) */
> #define MARIA_KEY_BLOCK_LENGTH 8192/* default key block
> length */
> #define MARIA_MIN_KEY_BLOCK_LENGTH 1024/* Min key block length */
Was this used at all? May be remove it?
> diff --git a/sql/log_slow.h b/sql/log_slow.h
> index 2ae07da..3ae2060 100644
> --- a/sql/log_slow.h
> +++ b/sql/log_slow.h
> @@ -16,23 +16,23 @@
> /* Defining what to log to slow log */
>
> #define LOG_SLOW_VERBOSITY_INIT 0
> -#define LOG_SLOW_VERBOSITY_INNODB 1 << 0
> -#define LOG_SLOW_VERBOSITY_QUERY_PLAN 1 << 1
> -#define LOG_SLOW_VERBOSITY_EXPLAIN1 << 2
> +#define LOG_SLOW_VERBOSITY_INNODB (1 << 0)
Was LOG_SLOW_VERBOSITY_INNODB used at all? May be remove it?
> +#define LOG_SLOW_VERBOSITY_QUERY_PLAN (1 << 1)
> +#define LOG_SLOW_VERBOSITY_EXPLAIN(1 << 2)
>
> #define QPLAN_INITQPLAN_QC_NO
>
> -#define QPLAN_ADMIN 1 << 0
> -#define QPLAN_FILESORT1 << 1
> -#define QPLAN_FILESORT_DISK 1 << 2
> -#define QPLAN_FULL_JOIN 1 << 3
> -#define QPLAN_FULL_SCAN 1 << 4
> -#define QPLAN_QC 1 << 5
> -#define QPLAN_QC_NO 1 << 6
> -#define QPLAN_TMP_DISK1 << 7
> -#define QPLAN_TMP_TABLE 1 << 8
> -#define QPLAN_FILESORT_PRIORITY_QUEUE 1 << 9
> +#define QPLAN_ADMIN (1 << 0)
> +#define QPLAN_FILESORT(1 << 1)
> +#define QPLAN_FILESORT_DISK (1 << 2)
> +#define QPLAN_FULL_JOIN (1 << 3)
> +#define QPLAN_FULL_SCAN (1 << 4)
> +#define QPLAN_QC (1 << 5)
> +#define QPLAN_QC_NO (1 << 6)
> +#define QPLAN_TMP_DISK(1 << 7)
> +#define QPLAN_TMP_TABLE (1 << 8)
> +#define QPLAN_FILESORT_PRIORITY_QUEUE (1 << 9)
>
> /* ... */
> -#define QPLAN_MAX ((ulong) 1) << 31 /* reserved as placeholder */
> +#define QPLAN_MAX (((ulong) 1) << 31) /* reserved as placeholder
> */
>
log_slow.h was fixed in rev.203f4d419 in my 10.0. Did you use outdated tree?
I may be wrong, but there seem to be no real bug with QPLAN_QC_NO currently.
Just because nobody sets it. So in fact it clears QPLAN_ADMIN, which FWICS is
never set at this point.
But it is a good to have it fixed nevertheless.
Btw, QPLAN_QC_NO doesn't seem to be used in fact. May be remove it?
> diff --git a/storage/archive/archive_test.c b/storage/archive/archive_test.c
> index d01c1e0..bb052b8 100644
> --- a/storage/archive/archive_test.c
> +++ b/storage/archive/archive_test.c
> @@ -33,7 +33,7 @@
>
> #define ARCHIVE_ROW_HEADER_SIZE 4
>
> -#define BUFFER_LEN 1024 + ARCHIVE_ROW_HEADER_SIZE
> +#define BUFFER_LEN (1024 + ARCHIVE_ROW_HEADER_SIZE)
>
> char test_string[BUFFER_LEN];
Alright, there was a bug in this line:
assert(write_length != count * BUFFER_LEN);
> diff --git a/strings/ctype.c b/strings/ctype.c
> index 048fbe3..d8a1dd7 100644
> --- a/strings/ctype.c
> +++ b/strings/ctype.c
> @@ -264,7 +264,7 @@ static const struct my_cs_file_section_st
> }
>
> #define MY_CS_CSDESCR_SIZE 64
> -#define MY_CS_TAILORING_SIZE 32*1024
> +#define MY_CS_TAILORING_SIZE (32*1024)
> #define MY_CS_UCA_VERSION_SIZE 64
> #define MY_CS_CONTEXT_SIZE 64
Was this used at all? May be remove it?
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4728581: MDEV-7652 - More explanatory ERROR and WARNING messages when loading plugins
Hi Sergei,
thanks for your review. Answers inline.
On Tue, Jul 28, 2015 at 07:36:29PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Looks ok, just a couple of comments about the messages.
If you're agree - please, fix and push.
On Jul 28, Sergey Vojtovich wrote:
revision-id: 4728581813000522d05ab589df8e5f8cb6ad1ee6
parent(s): cf30074c3ff6815d85bbd864b28adfe7949d340c
committer: Sergey Vojtovich
branch nick: mariadb
Please, update the git post-commit hook (bzr-pull from mariadb-tools).
I've just changed it to report a branch better than just mariadb.
Thanks, that was quite annoying.
timestamp: 2015-07-28 10:18:55 +0400
message:
MDEV-7652 - More explanatory ERROR and WARNING messages when loading plugins
with plugin-load-add that are already registered at mysql.plugin
- issue just one error message, without this extra warning
- don't abuse ER_UDF_EXISTS, instead add a proper error message for plugins
- report started initialization for each plugin source
---
sql/share/errmsg-utf8.txt | 3 +++
sql/sql_plugin.cc | 15 +++
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
index 6954170..374e31f 100644
--- a/sql/share/errmsg-utf8.txt
+++ b/sql/share/errmsg-utf8.txt
@@ -7111,3 +7111,6 @@ ER_SLAVE_SKIP_NOT_IN_GTID
eng When using GTID, @@sql_slave_skip_counter can not be used.
Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a
given GTID position.
ER_TABLE_DEFINITION_TOO_BIG
eng The definition for table %`s is too big
+ER_PLUGIN_EXISTS
+eng Plugin '%-.192s' already exists
+rus Плагин '%-.192s' уже существует
May be already installed ? The command is INSTALL PLUGIN.
For UDF it was CREATE FUNCTION, that's why already exists, I suppose.
I just wonder if term installed is Ok for built-in plugins and those that
were loaded by command line.
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
index dc40870..21efeb0 100644
--- a/sql/sql_plugin.cc
+++ b/sql/sql_plugin.cc
@@ -1546,6 +1546,9 @@ int plugin_init(int *argc, char **argv, int flags)
/*
First we register builtin plugins
*/
+ if (global_system_variables.log_warnings = 9)
+sql_print_information(Initializing mandatory plugins);
+
for (builtins= mysql_mandatory_plugins; *builtins || mandatory;
builtins++)
{
if (!*builtins)
Here you should've put Initializing optional plugins.
Actually, a better message would be Initializing other built-in plugins
or Initializing remaining built-in plugins.
Sorry, I missed this. But do we need to distinguish between mandatory and
optional built-ins? May be change Initializing mandatory plugins with
Initializing built-in plugins?
@@ -1627,11 +1630,17 @@ int plugin_init(int *argc, char **argv, int flags)
{
I_List_iteratori_string iter(opt_plugin_load_list);
i_string *item;
+if (global_system_variables.log_warnings = 9)
+ sql_print_information(Initializinig plugins specified by command
line);
Initializinig plugins specified on the command line
Ok.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] mariadb 10.1.5 crash
Hi Jean-François, thanks for this information. Bug report concerning this issue: https://mariadb.atlassian.net/browse/MDEV-8339 Thanks, Sergey On Fri, Jun 19, 2015 at 12:45:17PM +0200, Jean-François Gagné wrote: Hi, I have a very similar crash while testing optimistic parallel replication (stack trace in attachment). MariaDB was running with: * binlog enabled, but log-slave-updates disabled * reduced durability (trx-commit=2 + sync_binlog=0) * slave_parallel_mode=aggressive + slave_parallel_threads=320 (yes, this number of thread can make sense, but now is not the time to extend on that) I also have crashes, but without stack trace at the moment, for: * Same as above, but with 240 threads * high durability (trx-commit=1 + sync_binlog=1) and 320 threads I will gather more information on the crashes, but it might take a few days (those are benchmarks of the same workload, so I can rerun it). When the bug is created, please provide bug number so I can add the relevant information in the ticket. Note that I had no crashes with: * high durability and 5,10,20,40,80,120,160,240 threads, with 5,10,20 ran twice * low durability and 5,10,20,40,80,120,160 threads So the bug looks like a race condition that does not happen very often. Thanks, Jean-François Gagné On 2015-06-18 18:53, Roberto Spadim wrote: Hi sergey, i'm loggins querirss now to check when the crash occur I think it's not related with create/drop, vit since i don't have the log i cant provide some kind of 100% information just the log You can create the bug eithout problemns It's mariadb 10.1.5 glibc2.14 The serve load was low i was testing only, near to 5 active connections, others was connections not running anything just connections waiting queries Em quinta-feira, 18 de junho de 2015, Sergey Vojtovich s...@mariadb.org mailto:s...@mariadb.org escreveu: Hi Roberto, according to this trace server crashed while evicting an object from table cache (happens when we run out of table_open_cache). Likely because per-share free tables list was broken. The rest is quite moot yet: e.g. it may (or may not) also involve table definition cache eviction. Will you report a bug or should I? A valuable information would be if this load involved DDL and admin statements at least. table_open_cache and table_definition_cache values, number of active connections, number of tables participating in this load. Thanks, Sergey On Wed, Jun 17, 2015 at 07:46:34PM -0300, Roberto Spadim wrote: hi guys, i was testing mariadb with test servers and i hit a crash could anyone check? server has 8GB of ram, i don't know what query is 2015-06-17 14:01:32 140497706510080 [Note] WSREP: Stop replication 2015-06-17 14:01:32 140497706510080 [Note] Event Scheduler: Purging the queue. 0 events 2015-06-17 14:01:32 140497706510080 [Note] /usr/mariadb/mariadb/bin//mysqld: Shutdown complete 150617 14:01:32 mysqld_safe mysqld from pid file /home/mysql/pid/local.pid ended 150617 14:02:38 mysqld_safe Starting mysqld daemon with databases from /home/mysql/data/local/ 2015-06-17 14:02:38 140260849645440 [Warning] 'THREAD_CONCURRENCY' is deprecated and will be removed in a future release. 2015-06-17 14:02:38 140260849645440 [Note] /usr/mariadb/mariadb/bin//mysqld (mysqld 10.1.5-MariaDB-log) starting as process 2321 ... 2015-06-17 14:02:38 140260849645440 [Note] Plugin 'InnoDB' is disabled. 2015-06-17 14:02:38 140260849645440 [Note] Plugin 'FEEDBACK' is disabled. 2015-06-17 14:02:38 140260849645440 [Note] Server socket created on IP: '::'. 2015-06-17 14:02:38 140260849645440 [Note] Event Scheduler: Loaded 0 events 2015-06-17 14:02:38 140260849645440 [Note] WSREP: Read nil XID from storage engines, skipping position init 2015-06-17 14:02:38 140260849645440 [Note] WSREP: wsrep_load(): loading provider library 'none' 2015-06-17 14:02:38 140260849645440 [Note] /usr/mariadb/mariadb/bin//mysqld: ready for connections. Version: '10.1.5-MariaDB-log' socket: '/tmp/mysql.BH.sock' port: 3307 MariaDB Server 150617 17:02:14 [ERROR] mysqld got signal 11 ; This could be because you hit a bug. It is also possible that this binary or one of the libraries it was linked against is corrupt, improperly built, or misconfigured. This error can also be caused by malfunctioning hardware. To report this bug, see http://kb.askmonty.org/en/reporting-bugs We will try our best to scrape up some info that will hopefully help diagnose the problem, but since we have already crashed, something is definitely wrong and this may fail
Re: [Maria-developers] mariadb 10.1.5 crash
Hi Roberto, according to this trace server crashed while evicting an object from table cache (happens when we run out of table_open_cache). Likely because per-share free tables list was broken. The rest is quite moot yet: e.g. it may (or may not) also involve table definition cache eviction. Will you report a bug or should I? A valuable information would be if this load involved DDL and admin statements at least. table_open_cache and table_definition_cache values, number of active connections, number of tables participating in this load. Thanks, Sergey On Wed, Jun 17, 2015 at 07:46:34PM -0300, Roberto Spadim wrote: hi guys, i was testing mariadb with test servers and i hit a crash could anyone check? server has 8GB of ram, i don't know what query is 2015-06-17 14:01:32 140497706510080 [Note] WSREP: Stop replication 2015-06-17 14:01:32 140497706510080 [Note] Event Scheduler: Purging the queue. 0 events 2015-06-17 14:01:32 140497706510080 [Note] /usr/mariadb/mariadb/bin//mysqld: Shutdown complete 150617 14:01:32 mysqld_safe mysqld from pid file /home/mysql/pid/local.pid ended 150617 14:02:38 mysqld_safe Starting mysqld daemon with databases from /home/mysql/data/local/ 2015-06-17 14:02:38 140260849645440 [Warning] 'THREAD_CONCURRENCY' is deprecated and will be removed in a future release. 2015-06-17 14:02:38 140260849645440 [Note] /usr/mariadb/mariadb/bin//mysqld (mysqld 10.1.5-MariaDB-log) starting as process 2321 ... 2015-06-17 14:02:38 140260849645440 [Note] Plugin 'InnoDB' is disabled. 2015-06-17 14:02:38 140260849645440 [Note] Plugin 'FEEDBACK' is disabled. 2015-06-17 14:02:38 140260849645440 [Note] Server socket created on IP: '::'. 2015-06-17 14:02:38 140260849645440 [Note] Event Scheduler: Loaded 0 events 2015-06-17 14:02:38 140260849645440 [Note] WSREP: Read nil XID from storage engines, skipping position init 2015-06-17 14:02:38 140260849645440 [Note] WSREP: wsrep_load(): loading provider library 'none' 2015-06-17 14:02:38 140260849645440 [Note] /usr/mariadb/mariadb/bin//mysqld: ready for connections. Version: '10.1.5-MariaDB-log' socket: '/tmp/mysql.BH.sock' port: 3307 MariaDB Server 150617 17:02:14 [ERROR] mysqld got signal 11 ; This could be because you hit a bug. It is also possible that this binary or one of the libraries it was linked against is corrupt, improperly built, or misconfigured. This error can also be caused by malfunctioning hardware. To report this bug, see http://kb.askmonty.org/en/reporting-bugs We will try our best to scrape up some info that will hopefully help diagnose the problem, but since we have already crashed, something is definitely wrong and this may fail. Server version: 10.1.5-MariaDB-log key_buffer_size=629145600 read_buffer_size=4194304 max_used_connections=23 max_threads=102 thread_count=16 It is possible that mysqld could use up to key_buffer_size + (read_buffer_size + sort_buffer_size)*max_threads = 1452041 K bytes of memory Hope that's ok; if not, decrease some variables in the equation. Thread pointer: 0x0x7f90d0353008 Attempting backtrace. You can use the following information to find out where mysqld died. If you see no messages after this, something went terribly wrong... stack_bottom = 0x7f9103539e80 thread_stack 0x48000 mysys/stacktrace.c:247(my_print_stacktrace)[0x7f9106c50b0b] sql/signal_handler.cc:153(handle_fatal_signal)[0x7f91067eadb5] /lib64/libpthread.so.0(+0x10d00)[0x7f910537dd00] sql/sql_plist.h:181(I_P_List_iterator)[0x7f910679cf03] mysys/lf_hash.c:125(lfind)[0x7f9106c5a1a0] sql/table_cache.cc:1084(tdc_iterate(THD*, char (*)(void*, void*), void*, bool))[0x7f910679e4e3] sql/table_cache.cc:267(tc_add_table(THD*, TABLE*))[0x7f910679e808] sql/sql_base.cc:2572(open_table(THD*, TABLE_LIST*, Open_table_context*))[0x7f9106633272] sql/sql_base.cc:4016(open_and_process_table)[0x7f91066370c8] sql/sql_base.cc:5184(open_and_lock_tables(THD*, DDL_options_st const, TABLE_LIST*, bool, unsigned int, Prelocking_strategy*))[0x7f9106637394] sql/sql_parse.cc:5718(execute_sqlcom_select)[0x7f910666f083] sql/sql_parse.cc:2938(mysql_execute_command(THD*))[0x7f910667a9ce] sql/sql_parse.cc:7173(mysql_parse)[0x7f910667dc3d] sql/sql_parse.cc:1470(dispatch_command(enum_server_command, THD*, char*, unsigned int))[0x7f91066802a8] sql/sql_parse.cc:1095(do_command(THD*))[0x7f910668083b] sql/sql_connect.cc:1347(do_handle_one_connection(THD*))[0x7f91067435ef] sql/sql_connect.cc:1260(handle_one_connection)[0x7f9106743747] /lib64/libpthread.so.0(+0x7654)[0x7f9105374654] /lib64/libc.so.6(clone+0x6d)[0x7f9104a987dd] Trying to get some variables. Some pointers may be invalid and cause the dump to abort. Query (0x7f90c8424020): is an invalid pointer Connection ID (thread ID): 516 Status: NOT_KILLED Optimizer switch:
Re: [Maria-developers] [Commits] 3f0b023: MDEV-7952 - clock_gettime() takes 0.24% in OLTP RO
Hi Sergei, On Thu, Jun 18, 2015 at 11:04:15AM +0200, Sergei Golubchik wrote: Hi, Sergey! On Jun 17, s...@mariadb.org wrote: revision-id: 3f0b02351a53e0fcaee8554c2c852fb3da8fe7da parent(s): 5d03dd20a4abbfd9777b619dfd51a6927323955a committer: Sergey Vojtovich branch nick: mariadb timestamp: 2015-06-17 18:55:38 +0400 message: MDEV-7952 - clock_gettime() takes 0.24% in OLTP RO ...skip... @@ -2793,9 +2793,12 @@ int fill_schema_processlist(THD* thd, TABLE_LIST* tables, COND* cond) table-field[4]-store(command_name[tmp-get_command()].str, command_name[tmp-get_command()].length, cs); /* MYSQL_TIME */ - ulonglong start_utime= tmp-start_time * HRTIME_RESOLUTION + tmp-start_time_sec_part; - ulonglong utime= start_utime start_utime unow.val - ? unow.val - start_utime : 0; + ulonglong utime= tmp-start_utime; + ulonglong utime_after_query_snapshot= tmp-utime_after_query; + if (utime utime_after_query_snapshot) +utime= utime_after_query_snapshot; // COM_SLEEP + utime= utime utime unow ? unow - utime : 0; 1. Please add an assert here that utime=unow. It uses microsecond_interval_timer(), which is supposed to be strictly increasing (unlike my_hrtime()). Somehow I thought that interval_timer() is based on per-CPU clock. But apparently I was wrong. Nevertheless nothing prevents thread from updating start_utime/utime_after_query after we saved unow. So this assertion is not very valid. 2. You might need to update mysqld_list_processes() too. Right. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 7819ddb: MDEV-8192 - THD::set_command() takes 0.05% in OLTP RO
Hi Sergei, On Thu, May 28, 2015 at 06:06:45PM +0200, Sergei Golubchik wrote: Hi, Sergey! On May 20, s...@mariadb.org wrote: revision-id: 7819ddb99be5dff26c8a2109ba70a54289a950ad parent(s): fcb48ee418959ea2c05d528266bb2dcad5bce46b committer: Sergey Vojtovich branch nick: mariadb timestamp: 2015-05-20 16:55:20 +0400 message: MDEV-8192 - THD::set_command() takes 0.05% in OLTP RO Moved THD::set_command() definition to sql_class.h so that it can be inlined. ok Call PSI_server::set_thread_command() conditionally: it is cheaper than calling dummy function when PFS is off (though it adds some overhead when PFS is on). I'm not sure about it. P_S will be used more and more over time, let's not intentionally penalize it. A few arguments: - According to perf overhead of all these *_noop() on the calle side is ~0.5%. There's also caller overhead and some inlined functions that do if (psi). IMHO that's a bit too much for disabled PFS. - This newly added overhead is nothing compared to overhead PFS has now. - When one enables profiling he should accept some added overhead. When one disables profiling he may expect no profiling overhead (ideally). But in fact I don't like this compromise either. There're two good alternatives on my mind, both are probably quite complex: - package optimized mysqld (no PFS, no profiling, etc) - optimize PFS, e.g. PSI_server::set_thread_command() duplicates work of THD::set_command() so we may try to reuse effect of the latter Just let me know if you still don't like this so that I can push at least part of this patch that we agreed upon. Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 8a595a9: cleanup: LOAD DATA replication support in IO_CACHE
Hi Sergei,
ok to push.
On Wed, May 27, 2015 at 08:14:59PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On May 27, Sergey Vojtovich wrote:
On Tue, May 26, 2015 at 03:37:36PM +0200, s...@mariadb.org wrote:
revision-id: 8a595a9825de286ad073f6f043db4c0dce88a2ea
parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d
committer: Sergei Golubchik
branch nick: maria
timestamp: 2015-05-26 15:09:25 +0200
message:
cleanup: LOAD DATA replication support in IO_CACHE
...skip...
IOCACHE::read_function may change, e.g. on error in _my_b_async_read().
Shouldn't we handle this?
async is dead code, but theoretically it might change.
how would you suggest to handle that?
I'd leave it as is until we have a test case.
Maybe add an assertion if you can foresee some practical cases when
read_function may change. Up to you.
@@ -1432,8 +1389,7 @@ READ_INFO::READ_INFO(File file_par, uint
tot_length, CHARSET_INFO *cs,
READ_INFO::~READ_INFO()
{
- if (need_end_io_cache)
-::end_io_cache(cache);
+ ::end_io_cache(cache);
my_free(buffer);
List_iteratorXML_TAG xmlit(taglist);
XML_TAG *t;
Hmm... I may miss something, but shouldn't we call log_loaded_block() for
last
successfully read block?
Yes, I do that by calling log_loaded_block() explicitly.
But now end_io_cache() only closes the IO_CACHE it does not log
anything. Which means I don't need to call end_io_cache() early. And
don't need 'need_end_io_cache' variable - I can unconditionally call
end_io_cache() at the end.
Somehow I thought that the second log_loaded_block() was inside if (error)
too. Sorry for confusion.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
Hi Alexander,
what if we do something like this:
static SEL_TREE *get_mm_tree_XXX(Item *arg1, Item *arg2)
{
return !arg1-real_item()-type() == Item::FIELD_ITEM ||
arg1-real_item()-const_item() ||
arg2-is_expensive() ? 0 :
get_full_func_mm_tree(param, this, (Item_field*) (arg1-real_item()),
arg2, false);
}
then we should be able to do tricks like this:
if (!(ftree= get_mm_tree_XXX(arguments()[0], arguments()[1])))
ftree= get_mm_tree_XXX(arguments()[1], arguments()[0]);
DBUG_RETURN(ftree);
Thanks,
Sergey
On Thu, May 28, 2015 at 08:25:57AM +0400, Alexander Barkov wrote:
Hello Sergey,
Please review the next step for MDEV-7950.
It does the following:
- Adds specific implementations of virtual method get_mm_tree() into
Item_func_like, Item_bool_rowready_func2, Item_func_spatial_rel
- Gets rid of two virtual calls: select_optimize() and have_rev_func().
- Removes virtual methods Item_func::select_optimize(),
Item_func::have_rev_func(),
as well as enum Item_func::optimize_type,
because they are not needed any more.
Thanks.
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 93e4788..57a88e3 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -4806,22 +4806,21 @@ longlong Item_func_like::val_int()
We can optimize a where if first character isn't a wildcard
*/
-Item_func::optimize_type Item_func_like::select_optimize() const
+bool Item_func_like::is_sargable_pattern() const
{
if (!args[1]-const_item() || args[1]-is_expensive())
-return OPTIMIZE_NONE;
+return false;
String* res2= args[1]-val_str((String *)cmp.value2);
if (!res2)
-return OPTIMIZE_NONE;
+return false;
if (!res2-length()) // Can optimize empty wildcard: column LIKE ''
-return OPTIMIZE_OP;
+return true;
DBUG_ASSERT(res2-ptr());
char first= res2-ptr()[0];
- return (first == wild_many || first == wild_one) ?
-OPTIMIZE_NONE : OPTIMIZE_OP;
+ return first != wild_many first != wild_one;
}
diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
index 2f26e48..7e2c5a1 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -304,10 +304,7 @@ class Item_bool_func2 :public Item_bool_func
{
return cmp.set_cmp_func(this, tmp_arg, tmp_arg+1, TRUE);
}
- optimize_type select_optimize() const { return OPTIMIZE_OP; }
virtual enum Functype rev_functype() const { return UNKNOWN_FUNC; }
- bool have_rev_func() const { return rev_functype() != UNKNOWN_FUNC; }
-
virtual inline void print(String *str, enum_query_type query_type)
{
Item_func::print_op(str, query_type);
@@ -355,6 +352,10 @@ class Item_bool_rowready_func2 :public Item_bool_func2
return add_key_fields_optimize_op(join, key_fields, and_level,
usable_tables, sargables, false);
}
+ SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr)
+ {
+return get_mm_tree_op2(param, true);
+ }
};
/**
@@ -591,7 +592,6 @@ class Item_func_ne :public Item_bool_rowready_func2
longlong val_int();
enum Functype functype() const { return NE_FUNC; }
cond_result eq_cmp_result() const { return COND_FALSE; }
- optimize_type select_optimize() const { return OPTIMIZE_KEY; }
const char *func_name() const { return ; }
Item *negated_item();
void add_key_fields(JOIN *join, KEY_FIELD **key_fields, uint *and_level,
@@ -642,7 +642,6 @@ class Item_func_between :public Item_func_opt_neg
Item_func_between(Item *a, Item *b, Item *c)
:Item_func_opt_neg(a, b, c), compare_as_dates(FALSE) { sargable= TRUE; }
longlong val_int();
- optimize_type select_optimize() const { return OPTIMIZE_KEY; }
enum Functype functype() const { return BETWEEN; }
const char *func_name() const { return between; }
bool fix_fields(THD *, Item **);
@@ -1334,8 +1333,6 @@ class Item_func_in :public Item_func_opt_neg
}
DBUG_VOID_RETURN;
}
- optimize_type select_optimize() const
-{ return OPTIMIZE_KEY; }
void add_key_fields(JOIN *join, KEY_FIELD **key_fields, uint *and_level,
table_map usable_tables, SARGABLE_PARAM **sargables);
SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
@@ -1382,9 +1379,9 @@ class Item_func_null_predicate :public Item_bool_func
{
public:
Item_func_null_predicate(Item *a) :Item_bool_func(a) { sargable= true; }
- optimize_type select_optimize() const { return OPTIMIZE_NULL; }
void add_key_fields(JOIN *join, KEY_FIELD **key_fields, uint *and_level,
table_map usable_tables, SARGABLE_PARAM **sargables);
+ SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
CHARSET_INFO *compare_collation() const
{ return args[0]-collation.collation; }
void fix_length_and_dec() { decimals=0; max_length=1; maybe_null=0; }
@@ -1490,6 +1487,12 @@ class Item_func_like :public Item_bool_func2
Re: [Maria-developers] [Commits] 1cd9319: myisam/aria: don't mess with IO_CACHE::file
Hi Sergei,
my concern was that we do if (!rep_quick) twice in a very short distance.
Anyway, this was just a minor wish. Final decision is up to you.
Regards,
Sergey
On Wed, May 27, 2015 at 09:02:03PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On May 27, Sergey Vojtovich wrote:
Hi Sergei,
looks good. Ok to push, a few minor suggestions inline.
diff --git a/storage/maria/ma_check.c b/storage/maria/ma_check.c
index 8c012d2..d50528e 100644
--- a/storage/maria/ma_check.c
+++ b/storage/maria/ma_check.c
@@ -4267,20 +4267,22 @@ int maria_repair_parallel(HA_CHECK *param,
register MARIA_HA *info,
if (!(sort_info.key_block=
alloc_key_blocks(param, (uint) param-sort_key_blocks,
- share-base.max_key_block_length)) ||
- init_io_cache(param-read_cache, info-dfile.file,
+ share-base.max_key_block_length)))
+goto err;
+
+ if (init_io_cache(param-read_cache, info-dfile.file,
(uint) param-read_buffer_length,
-READ_CACHE, share-pack.header_length, 1,
MYF(MY_WME)) ||
- (!rep_quick
- (init_io_cache(info-rec_cache, info-dfile.file,
- (uint) param-write_buffer_length,
- WRITE_CACHE, new_header_length, 1,
- MYF(MY_WME | MY_WAIT_IF_FULL) param-myf_rw) ||
-init_io_cache(new_data_cache, -1,
- (uint) param-write_buffer_length,
- READ_CACHE, new_header_length, 1,
- MYF(MY_WME | MY_DONT_CHECK_FILESIZE)
+READ_CACHE, share-pack.header_length, 1,
MYF(MY_WME)))
goto err;
+
+ if (!rep_quick)
+ {
+if (init_io_cache(new_data_cache, -1,
+(uint) param-write_buffer_length,
+READ_CACHE, new_header_length, 1,
+MYF(MY_WME | MY_DONT_CHECK_FILESIZE)))
+ goto err;
+ }
Why not to move new_data_cache initialization down along with rec_cache?
I've simply kept it where it was, the goal was to avoid
io_cache.file=new_file_descriptor;
for example, there was
info-rec_cache.file=new_file;
and I moved info-rec_cache initialization down where new_file was known
and could be passed as an argument to init_io_cache().
There was no such problem with new_data_cache.
Regards,
Sergei
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] step7 (cleanup) MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
Hi Alexander,
ok to push. I just wish to study this code a bit before we continue.
Thanks,
Sergey
On Wed, May 27, 2015 at 08:03:57AM +0400, Alexander Barkov wrote:
Hi Sergey,
please review the next step for MDEV-7950.
It breaks get_mm_parts() into a virtual method in Item, so
- replaces one virtual call item-type() to another virtual call,
item-get_mm_tree().
- and also removes this virtual call for functype(), which used to
distinguish between COND_AND_FUNC and COND_OR_FUNC:
- if (((Item_cond*) cond)-functype() == Item_func::COND_AND_FUNC)
The next patches will:
1. Split the code in Item::get_mm_tree() responsible for functions into
methods in Item_func and its descendants:
Item_func
Item_func_like, to get rid of the select_optimize() call
Item_func_between
Item_func_in
Item_equal
2. Remove virtual method Item_func::select_optimize(),
as well as enum Item_func::optimize_type, because they won't be
needed any more.
Thanks.
diff --git a/sql/item.h b/sql/item.h
index 56b2edc..9cabbb1 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -62,6 +62,8 @@ class user_var_entry;
class JOIN;
struct KEY_FIELD;
struct SARGABLE_PARAM;
+class RANGE_OPT_PARAM;
+class SEL_TREE;
static inline uint32
@@ -1147,6 +1149,15 @@ class Item: public Type_std_attributes
{
return;
}
+ /*
+ Make a select tree for all keys in a condition or a condition part
+ @param param Context
+ @param cond_ptr[OUT] Store a replacement item here if the condition
+ can be simplified, e.g.:
+WHERE part1 OR part2 OR part3
+ with one of the partN evalutating to
SEL_TREE::ALWAYS.
+ */
+ virtual SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
/*
Checks whether the item is:
- a simple equality (field=field_item or field=constant_item), or
diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
index 204e6ed..f814666 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -1733,6 +1733,7 @@ class Item_cond :public Item_bool_func
void add_key_fields(JOIN *join, KEY_FIELD **key_fields,
uint *and_level, table_map usable_tables,
SARGABLE_PARAM **sargables);
+ SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
virtual void print(String *str, enum_query_type query_type);
void split_sum_func(THD *thd, Item **ref_pointer_array, ListItem
fields);
friend int setup_conds(THD *thd, TABLE_LIST *tables, TABLE_LIST *leaves,
@@ -2082,6 +2083,7 @@ class Item_cond_and :public Item_cond
COND_EQUAL **cond_equal_ref);
void add_key_fields(JOIN *join, KEY_FIELD **key_fields, uint *and_level,
table_map usable_tables, SARGABLE_PARAM **sargables);
+ SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
};
inline bool is_cond_and(Item *item)
diff --git a/sql/opt_range.cc b/sql/opt_range.cc
index 0f1fa40..d03e399 100644
--- a/sql/opt_range.cc
+++ b/sql/opt_range.cc
@@ -952,7 +952,6 @@ static SEL_TREE * get_mm_parts(RANGE_OPT_PARAM
*param,COND *cond_func,Field *fie
static SEL_ARG *get_mm_leaf(RANGE_OPT_PARAM *param,COND *cond_func,Field
*field,
KEY_PART *key_part,
Item_func::Functype type,Item *value);
-static SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond);
static bool is_key_scan_ror(PARAM *param, uint keynr, uint8 nparts);
static ha_rows check_quick_select(PARAM *param, uint idx, bool index_only,
@@ -3131,7 +3130,7 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map
keys_to_use,
if (cond)
{
- if ((tree= get_mm_tree(param, cond)))
+ if ((tree= cond-get_mm_tree(param, cond)))
{
if (tree-type == SEL_TREE::IMPOSSIBLE)
{
@@ -3628,7 +3627,7 @@ bool calculate_cond_selectivity_for_table(THD *thd,
TABLE *table, Item **cond)
thd-no_errors=1;
-tree= get_mm_tree(param, cond);
+tree= cond[0]-get_mm_tree(param, cond);
if (!tree)
goto free_alloc;
@@ -4061,7 +4060,7 @@ bool prune_partitions(THD *thd, TABLE *table, Item
*pprune_cond)
SEL_TREE *tree;
int res;
- tree= get_mm_tree(range_par, pprune_cond);
+ tree= pprune_cond-get_mm_tree(range_par, pprune_cond);
if (!tree)
goto all_used;
@@ -7967,104 +7966,106 @@ static SEL_TREE
*get_full_func_mm_tree(RANGE_OPT_PARAM *param,
NULL - Could not infer anything from condition cond.
SEL_TREE with type=IMPOSSIBLE - condition can never be true.
*/
-
-static SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr)
+SEL_TREE *Item_cond_and::get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr)
{
- SEL_TREE *tree=0;
- SEL_TREE *ftree= 0;
- Item_field *field_item= 0;
- bool inv= FALSE;
-
Re: [Maria-developers] [Commits] 8b30c0f: MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
Hi Sanja,
There're 3 bug fixes and 4 revisions concerning these lines:
https://bugs.mysql.com/bug.php?id=16716
git: 1991a87d831bb48b31989c622e35acda11df7fef
git: ca22a81b1c84ce81e1e9ea2c3ace7be1848027d8 (source for all used_tables())
https://bugs.mysql.com/bug.php?id=36488
git: 66367aeea83a0cd7f583a0194651a235b63975a9
https://bugs.mysql.com/bug.php?id=46815
git: 10406ae65871de074e807e626f9ede686e9321d4
If we don't have these bugs, why not to revert all the code introduced by these
patches?
Or we're not affected by the need to use used_tables() as an indication of
prepared statement? If so, why?
Thanks,
Sergey
On Sun, May 17, 2015 at 03:10:48PM +0200, sa...@mariadb.com wrote:
revision-id: 8b30c0f26c7b35fc73429f85aa35eb3614d60bd8
parent(s): b2db8e85422a455a10cd28ef7e44977182528f70
committer: Oleksandr Byelkin
branch nick: server
timestamp: 2015-05-17 15:10:45 +0200
message:
MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
Part 2: removed hack workaround for bug we do not have.
---
sql/item_strfunc.cc | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
index 4bf8dd5..7b9169e 100644
--- a/sql/item_strfunc.cc
+++ b/sql/item_strfunc.cc
@@ -626,8 +626,7 @@ String *Item_func_concat::val_str(String *str)
if (!(res=args[0]-val_str(str)))
goto null;
use_as_buff= tmp_value;
- /* Item_subselect in --ps-protocol mode will state it as a non-const */
- is_const= args[0]-const_item() || !args[0]-used_tables();
+ is_const= args[0]-const_item();
for (i=1 ; i arg_count ; i++)
{
if (res-length() == 0)
@@ -639,7 +638,7 @@ String *Item_func_concat::val_str(String *str)
non-empty argument. Because of this we need is_const to be
evaluated only for it.
*/
- is_const= args[i]-const_item() || !args[i]-used_tables();
+ is_const= args[i]-const_item();
}
else
{
@@ -975,7 +974,7 @@ String *Item_func_concat_ws::val_str(String *str)
for (i=1; i arg_count; i++)
if ((res= args[i]-val_str(str)))
{
- is_const= args[i]-const_item() || !args[i]-used_tables();
+ is_const= args[i]-const_item();
break;
}
___
commits mailing list
comm...@mariadb.org
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Step#6: MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
Hi Alexander,
looks good, just a few minor suggestions inline.
Some measures on my computer:
add_key_fields 0.20% - out of radar
Item_func_between::add_key_fields - 0.05%
Item_equal::add_key_fields - 0.03%
Item_equal::select_optimize 0.01% - out of radar
Item_func_between::select_optimize 0.00% - out of radar
Item_func_eq::functype 0.04% - 0.03%
Item_equal::functype 0.04% - 0.02%
Item_func_between::functype 0.03% - 0.02%
Item_func::functype 0.00% - 0.00%
Item_func::type 0.14% - 0.11%
Item_field::type 0.08% - 0.08%
Item_int::type 0.04% - 0.03%
Item_sum::type 0.02% - 0.01%
Total saving: 0.22%
On Fri, May 15, 2015 at 06:58:38PM +0400, Alexander Barkov wrote:
Hi Sergey,
Please review the next iteration for MDEV-7950.
This one splits the function add_key_fields() into a method in Item.
This change removes about 3 virtual calls item-type(), as well as some
virtual calls item_func-functype(), and adds one virtual call
item-add_key_fields() instead.
Thanks.
...skip...
diff --git a/sql/item_geofunc.h b/sql/item_geofunc.h
index 2dbb2cd..b8cbed6 100644
--- a/sql/item_geofunc.h
+++ b/sql/item_geofunc.h
@@ -404,7 +404,6 @@ class Item_func_isempty: public Item_bool_func
public:
Item_func_isempty(Item *a): Item_bool_func(a) {}
longlong val_int();
- optimize_type select_optimize() const { return OPTIMIZE_NONE; }
const char *func_name() const { return st_isempty; }
void fix_length_and_dec() { maybe_null= 1; }
};
Hmm... does select_optimize() make sense at all now? I mean won't code become
simpler if we move it's logic to appropriate callers?
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 83bf487..947592a 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -4602,240 +4603,283 @@ is_local_field (Item *field)
...skip...
+void
+Item_bool_func::add_key_fields(JOIN *join, KEY_FIELD **key_fields,
+ uint *and_level, table_map usable_tables,
+ SARGABLE_PARAM **sargables)
+{
+ /* If item is of type 'field op field/constant' add it to key_fields */
+ switch (select_optimize()) {
+ case Item_func::OPTIMIZE_NONE:
+break;
case Item_func::OPTIMIZE_OP:
{
-bool equal_func=(cond_func-functype() == Item_func::EQ_FUNC ||
- cond_func-functype() == Item_func::EQUAL_FUNC);
+bool equal_func= (functype() == Item_func::EQ_FUNC ||
+ functype() == Item_func::EQUAL_FUNC);
-if (is_local_field (cond_func-arguments()[0]))
+if (is_local_field(args[0]))
{
- add_key_equal_fields(join, key_fields, *and_level, cond_func,
- (Item_field*) (cond_func-arguments()[0])-
- real_item(),
-equal_func,
- cond_func-arguments()+1, 1, usable_tables,
- sargables);
+ add_key_equal_fields(join, key_fields, *and_level, this,
+ (Item_field*) args[0]-real_item(), equal_func,
+ args + 1, 1, usable_tables, sargables);
}
-if (is_local_field (cond_func-arguments()[1])
- cond_func-functype() != Item_func::LIKE_FUNC)
+if (is_local_field(args[1]))
{
- add_key_equal_fields(join, key_fields, *and_level, cond_func,
- (Item_field*) (cond_func-arguments()[1])-
- real_item(),
-equal_func,
- cond_func-arguments(),1,usable_tables,
- sargables);
+ add_key_equal_fields(join, key_fields, *and_level, this,
+ (Item_field*) args[1]-real_item(), equal_func,
+ args, 1, usable_tables, sargables);
}
break;
}
+ case Item_func::OPTIMIZE_KEY:
case Item_func::OPTIMIZE_NULL:
-/* column_name IS [NOT] NULL */
-if (is_local_field (cond_func-arguments()[0])
- !(cond_func-used_tables() OUTER_REF_TABLE_BIT))
-{
- Item *tmp=new Item_null;
- if (unlikely(!tmp)) // Should never be true
- return;
- add_key_equal_fields(join, key_fields, *and_level, cond_func,
- (Item_field*) (cond_func-arguments()[0])-
- real_item(),
- cond_func-functype() == Item_func::ISNULL_FUNC,
-tmp, 1, usable_tables, sargables);
-}
-break;
case Item_func::OPTIMIZE_EQUAL:
-Item_equal *item_equal= (Item_equal *) cond;
-Item *const_item= item_equal-get_const();
-Item_equal_fields_iterator it(*item_equal);
-if (const_item)
+DBUG_ASSERT(0);
+break;
+ }
+}
+
+
I'd better do:
Re: [Maria-developers] [Commits] b2db8e8: MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
Hi Sanja,
this patch looks Ok, just one minor suggestion inline. Though effect for given
workload is quite moderate:
Item_field::used_tables 0.21% - 0.20%
Item_basic_constant::used_tables 0.11% - 0.11%
Item_func::used_tables0.05% - 0.05%
Item_sum::used_tables 0.00% - 0.00%
On Wed, May 13, 2015 at 04:17:26PM +0200, sa...@mariadb.com wrote:
revision-id: b2db8e85422a455a10cd28ef7e44977182528f70
parent(s): b22959903b89e798f8804ec9a815c88f75915cd9
committer: Oleksandr Byelkin
branch nick: server
timestamp: 2015-05-13 16:17:22 +0200
message:
MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
small sixes of used_tables() usage
...skip...
diff --git a/sql/item_func.h b/sql/item_func.h
index 0d57c2b..c48cd84 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -1337,7 +1337,7 @@ class Item_func_sleep :public Item_int_func
const char *func_name() const { return sleep; }
table_map used_tables() const
{
-return Item_int_func::used_tables() | RAND_TABLE_BIT;
+return used_tables_cache | RAND_TABLE_BIT;
}
bool is_expensive() { return 1; }
longlong val_int();
@@ -1591,7 +1591,7 @@ class Item_func_get_lock :public Item_int_func
void fix_length_and_dec() { max_length=1; maybe_null=1;}
table_map used_tables() const
{
-return Item_int_func::used_tables() | RAND_TABLE_BIT;
+return used_tables_cache | RAND_TABLE_BIT;
}
bool const_item() const { return 0; }
bool is_expensive() { return 1; }
@@ -1611,7 +1611,7 @@ class Item_func_release_lock :public Item_int_func
void fix_length_and_dec() { max_length= 1; maybe_null= 1;}
table_map used_tables() const
{
-return Item_int_func::used_tables() | RAND_TABLE_BIT;
+return used_tables_cache | RAND_TABLE_BIT;
}
bool const_item() const { return 0; }
bool is_expensive() { return 1; }
@@ -1724,7 +1724,7 @@ class Item_func_set_user_var :public Item_func
void fix_length_and_dec();
table_map used_tables() const
{
-return Item_func::used_tables() | RAND_TABLE_BIT;
+return used_tables_cache | RAND_TABLE_BIT;
}
bool const_item() const { return 0; }
bool is_expensive() { return 1; }
Was there any reason for this change? Compiler should be clever enough to inline
used_tables() of base class here.
Regards,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8a0f331: MDEV-7943 - pthread_getspecific() takes 0.76% in OLTP RO
Hi Sergei, On Wed, May 13, 2015 at 05:25:21PM +0200, Sergei Golubchik wrote: Hi, Sergey! On May 13, Sergey Vojtovich wrote: -while (is_on() !thd_killed(NULL)) +while (is_on() !thd_killed(current_thd)) thd_killed() is a function of the kill_statement service (see include/mysql/service_kill_statement.h). It is created for plugins to use. But current_thd is not. Plugins generally have no access to it. Speaking of general sanity: there's no other code that does thd_killed(NULL). Speaking of semisync plugin sanity: it does lots of direct server function calls already, including current_thd. Do you think it is worth to preserve this thd_killed(NULL)? I believe it is quite easy to extend replication plugin interface so that THD is passed through. But is it worth it? There's no need to change semisync plugin, if it uses current_thd, then it can also use it for thd_killed(). But if you'll require thd_killed() always to take THD as an argument, then other plugins (that work strictly within plugin api limits) won't be able to check killed flag in places where THD isn't available. May be it's not a problem, though, I don't know. I can suggest 3 alternatives: - add thd_killed_current() (now or on demand?) - add current_thd() to API (now or on demand?) - keep if (!thd), even though it makes mostly useless branch per row Which one do you like most? Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Step#4: MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
Hi Alexander,
following profiler reports for build_equal_items* in your and mine patches.
For OLTP RO they're equal. I guess Item_func::build_equal_items() was not
inlined due to recursion?
Bar's patch
---
0.06% Item_func_eq::build_equal_items_and_cond()
0.05% build_equal_items()
0.01% Item_func::build_equal_items()
0.00% Item_func::build_equal_items_and_cond()
Svoj's patch
0.06% build_equal_items()
0.05% Item_func_eq::build_equal_items_and_cond()
0.01% Item_func::build_equal_items()
0.00% Item::build_equal_items_and_cond()
Regards,
Sergey
On Thu, May 07, 2015 at 01:58:28PM +0400, Sergey Vojtovich wrote:
Hi Alexander,
comments inline.
On Wed, May 06, 2015 at 07:51:18PM +0400, Alexander Barkov wrote:
Hi Sergey,
Please review a patch for the next step for MDEV-7950
(one small thing at a time, to avoid huge unclear patches)
Thanks.
diff --git a/sql/item.h b/sql/item.h
index 043605a..a665d23 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -1133,6 +1133,13 @@ class Item: public Type_std_attributes
update_used_tables();
return this;
}
+ virtual COND *build_equal_items_and_cond(THD *thd, COND_EQUAL *inherited,
+ bool link_item_fields,
+ COND_EQUAL **cond_equal_ref)
+ {
+*cond_equal_ref= NULL;
+return Item::build_equal_items(thd, inherited, link_item_fields);
+ }
/*
Checks whether the item is:
- a simple equality (field=field_item or field=constant_item), or
cond_equal_ref is initialized to 0 by caller, why duplicate initialization
here
and everywhere else?
When you write XXX::build_equal_items() you force static call indeed, but not
inlining, which we're aiming for. That is non-inlined static call as such
won't
save us much compared to virtual call.
I think our aim should be to keep build_equal_items_and_cond() virtual and
have
build_equal_items() inlined into the former. So we mostly rely on compiler's
optimizer.
I _believe_ it should be clever enough to optimize the following as described
above:
class Item
{
...
virtual build_equal_items_and_cond()
{ return build_equal_items(); }
...
}
This should be easy to test by adding inline to build_equal_items()
declaration and compiling with -Winline.
Makes sense?
...skip...
index 53f249d..0c279ed 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -1989,6 +2002,11 @@ class COND_EQUAL: public Sql_alloc
{
upper_levels= 0;
}
+ COND_EQUAL(Item_equal *item_equal)
+ :upper_levels(0)
+ {
+current_level.push_back(item_equal);
+ }
void copy(COND_EQUAL cond_equal)
{
max_members= cond_equal.max_members;
If possible, here and everywhere else please use
push_back(item_equal, thd-mem_root). This will save us one
pthread_getspecific() call.
@@ -1998,6 +2016,8 @@ class COND_EQUAL: public Sql_alloc
else
current_level= cond_equal.current_level;
}
+ Item_equal *build_item_equal_from_current_level(THD *thd,
+ COND_EQUAL *inherited);
};
@@ -2106,8 +2126,23 @@ class Item_cond_and :public Item_cond
void mark_as_condition_AND_part(TABLE_LIST *embedding);
virtual uint exists2in_reserved_items() { return list.elements; };
bool walk_top_and(Item_processor processor, uchar *arg);
+ void build_new_level(THD *thd, COND_EQUAL *cond_equal);
COND *build_equal_items(THD *thd, COND_EQUAL *inherited,
bool link_item_fields);
Didn't get the need for build_item_equal_from_current_level() and
build_new_level(). Is it just some minor refactoring?
...skip...
diff --git a/sql/item_func.h b/sql/item_func.h
index 0d57c2b..01e1527 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -133,6 +133,15 @@ class Item_func :public Item_func_or_sum, public
Used_tables_and_const_cache
}
COND *build_equal_items(THD *thd, COND_EQUAL *inherited,
bool link_item_fields);
+ COND *build_equal_items_and_cond(THD *thd, COND_EQUAL *inherited,
+ bool link_item_fields,
+ COND_EQUAL **cond_equal_ref)
+ {
+*cond_equal_ref= NULL;
+COND *cond= build_equal_items(thd, inherited, link_item_fields);
+DBUG_ASSERT(cond == this);
+return cond;
+ }
bool eq(const Item *item, bool binary_cmp) const;
virtual optimize_type select_optimize() const { return OPTIMIZE_NONE; }
virtual bool have_rev_func() const { return 0; }
The only difference from base build_equal_items_and_cond() is just
DBUG_ASSERT().
I think assert can be moved into base implementation with more complex
condition
too (assuming we'll decide to keep only base implementation).
...skip...
diff --git a/sql/sql_select.cc b/sql
Re: [Maria-developers] [Commits] 93e746a: MDEV-7793 - Race condition between XA COMMIT/ROLLBACK and disconnect
Hi Sergei,
On Thu, May 07, 2015 at 01:04:24PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Mar 17, s...@mariadb.org wrote:
revision-id: 93e746afb2d1a23cb933291ad736a20858b5ac3e
parent(s): ccc7297fe94af1129c717f91d31fa075d54a0371
committer: Sergey Vojtovich
branch nick: mariadb
timestamp: 2015-03-17 19:49:04 +0400
message:
MDEV-7793 - Race condition between XA COMMIT/ROLLBACK and disconnect
XA COMMIT/ROLLBACK of XA transaction owned by different thread may access
freed memory if that thread disconnects at the same time.
Also concurrent XA COMMIT/ROLLBACK of recovered XA transaction were not
serialized properly.
diff --git a/sql/sql_class.cc b/sql/sql_class.cc
index 3071ba6..f18231d 100644
--- a/sql/sql_class.cc
+++ b/sql/sql_class.cc
@@ -5355,7 +5378,7 @@ bool xid_cache_insert(THD *thd, XID_STATE *xid_state)
switch (res)
{
case 0:
-xid_state-xid_cache_element-mark_initialized();
+xid_state-xid_cache_element-set(XID_cache_element::ACQUIRED);
Why not mark_acquired() here?
mark_acquired() is intended to acquire RECOVERED elements. That is to allow
multiple threads to attempt to acquire the same XID. One thread will succeed,
others will get not found.
Can you be sure that no other thread has already acquired this
xid_cache_element after you've inserted it?
Only RECOVERED elements can be acquired. This newly inserted element has
m_state == 0.
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [Commits] 4bff8b0: MDEV-7004 - Merge scalability fixes from 10.0-power
Hi Jocelyn,
right, we didn't yet try P8 transactional memory functions. There're no firm
plans either, but there was definitely some research.
Regards,
Sergey
On Sat, May 02, 2015 at 09:26:42PM +0200, jocelyn fournier wrote:
Hi Sergey,
Just to be sure, currently MariaDB doesn't implement any Hardware
Transactional Memory Built-in Functions ?
If so, is there any plan to evaluate the gain they could offer to MariaDB ?
Thanks and regards,
Jocelyn Fournier
Le 04/12/2014 14:45, s...@mariadb.org a écrit :
revision-id: 4bff8b0b03ba82cd0ae6817b54de1780e89047ca
parent(s): 3526fe1b6000b2558290007e5cc3a1018850787c
committer: Sergey Vojtovich
branch nick: 10.1
timestamp: 2014-12-04 17:44:46 +0400
message:
MDEV-7004 - Merge scalability fixes from 10.0-power
Preallocate dynamic array and bitmap on mem_root to avoid expensive malloc.
This reduces number of allocations from 39 to 31 per OLTP RO transaction.
---
sql/opt_range.cc | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sql/opt_range.cc b/sql/opt_range.cc
index 1a9ae3d..813508c 100644
--- a/sql/opt_range.cc
+++ b/sql/opt_range.cc
@@ -1838,8 +1838,6 @@ void SQL_SELECT::cleanup()
index= key_nr;
head= table;
key_part_info= head-key_info[index].key_part;
- my_init_dynamic_array(ranges, sizeof(QUICK_RANGE*), 16, 16,
-MYF(MY_THREAD_SPECIFIC));
/* 'thd' is not accessible in QUICK_RANGE_SELECT::reset(). */
mrr_buf_size= thd-variables.mrr_buff_size;
@@ -1857,9 +1855,12 @@ void SQL_SELECT::cleanup()
file= head-file;
record= head-record[0];
- /* Allocate a bitmap for used columns (Q: why not on MEM_ROOT?) */
- if (!(bitmap= (my_bitmap_map*) my_malloc(head-s-column_bitmap_size,
- MYF(MY_WME |
MY_THREAD_SPECIFIC
+ my_init_dynamic_array2(ranges, sizeof(QUICK_RANGE*),
+ thd-alloc(sizeof(QUICK_RANGE*) * 16), 16, 16,
+ MYF(MY_THREAD_SPECIFIC));
+
+ /* Allocate a bitmap for used columns */
+ if (!(bitmap= (my_bitmap_map*) thd-alloc(head-s-column_bitmap_size)))
{
column_bitmap.bitmap= 0;
*create_error= 1;
@@ -1923,7 +1924,6 @@ void QUICK_RANGE_SELECT::range_end()
}
delete_dynamic(ranges); /* ranges are allocated in alloc */
free_root(alloc,MYF(0));
-my_free(column_bitmap.bitmap);
}
my_free(mrr_buf_desc);
DBUG_VOID_RETURN;
___
commits mailing list
comm...@mariadb.org
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] TRUE vs true
Hi Sanja, yes, it is not a requirement to use true/false in C++. I'd be Ok about my_bool if not one glitch: it can evaluate to anything that char can evaluate to. C99 bool behaves more as expected: quot _Bool (also accessible as the macro bool) - type, capable of holding one of the two values: 1 and 0 (also accessible as the macros true and false). Note that conversion to _Bool does not work the same as conversion to other integer types: (bool)0.5 evaluates to 1, whereas (int)0.5 evaluates to 0. /quot I'd vote for bool/true/false everywhere if I were sure about it's portability and about sizeof(_Bool) stability. Regards, Sergey On Thu, Apr 23, 2015 at 11:02:16AM +0200, Oleksandr Byelkin wrote: On 23.04.15 10:14, Sergey Vojtovich wrote: Hi! According to MySQL coding guidelines: https://dev.mysql.com/doc/internals/en/additional-suggestions.html quot * In C code, use TRUE and FALSE rather than 1/0 * In C++ code, it is OK to use true and false (do not use 1/0). You can use C++ bool/true/false when calling C functions (values will be safely promoted to my_bool). /quot It is OK == it is allowed to use it. (about true/false) you have not use 1/0 and TRUE/FALSE must in C and OK in C++ (there is no other told) So I should carefully check what code it is or just use TRUE/FALSE? I prefer TRUE/FALSE everywhere. [skip] ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] TRUE vs true (was: [Commits] ac05f41: postreview fix)
Hi! According to MySQL coding guidelines: https://dev.mysql.com/doc/internals/en/additional-suggestions.html quot * In C code, use TRUE and FALSE rather than 1/0 * In C++ code, it is OK to use true and false (do not use 1/0). You can use C++ bool/true/false when calling C functions (values will be safely promoted to my_bool). /quot I'm aware of only one more or less good reason for not using true/false in C++: it's easier to convert it to C code. But that's rare: we mostly convert in the opposite direction. Also C99 now supports true/false, e.g. see nice description: http://www.jacquesf.com/2011/04/in-defense-of-the-c99-boolean-type/ Regards, Sergey On Thu, Apr 23, 2015 at 09:33:07AM +0200, Oleksandr Byelkin wrote: Hi, Jan! On 23.04.15 08:38, Jan Lindström wrote: Sanja, Why you use TRUE/FALSE instead of true/false for bool ? Historical reasons. I was told to use TRUE/FALSE long time ago and nobody have told something else. We have no our code style doc so I use what I remember from old MySQL one. [skip] ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
[Maria-developers] MDEV-7660 - MySQL WL#6671 Improve scalability by not using thr_lock.c locks for InnoDB tables
Hi Jan, we're trying to adapt MySQL WL#6671 to MariaDB, which is expected to boost concurrent single table workloads. MySQL solution is fully based on MDL. It caused substential changes to MDL, which would be nice to avoid. We'd like to understand if we can achieve the same effect with InnoDB locking. According to WL#6671 InnoDB relied on THR_LOCK in following cases: 1. To isolate HANDLER READ statements from LOCK TABLES WRITE statements. It is likely not a big problem if we decide to go non-MDL way. 2. To isolate LOCK TABLES statements that lock tables for write implicitly or for read, both explicitly or implicitly, from concurrent DML statements. That's something that we're looking alternatives for. See below. 3. Due to coding mistake thr_lock.c lock was necessary to isolate ALTER TABLE IMPORT/DISCARD TABLESPACE under LOCK TABLES from concurrent I_S queries/ open HANDLERs. I agree with this patch, it is merged and pending approval (MDEV-7894). 4. To indicate that InnoDB tables don't support LOCK TABLES READ LOCAL by upgrading TL_READ lock requested by statement to TL_READ_NO_INSERT. It is likely not a big problem if we decide to go non-MDL way. The question is if InnoDB can do the following with reasonable effort: - LOCK TABLES ... READ - protect table data from concurrent writers - LOCK TABLES ... WRITE - pretect table data from concurrent readers and writers - honor SELECT HIGH_PRIORITY when there's pending LOCK TABLES ... WRITE (do we need this?) - honor LOCK TABLES ... WRITE LOW_PRIORITY when there're concurrent readers (do we need this?) Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] [MDEV-7922] pfs is not cahce definition info at db start (ver 10.1.13)
Hi SeungHoon, this is regression after MDEV-4262 - P_S discovery. To be fixed soon. Thanks, Sergey On Mon, Apr 06, 2015 at 02:01:45PM +0900, 유승훈 wrote: Hello, i did some test about pfs on Mariadb 10.1.13 ver (ubuntu 14.04) So I just add some issue about pfs in mariadb ver 10.1.13 like below jira link simple context : 1) db start 2) just query about pfs = error = but use pfs then sql run = no error https://mariadb.atlassian.net/browse/MDEV-7922 Any body some idea about this, reply mail or write on jira. Thanks for your time and attention. Best Regards! SeungHoon. _ 상기 메시지와 첨부화일 내에는 비밀정보가 포함되어 있을 수 있으며, 지정된 수신자에 한하여 조회 및 사용될 수 있습니다. 만약 송신자의 실수로 인하여 상기 메시지를 수신하였다면, 송신자에게 메시지를 반송해 주시고, 원본 메시지와 모든 사본을 폐기해 주시기 바랍니다. 상기 메시지의 전체 또는 일부에 대해 무단 열람, 사용, 공개, 배포하는 것은 금지되어 있습니다.(주)LG CNS. This message and its attachments may contain confidential information, and they are intended to be viewed or used by only the individuals specified in the message. If you have received this message in an error from the sender, please contact the sender immediately to notify the error and delete all of the message and its copies. It is prohibited to view, use, make public and/or distribute part or whole of this message without written permission. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 20e5c37: MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff
Hi Sergei,
thanks for you comments. Answers inline.
On Fri, Mar 13, 2015 at 08:58:38PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Mar 12, s...@mariadb.org wrote:
revision-id: 20e5c37ccfac9d95a2db1f295f211656df1ce160
parent(s): 190858d996e7dc90e01fe8a5e7daec6af0012b23
committer: Sergey Vojtovich
branch nick: 10.1
timestamp: 2015-03-12 18:15:08 +0400
message:
MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff
XID cache is now based on lock-free hash.
Also fixed lf_hash_destroy() to call alloc destructor.
Note that previous implementation had race condition when thread was
accessing
XA owned by different thread. This new implementation doesn't fix it either.
...skip...
@@ -5106,120 +5109,205 @@ void mark_transaction_to_rollback(THD *thd, bool
all)
/***
Handling of XA id cacheing
***/
+class XID_cache_element
+{
+ uint m_state;
shouldn't it be uint32 ?
That's the type that my_atomic_add32/cas32 work with.
Yes, it should. Thanks!
+ static const uint DELETED= 1 31;
+public:
+ XID_STATE *m_xid_state;
O-okay. I *think* your hand-written locking logic works.
But please add a comment, explaining how it works,
all four methods, the meaning of DELETED, etc.
Perhaps DELETED should rather be NOT_INITIALIZED?
A very valid request. IMHO mutex is overkill here, that's why I implemented this
trivial locking. I'll add comments and think about better names.
+ bool lock()
+ {
+if (my_atomic_add32_explicit(m_state, 1, MY_MEMORY_ORDER_ACQUIRE)
DELETED)
+{
+ unlock();
+ return false;
+}
+return true;
+ }
+ void unlock()
+ {
+my_atomic_add32_explicit(m_state, -1, MY_MEMORY_ORDER_RELEASE);
+ }
+ void mark_deleted()
+ {
+uint old= 0;
+while (!my_atomic_cas32_weak_explicit(m_state, old, DELETED,
+ MY_MEMORY_ORDER_RELAXED,
+ MY_MEMORY_ORDER_RELAXED))
usually one should use LF_BACKOFF inside spin-loops.
Good point. Though LF_BACKOFF doesn't seem to do anything useful.
+ old= 0;
+ }
+ void mark_not_deleted()
+ {
+DBUG_ASSERT(m_state DELETED);
+my_atomic_add32_explicit(m_state, -DELETED, MY_MEMORY_ORDER_RELAXED);
+ }
+ static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)),
+ XID_cache_element *element,
+ XID_STATE *xid_state)
+ {
+element-m_xid_state= xid_state;
+xid_state-xid_cache_element= element;
+ }
+ static void lf_alloc_constructor(uchar *ptr)
+ {
+XID_cache_element *element= (XID_cache_element*) (ptr +
LF_HASH_OVERHEAD);
+element-m_state= DELETED;
+ }
+ static void lf_alloc_destructor(uchar *ptr)
+ {
+XID_cache_element *element= (XID_cache_element*) (ptr +
LF_HASH_OVERHEAD);
+if (element-m_state != DELETED)
How can this happen? You mark an element DELETED before lf_hash_delete()
That's for elements inserted by ha_recover(). There's no guarantee that they
will be deleted.
...skip...
-XID_STATE *xid_cache_search(XID *xid)
+
+XID_STATE *xid_cache_search(THD *thd, XID *xid)
{
- mysql_mutex_lock(LOCK_xid_cache);
- XID_STATE *res=(XID_STATE *)my_hash_search(xid_cache, xid-key(),
- xid-key_length());
- mysql_mutex_unlock(LOCK_xid_cache);
- return res;
+ DBUG_ASSERT(thd-xid_hash_pins);
+ XID_cache_element *element=
+(XID_cache_element*) lf_hash_search(xid_cache, thd-xid_hash_pins,
+xid-key(), xid-key_length());
+ if (element)
+ {
+lf_hash_search_unpin(thd-xid_hash_pins);
+return element-m_xid_state;
Normally, one should not access the element after it's unpinned, so the
protocol is
XID_STATE *state= element-m_xid_state;
lf_hash_search_unpin(thd-xid_hash_pins);
return state;
Perhaps your locking/deleted m_state guarantees that element is safe to
access? But I failed to see that :(
This is noted in cset comment:
Note that previous implementation had race condition when thread was accessing
XA owned by different thread. This new implementation doesn't fix it either.
I can fix it if you like.
diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc
index 36768ae..411c7ae 100644
--- a/storage/spider/spd_table.cc
+++ b/storage/spider/spd_table.cc
@@ -41,11 +41,13 @@
I'd prefer to have spider changes in a separate commit
Ok, I'll split them.
...skip...
Thanks,
Sergey
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Re: [Maria-developers] MDEV-6089 review
Hi Sergei, On Tue, Mar 03, 2015 at 03:38:31PM +0100, Sergei Golubchik wrote: Hi. Here're I'll be asking questions about the patches I'm looking at. 1. Is this mostly a merge of WL#7305 or mostly your own implementation? That is, most of the changes and design decisions are yours or because MySQL did it? Implementations differ. All design decisions are mine, like if I were to implement this from scratch. There is nothing like because MySQL did it. Though I must admit I studied MySQL implementation and used some of it's comments and names. Things that I didn't like in MySQL implementation were mainly: - changes to lf hash (not very clean) - big workaround for that lf-allocator purgatory bug - huge MDL_lock_strategy - MDL_lock construction/destruction and re-initialization is not very clean either 2. Do you work like I do (commit, push, fix failing tests, combine commits with git rebase -i to get a clean history) or you've just magically got 32bit rdiffs correctly in every commit? :) There was some magic indeed. At least non-embedded rdiffs worked according to bb. No results for embedded though. But yes, normally I'd fix it so that history is kept clean. 3. Why MDL objects cache won't be needed? The comment said On some systems (e.g. Windows XP) constructing/destructing MDL_lock objects can be fairly expensive. We use this cache to avoid these costs... Why is this no longer an issue? Because MDL_lock's are now cached by lf-allocator. 4. MDL_lock_strategy - this object is in 5.7 too, but you've done it very differently. Why? And I wonder why didn't you move the common part of two strategies to the base class. You wanted a pure interface class? Probably the main difference is that MySQL implemented it as C-like structure with function pointers, while I did it as C++ classes. MySQL's implementation takes like 10x more lines than mine. What are those common parts? They should have nothing in common. 5. Added initializer callback to lf-hash. May be you should remove this hack with decreasing LF_HASH::element_size etc and move this functionality to a custom initializer? It was added in 6ba12f. In particular, see changes to lf* files and whether they can use your initializer instead. You mean I should fix waiting threads (and probably table cache) to use initializer function instead of decreasing LF_HASH::element_size? I don't mind, but probably in a separate patch. 6. Replaced hash with lock-free hash: test result changes. Is the order of rows stable? perhaps you should use --sort_results ? Order of rows depends on hash implementation. If we'll have to change this again, then it is definitely a good idea to fix it. in MDL_lock constructor, I'd set m_strategy to 0 (ori to 1 or, may be, keep it uninitialized) as a safety measure, to ensure it's not used before MDL_lock::lf_hash_initializer(). I guess it's for one of intermediate patches. Last version does it. Otherwise looks ok. Mostly a straightforward change. Should I take it as ok to push? :) Thanks, Sergey ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
