November 2014 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] Rev 4367: MDEV-7026: Race in InnoDB/XtraDB mutex implementation can stall or hang the server. in http://bazaar.launchpad.net/~maria-captains/maria/5.5
by Sergey Vojtovich 20 Nov '14

20 Nov '14

Hi Kristian, thanks for taking over this bug! I believe this is the patch that was pushed into 5.5. Unfortunately I think there is still one potential for a hang left (details below). :( Since InnoDB and XtraDB changes are identical I'll comment only XtraDB changes. So you removed os_isync and implemented os_mb instead. That's good! You also removed a bunch of os_wmb/os_rmb calls. That's good too. And you kept RELEASE barrier for mutex_exit() on PPC64. Very good! I will also comment things that I don't really like, but they're just for your information (no action required). On Wed, Nov 19, 2014 at 01:56:46PM +0100, knielsen(a)knielsen-hq.org wrote: > At http://bazaar.launchpad.net/~maria-captains/maria/5.5 > > ------------------------------------------------------------ > revno: 4367 > revision-id: knielsen(a)knielsen-hq.org-20141119125646-njq02h7irmu9s71b > parent: sergii(a)pisem.net-20141118231952-fk3o69075dn3ravv > committer: Kristian Nielsen <knielsen(a)knielsen-hq.org> > branch nick: mariadb-5.5 > timestamp: Wed 2014-11-19 13:56:46 +0100 > message: > MDEV-7026: Race in InnoDB/XtraDB mutex implementation can stall or hang the server. > > The bug was that full memory barrier was missing in the code that ensures that > a waiter on an InnoDB mutex will not go to sleep unless it is guaranteed to be > woken up again by another thread currently holding the mutex. This made > possible a race where a thread could get stuck waiting for a mutex that is in > fact no longer locked. If that thread was also holding other critical locks, > this could stall the entire server. There is an error monitor thread than can > break the stall, it runs about once per second. But if the error monitor > thread itself got stuck or was not running, then the entire server could hang > infinitely. > > This was introduced on i386/amd64 platforms in 5.5.40 and 10.0.13 by an > incorrect patch that tried to fix the similar problem for PowerPC. JFYI: Strictly speaking that "incorrect patch" attempted to fix different problem (read carefully): it added RELEASE barrier between "stores inside mutex" and "lock_word= 0" while this patch adds full barrier between "lock_word= 0" and "if (waiters)". > > This commit reverts the incorrect PowerPC patch, and instead implements a fix > for PowerPC that does not change i386/amd64 behaviour, making PowerPC work > similarly to i386/amd64. ...skip... > === modified file 'storage/xtradb/include/os0sync.h' > --- a/storage/xtradb/include/os0sync.h 2014-09-08 15:10:48 +0000 > +++ b/storage/xtradb/include/os0sync.h 2014-11-19 12:56:46 +0000 > @@ -314,11 +331,28 @@ amount of increment. */ > /**********************************************************//** > Returns the old value of *ptr, atomically sets *ptr to new_val */ > > -# define os_atomic_test_and_set_byte(ptr, new_val) \ > +#ifdef __powerpc__ > +/* > + 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 > +/* > + On x86, __sync_lock_test_and_set() happens to be full barrier, due to > + LOCK prefix. > +*/ > +# define os_atomic_test_and_set_byte_release(ptr, new_val) \ > + __sync_lock_test_and_set(ptr, (byte) new_val) > +#endif JFYI: "os_atomic_test_and_set_byte_release" sounds odd to me. Look at gcc manual: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html ... This builtin is not a full barrier, but rather an acquire barrier. This means that references after the builtin cannot move to (or be speculated to) before the builtin, but previous memory stores may not be globally visible yet, and previous memory loads may not yet be satisfied. ... You name RELEASE something that is acqually ACQUIRE. So why didn't you like "os_atomic_lock_release_byte" which is RELASE btw: ... This builtin is not a full barrier, but rather a release barrier. This means that all previous memory stores are globally visible, and all previous memory loads have been satisfied, but following memory reads are not prevented from being speculated to before the barrier. ... > +/* > + os_atomic_test_and_set_byte_acquire() is a full memory barrier on x86. But > + in general, just an aquire barrier should be sufficient. */ > +# define os_atomic_test_and_set_byte_acquire(ptr, new_val) \ > __sync_lock_test_and_set(ptr, (byte) new_val) !!! The problem is here!!! Strictly speaking not here, but in mutex_spin_wait(): ... mutex_set_waiters(mutex, 1); /* Try to reserve still a few times */ for (i = 0; i < 4; i++) { if (mutex_test_and_set(mutex) == 0) { ... We'll get stuck if "waiters= 1" will become visible after "if (mutex_test_and_set(mutex))". I don't see anything preventing this reorder. ...skip... > === modified file 'storage/xtradb/include/sync0sync.ic' > --- a/storage/xtradb/include/sync0sync.ic 2014-08-29 12:02:46 +0000 > +++ b/storage/xtradb/include/sync0sync.ic 2014-11-19 12:56:46 +0000 > @@ -80,11 +80,11 @@ mutex_test_and_set( > mutex_t* mutex) /*!< in: mutex */ > { > #if defined(HAVE_ATOMIC_BUILTINS) > - return(os_atomic_test_and_set_byte(&mutex->lock_word, 1)); > + return(os_atomic_test_and_set_byte_acquire(&mutex->lock_word, 1)); > #else > ibool ret; > > - ret = os_fast_mutex_trylock(&(mutex->os_fast_mutex)); > + ret = os_fast_mutex_trylock_full_barrier(&(mutex->os_fast_mutex)); > > if (ret == 0) { > /* We check that os_fast_mutex_trylock does not leak JFYI: This is probably the most funny thing. Here we're in the "#else" branch of "#if defined(HAVE_ATOMIC_BUILTINS)". E.g. if we compile with gcc and enter this branch it means: sync builtins are not avialble (gcc atomic builtins are very likely not available too). Now you say "os_fast_mutex_trylock_full_barrier" which does os_mb. But since neither sync bultins nor atomic builtins are available, os_mb is basically no-op. This may be different on other platforms, but to my knowledge memory barriers and atomic operations normally come together. Another thing: did you try compiling with this branch enabled? IIRC it didn't compile even berfore we started fixing it. ...skip... Regards, Sergey

2 2

[Maria-developers] constant propagation & LIKE
by Alexander Barkov 20 Nov '14

20 Nov '14

Hello Sergei, The attached patch fixes MDEV-7149 Constant propagation erroneously applied for LIKE All tests pass. But I'm not 100% sure about the patch. Is eq_cmp_result() proper place to fix this? If it's the right place, I can test the collation properties and return: - COND_TRUE for simple collations with one-character-to-one-weight rules, - COND_OK for the complex collations with tricky character to weight mapping. Thanks.

1 0

Re: [Maria-developers] [Commits] Rev 4358: MDEV-7113 difference between check_vcol_func_processor and check_partition_func_processor in lp:~maria-captains/maria/5.5
by Sergey Petrunia 20 Nov '14

20 Nov '14

Hi Sergei, * what about persistent virtual columns? Do we still support environment-dependent virtual functions for those? * the behavior of current solution is not deterministic: thread1> select @@time_zone; +-------------+ | @@time_zone | +-------------+ | SYSTEM | +-------------+ 1 row in set (0.00 sec) thread1> create table t2 ( a varchar(32), b int, v varchar(100) as (from_unixtime(b))); Query OK, 0 rows affected (0.28 sec) thread1> insert into t2 (a,b) values ('local-tz-23-11', unix_timestamp()); Query OK, 1 row affected (0.04 sec) thread1> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 23:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) thread2> set time_zone='Europe/Helsinki'; Query OK, 0 rows affected (0.00 sec) thread2> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 23:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) thread2> flush tables; Query OK, 0 rows affected (0.00 sec) thread2> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 22:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) Is this really ok? Maybe, it makes sense to use some some "default" (e.g. global variables) environment on the non-deterministic virtual column functions? Or, make these functions always fetch the environment from the current thread? On Mon, Nov 17, 2014 at 09:43:46AM +0100, Sergei Golubchik wrote: > At lp:~maria-captains/maria/5.5 > > ------------------------------------------------------------ > revno: 4358 > revision-id: sergii(a)pisem.net-20141115155753-rx5twf6utu1isczh > parent: sergii(a)pisem.net-20141113173729-hirl67og6at3xbni > fixes bugs: https://mariadb.atlassian.net/browse/MDEV-6789 https://mariadb.atlassian.net/browse/MDEV-7113 > committer: Sergei Golubchik <sergii(a)pisem.net> > branch nick: 5.5 > timestamp: Sat 2014-11-15 16:57:53 +0100 > message: > MDEV-7113 difference between check_vcol_func_processor and check_partition_func_processor > MDEV-6789 segfault in Item_func_from_unixtime::get_date on updating table with virtual columns > > * prohibit VALUES in partitioning expression > * prohibit user and system variables in virtual column expressions > * fix Item_func_date_format to cache locale (for %M/%W to return the same as MONTHNAME/DAYNAME) > * fix Item_func_from_unixtime to cache time_zone directly, not THD (and not to crash) > * added tests for other incorrectly allowed (in vcols) functions to see that they don't crash ... BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 2

