- developers - lists.mariadb.org

[Maria-developers] Fwd: Mdev-10436 - Non-deterministic vcol does not force rbr
by Sachin Setiya 24 Jan '17

24 Jan '17

Hi Igor, Need a little help in Mdev-10436, I an sanja discussed that we need some method is Item class to see whether it can be determined deterministicly , Is it okay ? Can I change Item class to incorporate this change ? -- Regards Sachin Setiya Software Engineer at MariaDB -- Regards Sachin Setiya Software Engineer at MariaDB

2 1

[Maria-developers] Please review MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
by Alexander Barkov 23 Jan '17

23 Jan '17

Hello Sergei, can you please review a fix for MDEV-11134. I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table(). But perhaps it can be done in different ways: 1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false. or 2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1) Thanks!

2 6

Re: [Maria-developers] [Commits] 8140454d2b7: MDEV-11254: innodb-use-trim has no effect in 10.2
by Marko Mäkelä 23 Jan '17

23 Jan '17

This is somewhat better, but I think we should still clean up the code a little more. On Mon, Jan 23, 2017 at 1:32 PM, jan <jan.lindstrom(a)mariadb.com> wrote: > fil0fil.cc: Remove unneeded status call and add test is > spare files and punch hole supported by file system where > tablespace is created. Add call to get file system > block size. Used file node is added to IORequest. Added > functions to check is punch hole supported and setting > punch hole. s/is spare files/if sparse files/ s/where/when/ > dberr.h: Add error code if punch hole operation fails. I think that it would be better to explicitly mention the error code here. The name of the code matters more than the file name where it is declared. > os0file.h: Remove unneeded m_block size. Add m_bpage > to know actual size of the block and m_fil_node > to know tablespace file system block size and does it > support punch hole. Mention the class or struct name too. There could be multiple classes declared in the file os0file.h. > os0file.cc: Add function to do punch_hole operation, > get the file system block size and determine > does file system support sparse files (for punch hole). What is the name of the added function? > +--disable_warnings > +let $innodb_compression_algorithm_orig=`SELECT @@innodb_compression_algorithm`; > +let $innodb_file_format_orig = `SELECT @@innodb_file_format`; > +let $innodb_file_per_table_orig = `SELECT @@innodb_file_per_table`; > + > +SET GLOBAL innodb_file_format = `Barracuda`; > +SET GLOBAL innodb_file_per_table = ON; > +--enable_warnings > +--enable_query_log In 10.2, do not add unnecessary references to the deprecated variable innodb_file_format, and do not bother to mess with innodb_file_per_table. The defaults are fine as is. > +let $wait_condition= SELECT variable_value > 0 FROM information_schema.global_status WHERE variable_name = 'innodb_num_pages_page_compressed'; > +--source include/wait_condition.inc > + > +let $wait_condition= SELECT variable_value > 0 FROM information_schema.global_status WHERE variable_name = 'innodb_num_page_compressed_trim_op'; > +--source include/wait_condition.inc > + > +SELECT VARIABLE_VALUE > 0 FROM information_schema.global_status where variable_name = 'INNODB_NUM_PAGES_PAGE_COMPRESSED'; > +SELECT VARIABLE_VALUE > 0 FROM information_schema.global_status where variable_name = 'INNODB_NUM_PAGE_COMPRESSED_TRIM_OP'; Can we wait for a single condition here, using AND in the WHERE condition? I do not think that we need the subsequent SELECT statements. > +/** > +Should we punch hole to deallocate unused portion of the page. > +@param[in] bpage Page control block > +@return true if punch hole should be used, false if not */ > +bool > +buf_page_should_punch_hole( > + const buf_page_t* bpage) > +{ > + return (bpage && bpage->real_size != bpage->size.physical()); > +} > + > +/** > +Calculate the length of trim (punch_hole) operation. > +@param[in] bpage Page control block > +@param[in] write_length Write length > +@return length of the trim or zero. */ > +ulint > +buf_page_get_trim_length( > + const buf_page_t* bpage, > + ulint write_length) > +{ > + return (bpage ? bpage->size.physical() - write_length : 0); > +} Please declare these as methods of buf_page_t, and move the NULL check to the caller. > @@ -912,7 +912,7 @@ buf_dblwr_write_block_to_datafile( > type |= IORequest::DO_NOT_WAKE; > } > > - IORequest request(type); > + IORequest request(type, bpage); > > /* We request frame here to get correct buffer in case of > encryption and/or page compression */ > @@ -924,7 +924,7 @@ buf_dblwr_write_block_to_datafile( > fil_io(request, sync, bpage->id, bpage->size, 0, > bpage->size.physical(), > (void*) frame, > - (void*) bpage, NULL); > + (void*) bpage); > } else { > ut_ad(!bpage->size.is_compressed()); > It feels redundant that we are passing bpage both in the IORequest request and directly to the fil_io() call. One parameter should be enough. And there should be no need for the parameters that are derived directly from bpage. Can we have a separate fil_io() call for the redo log, and a separate one for buffer pool blocks? > +/** > +Get should we punch hole to tablespace. > +@param[in] node File node > +@return true, if punch hole should be tried, false if not. */ > +bool > +fil_node_should_punch_hole( > + const fil_node_t* node) > +{ > + return (node && node->space && node->space->punch_hole); > +} Who would call this with node=NULL? And when would node->space ever be NULL for a valid fil_node_t? > +/** > +Set punch hole to tablespace to given value. > +@param[in] node File node > +@param[in] val value to be set. */ > +void > +fil_space_set_punch_hole( > + fil_node_t* node, > + bool val) > +{ > + if (node && node->space) { > + node->space->punch_hole = val; > + } > +} Also here, I would remove the unnecessary if condition. > diff --git a/storage/innobase/include/os0api.h b/storage/innobase/include/os0api.h > new file mode 100644 > index 00000000000..ea2a113bdec > --- /dev/null > +++ b/storage/innobase/include/os0api.h Instead of creating a new file, can we please move these declarations into an existing file, such as fsp0types.h? The file name prefix is misleading, because there is nothing operating system dependent about the contents. > diff --git a/storage/innobase/include/os0file.h b/storage/innobase/include/os0file.h > index 57ee015dfdd..cd016a46972 100644 > --- a/storage/innobase/include/os0file.h > +++ b/storage/innobase/include/os0file.h > @@ -36,7 +36,8 @@ Created 10/21/1995 Heikki Tuuri > #ifndef os0file_h > #define os0file_h > > -#include "univ.i" > +#include "page0size.h" > +#include "os0api.h" > > #ifndef _WIN32 > #include <dirent.h> > @@ -46,8 +47,10 @@ Created 10/21/1995 Heikki Tuuri > > /** File node of a tablespace or the log data space */ > struct fil_node_t; > +struct fil_space_t; > > extern bool os_has_said_disk_full; > +extern my_bool srv_use_trim; > > /** Number of pending read operations */ > extern ulint os_n_pending_reads; Instead of duplicating declarations, please add #include directives. > @@ -177,6 +180,8 @@ static const ulint OS_FILE_ERROR_MAX = 200; > #define IORequestLogRead IORequest(IORequest::LOG | IORequest::READ) > #define IORequestLogWrite IORequest(IORequest::LOG | IORequest::WRITE) > > + > + > /** > The IO Context that is passed down to the low level IO code */ > class IORequest { Do not add empty lines. > + /** @return true if punch hole should be used */ > + bool punch_hole() const > + MY_ATTRIBUTE((warn_unused_result)) > + { > + return((m_type & PUNCH_HOLE) == PUNCH_HOLE); > + } > + Comparing against 0 should be more efficient. > - /** @return the block size to use for IO */ > - ulint block_size() const > - MY_ATTRIBUTE((warn_unused_result)) > + /** Set the pointer to file node for IO > + @param[in] node File node */ > + void set_fil_node(fil_node_t* node) Consider removing this method, and setting this in the constructor. Why fil_node_t and not fil_space_t? > + /** @return true if punch hole is supported */ > + static bool is_punch_hole_supported() > + { > + > + /* In this debugging mode, we act as if punch hole is supported, > + and then skip any calls to actually punch a hole here. > + In this way, Transparent Page Compression is still being tested. */ > + DBUG_EXECUTE_IF("ignore_punch_hole", > + return(true); > + ); Please add a test that uses SET DEBUG_DBUG='+d,ignore_punch_hole'. > +#if defined(HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE) || defined(_WIN32) > + return(true); > +#else > + return(false); > +#endif /* HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE || _WIN32 */ > + } This looks wrong design to me. IORequest::is_punch_hole_supported() should depend on the fil_node_t and the actual file system capabilities. > + void space_no_punch_hole(void) { > + fil_space_set_punch_hole(m_fil_node, false); > + } I would prefer more straight code, such as m_fil_node->punch_hole = false; or m_fil_node->clear_punch_hole(); > +/** Check if the file system supports sparse files. > + > +Warning: On POSIX systems we try and punch a hole from offset 0 to > +the system configured page size. This should only be called on an empty > +file. > + > +Note: On Windows we use the name and on Unices we use the file handle. > + > +@param[in] name File name > +@param[in] fh File handle for the file - if opened > +@return true if the file system supports sparse files */ > +bool > +os_is_sparse_file_supported( > + const char* path, > + os_file_t fh) > + MY_ATTRIBUTE((warn_unused_result)); Is it possible to use the file handle on Windows? Or can we define two different functions, one for Windows and another for POSIX? > +/** Free storage space associated with a section of the file. > +@param[in] fh Open file handle > +@param[in] off Starting offset (SEEK_SET) > +@param[in] len Size of the hole > +@return DB_SUCCESS or error code */ > +dberr_t > +os_file_punch_hole( > + IORequest& type, > + os_file_t fh, > + os_offset_t off, > + os_offset_t len) > + MY_ATTRIBUTE((warn_unused_result)); > + > +/** Free storage space associated with a section of the file. > +@param[in] fh Open file handle > +@param[in] off Starting offset (SEEK_SET) > +@param[in] len Size of the hole > +@return DB_SUCCESS or error code */ > +dberr_t > +os_file_punch_hole( > + os_file_t fh, > + os_offset_t off, > + os_offset_t len) > + MY_ATTRIBUTE((warn_unused_result)); The parameter 'type' of the first function is not defined. Why do we need two very similar-looking functions? Could one of them be defined inline as a wrapper of the other? > @@ -156,9 +157,6 @@ class page_size_t { > > private: > > - /* Disable implicit copying. */ > - void operator=(const page_size_t&); > - > /* For non compressed tablespaces, physical page size is equal to > the logical page size and the data is stored in buf_page_t::frame > (and is also always equal to univ_page_size (--innodb-page-size=)). Also reimplement page_size_t::copy_from() as { *this = src; } > @@ -737,6 +740,19 @@ os_file_handle_error_no_exit( > const char* operation, > bool silent); > > +/** Punch a hole in the file if it was a write > +@param[in] type IO context > +@param[in] fh Open file handle > +@param[in] len Compressed buffer length for write > +@return DB_SUCCESS or error code */ > +static > +dberr_t > +os_file_io_complete( > + IORequest& type, > + os_file_t fh, > + ulint offset, > + ulint len); Can this be made a method of IORequest? > + if (err != 0) { > + ib::warn() << "fstat() failed on file " << name > + << " error "<< errno << " : " << strerror(errno); > + os_file_handle_error_no_exit(name, "fstat()", FALSE); Remove the ib::warn(). One error report should be enough, just like we would do it on Windows. > + ib::warn() > + << "fallocate(" << fh > + <<", FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, " > + << off << ", " << len << ") returned errno: " > + << errno; Do we need this detailed reporting? What use is the numeric value of the file handle to the user? A file name would be much more useful. And ideally, this error should be pushed to the client. > + /* Deallocate unused blocks from file system. > + This is newer done to page 0 or to log files.*/ s/newer/never/. However, during .ibd file creation, we do punch a hole to page 0. > + if (offset > 0 > + && !type.is_log() > + && type.is_write() > + && type.punch_hole()) { > + *err = os_file_io_complete( > + const_cast<IORequest&>(type), > + file, > + static_cast<ulint>(offset), > + n); Remove the const_cast, and properly change all IORequest parameters to non-const. > @@ -4680,7 +4913,7 @@ os_file_pwrite( > (void) my_atomic_addlint(&os_n_pending_writes, 1); > MONITOR_ATOMIC_INC(MONITOR_OS_PENDING_WRITES); > > - ssize_t n_bytes = os_file_io(type, file, const_cast<void*>(buf), > + ssize_t n_bytes = os_file_io(type, file, (void*)buf, > n, offset, err); Avoid C-style casts at any cost. Can we split the os_file_io() into os_file_read() and os_file_write() somehow? How much common code is there actually between the two? Also, could that function be made a method of IORequest? > @@ -5195,6 +5429,31 @@ os_file_read_no_error_handling_func( > +/** NOTE! Use the corresponding macro os_file_write(), not directly > +Requests a synchronous write operation. > +@param[in] type IO flags > +@param[in] file handle to an open file > +@param[out] buf buffer from which to write > +@param[in] offset file offset from the start where to read > +@param[in] n number of bytes to read, starting from offset > +@return DB_SUCCESS if request was successful, false if fail */ > +dberr_t > +os_file_write_func( > + IORequest& type, > + const char* name, > + os_file_t file, > + const void* buf, > + os_offset_t offset, > + ulint n) Can this be made a method of IORequest? > diff --git a/storage/innobase/srv/srv0srv.cc b/storage/innobase/srv/srv0srv.cc > index 5d478e4529f..4b862cc6267 100644 > --- a/storage/innobase/srv/srv0srv.cc > +++ b/storage/innobase/srv/srv0srv.cc > @@ -184,7 +184,7 @@ my_bool srv_use_native_aio = TRUE; > my_bool srv_numa_interleave = FALSE; > /* If this flag is TRUE, then we will use fallocate(PUCH_HOLE) > to the pages */ > -UNIV_INTERN my_bool srv_use_trim = FALSE; > +UNIV_INTERN my_bool srv_use_trim = TRUE; > /* If this flag is TRUE, then we disable doublewrite buffer */ > UNIV_INTERN my_bool srv_use_atomic_writes = FALSE; > /* If this flag IS TRUE, then we use this algorithm for page compressing the pages */ Omit the redundant initialization and let the variable live in the BSS segment. Global parameters will be initialized via the table in ha_innodb.cc. -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

Re: [Maria-developers] [Commits] 98b2a9c: MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings
by Nirbhay Choubey 20 Jan '17

20 Jan '17

Hi Sachin, The overall patch looks ok. I, however, have a few minor comments inline. On Thu, Jan 19, 2017 at 1:21 AM, SachinSetiya <sachin.setiya(a)mariadb.com> wrote: > revision-id: 98b2a9c967a5eaa1f99bb3ef229ff2af62018ffe > (mariadb-10.0.28-34-g98b2a9c) > parent(s): 9bf92706d19761722b46d66a671734466cb6e98e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-19 11:50:59 +0530 > message: > > MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings > > Problem:- When setting max_binlog_stmt_cache_size=18446744073709547520 > from either command line or .cnf file, server fails to start. > > Solution:- Added one more function eval_num_suffix_ull , which uses > strtoull to get unsigned ulonglong from string. And getopt_ull calles this > Typo s/calles/calls/ > function instead of eval_num_suffix. Also changed previous eval_num_suffix > to > eval_num_suffix_ll to remain consistent. > > --- > .../r/binlog_max_binlog_stmt_cache_size.result | 14 +++++ > .../binlog/t/binlog_max_binlog_stmt_cache_size.opt | 1 + > .../t/binlog_max_binlog_stmt_cache_size.test | 11 ++++ > mysys/my_getopt.c | 66 > ++++++++++++++++++---- > 4 files changed, 81 insertions(+), 11 deletions(-) > > diff --git a/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > new file mode 100644 > index 0000000..6fbec90 > --- /dev/null > +++ b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > @@ -0,0 +1,14 @@ > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +Warnings: > +Warning 1292 Truncated incorrect max_binlog_stmt_cache_size > value: '18446744073709547519' > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709543424 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > new file mode 100644 > index 0000000..c52ef14 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > @@ -0,0 +1 @@ > +--max_binlog_stmt_cache_size=18446744073709547520 > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > new file mode 100644 > index 0000000..bc30b48 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > @@ -0,0 +1,11 @@ > +source include/have_log_bin.inc; > +select @@max_binlog_stmt_cache_size; > + > +--let $cache_size=`select @@max_binlog_stmt_cache_size;` > + > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > + > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +select @@max_binlog_stmt_cache_size; > I would also add tests for ULLONG_MAX and ULLONG_MAX +/- 1. +--eval set global max_binlog_stmt_cache_size= $cache_size > diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c > index 2a45571..0934a50 100644 > --- a/mysys/my_getopt.c > +++ b/mysys/my_getopt.c > @@ -895,11 +895,32 @@ my_bool getopt_compare_strings(register const char > *s, register const char *t, > /* > function: eval_num_suffix > > + Transforms suffix like k/m/g to there real value. > Typo : s/there/their/ > +*/ > +static inline long eval_num_suffix(char *suffix, int *error) > +{ > + long num= 1; > + if (*suffix == 'k' || *suffix == 'K') > + num*= 1024L; > + else if (*suffix == 'm' || *suffix == 'M') > + num*= 1024L * 1024L; > + else if (*suffix == 'g' || *suffix == 'G') > + num*= 1024L * 1024L * 1024L; > + else if (*suffix) > + { > + *error= 1; > + return 0; > + } > + return num; > +} > Please add a new line here (as a separator). > +/* > + function: eval_num_suffix_ll > + > Transforms a number with a suffix to real number. Suffix can > be k|K for kilo, m|M for mega or g|G for giga. > */ > > -static longlong eval_num_suffix(char *argument, int *error, char > *option_name) > +static longlong eval_num_suffix_ll(char *argument, int *error, char > *option_name) > { > char *endchar; > longlong num; > DBUG_ENTER("eval_num_suffix"); You should also update the function name. > @@ -916,23 +937,46 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > *error= 1; > DBUG_RETURN(0); > } > - if (*endchar == 'k' || *endchar == 'K') > - num*= 1024L; > - else if (*endchar == 'm' || *endchar == 'M') > - num*= 1024L * 1024L; > - else if (*endchar == 'g' || *endchar == 'G') > - num*= 1024L * 1024L * 1024L; > - else if (*endchar) > - { > + num*= eval_num_suffix(endchar, error); > + if (*error) > fprintf(stderr, > "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > *endchar, option_name, argument); > + DBUG_RETURN(num); > +} > + > +/* > + function: eval_num_suffix_ull > + > + Transforms a number with a suffix to positive Integer. Suffix can > + be k|K for kilo, m|M for mega or g|G for giga. > +*/ > + > +static ulonglong eval_num_suffix_ull(char *argument, int *error, char > *option_name) > The above line >80 chars, please break it into 2 lines. > +{ > + char *endchar; > + ulonglong num; > + DBUG_ENTER("eval_num_suffix"); > Update the function name. > + > + *error= 0; > + errno= 0; > + num= strtoull(argument, &endchar, 10); > + if (errno == ERANGE) > + { > + my_getopt_error_reporter(ERROR_LEVEL, > + "Incorrect integer value: '%s'", argument); > *error= 1; > DBUG_RETURN(0); > } > + num*= eval_num_suffix(endchar, error); > + if (*error) > + fprintf(stderr, > + "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > + *endchar, option_name, argument); > DBUG_RETURN(num); > } > > + > /* > function: getopt_ll > > @@ -946,7 +990,7 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > > static longlong getopt_ll(char *arg, const struct my_option *optp, int > *err) > { > - longlong num=eval_num_suffix(arg, err, (char*) optp->name); > + longlong num=eval_num_suffix_ll(arg, err, (char*) optp->name); > return getopt_ll_limit_value(num, optp, NULL); > } > > @@ -1023,7 +1067,7 @@ longlong getopt_ll_limit_value(longlong num, const > struct my_option *optp, > > static ulonglong getopt_ull(char *arg, const struct my_option *optp, int > *err) > { > - ulonglong num= eval_num_suffix(arg, err, (char*) optp->name); > + ulonglong num= eval_num_suffix_ull(arg, err, (char*) optp->name); > return getopt_ull_limit_value(num, optp, NULL); > } > > Best, Nirbhay

2 3

[Maria-developers] Contribution in mariadb
by Dulton Ghosh 19 Jan '17

19 Jan '17

Respected Sir/Ma'am, I am Dulton Ghosh, a final year student of IIT Guwahati. I want to contribute in MariaDB in upcoming Google Summer of Code. Please give me some instuctions to do so. Regards, Dulton Ghosh IIT Guwahati

2 1

Re: [Maria-developers] Creating test cases for Spider
by Michael Widenius 18 Jan '17

18 Jan '17

Hi! > I will today update the 10.2 spider tree with the latest 10.2 tree > that includes all gcov patches and then try this myself. I have now merged 10.2-spider tree with the latest 10.2 tree, which includes the new HASH and better support for gcov. I am now reviewing the last part of multi_range_read (all files except ha_partition.cc are reviewed). Hope to have this last part done soon. Regards, Monty

2 1

Re: [Maria-developers] [Commits] 14747e4: MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242)
by Sergey Petrunia 17 Jan '17

17 Jan '17

Hi Varun, On Tue, Jan 17, 2017 at 03:24:39PM +0530, Varun wrote: > revision-id: 14747e4b84f68ee7e10a6e5a24f4b7ec6f3240f9 (mariadb-10.1.20-38-g14747e4) > parent(s): b56f726e42e8bc0427a0c33cfffb7e1c5399ea16 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-17 15:22:22 +0530 > message: > > MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242) > > NOT NULL predicate was not added to tables in case of an update query having a subquery. > > --- > mysql-test/r/update_innodb.result | 30 +++++++++++++++++ > mysql-test/t/update_innodb.test | 31 +++++++++++++++++ > sql/sql_select.cc | 70 +++++++++++++++++++++++++-------------- > 3 files changed, 106 insertions(+), 25 deletions(-) > > diff --git a/mysql-test/r/update_innodb.result b/mysql-test/r/update_innodb.result > index 88c86c5..6a02dfd 100644 > --- a/mysql-test/r/update_innodb.result > +++ b/mysql-test/r/update_innodb.result > @@ -29,3 +29,33 @@ CREATE ALGORITHM=UNDEFINED VIEW `v1` AS select `t4`.`c1` AS `c1`,`t4`.`c2` AS `c > UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON vw.c2 = del.c2 SET a.c2 = ( SELECT max(t.c1) FROM t3 t, v1 i WHERE del.c2 = t.c2 AND vw.c3 = i.c3 AND t.c3 = 4 ) WHERE a.c2 IS NULL OR a.c2 < '2011-05-01'; > drop view v1; > drop table t1,t2,t3,t4; > +# > +# MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +# The MDEV number and title are wrong! Please fix. > +CREATE TABLE a ( Please make tables names to follow the t1,t2,t3,... naming convention used in the testcase. > +a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +b_id INT(20) UNSIGNED NULL DEFAULT NULL, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (a_id) > +) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > +CREATE TABLE b ( > +b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (b_id), > +INDEX idx_c_id (c_id) > +) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > +INSERT INTO a (b_id, c_id) VALUES (1, NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +select b_id from a; > +b_id > +1 > +UPDATE a SET b_id = (SELECT b.b_id FROM b b WHERE b.c_id = a.c_id); > +select b_id from a; > +b_id > +NULL > +drop table a,b; > diff --git a/mysql-test/t/update_innodb.test b/mysql-test/t/update_innodb.test > index 67c356c..f2b7ade 100644 > --- a/mysql-test/t/update_innodb.test > +++ b/mysql-test/t/update_innodb.test > @@ -37,3 +37,34 @@ UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON v > > drop view v1; > drop table t1,t2,t3,t4; > + > +--echo # > +--echo # MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +--echo # > + > +CREATE TABLE a ( > + a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + b_id INT(20) UNSIGNED NULL DEFAULT NULL, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (a_id) > + ) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > + > +CREATE TABLE b ( > + b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (b_id), > + INDEX idx_c_id (c_id) > + ) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > + > +INSERT INTO a (b_id, c_id) VALUES (1, NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +INSERT INTO b (c_id) VALUES (NULL); > + > +select b_id from a; > +UPDATE a SET b_id = (SELECT b.b_id FROM b b WHERE b.c_id = a.c_id); > +select b_id from a; > +drop table a,b; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 85bfb6b..3543cc2 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -9363,34 +9363,54 @@ static void add_not_null_conds(JOIN *join) > not_null_item is the t1.f1, but it's referred_tab is 0. > */ > if (!referred_tab) > - continue; > - if (!(notnull= new (join->thd->mem_root) > + { > + if (!(notnull= new (join->thd->mem_root) > Item_func_isnotnull(join->thd, item))) > - DBUG_VOID_RETURN; > - /* > - We need to do full fix_fields() call here in order to have correct > - notnull->const_item(). This is needed e.g. by test_quick_select > - when it is called from make_join_select after this function is > - called. > - */ Note this as (LOC1) > - if (notnull->fix_fields(join->thd, &notnull)) > - DBUG_VOID_RETURN; > - DBUG_EXECUTE("where",print_where(notnull, > - referred_tab->table->alias.c_ptr(), > - QT_ORDINARY);); > - if (!tab->first_inner) > - { > - COND *new_cond= referred_tab->join == join ? > - referred_tab->select_cond : > - join->outer_ref_cond; > - add_cond_and_fix(join->thd, &new_cond, notnull); > - if (referred_tab->join == join) > - referred_tab->set_select_cond(new_cond, __LINE__); > - else > - join->outer_ref_cond= new_cond; > + DBUG_VOID_RETURN; > + /* > + We need to do full fix_fields() call here in order to have correct > + notnull->const_item(). This is needed e.g. by test_quick_select > + when it is called from make_join_select after this function is > + called. > + */ > + if (notnull->fix_fields(join->thd, &notnull)) > + DBUG_VOID_RETURN; > + > + if (!tab->first_inner) > + add_cond_and_fix(join->thd, &join->outer_ref_cond, notnull); > + else > + add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > } > else > - add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > + { > + if (!(notnull= new (join->thd->mem_root) > + Item_func_isnotnull(join->thd, item))) > + DBUG_VOID_RETURN; > + /* > + We need to do full fix_fields() call here in order to have correct > + notnull->const_item(). This is needed e.g. by test_quick_select > + when it is called from make_join_select after this function is > + called. > + */ This is LOC2. After the patch, there is a lot of code duplication between LOC1 and LOC2: - creation of Item_func_isnotnull - fix_fields call and its comment - etc Please factor out the common code. The "print_where" call can print "outer_ref_table_cond" when referred_tab=NULL. > + if (notnull->fix_fields(join->thd, &notnull)) > + DBUG_VOID_RETURN; > + DBUG_EXECUTE("where",print_where(notnull, > + referred_tab->table->alias.c_ptr(), > + QT_ORDINARY);); > + if (!tab->first_inner) > + { > + COND *new_cond= referred_tab->join == join ? > + referred_tab->select_cond : > + join->outer_ref_cond; > + add_cond_and_fix(join->thd, &new_cond, notnull); > + if (referred_tab->join == join) > + referred_tab->set_select_cond(new_cond, __LINE__); > + else > + join->outer_ref_cond= new_cond; > + } > + else > + add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > + } > } > } > } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 43e2a78f2a0: Remove MYSQL_COMPRESSION.
by Jan Lindström 16 Jan '17

16 Jan '17

Hi, On Mon, Jan 16, 2017 at 2:10 PM, <marko.makela(a)mariadb.com> wrote: > > diff --git a/storage/innobase/btr/btr0btr.cc b/storage/innobase/btr/ > btr0btr.cc > index d1d9dfe64fe..1fb5cb06949 100644 > --- a/storage/innobase/btr/btr0btr.cc > +++ b/storage/innobase/btr/btr0btr.cc > @@ -215,10 +215,6 @@ btr_root_get( > buf_block_t* root = btr_root_block_get(index, RW_SX_LATCH, > mtr); > > - if (root && root->page.encrypted == true) { > - root = NULL; > - } > - > return(root ? buf_block_get_frame(root) : NULL); > This is unrelated to MySQL encryption, it is related to MariaDB encryption, not sure if btr_root_block_get would always return NULL when page is encrypted, upper code is peppered with assertions, thus this might not be safe. > > - if (ptype == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { > - header_len += FIL_PAGE_COMPRESSION_METHOD_SIZE; > - } > - > - /* Do not try to uncompressed pages that are not compressed */ > - if (ptype != FIL_PAGE_PAGE_COMPRESSED && > - ptype != FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED && > - ptype != FIL_PAGE_COMPRESSED) { > + switch (ptype) { > + case FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED: > + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE > + + FIL_PAGE_COMPRESSION_METHOD_SIZE; > + break; > + case FIL_PAGE_PAGE_COMPRESSED: > + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE; > + break; > + default: > + /* The page is not in our format. */ > return; > This is also unrelated change but more like cosmetic, so ok. > } > > > diff --git a/storage/innobase/include/buf0buf.ic > b/storage/innobase/include/buf0buf.ic > index f2b1b151598..5e75c446bbd 100644 > --- a/storage/innobase/include/buf0buf.ic > +++ b/storage/innobase/include/buf0buf.ic > @@ -734,9 +734,6 @@ buf_block_get_frame( > case BUF_BLOCK_ZIP_PAGE: > case BUF_BLOCK_ZIP_DIRTY: > case BUF_BLOCK_NOT_USED: > - if (block->page.encrypted) { > - goto ok; > - } > Hmm, this is unrelated but could be actually correct. > ut_error; > break; > case BUF_BLOCK_FILE_PAGE: > > > @@ -145,52 +137,22 @@ fil_page_type_validate( > page_type == FIL_PAGE_TYPE_BLOB || > page_type == FIL_PAGE_TYPE_ZBLOB || > page_type == FIL_PAGE_TYPE_ZBLOB2 || > - page_type == FIL_PAGE_COMPRESSED || > - page_type == FIL_PAGE_TYPE_UNKNOWN || > - page_type == FIL_PAGE_ENCRYPTED || > - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || > - page_type == FIL_PAGE_ENCRYPTED_RTREE))) { > - > - uint key_version = mach_read_from_4(page + > FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION); > - bool page_compressed = (page_type == > FIL_PAGE_PAGE_COMPRESSED); > - bool page_compressed_encrypted = (page_type == > FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED); > + page_type == FIL_PAGE_TYPE_UNKNOWN))) { > Why you change this code, it is not MySQL compression code ? > + > ulint space = mach_read_from_4(page + > FIL_PAGE_ARCH_LOG_NO_OR_SPACE_ID); > ulint offset = mach_read_from_4(page + FIL_PAGE_OFFSET); > - ib_uint64_t lsn = mach_read_from_8(page + FIL_PAGE_LSN); > - ulint compressed_len = mach_read_from_2(page + > FIL_PAGE_DATA); > fil_system_enter(); > fil_space_t* rspace = fil_space_get_by_id(space); > fil_system_exit(); > > /* Dump out the page info */ > - fprintf(stderr, "InnoDB: Space %lu offset %lu name %s > page_type %lu page_type_name %s\n" > - "InnoDB: key_version %u page_compressed %d > page_compressed_encrypted %d lsn %llu compressed_len %lu\n", > - space, offset, rspace->name, page_type, > fil_get_page_type_name(page_type), > - key_version, page_compressed, > page_compressed_encrypted, (ulonglong)lsn, compressed_len); > - fflush(stderr); > - > - ut_ad(page_type == FIL_PAGE_PAGE_COMPRESSED || > - page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED || > - page_type == FIL_PAGE_INDEX || > - page_type == FIL_PAGE_RTREE || > - page_type == FIL_PAGE_UNDO_LOG || > - page_type == FIL_PAGE_INODE || > - page_type == FIL_PAGE_IBUF_FREE_LIST || > - page_type == FIL_PAGE_TYPE_ALLOCATED || > - page_type == FIL_PAGE_IBUF_BITMAP || > - page_type == FIL_PAGE_TYPE_SYS || > - page_type == FIL_PAGE_TYPE_TRX_SYS || > - page_type == FIL_PAGE_TYPE_FSP_HDR || > - page_type == FIL_PAGE_TYPE_XDES || > - page_type == FIL_PAGE_TYPE_BLOB || > - page_type == FIL_PAGE_TYPE_ZBLOB || > - page_type == FIL_PAGE_TYPE_ZBLOB2 || > - page_type == FIL_PAGE_COMPRESSED || > - page_type == FIL_PAGE_TYPE_UNKNOWN || > - page_type == FIL_PAGE_ENCRYPTED || > - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || > - page_type == FIL_PAGE_ENCRYPTED_RTREE); > - > + ib::fatal() << "Page " << space << ":" << offset > + << " name " << (rspace ? rspace->name : "???") > + << " page_type " << page_type > + << " key_version " > + << mach_read_from_4(page + > FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION) > + << " lsn " << mach_read_from_8(page + FIL_PAGE_LSN) > + << " compressed_len " << mach_read_from_2(page + > FIL_PAGE_DATA); > return false; > } > > > ok to push, rest of the changes. R: Jan

2 1

Re: [Maria-developers] de9ef13: MDEV-10956 Strict Password Validation Breaks Replication.
by Sergei Golubchik 16 Jan '17

16 Jan '17

Hi, Alexey! On Jan 16, Alexey Botchkov wrote: > revision-id: de9ef1390655abfb2b46a158148a9e27410298a2 (mariadb-10.1.20-44-gde9ef13) > parent(s): ab1e6fefd869242d962cb91a006f37bb9ad534a7 > committer: Alexey Botchkov > timestamp: 2017-01-16 02:20:15 +0400 > message: > > MDEV-10956 Strict Password Validation Breaks Replication. > > Password validation switched off completely for slave threads. I'd rather disable only strict_password_validation check in replication threads. Because it, indeed, makes no sense there. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] eb8d31d: MDEV-10232: Scalar result of subquery changes after adding an outer select stmt
by Sergey Petrunia 16 Jan '17

16 Jan '17

Hi Varun, Please make the fix for MDEV-10232 to be one changeset. Ok to push that changeset. On Sun, Jan 15, 2017 at 07:44:35PM +0530, Varun wrote: > revision-id: eb8d31d96f32da53d34d4c992fba061fc8732078 (mariadb-10.1.20-38-geb8d31d) > parent(s): 905aac9c78068224839db551a84ed080167d1657 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-15 19:40:46 +0530 > message: > > MDEV-10232: Scalar result of subquery changes after adding an outer select stmt > > postreview > > --- > sql/sql_select.cc | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 13eda46..85bfb6b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1614,7 +1614,8 @@ JOIN::optimize_inner() > <fields> to ORDER BY <fields>. There are three exceptions: > - if skip_sort_order is set (see above), then we can simply skip > GROUP BY; > - - if we are in a subquery, we don't have to maintain order > + - if we are in a subquery, we don't have to maintain order unless there > + is a limit clause in the subquery. > - we can only rewrite ORDER BY if the ORDER BY fields are 'compatible' > with the GROUP BY ones, i.e. either one is a prefix of another. > We only check if the ORDER BY is a prefix of GROUP BY. In this case > @@ -1626,7 +1627,7 @@ JOIN::optimize_inner() > if (!order || test_if_subpart(group_list, order)) > { > if (skip_sort_order || > - select_lex->master_unit()->item && !select_limit) // This is a subquery > + (select_lex->master_unit()->item && select_limit == HA_POS_ERROR)) // This is a subquery > order= NULL; > else > order= group_list; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0