- developers - lists.mariadb.org

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

14 Jan '17

Hi Varun, On Fri, Jan 13, 2017 at 08:22:49PM +0530, Varun wrote: > revision-id: 905aac9c78068224839db551a84ed080167d1657 (mariadb-10.1.20-37-g905aac9) > parent(s): ebb8c9fb26f86cff8c0d81bd2415f415cef952bb > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-13 20:17:33 +0530 > message: > > MDEV-10232: Scalar result of subquery changes after adding an outer select stmt > > In a subquery, we don't have to maintain order > Added a fix such that order is considered when there is a limit clause. > > --- > mysql-test/r/subselect4.result | 12 ++++++++++++ > mysql-test/t/subselect4.test | 10 ++++++++++ > sql/sql_select.cc | 2 +- > 3 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/subselect4.result b/mysql-test/r/subselect4.result > index 87645d1..7764783 100644 > --- a/mysql-test/r/subselect4.result > +++ b/mysql-test/r/subselect4.result > @@ -2401,5 +2401,17 @@ SELECT x FROM t1 WHERE id > (SELECT MAX(id) - 1000 FROM t1) ORDER BY x LIMIT 1; > x > 0 > drop table t1; > +# > +# MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +# > +create table t1(c1 int, c2 int, primary key(c2)); > +insert into t1 values(2,1),(1,2); > +select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; > +x > +1 > +(select c1 from t1 group by c1,c2 order by c1 limit 1); > +c1 > +1 > +drop table t1; > SET optimizer_switch= @@global.optimizer_switch; > set @@tmp_table_size= @@global.tmp_table_size; > diff --git a/mysql-test/t/subselect4.test b/mysql-test/t/subselect4.test > index 4eb9701..787c579 100644 > --- a/mysql-test/t/subselect4.test > +++ b/mysql-test/t/subselect4.test > @@ -1956,5 +1956,15 @@ SELECT x FROM t1 WHERE id > (SELECT MAX(id) - 1000 FROM t1) ORDER BY x LIMIT 1; > > drop table t1; > > +--echo # > +--echo # MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +--echo # > + > +create table t1(c1 int, c2 int, primary key(c2)); > +insert into t1 values(2,1),(1,2); > +select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; > +(select c1 from t1 group by c1,c2 order by c1 limit 1); > +drop table t1; > + > SET optimizer_switch= @@global.optimizer_switch; > set @@tmp_table_size= @@global.tmp_table_size; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 3f06ec8..13eda46 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1626,7 +1626,7 @@ JOIN::optimize_inner() Above this change we still have a comment with this statement: >> - if we are in a subquery, we don't have to maintain order Please adjust it accordingly. > if (!order || test_if_subpart(group_list, order)) > { > if (skip_sort_order || > - select_lex->master_unit()->item) // This is a subquery > + select_lex->master_unit()->item && !select_limit) // This is a subquery > order= NULL; > else > order= group_list; So I'm trying with the patch: MariaDB [test]> explain select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | 1 | PRIMARY | NULL | NULL | NULL | NULL | NULL | NULL | NULL | No tables used | | 2 | SUBQUERY | t1 | ALL | NULL | NULL | NULL | NULL | 2 | Using filesort | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ 2 rows in set (20.08 sec) Good. What if there is no LIMIT clause? MariaDB [test]> explain select (select c1 from t1 group by c1,c2 order by c1 ) as x; +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | 1 | PRIMARY | NULL | NULL | NULL | NULL | NULL | NULL | NULL | No tables used | | 2 | SUBQUERY | t1 | ALL | NULL | NULL | NULL | NULL | 2 | Using filesort | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ 2 rows in set (6.06 sec) Bad. "Using filesort" should have been removed. The reason it fails is that "!select_limit" is not the right way to check the presense of LIMIT clause. Without limit, I have: (gdb) p/x select_limit $15 = 0xffffffffffffffff (the name of the constant to use is "HA_POS_ERROR") Another issue: I get this warning with the new code: || /home/psergey/dev-git/10.1/sql/sql_select.cc: In member function 'int JOIN::optimize_inner()': sql_select.cc|1629 col 45| warning: suggest parentheses around '&&' within '||' [-Wparentheses] || select_lex->master_unit()->item && !select_limit) // This is a subquery || ^ BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] a2a42a9: MDEV-11623: MariaDB 10.1 fails to start datadir created with MariaDB 10.0/MySQL 5.6 using innodb-page-size!=16K
by Marko Mäkelä 13 Jan '17

13 Jan '17

Sorry for the delay. This patch looks great to me, except for one thing: I think that we should write the corrected flags back to the data file. Given that 10.1 is already in GA status, we should consider our options carefully. My preference would be (1) below: (1) If we adjust 10.1.21 so that it writes back corrected flags for both old and new data files, then older 10.1.x would be unable to open the files (if compression, DATA_DIRECTORY or non-default innodb_page_size was used). (2) If we adjust 10.1.21 so that it writes correct flags for new data files (like the current patch does), then older 10.1.x would not be able to read those created-with-newer-version data files. (3) If we adjust 10.2 only, so that it writes the correct flags for new files and also adjusts the flags in old files, then we would seem to have a clear upgrade path. Whatever we choose, I would like to remove the flag-adjustment code from 10.3 or 10.4. This would mean that an upgrade (or import) directly from 10.1 is only possible if the problematic features (DATA_DIRECTORY, compression, non-standard innodb_page_size) were not used. Otherwise the upgrade would have to be done through 10.2. On Thu, Dec 29, 2016 at 10:41 PM, Jan Lindström <jan.lindstrom(a)mariadb.com> wrote: > revision-id: a2a42a9181729b661ba308f2d0a75d8b547ed09d (mariadb-10.1.20-15-ga2a42a9) > parent(s): 23cc1be270c7304963643947d8e5ab88f4e312ee > author: Jan Lindström > committer: Jan Lindström > timestamp: 2016-12-29 22:40:30 +0200 > message: > > MDEV-11623: MariaDB 10.1 fails to start datadir created with MariaDB 10.0/MySQL 5.6 using innodb-page-size!=16K > > Problem was that in MariaDB 10.1 page size flag is on different > position (13) on tablespace flags compared to MySQL 5.6 and > MariaDB 10.0 or older position (6). DATA_DIR flag position difference > was already handled. > > /** Tablespace flags position and name in MySQL 5.6/MariaDB 10.0 or older > and MariaDB 10.1.20 or older MariaDB 10.1 and in MariaDB 10.1.21 > or newer. > MySQL 5.6 MariaDB 10.1.x MariaDB 10.1.21 > ==================================================================== > Below flags in same offset > ==================================================================== > 0: POST_ANTELOPE 0:POST_ANTELOPE 0: POST_ANTELOPE > 1: ZIP_SSIZE 1:ZIP_SSIZE 1: ZIP_SSIZE > 5: ATOMIC_BLOBS 5:ATOMIC_BLOBS 5: ATOMIC_BLOBS It could be clearer to explicitly identify multi-bit fields, e.g., 1..4 for ZIP_SSIZE. > ===================================================================== > Below note the difference in order > ===================================================================== > 6: PAGE_SSIZE 6:PAGE_COMPRESSION 6: PAGE_SSIZE > 10: DATA_DIR 7:PAGE_COMPRESSION_LEVEL 10: DATA_DIR > 11: UNUSED 11:ATOMIC_WRITES > ===================================================================== > Note that below flags have been moved > ===================================================================== > 13:PAGE_SSIZE 11: RESERVED (5.7 SHARED) > 17:DATA_DIR 12: RESERVED (5.7 TEMPORARY) > 18:UNUSED 13: RESERVED (5.7 ENCRYPTION) > 14: PAGE_COMPRESSION > 15: PAGE_COMPRESSION_LEVEL > 19: ATOMIC_WRITES > 21: UNUSED s/have been moved/were in incorrect position in MariaDB 10.1.x/ It could be good to explain the impact of this. That is, what would happen if we upgrade from MariaDB 10.0 to the buggy MariaDB 10.1, or if we upgrade from the buggy MariaDB 10.1 to 10.1.21 where this has been fixed. As far as I can tell, with respect to the DATA_DIR flag we should be mostly OK, because that flag should only play a role in the table flags when determining where the data file is located, not so much in the tablespace flags inside the data file. (Moving files from MariaDB 10.1 to MySQL 5.7 could be hurt, because the DATA_DIR flag would be interpreted as the 5.7 ENCRYPTION flag.) Next, let us consider PAGE_SSIZE (bits 13..16 in the buggy MariaDB 10.1.x, 6..9 elsewhere). Upgrade from 10.0 or 5.6 to the buggy 10.1.x: We read the bits 13..16. These would always be 0 in the old data files, so we will interpret the data file as if it had innodb_page_size=16k. That is, upgrade or import from 5.6 or 10.0 to the buggy 10.1.x will be refused if innodb_page_size differs from 16k. Is this correct? (This is something that we cannot fix, because we cannot retroactively change old MariaDB 10.1.x versions.) Upgrade from the buggy 10.1.x to the fixed 10.1.x or 10.2: We read the bits 6..9 which were incorrectly used as PAGE_COMPRESSION, PAGE_COMPRESSION_LEVEL. That is, upgrade should work if innodb_page_size=16k (the flags are expected to be 0) and compression is not used (the bits 6..9 were written as 0 by the buggy 10.1.x). To help users who use the buggy 10.1.x with innodb_page_size!=16k or with compression, we can adjust the flags: > fsp0fsp.ic: fsp_flags_get_page_size() : By default read > used page size from original position i.e. after ATOMIC_BLOBS. > But if we detect that buggy MariaDB 10.1 flags are used, then > we read it from FSP_FLAGS_PAGE_SSIZE_MARIADB101. [snip] > Tested also with MySQL 5.7 datadir (after deleting redo logs > and copying mysql/* files from 10.1) but that failed because > SYS_INDEXES got an extra column in 5.7 SYS_INDEXES.MERGE_THRESHOLD. You could theoretically have initialized the database in MySQL 5.6 and then upgraded it to 5.7. All tables that were created in 5.6 should be accessible. But would not have added much value. > After shutdown, started MariaDB 10.1.21 with fix and > verified that all tables created are accessible. Were the flags corrected in the data file afterwards? I think that we should correct the flags. Please add a mysql-test case that demonstrates that the flags are corrected. Something like this: --source include/have_innodb.inc --source include/not_embedded.inc # restart will restore the old values SET GLOBAL innodb_file_per_table=1; SET GLOBAL innodb_file_format=Barracuda; let INNODB_PAGE_SIZE=`select @@innodb_page_size`; let MYSQLD_DATADIR=`select @@datadir`; CREATE TABLE tr(a INT)ENGINE=InnoDB ROW_FORMAT=REDUNDANT; CREATE TABLE tc(a INT)ENGINE=InnoDB ROW_FORMAT=COMPACT; CREATE TABLE td(a INT)ENGINE=InnoDB ROW_FORMAT=DYNAMIC; CREATE TABLE tz(a INT)ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=1; # TODO: repeat the same with DATA_DIRECTORY and COMPRESSION --source include/shutdown_mysqld.inc perl; # corrupt the FSP_FLAGS in each file; write the page checksum as 0xdeadbeef # (see log_data_file_size.test or some recent test from MySQL) EOF; --let $restart_parameters = --innodb-read-only; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc perl; # check that the FSP_FLAGS are still corrupted # (they must not be adjusted in read-only mode) EOF; --let $restart_parameters=; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc perl; # check that the FSP_FLAGS now correspond to $ENV{INNODB_PAGE_SIZE} EOF; --source include/start_mysqld.inc DROP TABLE tr,tc,td,tz; > @@ -3859,21 +3837,11 @@ fil_open_single_table_tablespace( > + /* Validate this single-table-tablespace with SYS_TABLES. */ > + bool flags_correct = dict_compare_flags(def.flags, flags); I think that the function name should be more descriptive, making clear that the first argument contains the table flags and the second the FIL_SPACE_FLAGS: dict_tf_match_space(def.flags, flags) The comment does not really add any value. I would omit it. > diff --git a/storage/innobase/fsp/fsp0fsp.cc b/storage/innobase/fsp/fsp0fsp.cc > index 87aa5f7..7ff5244 100644 > --- a/storage/innobase/fsp/fsp0fsp.cc > +++ b/storage/innobase/fsp/fsp0fsp.cc > @@ -4182,3 +4182,141 @@ fsp_page_is_free_func( > return xdes_mtr_get_bit( > descr, XDES_FREE_BIT, page_no % FSP_EXTENT_SIZE, mtr); > } > + > +/********************************************************************//** > +Verify that dictionary flags modified to tablespace flags > +and actual tablespace flags stored to FSP header match. > +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) > +@param[in] fsp_flags Actual flags stored to FSP header in page 0 > +@return true if flags match, false if not */ > +bool > +fsp_verify_flags( > + ulint dict_flags, > + ulint fsp_flags) > +{ > + ulint dict_unused = FSP_FLAGS_GET_UNUSED(dict_flags); > + ulint dict_antelope = FSP_FLAGS_GET_POST_ANTELOPE(dict_flags); > + ulint dict_zssize = FSP_FLAGS_GET_ZIP_SSIZE(dict_flags); > + ulint dict_ablobs = FSP_FLAGS_HAS_ATOMIC_BLOBS(dict_flags); Add an early return before all these computations if dict_flags==fsp_flags. I think that the name dict_flags is misleading. Both flags are supposed to be tablespace flags, not data dictionary (or table) flags. How about this: s/dict_flags/expected/ s/fsp_flags/actual/ And then define the local variables with expected_ and actual_ prefixes, instead of dict_ and fsp_. That would make the code easier to follow. > + DBUG_EXECUTE_IF("fsp_verify_flags_failure", > + return(false);); Where is the test to exercise this? Do we even need this? I think it is better to inject faults into the data files using Perl code in mysql-test (see my previous suggestion). Other than this, the function looks OK to me. > diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h > index 42f93b5..479954d 100644 > --- a/storage/innobase/include/dict0dict.h > +++ b/storage/innobase/include/dict0dict.h > @@ -1904,6 +1904,17 @@ dict_table_get_index_on_first_col( > #endif /* !UNIV_HOTBACKUP */ > > > +/** Compare tablespace flags. > +@param[in] flags SYS_TABLES.flags > +@param[in] mod_flags Tablespace flags. > +@return true if flags match, false if not */ > +UNIV_INLINE > +bool > +dict_compare_flags( > + ulint flags, > + ulint mod_flags) > + MY_ATTRIBUTE((warn_unused_result)); > + Can we declare the inline function directly in the .h file, instead of splitting it into the .ic file? I would suggest different names: bool dict_tf_match_space(ulint table_flags, ulint fsp_flags); > @@ -980,12 +982,11 @@ dict_sys_tables_type_to_tf( > PAGE_COMPRESSION_LEVEL, ATOMIC_WRITES are the same. */ > flags |= type & (DICT_TF_MASK_ZIP_SSIZE > | DICT_TF_MASK_ATOMIC_BLOBS > - | DICT_TF_MASK_DATA_DIR > - | DICT_TF_MASK_PAGE_COMPRESSION > - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > - | DICT_TF_MASK_ATOMIC_WRITES > + | DICT_TF_MASK_DATA_DIR > + | DICT_TF_MASK_PAGE_COMPRESSION > + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > + | DICT_TF_MASK_ATOMIC_WRITES); > > - ); > > return(flags); > } This appears to be a white-space-only change. We could avoid it, unless this is actually fixing the indentation. The spaces and TABs got lost in transit, so I cannot tell.. > @@ -1018,8 +1019,8 @@ dict_tf_to_sys_tables_type( > type |= flags & (DICT_TF_MASK_ZIP_SSIZE > | DICT_TF_MASK_ATOMIC_BLOBS > | DICT_TF_MASK_DATA_DIR > - | DICT_TF_MASK_PAGE_COMPRESSION > - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > + | DICT_TF_MASK_PAGE_COMPRESSION > + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > | DICT_TF_MASK_ATOMIC_WRITES); > > return(type); Same here. > DBUG_EXECUTE_IF("dict_tf_verify_flags_failure", > - return(ULINT_UNDEFINED);); > + return(false);); Nice catch. But is there any test to exercise this? If not, I would remove the DBUG_EXECUTE_IF altogether. > ut_a(page_ssize == 0 || page_ssize != 0); /* silence compiler */ > ut_a(compact == 0 || compact == 1); /* silence compiler */ > ut_a(data_dir == 0 || data_dir == 1); /* silence compiler */ > - ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler */ > + ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler > + */ I think it would be better to declare these variables as __attribute__((unused)) or to remove the variables altogether if they are unused. > + > + if (table_unused || fsp_unused) { > + ib_logf(IB_LOG_LEVEL_ERROR, > + "Table flags has unused %ld" > + " in the data dictionary" > + " but the flags in file has unused %ld\n", > + table_unused, fsp_unused); > + > + return (false); > + } 0x%lx would be more user-friendly here. > + ib_logf(IB_LOG_LEVEL_ERROR, > + "Table flags has page_compression %ld" > + " in the data dictionary" > + " but the flags in file ahas page_compression %ld\n", > page_compression, fsp_page_compression); s/ahas/has/ > +/********************************************************************//** > +Verify that dictionary flags modified to tablespace flags > +and actual tablespace flags stored to FSP header match. > +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) > +@param[in] fsp_flags Actual flags stored to FSP header in page 0 > +@return true if flags match, false if not */ > +bool > +fsp_verify_flags( > + ulint dict_flags, > + ulint fsp_flags) > + MY_ATTRIBUTE((warn_unused_result)); > + This seems to be missing UNIV_INTERN? I think we need it in 10.1 at least for innodb_plugin. > + /** In MariaDB 10.1.20 or older MariaDB 10.1 PAGE_SSIZE > + was stored after ATOMIC_WRITES not after ATOMIC_BLOBS > + as in MySQL 5.6, MariaDB 10.0 or older. New tables > + in MariDB 10.1.21 or newer store PAGE_SSIZE after ATOMIC_BLOBS. > + */ > + ssize = FSP_FLAGS_GET_PAGE_SSIZE(flags); Do not use /** comments before local variables. I think that one of the ‘after’ in the comment should be ‘before’. Also fix the typo MariDB. -- 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 1

[Maria-developers] Please review MDEV-9522 Split sql_select.cc:can_change_cond_ref_to_const into virtual methods in Type_handler
by Alexander Barkov 13 Jan '17

13 Jan '17

Hello Vicențiu, can you please review a patch for MDEV-9522 ? Thanks!

2 3

[Maria-developers] MDEV-11030 Assertion `precision > 0' failed in decimal_bin_size
by Alexander Barkov 12 Jan '17

12 Jan '17

Hello Sanja, Can you please review a patch for MDEV-11030? Thanks!

1 1

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

11 Jan '17

On Tue, Jan 10, 2017 at 11:49 AM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: c32bc1afe820aa90c2e2666a77ce2bb43019df3d (mariadb-10.2.3-43-gc32bc1afe82) > parent(s): 3d46768da2a784ddc9c341d1fb03468525bd38f1 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-01-10 11:36:42 +0200 > message: > > MDEV-11254: innodb-use-trim has no effect in 10.2 > > In MySQL 5.7 InnoDB os0file.[h|cc] was refactored making > trim or punch hole operation not work with MariaDB. > > buf0buf.cc: buf_page_is_corrupted() remember that compressed > pages do not contain checksum and FIL_TAIL header. What is a FIL_TAIL header? I suppose it is referring to the 8 bytes FIL_PAGE_DATA_END at the end of the page frame? And which compressed pages? ROW_FORMAT=COMPRESSED pages do contain a page footer. > buf0dblwr.cc: Set up the page size and write size to IORequest > and set up punch_hole() if actual write is not physical size. Actual write to where? Apparently (looking at the code), it is to the user data file, not to the doublewrite buffer. The commit message should mention this important detail. (I think we should not punch holes to the doublewrite buffer area (pages 64..191 in the system tablespace).) > buf0rea.cc, fil0fil.cc, fil0fil.h: Remove unnecessary parameter > write_size from os_file_write, fil_io, os_aio functions. I see that this parameter does not exist in MySQL 5.7. OK to remove. > os0file.h, os0file.cc: Implement new version of os_file_io_complete > to do punch_hole operation and punch hole operation should > be based on file system block size determined using fstat or > DeviceIOControl. Please also mention the InnoDB wrapper function os_file_get_block_size(), which was added. But, where is that function being called? Did you forget to adjust fil_node_open_file()? Please submit a revised patch for review. Some detailed comments follow. > +set autocommit=0; > +call innodb_insert_proc(16000); > +commit; > +set autocommit=1; Do we really have to insert this many records? What would be the minimum test case? Did you test with innodb_page_size=4k or 8k? Consider removing the two SET statements and adding a BEGIN before the CALL instead. That would be more standard SQL. > +--disable_query_log > +--disable_warnings > +EVAL SET GLOBAL innodb_compression_algorithm = $innodb_compression_algorithm_orig; > +EVAL SET GLOBAL innodb_file_per_table = $innodb_file_per_table_orig; > +EVAL SET GLOBAL innodb_file_format = $innodb_file_format_orig; > +--enable_warnings > +--enable_query_log There is no need to assign innodb_file_per_table or innodb_file_format in 10.2. The defaults are just fine. If you keep adding these statements in 10.2, merging to 10.3 will require extra work after 10.3 removes these deprecated parameters (copying WL#7704 from MySQL 8.0.0). > + bool page_encrypted = false; > + bool page_compressed = false; > + ulint page_type = mach_read_from_2(read_buf+FIL_PAGE_TYPE); > + > + page_compressed = (page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED > + || page_type == FIL_PAGE_PAGE_COMPRESSED); Please remove the initialization of page_compressed to false. Could we fuse page_compressed into page_encrypted to simplify those conditions that currently check for page_compressed&&page_encrypted? > @@ -978,9 +978,13 @@ buf_dblwr_write_block_to_datafile( > ut_a(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE); > buf_dblwr_check_page_lsn(block->frame); > > + if (bpage->real_size != bpage->size.physical()) { > + request.set_punch_hole(); > + } > + > fil_io(request, > - sync, bpage->id, bpage->size, 0, bpage->size.physical(), > - frame, block, (ulint *)&bpage->write_size); > + sync, bpage->id, bpage->size, 0, bpage->real_size, > + frame, block); > } > } I understand that this is the write to the final location. Sure, we should punch the hole there. > diff --git a/storage/innobase/buf/buf0flu.cc b/storage/innobase/buf/buf0flu.cc > index b82c4db18ad..de534cc6e89 100644 > --- a/storage/innobase/buf/buf0flu.cc > +++ b/storage/innobase/buf/buf0flu.cc > @@ -1093,11 +1093,15 @@ buf_flush_write_block_low( > > ulint type = IORequest::WRITE | IORequest::DO_NOT_WAKE; > > - IORequest request(type); > + IORequest request(type, bpage->size, (ulint *)&bpage->write_size); > + > + if (bpage->real_size != bpage->size.physical()) { > + request.set_punch_hole(); > + } Could we pass bpage as a parameter to the IORequest::IORequest(), and also include the set_punch_hole() call there? I think that buf_page_t is already too big, and we should not bloat it with a 64-bit field for write_size. The reason that I separated buf_page_t from buf_block_t was that for ROW_FORMAT=COMPRESSED, the block descriptors would use less space. Do we even need write_size in buf_page_t? I think it would better belong to buf_block_t. That is because ROW_FORMAT=COMPRESSED is never used together with hole-punching. > @@ -37,6 +37,7 @@ Created 10/21/1995 Heikki Tuuri > #define os0file_h > > #include "univ.i" > +#include "page0size.h" #include "page0size.h" also does #include "univ.i" > > #ifndef _WIN32 > #include <dirent.h> > @@ -554,6 +555,8 @@ class IORequest { > /** Default constructor */ > IORequest() > : > + m_page_size(univ_page_size), > + m_write_size(NULL), > m_block_size(UNIV_SECTOR_SIZE), > m_type(READ), > m_compression() We only have to add a single buf_page_t* const parameter. Maybe we could also remove m_block_size? > + /** > + @param[in] type Request type, can be a value that is > + ORed from the above enum > + @param[in] page_size Page size > + @param[in] write_size Actual write size */ > + IORequest(ulint type, > + const page_size_t& page_size, > + ulint* write_size) > + : > + m_page_size(page_size), > + m_write_size(write_size), > + m_block_size(UNIV_SECTOR_SIZE), > + m_type(static_cast<uint16_t>(type)), > + m_compression() > + { > + if (is_log()) { > + disable_compression(); > + } > + > + if (!is_punch_hole_supported()) { > + clear_punch_hole(); > + } > + } I do not think we need this constructor. > + /** operator = > + @param[in] from Source to copy */ > + IORequest& operator=(const IORequest& from) > + { > + m_page_size.copy_from(from.m_page_size); > + m_write_size = from.m_write_size; > + m_block_size = from.m_block_size; > + m_type = from.m_type; > + m_compression = from.m_compression; > + return (*this); > + } What is wrong with the compiler-generated assignment operator? If it complains that there is no page_size_t::operator=(), we can easily let it be generated by the compiler. What is this assignment needed for? > + /** Copy constructor. > + @param[in] from source. */ > + IORequest(const IORequest& from) > + : m_page_size(from.m_page_size.physical(), > + from.m_page_size.logical(), > + from.m_page_size.is_compressed()), > + m_write_size(from.m_write_size), > + m_block_size(from.m_block_size), > + m_type(from.m_type), > + m_compression(from.m_compression) > + { > + } > + What is this needed for? Why cannot the compiler generate this? As far as I can tell, this is just a bitwise copy. > @@ -688,6 +742,22 @@ class IORequest { > m_block_size = static_cast<uint32_t>(block_size); > } > > + /** Set the write size variable for later use. > + @param[in] write_size Write size variable */ > + void write_size(ulint* write_size) > + { > + m_write_size = write_size; > + } > + > + /** Set the actual write size done in IO > + @param[in] write_size Write size to set */ > + void write_size(ulint write_size) > + { > + if (m_write_size) { > + *m_write_size = write_size; > + } > + } > + The two methods do two completely different things, and at the very least they should have distinctive names. Do we need the first method at all? > +/***********************************************************************//** > +Try to get number of bytes per sector from file system. > +@return file block size */ > +UNIV_INTERN > +ulint > +os_file_get_block_size( > +/*===================*/ > + os_file_t file, /*!< in: handle to a file */ > + const char* name); /*!< in: file name */ Do not use /*****/ or /*====*/ or /*!< comments in new code. This function appears to be unused. > -/** Decompress after a read and punch a hole in the file if it was a write > +/** Punch a hole in the file if it was a write > @param[in] type IO context > @param[in] fh Open file handle > -@param[in,out] buf Buffer to transform > -@param[in,out] scratch Scratch area for read decompression > -@param[in] src_len Length of the buffer before compression > -@param[in] len Compressed buffer length for write and size > - of buf len for read > +@param[in] len Compressed buffer length for write > @return DB_SUCCESS or error code */ > static > dberr_t > os_file_io_complete( > - const IORequest&type, > + IORequest& type, > os_file_t fh, > - byte* buf, > - byte* scratch, > - ulint src_len, > ulint offset, > ulint len); Why is the const qualifier removed? > @@ -832,6 +825,69 @@ os_aio_simulated_handler( > void** m2, > IORequest* type); > > +/***********************************************************************//** > +Try to get number of bytes per sector from file system. > +@return file block size */ > +UNIV_INTERN > +ulint > +os_file_get_block_size( > +/*===================*/ > + os_file_t file, /*!< in: handle to a file */ > + const char* name) /*!< in: file name */ > +{ > + ulint fblock_size = 512; > + > +#if defined(UNIV_LINUX) > + struct stat local_stat; > + int err; > + > + err = fstat((int)file, &local_stat); > + > + if (err != 0) { > + ib::warn() << "fstat() failed on file " << name > + << " error "<< errno << " : " << strerror(errno); > + os_file_handle_error_no_exit(name, "fstat()", FALSE); > + } else { > + fblock_size = local_stat.st_blksize; > + } > +#endif /* UNIV_LINUX */ Why only Linux? fstat() is already in POSIX.1-2001. > @@ -953,8 +1009,8 @@ class AIOHandler { > ut_a(slot->type.is_read() || !slot->skip_punch_hole); > > return(os_file_io_complete( > - slot->type, slot->file, slot->buf, > - slot->compressed_page, slot->original_len, > + const_cast<IORequest&>(slot->type), > + slot->file, > static_cast<ulint>(slot->offset), > slot->len)); > } Please never use const_cast in a context where the object can be modified. It is unclear to me why we would need to lose the constness of IORequest here. (The members of IORequest do not need changes. Writes through pointers in IORequest are allowed even if IORequest itself is const.) > @@ -1659,85 +1715,78 @@ os_file_read_string( > + ulint amount = trim_len / type.block_size(); > + switch(type.block_size()) { > + case 512: > + srv_stats.page_compression_trim_sect512.add(amount); > + break; Can we store the log2 of the block_size somewhere and use an array of compression attributes, to avoid so much repeated code? > @@ -4252,6 +4300,7 @@ os_file_punch_hole_win32( > fh, FSCTL_SET_ZERO_DATA, &punch, sizeof(punch), > NULL, 0, &temp, NULL); > > + > return(!result ? DB_IO_NO_PUNCH_HOLE : DB_SUCCESS); > } > One empty line is enough. Marko -- 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] [JIRA] (MDEV-11685) sql_mode can't be set with non-ascii connection charset
by Alexander Barkov 11 Jan '17

11 Jan '17

Hello Nirbhay, On 01/09/2017 09:48 PM, Nirbhay Choubey wrote: > Hi Bar, > > On Mon, Jan 9, 2017 at 1:00 AM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Nirbhay, > > IIRC, you was working on this issue. > What is the current status? > > > Yes, I have just committed a patch. > Could you please take a look at it? The patch http://lists.askmonty.org/pipermail/commits/2017-January/010405.html looks Ok. I have a proposal for tests. The problem is that utf32, utf16 and ucs2 are not always compiled-in. So in case when some of the affected charsets is missing, sql_mode.test will fail. Can you please move the tests into ctype_utf32.test, ctype_utf16.test, ctype_ucs2.test ? I also suggest to add "SELECT @@sql_mode", to make sure that the assignment actually does what it's supposed to do. Remembering sql_mode is not necessary. It's OK to restore it to DEFAULT. Remembering character_set_connection is not necessary. It's Ok to do "SET NAMES utf8" at the end. Collecting all together, I'd suggest this chuck for ctype_utf32.test: --echo # --echo # MDEV-11685: sql_mode can't be set with non-ascii connection charset --echo # SET character_set_connection=utf32; SET sql_mode='NO_ENGINE_SUBSTITUTION'; SELECT @@sql_mode; SET sql_mode=DEFAULT; SET NAMES utf8; and similar chunks for ctype_utf16.test and ctype_ucs2.test. Thanks! > > Best, > Nirbhay > > > > Thanks! > > On 01/03/2017 09:31 PM, Elena Stepanova (JIRA) wrote: > > > > [ > https://jira.mariadb.org/browse/MDEV-11685?page=com.atlassian.jira.plugin.s… > <https://jira.mariadb.org/browse/MDEV-11685?page=com.atlassian.jira.plugin.s…> > ] > > > > Elena Stepanova reassigned MDEV-11685: > > -------------------------------------- > > > > Labels: upstream (was: ) > > Affects Version/s: 5.5 > > 10.0 > > 10.1 > > 10.2 > > Assignee: Alexander Barkov > > > >> sql_mode can't be set with non-ascii connection charset > >> ------------------------------------------------------- > >> > >> Key: MDEV-11685 > >> URL: https://jira.mariadb.org/browse/MDEV-11685 > <https://jira.mariadb.org/browse/MDEV-11685> > >> Project: MariaDB Server > >> Issue Type: Bug > >> Components: Character Sets > >> Affects Versions: 10.2.3, 5.5, 10.0, 10.1, 10.2 > >> Reporter: Nirbhay Choubey > >> Assignee: Alexander Barkov > >> Labels: upstream > >> > >> {code} > >> MariaDB [test]> set sql_mode = 'NO_ENGINE_SUBSTITUTION'; > >> Query OK, 0 rows affected (0.00 sec) > >> MariaDB [test]> set character_set_connection=utf32; > >> Query OK, 0 rows affected (0.00 sec) > >> MariaDB [test]> set sql_mode = 'NO_ENGINE_SUBSTITUTION'; > >> ERROR 1231 (42000): Variable 'sql_mode' can't be set to the value > of 'NO_ENGINE_SUBSTITUTION' > >> {code} > > > > > > > > -- > > This message was sent by Atlassian JIRA > > (v7.2.1#72003) > > > >

2 2

[Maria-developers] InnoDB: Instant ADD COLUMN that works on old data files
by Marko Mäkelä 11 Jan '17

11 Jan '17

I would like to seek technical review and feedback for an idea. In the long term, I would like InnoDB to support instant ALTER TABLE for a larger set of operations: https://jira.mariadb.org/browse/MDEV-11424 Instant ALTER TABLE of failure-free record format changes Probably one of the most common operations is ADD COLUMN. For that, a ticket had already been filed and some patches exist: https://jira.mariadb.org/browse/MDEV-11369 Instant add column for InnoDB Tencent and Alibaba have ADD COLUMN patches that basically introduce new InnoDB record formats so that each record will carry the number of columns, similar to how ROW_FORMAT=REDUNDANT (the original InnoDB row format) works. A problem with that approach is that we would only be able to support instant ADD COLUMN on data files that already are in a suitable format. The whole purpose of instant ALTER TABLE is to avoid conversions of huge data files. Requiring a conversion before the feature can be used would in my opinion defeat the purpose. I came up with a solution that only requires some changes to the InnoDB page format, not to the record formats: Require each record in a clustered index leaf page to contain the same number of fields (on write, convert the whole page). We can repurpose the previously unused (always 0) field PAGE_MAX_TRX_ID on clustered index leaf pages so that the number of fields in each leaf page can be determined. This is the idea that I posted in MDEV-11369 last week and edited today. As far as I can tell, it should work on any existing data file, and it would be a step towards the full-blown MDEV-11424 in a distant future version of MariaDB. One challenge with the page-at-a-time conversion is that inserting or updating records into a leaf page may require the page to be split into several pages, especially in the case where multiple CHAR(xxx) NOT NULL columns would need to be materialized as a result of the insert or update. Does this seem to make sense to you? I am looking forward to your feedback. Best regards, Marko -- 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

2 2

[Maria-developers] Fwd: Total No of events in event group.
by Sachin Setiya 10 Jan '17

10 Jan '17

Hi Kristian! On Tue, Jan 10, 2017 at 4:41 PM, Kristian Nielsen <knielsen(a)knielsen-hq.org> wrote: > Sachin Setiya <sachin.setiya(a)mariadb.com> writes: > >> Can we do something like send total no of Rows_log_events in a event >> group to slave. We can write this info in Gtid_log_event. Reason for >> doing this is that on slave we can show how much progress we have >> made. > > My immediate impression is that this sounds like a bad idea. > > It increases the binlog size without adding any real information - after all > the event count can be determined easily by just reading the events. > You mean counting the all events in event group without applying them. > It also requires careful design and testing for impact on forwards- and > backwards compatibility. > > And event count is not a very meaningful progress indicator. One event group > could have 10 Rows_log_event each with a single row operation. While another > could have a single Rows_log_event that contains a hundred row operations. > Okay, So it there a better way to show progress in slave ? > And why only rows event? What about Query_log_events in a transaction in > statement-based replication? We can generalize this idea to Query_log_events also. > > - Kristian. Regards sachin

2 3

[Maria-developers] Total No of events in event group.
by Sachin Setiya 10 Jan '17

10 Jan '17

Hi Kristian! Can we do something like send total no of Rows_log_events in a event group to slave. We can write this info in Gtid_log_event. Reason for doing this is that on slave we can show how much progress we have made. -- Regards sachin

2 1

[Maria-developers] Status of MDEV-4557 / 7409
by Sachin Setiya 08 Jan '17

08 Jan '17

Hi Daniel! I was thinking of taking 4557 , but in comment I see you have ported the patch for 10.0 but it was not merged. Are you going to take 4557 , or can I take it ? -- Regards Sachin Setiya Software Engineer at MariaDB

2 1