Re: [Maria-developers] [Commits] Rev 4346: MDEV-7026 - Occasional hang during startup on Power8 in lp:maria/5.5
by Kristian Nielsen 19 Nov '14

19 Nov '14

Sergey Vojtovich <svoj(a)mariadb.org> writes: > revno: 4346 > revision-id: svoj(a)mariadb.org-20141118080742-sdf93kgsgxulenvb > parent: knielsen(a)knielsen-hq.org-20141112101013-t63ayylsk08ncgs3 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 5.5 > timestamp: Tue 2014-11-18 12:07:42 +0400 > message: > MDEV-7026 - Occasional hang during startup on Power8 Right, so you requested second review ASAP, I'm trying to understand what's going on here. If my understanding is correct, this patch goes on top of current 5.5.40, not on top of the patch I reviewed previously. So there are two cases, depending on whether INNODB_RW_LOCKS_USE_ATOMICS is set or not. It is my understanding that this is "usually" set, eg. on our common x86 and powerpc linux binaries. The critical thing for the patch at hand is the memory barrier between mutex_set_waiters() and mutex_test_and_set() in the waiter, and mutex_reset_lock_word() and mutex_get_waiters() in the thread releasing the lock. A full memory barrier is needed in the waiter, to be sure that mutex_set_waiters() becomes visible to the releasing thread before mutex_test_and_set() runs. And a full barrier is needed in the releasing thread, to be sure that mutex_reset_lock_word() becomes visible before mutex_get_waiters() runs. So let's see how this is ensured. First the case of x86 with INNODB_RW_LOCKS_USE_ATOMICS set, HAVE_ATOMIC_BUILTINS, pre-5.5.40: - mutex_set_waiters() uses os_compare_and_swap_ulint() which is __sync_bool_compare_and_swap() which is full barrier. - mutex_reset_lock_word() uses os_atomic_test_and_set_byte() which is __sync_lock_test_and_set(). This is documented only as a release barrier, but is in fact a full barrier on x86 with current GCC. So in this case full barrier is there as needed. If HAVE_ATOMIC_BUILTINS is not set, then mutex_reset_lock_word() uses os_fast_mutex_unlock() which is pthread_mutex_unlock(), which is also full barrier on x86. If INNODB_RW_LOCKS_USE_ATOMICS is not set, then mutex_set_waiters() has no barriers. But then mutex_test_and_set() instead has a full barrier on x86. Either in os_atomic_test_and_set_byte() which is __sync_lock_test_and_set(), if HAVE_ATOMIC_BUILTINS is set, or in os_fast_mutex_trylock() otherwise. Both of these are full barriers on x86. So this is why things work ok on x86, apparently. Very bad code that seriously needs cleanup and proper documentation of memory barrier semantics. But it is what we have to work with in 5.5. So to make things work on PowerPC without risk of breaking x86, we could do this: - Make sure os_atomic_test_and_set_byte() is full barrier on PowerPC: #ifdef __powerpc__ /* os_atomic_test_and_set_byte() should imply a full barrier. 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(ptr, new_val) \ do { __sync_lock_test_and_set(ptr, (byte) new_val); \ __atomic_thread_fence(__ATOMIC_SEQ_CST); } while(0) #else /* On x86, __sync_lock_test_and_set() happens to be full barrier, due to LOCK prefix. */ # define os_atomic_test_and_set_byte(ptr, new_val) \ __sync_lock_test_and_set(ptr, (byte) new_val) #endif - Make sure that mutex_reset_lock_word() is a full barrier when HAVE_ATOMIC_BUILTINS is not set, by adding a memory barrier: os_fast_mutex_unlock(&(mutex->os_fast_mutex)); #ifdef __powerpc__ /* On PowerPC, pthread_mutex does not imply full barrier, so we need to add it explicitly. */ __atomic_thread_fence(__ATOMIC_SEQ_CST); #endif - For the case of neither HAVE_ATOMIC_BUILTINS nor INNODB_RW_LOCKS_USE_ATOMICS set, similarly add a full barrier in mutex_test_and_set() for PowerPC: #ifdef __powerpc__ /* On PowerPC, pthread_mutex does not imply full barrier, so we need to add it explicitly. */ __atomic_thread_fence(__ATOMIC_SEQ_CST); #endif ret = os_fast_mutex_trylock(&(mutex->os_fast_mutex)); Then in 10.1. if someone is brave enough, we can clean this up properly. Clearly define what the memory barrier semantics is, and implement it using the correct primitives that work as needed on all platforms. So now that I understand (hopefully...) how the code used to work, and how it can be fixed for PowerPC without changing existing code, let me look at the new patch ... > === modified file 'storage/innobase/include/os0sync.h' > --- a/storage/innobase/include/os0sync.h 2014-09-08 15:10:48 +0000 > +++ b/storage/innobase/include/os0sync.h 2014-11-18 08:07:42 +0000 > @@ -310,8 +310,16 @@ Returns the old value of *ptr, atomicall > # define os_atomic_test_and_set_byte(ptr, new_val) \ > __sync_lock_test_and_set(ptr, (byte) new_val) > > +#ifdef __powerpc__ > # define os_atomic_lock_release_byte(ptr) \ > __sync_lock_release(ptr) > +#else > +/* 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. */ > +# define os_atomic_lock_release_byte(ptr) \ > + __sync_lock_test_and_set(ptr, 0) > +#endif This is very confusing. You define a method with full barrier semantics, but call it something suggesting release semantics. Why not revert the original change so that mutex_reset_lock_word() uses os_atomic_test_and_set_byte()? And then fix the implementation of os_atomic_test_and_set_byte() to be full barrier on PowerPC, as I suggested above? The term "atomic" generally implies full barrier in the MySQL source code... > #elif defined(HAVE_IB_SOLARIS_ATOMICS) > > @@ -428,7 +436,7 @@ clobbered */ > # define os_rmb __atomic_thread_fence(__ATOMIC_ACQUIRE) > # define os_wmb __atomic_thread_fence(__ATOMIC_RELEASE) > #ifdef __powerpc__ > -# define os_isync __asm __volatile ("isync":::"memory") > +# define os_isync __atomic_thread_fence(__ATOMIC_SEQ_CST) > #else > #define os_isync do { } while(0) > #endif As Jonas suggested, don't call a full memory barrier "isync". And do not introduce something that is a barrier on one platform but a no-operation on another platform. But ... > === modified file 'storage/innobase/sync/sync0sync.c' > --- a/storage/innobase/sync/sync0sync.c 2014-11-03 13:43:44 +0000 > +++ b/storage/innobase/sync/sync0sync.c 2014-11-18 08:07:42 +0000 > @@ -474,10 +474,9 @@ mutex_set_waiters( > > ptr = &(mutex->waiters); > > - os_wmb; > - > *ptr = n; /* Here we assume that the write of a single > word in memory is atomic */ > + os_isync; > } With this, the semantics of how things work becomes even more complicated. Now on PowerPC the barrier is in one function, but on x86 the barrier is in another function. Why not instead do as I suggested above? Put in full barriers for PowerPC in the same places that they are in x86? ---- Maybe I should try to say if I think the patch fixes the actual bug, in case you don't want to go with my suggestions and instead keep your approach? Since it goes back to using __sync_lock_test_and_set() on x86, and since it has a full barrier in mutex_exit_func() on PowerPC, I think perhaps it does. But it is really hard to understand exactly what is going on, as you can see above I had to do a lot of reasoning to understand even why the old code worked... My advice is still to revert the original incorrect PowerPC patch and do something that is at least consistent with the existing code... (And then feel free to do a proper cleanup in 10.1). Hope this helps, - Kristian.

