September 2011 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] Rev 3020: MWL#192: Non-blocking client API for libmysqlclient. in http://bazaar.launchpad.net/~maria-captains/maria/5.2
by Michael Widenius 09 Jan '12

09 Jan '12

Hi! >>>>> "knielsen" == knielsen <knielsen(a)knielsen-hq.org> writes: knielsen> At http://bazaar.launchpad.net/~maria-captains/maria/5.2 knielsen> ------------------------------------------------------------ knielsen> revno: 3020 knielsen> revision-id: knielsen(a)knielsen-hq.org-20110920104925-huxke80yuq1zrvcx knielsen> parent: monty(a)askmonty.org-20110816160131-4kju8si1nqqnohxg knielsen> committer: knielsen(a)knielsen-hq.org knielsen> branch nick: tmp2 knielsen> timestamp: Tue 2011-09-20 12:49:25 +0200 knielsen> message: knielsen> MWL#192: Non-blocking client API for libmysqlclient. knielsen> All client functions that can block on I/O have alternate _start() and knielsen> _cont() versions that do not block but return control back to the knielsen> application, which can then issue I/O wait in its own fashion and later knielsen> call back into the library to continue the operation. knielsen> Works behind the scenes by spawning a co-routine/fiber to run the knielsen> blocking operation and suspend it while waiting for I/O. This knielsen> co-routine/fiber use is invisible to applications. knielsen> For i368/x86_64 on GCC, uses very fast assembler co-routine support. On knielsen> Windows uses native Win32 Fibers. Falls back to POSIX ucontext on other knielsen> platforms. Assembler routines for more platforms are relatively easy to knielsen> add by extending mysys/my_context.c, eg. similar to the Lua lcoco knielsen> library. knielsen> For testing, mysqltest and mysql_client_test are extended with the knielsen> option --non-blocking-api. This causes the programs to use the knielsen> non-blocking API for database access. mysql-test-run.pl has a similar knielsen> option --non-blocking-api that uses this, as well as additional knielsen> testcases. knielsen> An example program tests/async_queries.c is included that uses the new knielsen> non-blocking API with libevent to show how, in a single-threaded knielsen> program, to issue many queries in parallel against a database. <cut> +++ client/async_example.c 2011-09-20 10:49:25 +0000 @@ -0,0 +1,207 @@ +/* + Copyright 2011 Kristian Nielsen and Monty Program Ab. + + Experiments with non-blocking libmysql. + + This 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, either version 2 of the License, or + (at your option) any later version. + + This 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. If not, see <http://www.gnu.org/licenses/>. +*/ Please change license to LGPL for this and the other new files (so we can add them to our new LGPL client library) + + +#ifndef __WIN__ +#include <poll.h> +#else +#include <WinSock2.h> +#endif You don't need to incldue Winsock2.h; This is automaticly included by mysql.h. +#include <stdlib.h> +#include <stdio.h> +#include <mysql.h> <cut> === modified file 'client/mysqltest.cc' --- client/mysqltest.cc 2011-05-18 13:17:26 +0000 +++ client/mysqltest.cc 2011-09-20 10:49:25 +0000 @@ -60,6 +60,12 @@ #define SIGNAL_FMT "signal %d" #endif === modified file 'include/Makefile.am' --- include/Makefile.am 2011-05-03 16:10:10 +0000 +++ include/Makefile.am 2011-09-20 10:49:25 +0000 @@ -46,7 +46,7 @@ atomic/rwlock.h atomic/x86-gcc.h \ atomic/generic-msvc.h \ atomic/gcc_builtins.h my_libwrap.h my_stacktrace.h \ - wqueue.h waiting_threads.h + wqueue.h waiting_threads.h my_context.h EXTRA_DIST = mysql.h.pp mysql/plugin_auth.h.pp mysql/client_plugin.h.pp CMakeLists.txt === added file 'include/my_context.h' --- include/my_context.h 1970-01-01 00:00:00 +0000 +++ include/my_context.h 2011-09-20 10:49:25 +0000 @@ -0,0 +1,222 @@ +/* + Copyright 2011 Kristian Nielsen + + Experiments with non-blocking libmysql. + + This 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, either version 2 of the License, or + (at your option) any later version. + + This 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. If not, see <http://www.gnu.org/licenses/>. +*/ + +/* + Simple API for spawning a co-routine, to be used for async libmysqlclient. + + Idea is that by implementing this interface using whatever facilities are + available for given platform, we can use the same code for the generic + libmysqlclient-async code. + + (This particular implementation uses Posix ucontext swapcontext().) +*/ + +#ifdef __WIN__ +#define MY_CONTEXT_USE_WIN32_FIBERS 1 +#elif defined(__GNUC__) && __GNUC__ >= 3 && defined(__x86_64__) +#define MY_CONTEXT_USE_X86_64_GCC_ASM +#elif defined(__GNUC__) && __GNUC__ >= 3 && defined(__i386__) +#define MY_CONTEXT_USE_I386_GCC_ASM +#else +#define MY_CONTEXT_USE_UCONTEXT +#endif + +#ifdef MY_CONTEXT_USE_WIN32_FIBERS +struct my_context { + void (*user_func)(void *); + void *user_arg; + void *app_fiber; + void *lib_fiber; + int return_value; +#ifndef DBUG_OFF + void *dbug_state; +#endif +}; +#endif + + +#ifdef MY_CONTEXT_USE_UCONTEXT +#include <ucontext.h> + +struct my_context { + void (*user_func)(void *); + void *user_data; + void *stack; + size_t stack_size; + ucontext_t base_context; + ucontext_t spawned_context; + int active; +#ifdef HAVE_VALGRIND_VALGRIND_H + unsigned int valgrind_stack_id; +#endif +#ifndef DBUG_OFF + void *dbug_state; +#endif +}; +#endif + + +#ifdef MY_CONTEXT_USE_X86_64_GCC_ASM +#include <stdint.h> + +struct my_context { + uint64_t save[9]; + void *stack_top; + void *stack_bot; +#ifdef HAVE_VALGRIND_VALGRIND_H + unsigned int valgrind_stack_id; +#endif +#ifndef DBUG_OFF + void *dbug_state; +#endif +}; +#endif + + +#ifdef MY_CONTEXT_USE_I386_GCC_ASM +#include <stdint.h> + +struct my_context { + uint64_t save[7]; + void *stack_top; + void *stack_bot; +#ifdef HAVE_VALGRIND_VALGRIND_H + unsigned int valgrind_stack_id; +#endif +#ifndef DBUG_OFF + void *dbug_state; +#endif +}; +#endif + + +/* + Initialize an asynchroneous context object. + Returns 0 on success, non-zero on failure. +*/ +extern int my_context_init(struct my_context *c, size_t stack_size); + +/* Free an asynchroneous context object, deallocating any resources used. */ +extern void my_context_destroy(struct my_context *c); + +/* + Spawn an asynchroneous context. The context will run the supplied user + function, passing the supplied user data pointer. + + The context must have been initialised with my_context_init() prior to + this call. + + The user function may call my_context_yield(), which will cause this + function to return 1. Then later my_context_continue() may be called, which + will resume the asynchroneous context by returning from the previous + my_context_yield() call. + + When the user function returns, this function returns 0. + + In case of error, -1 is returned. +*/ +extern int my_context_spawn(struct my_context *c, void (*f)(void *), void *d); + +/* + Suspend an asynchroneous context started with my_context_spawn. + + When my_context_yield() is called, execution immediately returns from the + last my_context_spawn() or my_context_continue() call. Then when later + my_context_continue() is called, execution resumes by returning from this + my_context_yield() call. + + Returns 0 if ok, -1 in case of error. +*/ +extern int my_context_yield(struct my_context *c); + +/* + Resume an asynchroneous context. The context was spawned by + my_context_spawn(), and later suspended inside my_context_yield(). + + The asynchroneous context may be repeatedly suspended with + my_context_yield() and resumed with my_context_continue(). + + Each time it is suspended, this function returns 1. When the originally + spawned user function returns, this function returns 0. + + In case of error, -1 is returned. +*/ +extern int my_context_continue(struct my_context *c); + + +struct mysql_async_context { + /* + This is set to the value that should be returned from foo_start() or + foo_cont() when a call is suspended. + It is also set to the event(s) that triggered when a suspended call is + resumed, eg. whether we woke up due to connection completed or timeout + in mysql_real_connect_cont(). + */ + unsigned int ret_status; + /* + This is set to the result of the whole asynchronous operation when it + completes. It uses a union, as different calls have different return + types. + */ + union { + void *r_ptr; + const void *r_const_ptr; + int r_int; + my_bool r_my_bool; + } ret_result; + /* + The timeout value, for suspended calls that need to wake up on a timeout + (eg. mysql_real_connect_start(). + */ + unsigned int timeout_value; + /* + This flag is set when we are executing inside some asynchronous call + foo_start() or foo_cont(). It is used to decide whether to use the + synchronous or asynchronous version of calls that may block such as + recv(). + + Note that this flag is not set when a call is suspended, eg. after + returning from foo_start() and before re-entering foo_cont(). + */ + my_bool active; + /* + This flag is set when an asynchronous operation is in progress, but + suspended. Ie. it is set when foo_start() or foo_cont() returns because + the operation needs to block, suspending the operation. + + It is used to give an error (rather than crash) if the application + attempts to call some foo_cont() method when no suspended operation foo is + in progress. + */ + my_bool suspended; + /* + If non-NULL, this is a pointer to a callback hook that will be invoked with + the user data argument just before the context is suspended, and just after + it is resumed. + */ + void (*suspend_resume_hook)(my_bool suspend, void *user_data); + void *suspend_resume_hook_user_data; + /* + This is used to save the execution contexts so that we can suspend an + operation and switch back to the application context, to resume the + suspended context later when the application re-invokes us with + foo_cont(). + */ + struct my_context async_context; +}; === modified file 'include/my_dbug.h' --- include/my_dbug.h 2011-02-20 16:51:43 +0000 +++ include/my_dbug.h 2011-09-20 10:49:25 +0000 @@ -83,6 +83,8 @@ extern void _db_unlock_file_(void); extern FILE *_db_fp_(void); extern void _db_flush_(); +extern void dbug_swap_code_state(void **code_state_store); +extern void dbug_free_code_state(void **code_state_store); Please define a macros DBUG_FREE_CODE_STATE / DBUG_SWAP_CODE_STATE that are mapped to the aboe if DBUG is defined and to "do {} while (0)" otherwise. This allows you to replace: #ifndef DBUG_OFF dbug_free_code_state(&c->dbug_state); #endif with DBUG_FREE_CODE_STATE(&c->dbug_state); (It's always nice to not have a lot of #ifdef in the code). +++ include/mysql.h 2011-09-20 10:49:25 +0000 void *extension; } MYSQL_PARAMETERS; +/* + Flag bits, the asynchronous methods return a combination of these ORed + together to let the application know when to resume the suspended operation. +*/ +typedef enum { + MYSQL_WAIT_READ= 1, /* Wait for data to be available on socket to read */ + /* mysql_get_socket_fd() will return socket descriptor*/ + MYSQL_WAIT_WRITE= 2, /* Wait for socket to be ready to write data */ + MYSQL_WAIT_EXCEPT= 4, /* Wait for select() to mark exception on socket */ + MYSQL_WAIT_TIMEOUT= 8 /* Wait until timeout occurs. Value of timeout can be */ + /* obtained from mysql_get_timeout_value() */ +} MYSQL_ASYNC_STATUS; + Why have enum's instead of defines ? (As these are bits and you are not going to use 'switch' on these, defines makes more sense). #ifdef USE_OLD_FUNCTIONS MYSQL * STDCALL mysql_connect(MYSQL *mysql, const char *host, const char *user, const char *passwd); +int STDCALL mysql_connect_start(MYSQL **ret, MYSQL *mysql, + const char *host, const char *user, + const char *passwd); +int STDCALL mysql_connect_cont(MYSQL **ret, MYSQL *mysql, + int status); int STDCALL mysql_create_db(MYSQL *mysql, const char *DB); +int STDCALL mysql_create_db_start(int *ret, MYSQL *mysql, + const char *DB); +int STDCALL mysql_create_db_cont(int *ret, MYSQL *mysql, + int status); +int STDCALL mysql_drop_db(MYSQL *mysql, const char *DB); +int STDCALL mysql_drop_db_start(int *ret, MYSQL *mysql, + const char *DB); +int STDCALL mysql_drop_db_cont(int *ret, MYSQL *mysql, int status); int STDCALL mysql_drop_db(MYSQL *mysql, const char *DB); #define mysql_reload(mysql) mysql_refresh((mysql),REFRESH_GRANT) #endif If USE_OLD_FUNCTIONS is defined, you don't have to provide the new api. These are deprecated functions that we don't want people to use so there is no reason to provide 'better' versions of them... === modified file 'include/sql_common.h' --- include/sql_common.h 2010-04-01 09:04:26 +0000 +++ include/sql_common.h 2011-09-20 10:49:25 +0000 @@ -26,11 +26,18 @@ extern const char *cant_connect_sqlstate; extern const char *not_error_sqlstate; + +struct mysql_async_context; + struct st_mysql_options_extention { char *plugin_dir; char *default_auth; }; +struct st_mysql_extension { + struct mysql_async_context *async_context; +}; + Why provide 'st_mysql_extension' in a public file? Isn't it enough to have this only in a local file in the client directory? (We don't want anyone to access this structure in any clients) === modified file 'include/violite.h' --- include/violite.h 2010-01-29 10:42:31 +0000 +++ include/violite.h 2011-09-20 10:49:25 +0000 @@ -194,6 +194,8 @@ char *read_pos; /* start of unfetched data in the read buffer */ char *read_end; /* end of unfetched data */ + struct mysql_async_context *async_context; /* For non-blocking API */ + uint read_timeout, write_timeout; /* function pointers. They are similar for socket/SSL/whatever */ void (*viodelete)(Vio*); int (*vioerrno)(Vio*); Hm, I haven't thought about that it's safe to add new things to struct st_vio without breaking the interface. That's good and useful to know! <cut> <cut> === added file 'mysql-test/t/non_blocking_api.test' --- mysql-test/t/non_blocking_api.test 1970-01-01 00:00:00 +0000 +++ mysql-test/t/non_blocking_api.test 2011-09-20 10:49:25 +0000 @@ -0,0 +1,18 @@ +# Test mixing the use of blocking and non-blocking API in a single connection. + +--enable_non_blocking_api +connect (con_nonblock,localhost,root,,test); +--disable_non_blocking_api +connect (con_normal,localhost,root,,test); + Usually all test starts with drop table if exists t1; This is mostly useful when you want to run the tests against an external server. Not critical but good practice to do... <cut> === added file 'mysys/my_context.c' --- mysys/my_context.c 1970-01-01 00:00:00 +0000 +++ mysys/my_context.c 2011-09-20 10:49:25 +0000 @@ -0,0 +1,749 @@ +/* + Copyright 2011 Kristian Nielsen + + Experiments with non-blocking libmysql. + Add the information text after the copyright. (Is the comment still relevant?) + This 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, either version 2 of the License, or + (at your option) any later version. <cut> + Implementation of async context spawning using Posix ucontext and + swapcontext(). +*/ + +#include <stdio.h> +#include <errno.h> + You can remove the above as mysys_priv.h will include these anyway. +#include "mysys_priv.h" +#include "my_context.h" + +#ifdef HAVE_VALGRIND_VALGRIND_H +#include <valgrind/valgrind.h> +#endif Shouldn't this also be depending on the HAVE_valgrind define? <cut> +int +my_context_init(struct my_context *c, size_t stack_size) +{ + if (2*sizeof(int) < sizeof(void *)) + { + fprintf(stderr, + "Error: Unable to store pointer in 2 ints on this architecture\n"); + return -1; + } Please make the above a compile time error instead. #if SIZEOF_CHARP < SIZEOF_INT*2 #error Error: Unable to store pointer in 2 ints on this architecture #endif I think it would be a good idea to do here: bzero(*c, sizeof(*c)); This is better than assuming that 'c' is zeroed on entry. + if (!(c->stack= malloc(stack_size))) + return -1; /* Out of memory */ + c->stack_size= stack_size; +#ifdef HAVE_VALGRIND_VALGRIND_H + c->valgrind_stack_id= + VALGRIND_STACK_REGISTER(c->stack, ((unsigned char *)(c->stack))+stack_size); +#endif +#ifndef DBUG_OFF + c->dbug_state= NULL; +#endif If we do the bzero, you can remove the above + return 0; +} <cut> +int +my_context_init(struct my_context *c, size_t stack_size) +{ + if (!(c->stack_bot= malloc(stack_size))) + return -1; /* Out of memory */ Add bzero of 'c' here. <cut> +int +my_context_init(struct my_context *c, size_t stack_size) +{ Add bzero() here. <cut> +int +my_context_init(struct my_context *c, size_t stack_size) +{ Add bzero here. +#ifndef DBUG_OFF + c->dbug_state= NULL; +#endif + c->lib_fiber= CreateFiber(stack_size, my_context_trampoline, c); + if (c->lib_fiber) + return 0; + else + return -1; Remove else or do: return c->lib_fiber ? 0 : -1; +} <cut> === modified file 'sql-common/client.c' --- sql-common/client.c 2011-03-18 15:03:43 +0000 +++ sql-common/client.c 2011-09-20 10:49:25 +0000 @@ -108,6 +108,7 @@ #include "client_settings.h" #include <sql_common.h> #include <mysql/client_plugin.h> +#include "my_context.h" Should probably be: #include <my_context.h> +#define mysql_extension_get(MYSQL, X) \ + ((MYSQL)->extension ? (MYSQL)->extension->X : NULL) +#define mysql_extension_set(MYSQL, X, VAL) \ + if (!(MYSQL)->extension) \ + (MYSQL)->extension= (struct st_mysql_extension *) \ + my_malloc(sizeof(struct st_mysql_extension), \ + MYF(MY_WME | MY_ZEROFILL)); \ + (MYSQL)->extension->X= VAL; + In 5.3 and 5.5 we have the following macro: #define extension_set(OPTS, X, VAL) \ if (!(OPTS)->extension) \ (OPTS)->extension= (struct st_mysql_options_extention *) \ my_malloc(sizeof(struct st_mysql_options_extention), \ MYF(MY_WME | MY_ZEROFILL)); \ (OPTS)->extension->X= VAL; Why not also store your new extension in mysql->option->extension instead of mysql->extensions ? This would allow you to reuse the existing macros.. +/* + Fetch the context for asynchronous API calls, allocating a new one if + necessary. +*/ +#define STACK_SIZE (4096*15) + +struct mysql_async_context * +mysql_get_async_context(MYSQL *mysql) +{ + struct mysql_async_context *b; + if ((b= mysql_extension_get(mysql, async_context))) + return b; + + if (!(b= (struct mysql_async_context *) + my_malloc(sizeof(*b), MYF(MY_ZEROFILL)))) + { + set_mysql_error(mysql, CR_OUT_OF_MEMORY, unknown_sqlstate); + return NULL; + } Remove the MY_ZEROFILL and let my_context_init() do it. (Makes the function cleaner). + if (my_context_init(&b->async_context, STACK_SIZE)) + { + my_free(b, MYF(0)); + return NULL; + } <cut> /************************************************************************** Get column lengths of the current row @@ -2537,6 +2577,26 @@ } +static int +connect_sync_or_async(MYSQL *mysql, NET *net, my_socket fd, + const struct sockaddr *name, uint namelen) +{ + extern int my_connect_async(struct mysql_async_context *b, my_socket fd, + const struct sockaddr *name, uint namelen, + uint timeout); Please move the above definitioon to header file that is also read by the file the defines the function. (Otherwise someone can change the function without you getting a compiler error). + struct mysql_async_context *actxt= mysql_extension_get(mysql, async_context); + + if (actxt && actxt->active) + { + my_bool old_mode; + vio_blocking(net->vio, FALSE, &old_mode); + return my_connect_async(actxt, fd, name, namelen, + mysql->options.connect_timeout); + } + else Remove else + return my_connect(fd, name, namelen, mysql->options.connect_timeout); +} + <cut> my_bool mysql_reconnect(MYSQL *mysql) { <cut> + if ((ctxt= mysql_extension_get(mysql, async_context)) && ctxt->active) + { + hook_data.orig_mysql= mysql; + hook_data.new_mysql= &tmp_mysql; + hook_data.orig_vio= mysql->net.vio; + my_context_install_suspend_resume_hook(ctxt, my_suspend_hook, &hook_data); + } if (!mysql_real_connect(&tmp_mysql,mysql->host,mysql->user,mysql->passwd, mysql->db, mysql->port, mysql->unix_socket, mysql->client_flag | CLIENT_REMEMBER_OPTIONS)) { + if (ctxt) + my_context_install_suspend_resume_hook(ctxt, NULL, NULL); Change the above to: res= mysql_real_connect(&tmp_mysql,mysql->host,mysql->user,mysql->passwd, mysql->db, mysql->port, mysql->unix_socket, mysql->client_flag | CLIENT_REMEMBER_OPTIONS); if (ctxt) my_context_install_suspend_resume_hook(ctxt, NULL, NULL); if (!res) .... This way you only need to call my_context_install_suspend_resume_hook(ctxt, NULL, NULL) once. mysql->net.last_errno= tmp_mysql.net.last_errno; strmov(mysql->net.last_error, tmp_mysql.net.last_error); strmov(mysql->net.sqlstate, tmp_mysql.net.sqlstate); @@ -3109,13 +3220,18 @@ if (mysql_set_character_set(&tmp_mysql, mysql->charset->csname)) { DBUG_PRINT("error", ("mysql_set_character_set() failed")); + tmp_mysql.extension= NULL; bzero((char*) &tmp_mysql.options,sizeof(tmp_mysql.options)); mysql_close(&tmp_mysql); + if (ctxt) + my_context_install_suspend_resume_hook(ctxt, NULL, NULL); Please remove the above call. mysql->net.last_errno= tmp_mysql.net.last_errno; strmov(mysql->net.last_error, tmp_mysql.net.last_error); strmov(mysql->net.sqlstate, tmp_mysql.net.sqlstate); DBUG_RETURN(1); } + if (ctxt) + my_context_install_suspend_resume_hook(ctxt, NULL, NULL); Please remove the above call. <cut> +static void +mysql_close_free_extension(MYSQL *mysql) +{ + if (mysql->extension) + { + if (mysql->extension->async_context) + { + my_context_destroy(&mysql->extension->async_context->async_context); + my_free(mysql->extension->async_context, MYF(0)); + } + my_free(mysql->extension, MYF(0)); + mysql->extension= NULL; + } +} I still think it would be better to use mysql->option.extension as in this case you can do the above with 2 lines of code and we would not need another function. <discussions over IRC about this....> The reason for not using mysql->options.extension was only becasue mysql_real_connect() freed the options on failure. This code is not needed as they will be freed by mysql_close() anyway. So I suggest you remove the code: if (!(client_flag & CLIENT_REMEMBER_OPTIONS)) mysql_close_free_options(mysql); from mysql_real_connect() and change to use mysql->options.extension instead static void mysql_close_free(MYSQL *mysql) { my_free((uchar*) mysql->host_info,MYF(MY_ALLOW_ZERO_PTR)); @@ -3304,6 +3439,33 @@ (As some clients call this after mysql_real_connect() fails) */ +/* + mysql_close() can actually block, at least in theory, if the socket buffer + is full when sending the COM_QUIT command. + + On the other hand, the latter part of mysql_close() needs to free the stack + used for non-blocking operation of blocking stuff, so that later part can + _not_ be done non-blocking. + + Therefore, mysql_pre_close() is used to run the parts of mysql_close() that + may block. It can be called before mysql_close(), and in that case + mysql_close() is guaranteed not to need to block. +*/ Consider changing the name to 'mysql_slow_part_of_close()' and make it static if possible. +void mysql_pre_close(MYSQL *mysql) +{ + if (!mysql) + return; Remove test; impossible with current code, see belove in mysql_close. + /* If connection is still up, send a QUIT message */ + if (mysql->net.vio != 0) + { + free_old_query(mysql); + mysql->status=MYSQL_STATUS_READY; /* Force command */ + mysql->reconnect=0; + simple_command(mysql,COM_QUIT,(uchar*) 0,0,1); + end_server(mysql); /* Sets mysql->net.vio= 0 */ + } +} + void STDCALL mysql_close(MYSQL *mysql) { DBUG_ENTER("mysql_close"); @@ -3311,16 +3473,9 @@ + +my_socket STDCALL +mysql_get_socket(const MYSQL *mysql) +{ + if (mysql->net.vio) + return mysql->net.vio->sd; + else remove else + return INVALID_SOCKET; +} +++ sql-common/mysql_async.c 2011-09-20 10:49:25 +0000 +#ifdef __WIN__ +/* + Windows does not support MSG_DONTWAIT for send()/recv(). So we need to ensure + that the socket is non-blocking at the start of every operation. +*/ +#define WIN_SET_NONBLOCKING(mysql) { \ + my_bool old_mode__; \ + if ((mysql)->net.vio) vio_blocking((mysql)->net.vio, FALSE, &old_mode__); \ + } old_mode__ -> old_mode +#else +#define WIN_SET_NONBLOCKING(mysql) +#endif + +extern struct mysql_async_context *mysql_get_async_context(MYSQL *mysql); Move to header file. +/* Asynchronous connect(); socket must already be set non-blocking. */ +int +my_connect_async(struct mysql_async_context *b, my_socket fd, + const struct sockaddr *name, uint namelen, uint timeout) +{ + int res; +#ifdef __WIN__ + int s_err_size; +#else + socklen_t s_err_size; +#endif It would be nice to just define socklen_t for windows, but this isn't critical. You could have a DBUG_ASSERT here to check if socket is really in non blocking. + + /* + Start to connect asynchronously. + If this will block, we suspend the call and return control to the + application context. The application will then resume us when the socket + polls ready for write, indicating that the connection attempt completed. + */ + res= connect(fd, name, namelen); +#ifdef __WIN__ + if (res != 0) + { + int wsa_err= WSAGetLastError(); + if (wsa_err != WSAEWOULDBLOCK) + return res; +#else + if (res < 0) To not have only one { inside the #ifdef / #else I suggest that you move if (res != 0) { before the +#ifdef __WIN__. This works on both windows and Unix This will make the number of { balance + { + if (errno != EINPROGRESS && errno != EALREADY && errno != EAGAIN) + return res; +#endif + b->timeout_value= timeout; + b->ret_status= MYSQL_WAIT_WRITE | + (timeout ? MYSQL_WAIT_TIMEOUT : 0); +#ifdef __WIN__ + b->ret_status|= MYSQL_WAIT_EXCEPT; +#endif How about setting b->ret_status to 0 before res= connect() ? Then you can move the above code to the previous #ifdef __WIN__ and change the setting of b->ret_status to use |= This removes the extra ifdef __WIN__ and also ensures that ret_status is 0 if evertything went well. (not required but still better than a random value when trying to understand what's going in). + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(TRUE, b->suspend_resume_hook_user_data); + my_context_yield(&b->async_context); + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(FALSE, b->suspend_resume_hook_user_data); + if (b->ret_status & MYSQL_WAIT_TIMEOUT) + return -1; + + s_err_size= sizeof(int); Better to use (safer and more descriptive): s_err_size= sizeof(res); + if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (char*) &res, &s_err_size) != 0) + return -1; <cut> + +ssize_t +my_recv_async(struct mysql_async_context *b, int fd, + unsigned char *buf, size_t size, uint timeout) +{ + ssize_t res; + + for (;;) + { + res= recv(fd, buf, size, +#ifdef __WIN__ + 0 +#else + MSG_DONTWAIT +#endif + ); Better to do before function start: #ifdef __WIN__ #define MSG_DONTWAIT 0 #endif and remove the above #ifdef in middle of call. Another option is to use the IF_WIN() macro: IF_WIN(0, MSG_DONTWAIT) + if (res >= 0 || +#ifdef __WIN__ + WSAGetLastError() != WSAEWOULDBLOCK +#else + (errno != EAGAIN && errno != EINTR) +#endif Please define macro: #define IS_BLOCKING_ERROR IF_WIN(WSAGetLastError() != WSAEWOULDBLOCK, (errno != EAGAIN && errno != EINTR)) and use that instead of having an ifdef in the middle of the code. + ) + return res; + b->ret_status= MYSQL_WAIT_READ; + if (timeout) + { + b->ret_status|= MYSQL_WAIT_TIMEOUT; + b->timeout_value= timeout; + } + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(TRUE, b->suspend_resume_hook_user_data); + my_context_yield(&b->async_context); + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(FALSE, b->suspend_resume_hook_user_data); The following call was quite confusion as we are checking a variable that we where just setting. <phone call> The reasons is that b->ret_status is both an in an out parameter. We agree to do one of the following to make the code easier to understand: - Change to use different bits for in and out status. - Change to use different variables for in and out status. + if (b->ret_status & MYSQL_WAIT_TIMEOUT) + return -1; + } +} + Add empty row here +ssize_t +my_send_async(struct mysql_async_context *b, int fd, + const unsigned char *buf, size_t size, uint timeout) +{ + ssize_t res; + + for (;;) + { + res= send(fd, buf, size, +#ifdef __WIN__ + 0 +#else + MSG_DONTWAIT +#endif + ); + if (res >= 0 || +#ifdef __WIN__ + WSAGetLastError() != WSAEWOULDBLOCK +#else + (errno != EAGAIN && errno != EINTR) +#endif + ) See comments for previous function how to get rid of the ifdefs' <cut> +#ifdef HAVE_OPENSSL +int +my_ssl_read_async(struct mysql_async_context *b, SSL *ssl, + void *buf, int size) +{ + int res, ssl_err; + + for (;;) + { + res= SSL_read(ssl, buf, size); + if (res >= 0) + return res; + ssl_err= SSL_get_error(ssl, res); + if (ssl_err == SSL_ERROR_WANT_READ) + b->ret_status= MYSQL_WAIT_READ; + else if (ssl_err == SSL_ERROR_WANT_WRITE) + b->ret_status= MYSQL_WAIT_WRITE; + else + return res; + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(TRUE, b->suspend_resume_hook_user_data); + my_context_yield(&b->async_context); + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(FALSE, b->suspend_resume_hook_user_data); + } +} + +int +my_ssl_write_async(struct mysql_async_context *b, SSL *ssl, + const void *buf, int size) +{ + int res, ssl_err; + + for (;;) + { + res= SSL_write(ssl, buf, size); + if (res >= 0) + return res; + ssl_err= SSL_get_error(ssl, res); + if (ssl_err == SSL_ERROR_WANT_READ) + b->ret_status= MYSQL_WAIT_READ; + else if (ssl_err == SSL_ERROR_WANT_WRITE) + b->ret_status= MYSQL_WAIT_WRITE; + else + return res; + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(TRUE, b->suspend_resume_hook_user_data); + my_context_yield(&b->async_context); + if (b->suspend_resume_hook) + (*b->suspend_resume_hook)(FALSE, b->suspend_resume_hook_user_data); + } +} 90 % of the above functions are the same: I would do it this way: static my_bool my_ssl_async_check_result(int *res, struct mysql_async_context *b) { int ssl_err; if (*res >= 0) return 1; ssl_err= SSL_get_error(ssl, *res); if (ssl_err == SSL_ERROR_WANT_READ) b->ret_status= MYSQL_WAIT_READ; else if (ssl_err == SSL_ERROR_WANT_WRITE) b->ret_status= MYSQL_WAIT_WRITE; else return 1; if (b->suspend_resume_hook) (*b->suspend_resume_hook)(TRUE, b->suspend_resume_hook_user_data); my_context_yield(&b->async_context); if (b->suspend_resume_hook) (*b->suspend_resume_hook)(FALSE, b->suspend_resume_hook_user_data); return 0; } And then: my_ssl_read_async(struct mysql_async_context *b, SSL *ssl, void *buf, int size) { int res; for (;;) { res= SSL_read(ssl, buf, size); if (my_ssl_async_check_result(res, b)) return res; } } <cut> +#endif /* HAVE_OPENSSL */ + +unsigned int STDCALL +mysql_get_timeout_value(const MYSQL *mysql) +{ + if (mysql->extension && mysql->extension->async_context) + return mysql->extension->async_context->timeout_value; + else Remove else + return 0; +} + Add empty line +/* + Now create non-blocking definitions for all the calls that may block. + + Each call FOO gives rise to FOO_start() that prepares the MYSQL object for + doing non-blocking calls that can suspend operation mid-way, and then starts + the call itself. And a FOO_start_internal trampoline to assist with running + the real call in a co-routine that can be suspended. And a FOO_cont() that + can continue a suspended operation. +*/ + +#define MK_ASYNC_CALLS(call__, decl_args__, invoke_args__, cont_arg__, mysql_val__, parms_mysql_val__, parms_assign__, ret_type__, err_val__, ok_val__, extra1__) \ +static void \ +call__ ## _start_internal(void *d) \ +{ \ As discussed on IRC, it would be good to have the functions declarations outside of the macro; This makes it easier to make breakpoints in the code and help debugging. + struct call__ ## _params *parms; \ + ret_type__ ret; \ + struct mysql_async_context *b; \ + \ + parms= (struct call__ ## _params *)d; \ + b= (parms_mysql_val__)->extension->async_context; \ + \ + ret= call__ invoke_args__; \ + b->ret_result. ok_val__ = ret; \ + b->ret_status= 0; \ +} \ +int STDCALL \ +call__ ## _start decl_args__ \ +{ \ + int res; \ + struct mysql_async_context *b; \ + struct call__ ## _params parms; \ + \ + extra1__ \ + if (!(b= mysql_get_async_context((mysql_val__)))) \ + { \ + *ret= err_val__; \ + return 0; \ + } \ Instead of calling mysql_get_async_context, another option would be to do: mysql_options(mysql, MYSQL_OPT_ASYNC_IO, (char*) &STACK_SIZE) And if STACK_SIZE is 0, then we allocate our default stack size then you can change the above code to: b= mysql_val__->options.extension->async_context; If it doesn't exist, it's ok to crash on access. (Same as if mysql is 0) + parms_assign__ \ + \ + b->active= 1; \ + res= my_context_spawn(&b->async_context, call__ ## _start_internal, &parms);\ + b->active= 0; \ Set also b->suspended= 0 here. (Makes the rest of the code shorter) + if (res < 0) \ + { \ + set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate); \ How do we know the reason is out of memory? (Just curious) + b->suspended= 0; \ + *ret= err_val__; \ + return 0; \ + } \ + else if (res > 0) \ Remove else + { \ + /* Suspended. */ \ + b->suspended= 1; \ + return b->ret_status; \ + } \ + else \ Remove else and {} + { \ + /* Finished. */ \ + b->suspended= 0; \ + *ret= b->ret_result. ok_val__; \ + return 0; \ + } \ +} \ If you change to test for (res > 0) first, you can combine the two return 0 to one row and thus remove some of the { }. Makes the code shorter (at least to read): b->active= b->suspended= 0; if (res > 0) { b->suspended= 1; return b->ret_status; } if (res < 0) { set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate); *ret= err_val__; } else *ret= b->ret_result. ok_val__; /* Finished. */ return 0; +int STDCALL \ +call__ ## _cont(ret_type__ *ret, cont_arg__, int ready_status) \ +{ \ + int res; \ + struct mysql_async_context *b; \ + \ + b= (mysql_val__)->extension->async_context; \ + if (!b || !b->suspended) \ Remove test for !b; You can assume it's exists. (We can never protect aginst wrong usage) + { \ + set_mysql_error((mysql_val__), CR_COMMANDS_OUT_OF_SYNC, unknown_sqlstate);\ + *ret= err_val__; \ + return 0; \ + } \ + \ + b->active= 1; \ + b->ret_status= ready_status; \ + res= my_context_continue(&b->async_context); \ + b->active= 0; \ + if (res < 0) \ + { \ + set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate); \ + b->suspended= 0; \ + *ret= err_val__; \ + return 0; \ + } \ + else if (res > 0) \ + { \ + /* Suspended. */ \ + return b->ret_status; \ + } \ + else \ + { \ + /* Finished. */ \ + b->suspended= 0; \ + *ret= b->ret_result. ok_val__; \ + return 0; \ + } \ You can do the same 'suspend and check for error' optimization I did in previous function to reduce the above code with 4 lines. +} + +#define MK_ASYNC_CALLS_VOID_RETURN(call__, decl_args__, invoke_args__, cont_arg__, mysql_val__, parms_mysql_val__, parms_assign__, extra1__) \ +static void \ +call__ ## _start_internal(void *d) \ +{ \ + struct call__ ## _params *parms; \ + struct mysql_async_context *b; \ + \ + parms= (struct call__ ## _params *)d; \ + b= (parms_mysql_val__)->extension->async_context; \ + \ + call__ invoke_args__; \ + b->ret_status= 0; \ +} \ +int STDCALL \ +call__ ## _start decl_args__ \ +{ \ + int res; \ + struct mysql_async_context *b; \ + struct call__ ## _params parms; \ + \ + extra1__ \ + if (!(b= mysql_get_async_context((mysql_val__)))) \ + { \ + return 0; \ + } \ You can remove test if we know that b always exists. At least you can remove '{' + parms_assign__ \ + \ + b->active= 1; \ + res= my_context_spawn(&b->async_context, call__ ## _start_internal, &parms);\ + b->active= 0; \ + if (res < 0) \ + { \ + set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate); \ + b->suspended= 0; \ + return 0; \ + } \ + else if (res > 0) \ + { \ + /* Suspended. */ \ + b->suspended= 1; \ + return b->ret_status; \ + } \ + else \ + { \ + /* Finished. */ \ + b->suspended= 0; \ + return 0; \ + } \ +} \ See previous comment of how to remove 4 code lines. +int STDCALL \ +call__ ## _cont(cont_arg__, int ready_status) \ +{ \ + int res; \ + struct mysql_async_context *b; \ + \ + b= (mysql_val__)->extension->async_context; \ + if (!b || !b->suspended) \ Assume !b exists + { \ + set_mysql_error((mysql_val__), CR_COMMANDS_OUT_OF_SYNC, unknown_sqlstate);\ + return 0; \ + } \ + \ + b->active= 1; \ + b->ret_status= ready_status; \ + res= my_context_continue(&b->async_context); \ + b->active= 0; \ + if (res < 0) \ + { \ + set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate); \ + b->suspended= 0; \ + return 0; \ + } \ + else if (res > 0) \ + { \ + /* Suspended. */ \ + return b->ret_status; \ + } \ + else \ + { \ + /* Finished. */ \ + b->suspended= 0; \ + return 0; \ + } \ +} Remove extra lines above. Add comment here (and for other similar structs): /* This struct is used by the next MK_ASYNC_CALL */ + +struct mysql_real_connect_params { + MYSQL *mysql; + const char *host; + const char *user; + const char *passwd; + const char *db; + unsigned int port; + const char *unix_socket; + unsigned long client_flags; +}; +MK_ASYNC_CALLS( + mysql_real_connect, + (MYSQL **ret, MYSQL *mysql, const char *host, const char *user, + const char *passwd, const char *db, unsigned int port, + const char *unix_socket, unsigned long client_flags), + (parms->mysql, parms->host, parms->user, parms->passwd, parms->db, + parms->port, parms->unix_socket, parms->client_flags), <cut> +mysql_close_start(MYSQL *sock) +{ + int res; + + /* It is legitimate to have NULL sock argument, which will do nothing. */ + if (sock) + { + res= mysql_pre_close_start(sock); + /* If we need to block, return now and do the rest in mysql_close_cont(). */ + if (res) + return res; + } + mysql_close(sock); + return 0; +} +int STDCALL +mysql_close_cont(MYSQL *sock, int ready_status) +{ + int res; + + res= mysql_pre_close_cont(sock, ready_status); + if (res) + return res; + mysql_close(sock); + return 0; +} You should use the above mysql_close functions also in client/async_example.cc +#ifdef USE_OLD_FUNCTIONS Forget these! We should remove them soon anyway... <cut> === added file 'tests/async_queries.c' +void +add_query(const char *q) +{ + struct query_entry *e; + char *q2; + size_t len; + + e= malloc(sizeof(*e)); + q2= strdup(q); Would be better if you used my_malloc / my_strdup here. (To get things safemalloc checked). + if (!e || !q2) + fatal(NULL, "Out of memory"); + + /* Remove any trailing newline. */ + len= strlen(q2); + if (q2[len] == '\n') + q2[len--]= '\0'; + if (q2[len] == '\r') + q2[len--]= '\0'; + + e->next= NULL; + e->query= q2; + e->index= query_counter++; + *tail_ptr= e; + tail_ptr= &e->next; +} <cut> === modified file 'vio/viosocket.c' --- vio/viosocket.c 2011-05-14 16:42:07 +0000 +++ vio/viosocket.c 2011-09-20 10:49:25 +0000 @@ -21,6 +21,7 @@ */ #include "vio_priv.h" +#include "my_context.h" int vio_errno(Vio *vio __attribute__((unused))) { @@ -31,18 +32,34 @@ size_t vio_read(Vio * vio, uchar* buf, size_t size) { size_t r; + extern ssize_t my_recv_async(struct mysql_async_context *b, int fd, + unsigned char *buf, size_t size, uint timeout); Move to include file. DBUG_ENTER("vio_read"); DBUG_PRINT("enter", ("sd: %d buf: 0x%lx size: %u", vio->sd, (long) buf, (uint) size)); /* Ensure nobody uses vio_read_buff and vio_read simultaneously */ DBUG_ASSERT(vio->read_end == vio->read_pos); + if (vio->async_context && vio->async_context->active) + r= my_recv_async(vio->async_context, vio->sd, buf, size, vio->read_timeout); + else + { <cut> === modified file 'vio/viossl.c' @@ -21,6 +21,7 @@ */ #include "vio_priv.h" +#include "my_context.h" #ifdef HAVE_OPENSSL @@ -90,11 +91,16 @@ size_t vio_ssl_read(Vio *vio, uchar* buf, size_t size) { size_t r; + extern int my_ssl_read_async(struct mysql_async_context *b, SSL *ssl, + void *buf, int size); Move to extern file (Same for other extern declarations in this file). <cut> Good and intresting work! Regards, Monty