2 2

Re: [Maria-developers] [Commits] Rev 4346: MDEV-7026 - Occasional hang during startup on Power8 in lp:maria/5.5
by Jonas Oreland 18 Nov '14

18 Nov '14

> > Since isync is not a memory barrier and lwsync doesn't guarantee > StoreLoad > order, the only option we have is full memory barrier. > === modified file 'storage/innobase/include/os0sync.h' > --- a/storage/innobase/include/os0sync.h 2014-09-08 15:10:48 +0000 > +++ b/storage/innobase/include/os0sync.h 2014-11-18 08:07:42 +0000 > > #ifdef __powerpc__ > -# define os_isync __asm __volatile ("isync":::"memory") > +# define os_isync __atomic_thread_fence(__ATOMIC_SEQ_CST) > you have to rename to the #define too (e.g to os_mb). having a os_isync which is infact a mb is more obfuscation than anyone should have to put up with. /Jonas

2 1

Re: [Maria-developers] [Commits] Rev 4346: MDEV-7026 - Occasional hang during startup on Power8 in lp:maria/5.5
by Kristian Nielsen 17 Nov '14

17 Nov '14

Sergey Vojtovich <svoj(a)mariadb.org> writes: > revno: 4346 > revision-id: svoj(a)mariadb.org-20141112114907-thnro6e3kg2ofdsw > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: Wed 2014-11-12 15:49:07 +0400 > message: > MDEV-7026 - Occasional hang during startup on Power8 > === modified file 'storage/xtradb/sync/sync0sync.c' > --- a/storage/xtradb/sync/sync0sync.c 2014-11-03 13:43:44 +0000 > +++ b/storage/xtradb/sync/sync0sync.c 2014-11-12 11:49:07 +0000 > @@ -467,26 +467,15 @@ mutex_set_waiters( > mutex_t* mutex, /*!< in: mutex */ > ulint n) /*!< in: value to set */ > { > -#ifdef INNODB_RW_LOCKS_USE_ATOMICS > - ut_ad(mutex); > - > - if (n) { > - os_compare_and_swap_ulint(&mutex->waiters, 0, 1); > - } else { > - os_compare_and_swap_ulint(&mutex->waiters, 1, 0); > - } > -#else > volatile ulint* ptr; /* declared volatile to ensure that > the value is stored to memory */ > ut_ad(mutex); > > ptr = &(mutex->waiters); > > - os_wmb; > - > *ptr = n; /* Here we assume that the write of a single > word in memory is atomic */ > -#endif > + os_sync; > } This code is still substantially different from what it was in 5.5.39 and 10.0.12, right? I think we need to instead revert the code to what it was before introducing the problem. The InnoDB mutex code clearly does not work by careful design - mutex_reset_lock_word() needs a full barrier but uses what is documented by GCC as only an acquire barrier. Rather, it works by having been tweaked over the years to just happen to work (eg. GCC in fact uses a full barrier on x86). So changing this to be "correct" is very dangerous in a stable release. It is not possible to ensure correctness by testing, either. But it is not necessary to change it. Instead, leave the code as it was before, and use #ifdef __powerpc__ to ensure that it will work also on PowerPC. For example, something like this (just a sketch, I didn't check exactly how the code should look): #ifdef __powerpc__ /* InnoDB mutex implementation needs full memory barrier for this operation */ # define os_atomic_test_and_set_byte(ptr, new_val) \ do { sync(); __sync_lock_test_and_set(ptr, (byte) new_val); } while (0) #else # define os_atomic_test_and_set_byte(ptr, new_val) \ __sync_lock_test_and_set(ptr, (byte) new_val) #endif Then we leave stable releases 5.5 and 10.0 unchanged, reducing the risk of introducing another serious breakage. And we can still fix PowerPC to work correctly. And then perhaps in 10.1 we can do refactoring of the use of memory barriers in InnoDB, if we really want. (Just in case it's not obvious, yes, I _really_ have the InnoDB mutex code). Remember, the 5.5 and 10.0 minor releases appear as security updates in distros, so it is very important to minimise the risk of introducing serious regressions. > Since isync is not a memory barrier and lwsync doesn't guarantee StoreLoad > order, the only option we have is full memory barrier. It's not a problem to have a full barrier here, I think. Because at this point, the waiter has already gone through a lot of spin loops, and now it is going to take a global mutex on the sync wait array, scan the array, and go to sleep with a context switch. One less full barrier is not going to help a lot. To improve this contended case of the InnoDB mutexes, more radical changes are needed. > InnoDB/XtraDB was getting stuck during server startup. In particular (but not > limited to) while creating doublewrite buffer. The stuck during startup is kind of the minor part of the bug. The major part is stalling the server during operation. (The hang during startup was just how the problem was detected in Buildbot). Better clarify in the commit message that this fixes a bug that could cause the entire server to stall (or even hang completely if error monitor thread itself got stuck). This will help people who experienced the problem and look at the changelog. > It happens because mutex_exit() didn't wake up waiters of affected mutex. This > is kind of acceptable because error monitor thread is supposed to wake up such > waiters. But since error monitor thread hasn't been started that early, nobody > did inform waiting threads that mutex was released. I do not think the error monitor is "supposed to wake up such waiters". In fact, it seems clear that the error monitor wakeup is _not_ used, at least not in commonly used binaries. Because it just never worked - it itself needs to take mutexes, which will prevent it from doing the wakeup. So if it was needed, someone would have noticed this problem. The symptom is _very_ hard to miss, as the whole server gets stuck hard, unable to do anything. It really is not acceptable to have a mutex implementation that can miss wakeups. This can stall the entire server. Even if the error monitor resolves the stall after 1 second, that is still unacceptable for many applications. Hope this helps, - Kristian.

2 3

[Maria-developers] Review: [MDEV-7062] parser refactoring: don't store field properties in LEX
by Alexander Barkov 16 Nov '14

16 Nov '14

Hi Sergei, > Sergei Golubchik edited comment on MDEV-7062 at 11/10/14 11:40 AM: > ------------------------------------------------------------------- > > http://lists.askmonty.org/pipermail/commits/2014-November/006908.html > > {noformat} > From: serg(a)mariadb.org > To: commits(a)mariadb.org > Subject: [Commits] 2c904b9: parser cleanup: don't store field properties > in LEX, use Create_field directly > Date: Mon, 10 Nov 2014 10:26:48 +0100 > {noformat} > Please find my comments below: > > [Commits] 2c904b9: parser cleanup: don't store field properties in LEX, use Create_field directly > serg at mariadb.org serg at mariadb.org > Mon Nov 10 11:26:48 EET 2014 > > Previous message: [Commits] Rev 4344: MDEV-6179: dynamic columns functions/cast()/convert() doesn't play nice with CREATE/ALTER TABLE in lp:~maria-captains/maria/5.5 > Next message: [Commits] Rev 4345: MDEV-6862 "#error <my_config.h>" and third-party libraries in lp:~maria-captains/maria/5.5 > Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] > > revision-id: 2c904b937b5ef7fbec60ebb9cb514a501c5bd8e4 > parent(s): f59588b9112965849986c45fcd150d9abed74063 > committer: Sergei Golubchik > branch nick: maria > timestamp: 2014-11-09 22:33:17 +0100 > message: > > parser cleanup: don't store field properties in LEX, use Create_field directly > > length/dec/charset are still in LEX, because they're also used > for CAST and dynamic columns. > > also > 1. fix "MDEV-7041 COLLATION(CAST('a' AS CHAR BINARY)) returns a wrong result" > 2. allow BINARY modifier in stored function RETURN clause > 3. allow "COLLATION without CHARSET" in SP/SF (parameters, RETURN, DECLARE) > 4. print correct variable name in error messages for stored routine parameters A very nice clean up. I only found it a little bit not easy to track that all Lex and Create_field attributes get initialized and copied to each other properly. I suggest to go a little bit further and get rid from Lex->dec, Lex->length and Lex->charset at all, and write the attributes directly to the destination as soon as they get parsed. See a proposed addon patch attached. > diff --git a/mysql-test/r/cast.result b/mysql-test/r/cast.result > index c81af13..8ad4568 100644 > --- a/mysql-test/r/cast.result > +++ b/mysql-test/r/cast.result > @@ -796,3 +796,32 @@ DATE("foo") > NULL > Warnings: > Warning 1292 Incorrect datetime value: 'foo' > +create table t1 (a int, b char(5) as (cast("a" as char(10) binary) + a) ); > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL, > + `b` char(5) AS (cast("a" as char(10) binary) + a) VIRTUAL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +drop table t1; > +select collation(cast("a" as char(10) binary)); > +collation(cast("a" as char(10) binary)) > +latin1_bin > +select collation(cast("a" as char(10) charset utf8 binary)); > +collation(cast("a" as char(10) charset utf8 binary)) > +utf8_bin > +select collation(cast("a" as char(10) ascii binary)); > +collation(cast("a" as char(10) ascii binary)) > +latin1_bin > +select collation(cast("a" as char(10) unicode binary)); > +collation(cast("a" as char(10) unicode binary)) > +ucs2_bin UNICODE is an alias for UCS2, which is not always compiled. Please move ucs2 related tests to ctype_ucs.test. <skip> > diff --git a/mysql-test/t/cast.test b/mysql-test/t/cast.test > index b6c37ca..aa43ee6 100644 > --- a/mysql-test/t/cast.test > +++ b/mysql-test/t/cast.test > @@ -456,3 +456,21 @@ SELECT CAST(TIME('10:20:30') AS DATE) + INTERVAL 1 DAY; > SET SQL_MODE=ALLOW_INVALID_DATES; > SELECT DATE("foo"); > > +# > +# CAST and field definition using same fields in LEX > +# > +create table t1 (a int, b char(5) as (cast("a" as char(10) binary) + a) ); > +show create table t1; > +drop table t1; > + > +# > +# CAST (... BINARY) > +# > +select collation(cast("a" as char(10) binary)); > +select collation(cast("a" as char(10) charset utf8 binary)); > +select collation(cast("a" as char(10) ascii binary)); > +select collation(cast("a" as char(10) unicode binary)); > +select collation(cast("a" as char(10) binary charset utf8)); > +select collation(cast("a" as char(10) binary ascii)); > +select collation(cast("a" as char(10) binary unicode)); More UCS2. You removed this code in field.cc: > - if (fld_length != NULL) > - { > - errno= 0; > - length= strtoul(fld_length, NULL, 10); > - if ((errno != 0) || (length > MAX_FIELD_BLOBLENGTH)) > - { > - my_error(ER_TOO_BIG_DISPLAYWIDTH, MYF(0), fld_name, MAX_FIELD_BLOBLENGTH); > - DBUG_RETURN(TRUE); > - } > - > - if (length == 0) > - fld_length= NULL; /* purecov: inspected */ > - } and added this in sql_yacc.yy: > + if (length) > + { > + int err; > + last_field->length= (ulong)my_strtoll10(length, NULL, &err); > + if (err) > + last_field->length= ~0UL; // safety > + } > + else > + last_field->length= 0; I have a feeling that on a 32bit platforms it will stop sending errors on attempt to create a blob larger that 4Gb. last_field->length is ulong, which is 32bit is a 32 bit platform, and it gets assigned to a 64 bit value.

2 2

[Maria-developers] Please review: sql_yacc.yy and HA_CREATE_INFO refactoring (pre-requisite for MDEV-5359 CREATE OR REPLACE, CREATE IF NOT EXISTS, DROP IF EXISTS)
by Alexander Barkov 14 Nov '14

14 Nov '14

Hi Sergei, The patch is attached. Thanks.

1 1

Re: [Maria-developers] [JIRA] (MDEV-7076) Build fails around libgroonga on some platforms in buildbot
by Alexander Barkov 12 Nov '14

12 Nov '14

Hello Kentoku, Mroonga fails to compile on some platforms. Can you please have a look? Thanks. On 11/12/2014 04:39 PM, Alexander Barkov (JIRA) wrote: > > [ https://mariadb.atlassian.net/browse/MDEV-7076?page=com.atlassian.jira.plug… ] > > Alexander Barkov updated MDEV-7076: > ----------------------------------- > Assignee: Kentoku (was: Alexander Barkov) > >> Build fails around libgroonga on some platforms in buildbot >> ----------------------------------------------------------- >> >> Key: MDEV-7076 >> URL: https://mariadb.atlassian.net/browse/MDEV-7076 >> Project: MariaDB Server >> Issue Type: Bug >> Components: OTHER >> Affects Versions: 10.0 >> Reporter: Elena Stepanova >> Assignee: Kentoku >> Priority: Critical >> Labels: buildbot, compile >> Fix For: 10.0.15 >> >> >> http://buildbot.askmonty.org/buildbot/builders/kvm-bintar-centos5-amd64/bui… >> http://buildbot.askmonty.org/buildbot/builders/kvm-deb-debian6-amd64/builds… >> http://buildbot.askmonty.org/buildbot/builders/kvm-deb-lucid-amd64/builds/4… >> It looks approximately like this, with some variations >> {noformat} >> standard input}: Assembler messages: >> {standard input}:28252: Error: open CFI at the end of file; missing .cfi_endproc directive >> gcc-4.4.real: Internal error: Killed (program cc1) >> Please submit a full bug report. >> See <file:///usr/share/doc/gcc-4.4/README.Bugs> for instructions. >> make[3]: *** [storage/mroonga/vendor/groonga/lib/CMakeFiles/libgroonga.dir/expr.c.o] Error 1 >> make[3]: *** Waiting for unfinished jobs.... >> {noformat} >> Given the list of platforms, I guess they all feature pretty old compilers, maybe that's the distinction. > > > > -- > This message was sent by Atlassian JIRA > (v6.4-OD-09-008#64005) >

1 0

[Maria-developers] Mroonga: wrapper.version_56_or_later_performance_schema is failing on BuildBot
by Alexander Barkov 12 Nov '14

12 Nov '14

Hello Kentoku, This test is failing in the latest builds on BB: mroonga/wrapper.version_56_or_later_performance_schema http://buildbot.askmonty.org/buildbot/builders/bld-dan-release/builds/1793/… Can you please check it? Thanks.

2 1