4 9

[Maria-developers] Shared plugin library build question
by Steve Ellcey 12 Nov '11

12 Nov '11

I am looking at a difference in the HP-UX vs. Linux mariadb build and was wondering if someone could help me understand why something is set up the way it is. The problem I see is that on HP-UX, when I do a mariadb build I do not get shared plugin libraries like ha_federated. Looking into why this was happening I found that when the plugin libraries like ha_federated are built they have a dependency on libsqlservices and libsqlservices is built only as an archive library. HP-UX doesn't support building shared libaries that depend on archive libraries and so the build of the shared plugin libraries fails when no shared version of libsqlservices is found. So my main question is: why is libsqlservices built only as an archive library? The other question is: if libsqlservices has to be an archive only library, is there is another way to link it in to executables, such as linking it in explicitly when building an executable instead of counting on the shared plugin libraries to bring it in indirectly by making it a dependent library? Steve Ellcey sje(a)cup.hp.com

3 29

[Maria-developers] Preparations for 5.3.2 release
by Sergey Petrunya 10 Oct '11

10 Oct '11

Hi! As discussed on the yesterday's call, we're starting to prepare for the next 5.3 release, 5.3.2 beta. The intent is to build it on this or on the next weekend. Before the release, we need to do 1. Bugs 2. optimizer_switch changes. 3. Merge from mariadb-5.1 We don't need to address: 1. Pushing Kristian's replication features. If I heard correctly, the decision was that they won't be pushed into 5.3 (please correct me if I am wrong) == Bugs == Current situation with 5.3-targeted bugs is as follows: Crashing bugs: - SergeyP: 3 + 2 non-repeatable (#860535 #860553 #861147 #860561 (non-repeatable) #860580 (non-repeatable)) - Timour: 2 (#824425 #858148) - Nobody: 1 (#859375) Wrong query result bugs: - Timour: 8 (#858038 #817966 #833777 #825051 #826150 #747278 #833702 #856152) - Igor : 2 (#823301 #791761) - Sanja: 1 (#825075) I have a slight excuse that all my crasing bugs were filed just yesterday. I'll try to fix them before the release. Another problem area is Timour's crashes + wrong query result bugs. Also, we need to assign #859375. == optimizer_switch changes == Some of the new optimizations are already sufficiently stable so that they can be made enabled by default. >From the code quality point of view, Philip's opinion is: - subquery_cache can be turned ON. - join_cache_level can be set to value higher than 1, and also various variants of BKA/Hash join may be enabled. - MRR can be enabled - index_condition_pushdown can be enabled - Materialization still has bugs, cannot be enabled. - semi-joins+materialization cannot be enabled together, there are bugs. - semi-joins without materialization cause non-repeatable crashes, so cannot be enabled yet. - FROM subqueries/derived tables optimization didn't recieve sufficient testing yet, so cannot be enabled. Note that the above is based only on the number of known crashes/wrong query result problems, performance issues are not counted. My opinion is that - subquery_cache can be enabled. - index_condition_pushdown can be enabled. - I'm hesitant to enable MRR because it may cause slowdowns for small-dataset sysbench-like tests. - perhaps, some part of hash join could be enabled (as far as I understand, its overhead is rather small, so it doesn't cause regressions?) Anybody (and especially feature "owners") have any thoughts? == Merge from MariaDB 5.1 == Monty mentioned we'll need to do it, but nobody took this task? BR Sergey -- Sergey Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog

3 2

[Maria-developers] WL#240 New (by Mdcallag): client-side compressed types
by worklog-noreply＠askmonty.org 03 Oct '11

03 Oct '11

----------------------------------------------------------------------- WORKLOG TASK -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- TASK...........: client-side compressed types CREATION DATE..: Fri, 30 Sep 2011, 20:31 SUPERVISOR.....: IMPLEMENTOR....: COPIES TO......: CATEGORY.......: Client-BackLog TASK ID........: 240 (http://askmonty.org/worklog/?tid=240) VERSION........: WorkLog-4.0 STATUS.........: Un-Assigned PRIORITY.......: 60 WORKED HOURS...: 0 ESTIMATE.......: 0 (hours remain) ORIG. ESTIMATE.: 0 PROGRESS NOTES: DESCRIPTION: Is it possible to add a variant of the blob types that is compressed on the client side? By this I mean that the client library handles compression and decompression so that the client code does not have to deal with it. The benefit from this is reduced load. There will be less network traffic between client/server and master/slave. This uses less space in the binlog, relay log, innodb transaction log and database files. The alternative is to use compression between client/server and master/slave but that doesn't help with the size of the binlog file nor with the innodb datafiles or log file. We have too many cases where huge text/blob values store uncompressed JSON and the bloat from that hurts performance. ESTIMATED WORK TIME ESTIMATED COMPLETION DATE ----------------------------------------------------------------------- WorkLog (v4.0.0)

4 4

[Maria-developers] WL#239 New (by Jaequery): ability to have multiple TIMESTAMP columns with CURRENT_TIMESTAMP / on_update TIMESTAMP
by worklog-noreply＠askmonty.org 28 Sep '11

28 Sep '11

----------------------------------------------------------------------- WORKLOG TASK -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- TASK...........: ability to have multiple TIMESTAMP columns with CURRENT_TIMESTAMP / on_update TIMESTAMP CREATION DATE..: Wed, 28 Sep 2011, 19:37 SUPERVISOR.....: IMPLEMENTOR....: COPIES TO......: CATEGORY.......: Client-BackLog TASK ID........: 239 (http://askmonty.org/worklog/?tid=239) VERSION........: Benchmarks-3.0 STATUS.........: Un-Assigned PRIORITY.......: 60 WORKED HOURS...: 0 ESTIMATE.......: 0 (hours remain) ORIG. ESTIMATE.: 0 PROGRESS NOTES: DESCRIPTION: Let's say I have two columns: 'created' and 'updated' Currently I can only let 'created' use the CURRENT_TIMESTAMP attribute. But ideally, I'd like 'updated' to also use CURRENT_TIMESTAMP as well as on_update TIMESTAMP. This makes a lot more sense for operations like this when a developer would have 'created', 'updated', or even 'deleted', columns. Only way we can do this right now is via triggers or (shudder) setting them from the app server. I understand there would be a underlying change and would really appreciate some kind of change for this particular behavior. ESTIMATED WORK TIME ESTIMATED COMPLETION DATE ----------------------------------------------------------------------- WorkLog (v4.0.0)

1 0

Re: [Maria-developers] Please review: [Commits] Rev 3199: Fix bug lp:858148. in file:///home/tsk/mprog/src/5.3/
by Igor Babaev 28 Sep '11

28 Sep '11

OK to push. Regards, Igor. On 09/28/2011 07:23 AM, Timour Katchaounov wrote: > Hi Igor, > > Could you please review the following fix. > It is very much related to a bug you fixed: > https://bugs.launchpad.net/bugs/702301 > > Thanks, > Timour > > -------- Original Message -------- > Return-Path: <commits-bounces(a)mariadb.org> > X-Original-To: timour(a)montyprogram.com > Delivered-To: timour(a)montyprogram.com > Received: from localhost (localhost.localdomain [127.0.0.1]) by > hasky.askmonty.org (Postfix) with ESMTP id DFD38D0DCB; Wed, 28 Sep > 2011 17:20:59 +0300 (EEST) > X-Virus-Scanned: Debian amavisd-new at mail.askmonty.org > Received: from hasky.askmonty.org ([127.0.0.1]) by localhost > (mail.askmonty.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP > id lpHLfsC2eNw9; Wed, 28 Sep 2011 17:20:59 +0300 (EEST) > Received: from hasky.askmonty.org (localhost.localdomain [127.0.0.1]) > by hasky.askmonty.org (Postfix) with ESMTP id BE79DC8236; Wed, 28 Sep > 2011 17:20:59 +0300 (EEST) > Received: by hasky.askmonty.org (Postfix) id 38FC0C8234; Wed, 28 Sep > 2011 17:20:58 +0300 (EEST) > Delivered-To: commits(a)mariadb.org > Received: from localhost (localhost.localdomain [127.0.0.1]) by > hasky.askmonty.org (Postfix) with ESMTP id 31622C8236 for > <commits(a)mariadb.org>; Wed, 28 Sep 2011 17:20:58 +0300 (EEST) > X-Virus-Scanned: Debian amavisd-new at mail.askmonty.org > Received: from hasky.askmonty.org ([127.0.0.1]) by localhost > (mail.askmonty.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP > id BvGNnoY8SHUx for <commits(a)mariadb.org>; Wed, 28 Sep 2011 17:20:52 > +0300 (EEST) > Received: from localhost6.localdomain6 (unknown [212.91.171.126]) > (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No > client certificate requested) by hasky.askmonty.org (Postfix) with > ESMTPSA id 24886C8234 for <commits(a)mariadb.org>; Wed, 28 Sep 2011 > 17:20:52 +0300 (EEST) > Content-Type: multipart/mixed; > boundary="===============0198351398646356273==" > MIME-Version: 1.0 > From: <timour(a)askmonty.org> > User-Agent: bzr/2.3.4 > To: <commits(a)mariadb.org> > Message-Id: <20110928142052.24886C8234(a)hasky.askmonty.org> > Date: Wed, 28 Sep 2011 17:20:52 +0300 (EEST) > Subject: [Commits] Rev 3199: Fix bug lp:858148. in > file:///home/tsk/mprog/src/5.3/ > X-BeenThere: commits(a)mariadb.org > X-Mailman-Version: 2.1.9 > Precedence: list > Reply-To: maria-developers(a)lists.launchpad.net > List-Id: MariaDB Commits List <commits.mariadb.org> > List-Unsubscribe: > <https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits>, > <mailto:commits-request@mariadb.org?subject=unsubscribe> > List-Archive: <http://lists.askmonty.org/pipermail/commits> > List-Post: <mailto:commits@mariadb.org> > List-Help: <mailto:commits-request@mariadb.org?subject=help> > List-Subscribe: > <https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits>, > <mailto:commits-request@mariadb.org?subject=subscribe> > Sender: commits-bounces(a)mariadb.org > Errors-To: commits-bounces(a)mariadb.org > > At file:///home/tsk/mprog/src/5.3/ > > ------------------------------------------------------------ > revno: 3199 > revision-id: timour(a)askmonty.org-20110928142043-b88ob3qytbroxdks > parent: psergey(a)askmonty.org-20110928090147-izz3ebqbcrnakndu > fixes bug(s): https://launchpad.net/bugs/858148 > committer: timour(a)askmonty.org > branch nick: 5.3 > timestamp: Wed 2011-09-28 17:20:43 +0300 > message: > Fix bug lp:858148. > > Analysis: > The crash is a result of the same cause as all similar > bugs (lp:827416, lp:718763, lp:778413, lp:806943, > lp:611690). The general pattern is that some optimization > requires the evaluation of some condition (e.g. the WHERE > clause), and this condition contains a subquery, such that > the subquery itself requires a temporary table for its > execution. During the subquery execution the original > tables in the FROM clause are replaced by the temporary > table needed for the final GROUP or ORDER operation. All > this happens during optimization of the outer query. Later > when EXPLAIN is run for the subquery, explain attempts to > print the name of the tables in the FROM clause, but it > finds there a temporary table without a corresponding > TABLE_LIST object. The attempt to print the name of a > NULL table list results in a crash. > > Solution: > This patch extends the fix to bug lp:702301, and dissalows > constant substitution of aggregate functions if the filter > condition used to check MIN/MAX keys is an expensive condition.

1 0

[Maria-developers] WL#238 New (by Snabb): Implement NOW() default value for DATETIME columns
by worklog-noreply＠askmonty.org 27 Sep '11

27 Sep '11

----------------------------------------------------------------------- WORKLOG TASK -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- TASK...........: Implement NOW() default value for DATETIME columns CREATION DATE..: Tue, 27 Sep 2011, 14:05 SUPERVISOR.....: IMPLEMENTOR....: COPIES TO......: CATEGORY.......: Server-RawIdeaBin TASK ID........: 238 (http://askmonty.org/worklog/?tid=238) VERSION........: Server-9.x STATUS.........: Un-Assigned PRIORITY.......: 60 WORKED HOURS...: 0 ESTIMATE.......: 0 (hours remain) ORIG. ESTIMATE.: 0 PROGRESS NOTES: DESCRIPTION: Have a look at MySQL bug #27645: http://bugs.mysql.com/bug.php?id=27645 Currently it is possible to set the default value of a TIMESTAMP column to CURRENT_TIME but this is not possible with DATETIME columns. Looking at that bug report it looks like I am not the only one who wishes to use DATETIME columns and have a way of setting their default value to the time when the row was created. Also TIMESTAMP DEFAULT CURRENT_TIME can occur only once in a table. It should be possible to have several "DATETIME DEFAULT NOW()" columns (as this is useful for having automatically initialized created_time and modified_time columns for a row, for example). Looks like MariaDB might gain some supporters by implementing this much-wanted feature. Currently the only decent work-around is to create a trigger which populates DATETIME columns before insert. (I hope this is the proper way to file a feature request.) ESTIMATED WORK TIME ESTIMATED COMPLETION DATE ----------------------------------------------------------------------- WorkLog (v4.0.0)

1 0

Re: [Maria-developers] [Commits] Rev 3192: Added new options to KILL. New syntax is KILL [HARD|SOFT] [CONNECTION|QUERY] [ID | USER user_name] in lp:maria/5.3
by Sergei Golubchik 26 Sep '11

26 Sep '11

Hi, Michael! On Sep 23, Michael Widenius wrote: > revno: 3192 > revision-id: monty(a)askmonty.org-20110922221338-wxnyyd5grsn689dy > parent: sergii(a)pisem.net-20110922090400-jkxe2vagjakr4lbi > committer: Michael Widenius <monty(a)askmonty.org> > branch nick: maria-5.3 > timestamp: Fri 2011-09-23 01:13:38 +0300 > message: > Added new options to KILL. New syntax is > KILL [HARD|SOFT] [CONNECTION|QUERY] [ID | USER user_name] > - If USER is given, all threads for that user is signaled > - If SOFT is used then the KILL will not be sent to the handler. Hm. So, this is the definition? "not sent to the handler"? I liked the older version of it "not interrupt an operation that leaves the table corrupted or unusable". This definition is easier to understand for the user, who does not necessarily know or care how every particular statement is split between the server and the engine (especially, as this is different for different engines). So, the point is - no matter how this SOFT is internally implemented (by not sending to the handler or whatever), what is the official definition of this feature? What should the manual say? Currently is says "will not send the signal to storage engines" which - see above - is suboptimal, in my opinion. > This can be used to not interrupt critical things in the handler > like 'REPAIR'. > > Internally added more kill signals. This gives us more information > of why a query/connection was killed. > - KILL_SERVER is used when server is going down. In this case the > users gets ER_SHUTDOWN as the reason connection was killed. > - Changed signals to number in correct order, which makes it easier > to test how the signal should affect the code. > - New error message ER_CONNECTION_KILLED if connection was killed by > 'KILL CONNECTION'. Before we got error ER_SHUTDOWN. > > Changed names of not used parameters KILL_QUERY & KILL_CONNCTION to > mysql_kill() to not conflict with defines in the server > === modified file 'include/mysql.h.pp' > --- a/include/mysql.h.pp 2011-07-21 12:50:25 +0000 > +++ b/include/mysql.h.pp 2011-09-22 22:13:38 +0000 > @@ -67,8 +67,8 @@ enum mysql_enum_shutdown_level { > SHUTDOWN_WAIT_UPDATES= (unsigned char)(1 << 3), > SHUTDOWN_WAIT_ALL_BUFFERS= ((unsigned char)(1 << 3) << 1), > SHUTDOWN_WAIT_CRITICAL_BUFFERS= ((unsigned char)(1 << 3) << 1) + 1, > - KILL_QUERY= 254, > - KILL_CONNECTION= 255 > + SHUTDOWN_KILL_QUERY= 254, > + SHUTDOWN_KILL_CONNECTION= 255 Why wouldn't you completely remove KILL_QUERY and KILL_CONNECTION from this enum ? Strictly speaking they are not "shutdown levels", they make no sense as arguments to mysql_shutdown(). And, as you've noticed, shutdown levels are completely ignored at the moment anyway - so nobody should miss these constants if you remove them. > }; > enum enum_cursor_type > { > > === modified file 'mysql-test/t/kill.test' > --- a/mysql-test/t/kill.test 2009-12-03 11:19:05 +0000 > +++ b/mysql-test/t/kill.test 2011-09-22 22:13:38 +0000 > @@ -337,5 +337,35 @@ SELECT 1; > > ########################################################################### > > +--echo # > +--echo # Test kill USER > +--echo # > + > +grant ALL on test.* to test@localhost; > +grant ALL on test.* to test2@localhost; > +connect (con3, localhost, test,,); > +connect (con4, localhost, test2,,); > +connection default; > +--enable_info > +kill hard query user test2@nohost; > +kill soft query user test@localhost; > +kill hard query user test@localhost; > +kill soft connection user test2; > +kill hard connection user test@localhost; What about real tests? When you kill soft actually kills (and doesn't kill) a statement. You can ask Philip to write them, I suppose. > === modified file 'sql/sql_parse.cc' > --- a/sql/sql_parse.cc 2011-09-10 15:01:27 +0000 > +++ b/sql/sql_parse.cc 2011-09-22 22:13:38 +0000 > @@ -7231,6 +7240,76 @@ uint kill_one_thread(THD *thd, ulong id, > } ... > +static uint kill_threads_for_user(THD *thd, LEX_USER *user, > + killed_state kill_signal, ha_rows *rows) > +{ > + THD *tmp; > + List<THD> threads_to_kill; > + DBUG_ENTER("kill_threads_for_user"); > + > + *rows= 0; > + > + if (thd->is_fatal_error) // If we run out of memory > + DBUG_RETURN(ER_OUT_OF_RESOURCES); > + > + DBUG_PRINT("enter", ("user: %s signal: %u", user->user.str, > + (uint) kill_signal)); > + > + VOID(pthread_mutex_lock(&LOCK_thread_count)); // For unlink from list > + I_List_iterator<THD> it(threads); > + while ((tmp=it++)) > + { > + if (tmp->command == COM_DAEMON) > + continue; > + /* > + Check that hostname (if given) and user name matches. > + > + host.str[0] == '%' means that host name was not given. See sql_yacc.yy > + */ > + if (((user->host.str[0] == '%' && !user->host.str[1]) || > + !strcmp(tmp->security_ctx->host, user->host.str)) && > + !strcmp(tmp->security_ctx->user, user->user.str)) > + { > + if (!(thd->security_ctx->master_access & SUPER_ACL) && > + !thd->security_ctx->user_matches(tmp->security_ctx)) So, without SUPER a user can only kill his own connections with KILL USER. A question. Should "KILL USER my_name" also kill the current connection, or it should only kill all other connections? > + { > + VOID(pthread_mutex_unlock(&LOCK_thread_count)); > + DBUG_RETURN(ER_KILL_DENIED_ERROR); > + } > + if (!threads_to_kill.push_back(tmp, tmp->mem_root)) > + pthread_mutex_lock(&tmp->LOCK_thd_data); // Lock from delete > + } > + } > + VOID(pthread_mutex_unlock(&LOCK_thread_count)); > + if (!threads_to_kill.is_empty()) > + { > + List_iterator_fast<THD> it(threads_to_kill); > + THD *ptr; > + while ((ptr= it++)) > + { > + ptr->awake(kill_signal); > + pthread_mutex_unlock(&ptr->LOCK_thd_data); > + (*rows)++; > + } > + } It may be a bit problematic. You lock thd->LOCK_thd_data for many threads. This defines a specific locking order. Which sounds quite fragile - I see no guarantee that this order cannot be violated by another thread. It can be even violated by some other feature that we'll implement in the future - and we won't notice it, even your deadlock detector won't catch it, because you create the dependency only during KILL USER, and only for specific THD's. So, if we'll ever have this locking order violated, it will show up as very random lockups in the undefined future :( I would suggest to rewrite this code (if possible) to avoid locking all thd->LOCK_thd_data's at once. Note that this code doesn't have to be fast. > + DBUG_RETURN(0); > +} > + > @@ -7241,16 +7320,33 @@ uint kill_one_thread(THD *thd, ulong id, > +void sql_kill_user(THD *thd, LEX_USER *user, killed_state state) > +{ > + uint error; > + ha_rows rows; > + if (!(error= kill_threads_for_user(thd, user, state, &rows))) I don't really understand why you've created a separate kill_threads_for_user() instead of putting the code here. But ok, as you like. > + my_ok(thd, rows); > + else > + { > + /* > + This is probably ER_OUT_OF_RESOURCES, but in the future we may > + want to write the name of the user we tried to kill > + */ > + my_error(error, MYF(0), user->host.str, user->user.str); > + } > +} > + > === modified file 'storage/archive/ha_archive.cc' > --- a/storage/archive/ha_archive.cc 2011-07-05 19:46:53 +0000 > +++ b/storage/archive/ha_archive.cc 2011-09-22 22:13:38 +0000 > @@ -17,6 +17,7 @@ > #pragma implementation // gcc: Class implementation > #endif > > +#define MYSQL_SERVER 1 why? > #include "mysql_priv.h" > #include <myisam.h> > Regards, Sergei

2 2

Re: [Maria-developers] CloudLinux patches for MariaDB
by Michael Widenius 26 Sep '11

26 Sep '11

Hi! >>>>> "Sergei" == Sergei Golubchik <serg(a)askmonty.org> writes: Sergei> Hi, Michael! Sergei> On Sep 23, Michael Widenius wrote: >> === modified file 'mysql-test/r/user_limits.result' >> --- mysql-test/r/user_limits.result 2005-05-02 18:45:06 +0000 >> +++ mysql-test/r/user_limits.result 2011-09-23 11:50:40 +0000 >> @@ -65,6 +65,16 @@ select * from t1; >> connect(localhost,mysqltest_1,,test,MYSQL_PORT,MYSQL_SOCK); >> ERROR 42000: User 'mysqltest_1' has exceeded the 'max_user_connections' resource (current value: 3) >> +grant usage on *.* to mysqltest_1@localhost with max_user_connections -1; Sergei> I still think that -1 (and 0 too) as special values look like a very Sergei> unprofessional hack. I'd rather see keywords there like Sergei> with max_user_connections UNLIMITED Sergei> with max_user_connections NONE That would be harder to document and understand when you take into account how the global variable max_user_connections work. I prefer to have these work identical. Sergei> as for the SET, SHOW VARIABLES, and command-line options - we can easily Sergei> support keywords if we declare max_user_connections to be a string Sergei> variable, not numeric, and will parse it manually. That I see would be an even great hack. The 'proper' way for the future would be to change the default value of 'max_user_connections' to MAX_INT and in the future threat 0 to mean zero connections. >> === modified file 'scripts/mysql_system_tables_fix.sql' >> --- scripts/mysql_system_tables_fix.sql 2011-05-28 02:11:32 +0000 >> +++ scripts/mysql_system_tables_fix.sql 2011-09-22 21:41:45 +0000 >> @@ -326,8 +326,11 @@ UPDATE host SET Create_routine_priv=Crea >> >> # >> # Add max_user_connections resource limit >> +# this is signed in MariaDB so that if one sets it's to -1 then the user >> +# can't connect anymore. Sergei> You need one special value (-1) and to get it you cut the number of Sergei> allowed values in half. Instead, you could've kept max_user_connections Sergei> as unsigned and only reserve (uint)~0 as "cannot connect". It's a maxint; No one will *ever* have 2~31 connections. This is ok. >> switch (var->show_type()) >> { >> - case SHOW_INT: get_sys_var_safe (uint); >> + case SHOW_INT: get_sys_var_safe (int); >> case SHOW_LONG: get_sys_var_safe (ulong); >> case SHOW_LONGLONG: get_sys_var_safe (ulonglong); >> case SHOW_HA_ROWS: get_sys_var_safe (ha_rows); Sergei> This is a bit inconsistent. INT is signed, but LONG/LONGLONG are Sergei> unsigned. Do I understand correctly that there will be no way to create Sergei> an unsigned INT variable after your patch? If you want unsigned int, you just use 'long' (Most of our variables are 'long' anyway; We have only a handful of int variables and the range of MAX_INT is good enough for these). When we ever need unsigned int, we will add a SHOW_UINT. I wanted to keep the patch minimal and as no-intrusive as possible and thats why I only fixed SHOW_INT and not SHOW_LONG and SHOW_LONGLONG. It's actually a bug that SHOW_INT didn't take negative numbers before; It's supposed to work the same way as GET_INT works in my_getopt.c, which it didn't do. >> === modified file 'sql/sql_acl.cc' >> - /* Don't allow the user to connect if he has done too many queries */ >> - if ((acl_user->user_resource.questions || acl_user->user_resource.updates || >> - acl_user->user_resource.conn_per_hour || >> - acl_user->user_resource.user_conn || max_user_connections) && >> - get_or_create_user_conn(thd, >> - (opt_old_style_user_limits ? sctx->user : sctx->priv_user), >> - (opt_old_style_user_limits ? sctx->host_or_ip : sctx->priv_host), >> - &acl_user->user_resource)) >> + /* >> + Don't allow the user to connect if he has done too many queries >> + We always have to allocated this as someone may change >> + max_user_connections any time. Sergei> What do you mean? That the original code was wrong. We can *not* do the allocation based on the original 'if' as it's assumed that max_user_connections would never change. My comments explain that the we have always to call get_or_create_user_conn() because max_user_connections depends on this structure to exists. >> + */ >> + if (get_or_create_user_conn(thd, >> + (opt_old_style_user_limits ? sctx->user : >> + sctx->priv_user), >> + (opt_old_style_user_limits ? sctx->host_or_ip >> + : sctx->priv_host), >> + &acl_user->user_resource)) >> DBUG_RETURN(1); // The error is set by get_or_create_user_conn() >> } >> else >> >> === modified file 'sql/sql_connect.cc' >> --- sql/sql_connect.cc 2011-09-22 22:13:38 +0000 >> +++ sql/sql_connect.cc 2011-09-22 22:58:07 +0000 >> @@ -113,8 +113,11 @@ int check_for_max_user_connections(THD * >> DBUG_ENTER("check_for_max_user_connections"); >> >> (void) pthread_mutex_lock(&LOCK_user_conn); >> + >> + /* Root is not affected by the value of max_user_connections */ Sergei> It's an incompatible change. Why did you do it? If the point was to Sergei> allow SUPER connections when max_user_connections==-1, then it would be Sergei> enough to make only this value (-1) to be ignored by the SUPER user, and Sergei> any other value limit should still apply. root users should not be affected by the global variables max_user_connections or max_connections. Yes, it's incompatible, but ok and necessary in this case as otherwise no one could login if we set global.max_user_connections=1 which would not let you maintain the server at all. Sergei> SUPER user typically ignores features that prevent him from connecting Sergei> to the server. E.g incorrect init-connect option may do it. Or a normal Sergei> user may use up all max_connections limit and prevent SUPER from Sergei> connecting. That's why SUPER ignores init-connect and is allowed one Sergei> connection above max_connections. Sergei> But max_user_connections can not prevent SUPER from connecting (unless Sergei> it is -1). If it is set to, for example, 5, SUPER can have five Sergei> simultaneous connections and no normal user can prevent him from doing Sergei> it. This is why SUPER does *not* ignore max_user_connections. Sergei> So, by this logic, only the value max_user_connections==-1 should be Sergei> ignored by SUPER, and not any other value. I think it's better to not regard any value as special. It's easier to document it the current way; Doing it your way would make it hard to document and understand and that usually means it's a bad idea. (For example, what happens if you set max_user_connections=-1 and then later change it to 1; What limits apply to the root user that is now logged in twice). If you want to constrain the root user, you can do that with GRANT root ... max_user_connections=# Regards, Monty

1 0

[Maria-developers] SOCKET_SIZE_TYPE question on accept() call
by Steve Ellcey 23 Sep '11

23 Sep '11

Whild building mariadb on HP-UX, I ran into a problem with a call to accept(). The mariadb configure script uses MYSQL_TYPE_ACCEPT to determine if the third argument to accept should be type socklen_t or type int, and sets SOCKET_SIZE_TYPE accordingly, but in socket_accept (plugin/handler_socket/libhsclient/socket.cpp), the third argument to the accept call (addrlen_r) is hardcoded to be type socklen_t instead of using SOCKET_SIZE_TYPE. I think that the addrlen_r argument to socket_accept should be type SOCKET_SIZE_TYPE instead of socklent_t. I think this also means that the addrlen field in the socket_args structure (plugin/handler_socket/libhsclient/socket.hpp) should be type SOCKET_SIZE_TYPE instead of socklen_t. Does that sound right? Steve Ellcey sje(a)cup.hp.com

2 4