- developers - lists.mariadb.org

[Maria-developers] Fwd: [Commits] d1124c9: MDEV-10812 WSREP causes responses being sent to protocol commands that must not send a response
by Sachin Setiya 27 Jan '17

27 Jan '17

Hi Nirbhay! On Wed, Jan 25, 2017 at 7:53 PM, Nirbhay Choubey <nirbhay(a)mariadb.com> wrote: > Hi Sachin, > > My comments below. > > On Sun, Jan 22, 2017 at 7:18 AM, SachinSetiya <sachin.setiya(a)mariadb.com> > wrote: >> >> revision-id: d1124c98cbede6366e4b4d27a23dcf5b9d207cf0 >> (mariadb-10.1.21-2-gd1124c9) >> parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e >> author: Sachin Setiya >> committer: Sachin Setiya >> timestamp: 2017-01-22 17:47:29 +0530 >> message: >> >> MDEV-10812 WSREP causes responses being sent to protocol commands that >> must not send a response >> >> Problem:- When using wsrep (w/ galera) and issuing commands that can cause >> deadlocks, deadlock exception errors are sent in responses to commands >> such as close prepared statement () which, by spec, must not send a > > > s/close prepared statement ()/close prepared statement and close connection/ > >> response. >> >> Solution:- We will not jump to dispatch_end: when there is deadlock and >> query is either COM_QUIT or COM_STMT_CLOSE. > > > Or, I would say, in dispatch_command, we handle COM_QUIT and COM_STMT_CLOSE > commands even in case of error. Changed. > >> >> >> Patch Credit:- Jaka Močnik >> >> --- >> .../galera/r/galera_prepared_statement.result | 18 ++++++++++++++++ >> .../suite/galera/t/galera_prepared_statement.test | 25 >> ++++++++++++++++++++++ >> sql/sql_parse.cc | 6 +++++- >> 3 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/mysql-test/suite/galera/r/galera_prepared_statement.result >> b/mysql-test/suite/galera/r/galera_prepared_statement.result >> index de5ac9c..3425648 100644 >> --- a/mysql-test/suite/galera/r/galera_prepared_statement.result >> +++ b/mysql-test/suite/galera/r/galera_prepared_statement.result >> @@ -27,7 +27,25 @@ ALTER TABLE t1 ADD COLUMN f2 INTEGER; >> ALTER TABLE t1 DROP COLUMN f1; >> EXECUTE st1; >> ERROR 22007: Incorrect integer value: 'abc' for column 'f2' at row 1 >> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; >> +CREATE TABLE t5(a int primary key); >> +INSERT INTO t5 VALUES(1),(2),(3); >> +START TRANSACTION ; >> +PREPARE st5 FROM 'SELECT * FROM t5'; >> +EXECUTE st5; >> +a >> +1 >> +2 >> +3 >> +update t5 set a=a+100; >> +EXECUTE st5; >> +a >> +101 >> +102 >> +103 >> +update t5 set a=a+100; >> DROP TABLE t1; >> DROP TABLE t2; >> DROP TABLE t3; >> DROP TABLE t4; >> +DROP TABLE t5; >> >> diff --git a/mysql-test/suite/galera/t/galera_prepared_statement.test >> b/mysql-test/suite/galera/t/galera_prepared_statement.test >> index 3bee097..debc00e 100644 >> --- a/mysql-test/suite/galera/t/galera_prepared_statement.test >> +++ b/mysql-test/suite/galera/t/galera_prepared_statement.test >> @@ -38,8 +38,33 @@ ALTER TABLE t1 DROP COLUMN f1; >> --error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD >> EXECUTE st1; >> >> +# MDEV-10812 >> +# On Closing Prepare Stamemnt, When wsrep_conflict_state is ABORTED >> +# It causes wrong response to be sent on Clients. >> + >> +# First Create a Deadlock >> --connection node_1 >> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; >> +CREATE TABLE t5(a int primary key); >> +INSERT INTO t5 VALUES(1),(2),(3); >> +START TRANSACTION ; >> +PREPARE st5 FROM 'SELECT * FROM t5'; >> +EXECUTE st5; >> +update t5 set a=a+100; >> +EXECUTE st5; >> + >> +--sleep 2 >> +--connection node_2 >> +update t5 set a=a+100; >> + >> +--sleep 2 >> +--connection node_1 >> +# here we have deadlock >> +--disconnect node_1 >> + >> +--connection node_2 >> DROP TABLE t1; >> DROP TABLE t2; >> DROP TABLE t3; >> DROP TABLE t4; >> +DROP TABLE t5; > > > > The test always hits command != COM_QUIT assert if I revert : > > + if (thd->wsrep_conflict_state == ABORTED && > + command != COM_STMT_CLOSE && command != COM_QUIT) > > So, I modified your test a bit (simplified) to not use PS. > > --echo # > --echo # MDEV-10812: On closing prepare stamemnt, when wsrep_conflict_state > --echo # is ABORTED, it causes wrong response to be sent to the client > --echo # > > # First create a deadlock > --connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1 > SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > CREATE TABLE t1(a INT PRIMARY KEY); > INSERT INTO t1 VALUES(1),(2),(3); > START TRANSACTION ; > UPDATE t1 SET a=a+100; > > --sleep 2 > --connection node_2 > UPDATE t1 SET a=a+100; > > --sleep 2 > --connection node_1a > # here we get deadlock error > --disconnect node_1a > > --connection node_2 > DROP TABLE t1; > > I think it should be ok. > > Yes, also Because Initial prepared statement test was written because, I thought we will hit COM_STMT_CLOSE, but is simple not possible using mtr. Test changed. >> >> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc >> index 033e88a..d45b196 100644 >> --- a/sql/sql_parse.cc >> +++ b/sql/sql_parse.cc >> @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command >> command, THD *thd, >> { >> wsrep_client_rollback(thd); >> } >> - if (thd->wsrep_conflict_state== ABORTED) >> + /* we let COM_QUIT and STMT_CLOSE execute even if wsrep aborted */ > Changed. > > We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep aborted. > >> + if (thd->wsrep_conflict_state == ABORTED && >> + command != COM_STMT_CLOSE && command != COM_QUIT) >> { >> my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction"); >> WSREP_DEBUG("Deadlock error for: %s", thd->query()); >> @@ -1918,6 +1920,8 @@ bool dispatch_command(enum enum_server_command >> command, THD *thd, >> >> if (WSREP(thd)) >> { >> + DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE) >> + || thd->get_stmt_da()->is_disabled()); > > > Please add a comment explaining this assert. > Added. > Thanks, > Nirbhay > >> >> /* wsrep BF abort in query exec phase */ >> mysql_mutex_lock(&thd->LOCK_wsrep_thd); >> do_end_of_statement= thd->wsrep_conflict_state != REPLAYING && >> _______________________________________________ >> commits mailing list >> commits(a)mariadb.org >> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits > > http://lists.askmonty.org/pipermail/commits/2017-January/010534.html -- Regards Sachin Setiya Software Engineer at MariaDB -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

[Maria-developers] MDEV-10142 - / MDEV-10343 - bb-10.2-compatibility
by jerome brauge 27 Jan '17

27 Jan '17

Hi, I'm testing datatype compatibility provide by MDEV-10343 and I have some comments: 1) Oracle character datatype (char, varchar, varchar2) doesn't accept a length of 0 char Example : select cast('tt' as char(0)) from dual; ORA-01723: zero-length columns are not allowed MariaDB return an empty string (to relate with MDEV-10574) This point is a study case, no matter. 2) NUMBER Type You have choosing TYPE_DOUBLE if scale/prec is not specified and I think that it's a mistake for 2 reasons: - Oracle NUMBER() is near an "exact" datatype and cannot be replace by a standard double. - Convert number to char never use scientific notation The use of a decimal(64,24) makes it possible to obtain results very close to those of oracle. I attach two files to show this. In fact FLOAT and INTEGER are subtype of NUMBER for Oracle. MariaDB float and double seems to be like BINARY_FLOAT and BINARY_DOUBLE. See https://docs.oracle.com/cd/B28359_01/server.111/b28285/sqlqr06.htm#CHDBBHHE for more details. Best regard, Jérôme.

2 1

Re: [Maria-developers] [Commits] fb49b98: MDEV-10436 non-deterministic vcol does not force rbr
by Daniel Black 27 Jan '17

27 Jan '17

On 25/01/17 00:33, sachin wrote: > @@ -5824,6 +5826,16 @@ int THD::decide_logging_format(TABLE_LIST *tables) > table->table->file->ht) > multi_access_engine= TRUE; > > + /* > + Loop through each table field if it is virtual field check if it is deterministic > + or not. > + */ > + for (field_ptr= table->table->field; *field_ptr; field_ptr++) > + { > + field= *field_ptr; > + if (field->vcol_info && field->vcol_info->flags & VCOL_NOT_STRICTLY_DETERMINISTIC) > + lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_NON_DETER_VCOL); you could break the loop at this point. > + } > prev_access_table= table->table; > } >

1 0

[Maria-developers] MDEV-11829 - Questions on timezones
by Seth Shelnutt 26 Jan '17

26 Jan '17

I am working on a patch for adding datetime parsing support for RFC3339 as reported in MDEV-11829. In examining methods to use the provided offset to convert the parsed datetime to the timezone of mariadb. I have some questions on the my_system_gmt_sec function in sql-common/my_time.c . >From testing on linux (rhel 7 and gentoo), it seems the timezone computed by my_system_gmt_sec is off by 7200 seconds and has a inverse sign. I am trying to determine why this is and if it is designed to be this way. From the comments in the file, it seems my_system_gmt_sec was designed specifically due to cross-platform issues. However there is no mention that it the timezone computed is suppose to be off or have an inverse sign. I could not find any usages of the my_time_zone global variable or where another function used the timezone computed by my_system_gmt_sec, so this might not effect anything, but I wanted some clarification. Here is some testing results: | ---------------------------- | ------------------------------------------ | -------------- | | Server TimeZone | my_system_gmt_sec timezone | Difference | | ---------------------------- | ------------------------------------------ | -------------- | | -18000 (-05:00 EST) | 25200 | 43200 | | 3600 (+01:00 CET) | -10800 | 14400 | | 0 (+00:00 GMT) | 7200 | 7200 | | ---------------------------- | ------------------------------------------ | -------------- | In testing I've been able to determine that the 7200 second deviation is caused by compensating for the "-3600" seconds twice, first in setting an initial timezone in my_init_time: https://github.com/MariaDB/server/blob/10.1/sql-common/my_time.c#L709 Inside my_system_gmt_sec the "current_timezone" is set to the "my_time_zone" which is at 3600 initially. Then below another 3600 is added to the timezone computation: https://github.com/MariaDB/server/blob/10.1/sql-common/my_time.c#L946 The diff variable that is computed has the correct offset, with an inverse sign. If current_timezone were set to "diff" then it would have the correct timezone but again with an inverse sign. The inverse sign comes from the doing t - l_time instead of l_time - t. Should the timezone computed have an inverse sign? Is there a reason it is offset by 7200 seconds? Perhaps the behavior is different on other platform, that I have not tested. Thank you, Seth Shelnutt

1 0

[Maria-developers] MDEV-11913 Split sp_get_item_value() into methods in Type_handler
by Alexander Barkov 26 Jan '17

26 Jan '17

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

1 0

Re: [Maria-developers] [Commits] d1124c9: MDEV-10812 WSREP causes responses being sent to protocol commands that must not send a response
by Nirbhay Choubey 25 Jan '17

25 Jan '17

Hi Sachin, My comments below. On Sun, Jan 22, 2017 at 7:18 AM, SachinSetiya <sachin.setiya(a)mariadb.com> wrote: > revision-id: d1124c98cbede6366e4b4d27a23dcf5b9d207cf0 > (mariadb-10.1.21-2-gd1124c9) > parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-22 17:47:29 +0530 > message: > > MDEV-10812 WSREP causes responses being sent to protocol commands that > must not send a response > > Problem:- When using wsrep (w/ galera) and issuing commands that can cause > deadlocks, deadlock exception errors are sent in responses to commands > such as close prepared statement () which, by spec, must not send a > s/close prepared statement ()/close prepared statement and close connection/ response. > > Solution:- We will not jump to dispatch_end: when there is deadlock and > query is either COM_QUIT or COM_STMT_CLOSE. > Or, I would say, in dispatch_command, we handle COM_QUIT and COM_STMT_CLOSE commands even in case of error. > > Patch Credit:- Jaka Močnik > > --- > .../galera/r/galera_prepared_statement.result | 18 ++++++++++++++++ > .../suite/galera/t/galera_prepared_statement.test | 25 > ++++++++++++++++++++++ > sql/sql_parse.cc | 6 +++++- > 3 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/galera/r/galera_prepared_statement.result > b/mysql-test/suite/galera/r/galera_prepared_statement.result > index de5ac9c..3425648 100644 > --- a/mysql-test/suite/galera/r/galera_prepared_statement.result > +++ b/mysql-test/suite/galera/r/galera_prepared_statement.result > @@ -27,7 +27,25 @@ ALTER TABLE t1 ADD COLUMN f2 INTEGER; > ALTER TABLE t1 DROP COLUMN f1; > EXECUTE st1; > ERROR 22007: Incorrect integer value: 'abc' for column 'f2' at row 1 > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t5(a int primary key); > +INSERT INTO t5 VALUES(1),(2),(3); > +START TRANSACTION ; > +PREPARE st5 FROM 'SELECT * FROM t5'; > +EXECUTE st5; > +a > +1 > +2 > +3 > +update t5 set a=a+100; > +EXECUTE st5; > +a > +101 > +102 > +103 > +update t5 set a=a+100; > DROP TABLE t1; > DROP TABLE t2; > DROP TABLE t3; > DROP TABLE t4; > +DROP TABLE t5; diff --git a/mysql-test/suite/galera/t/galera_prepared_statement.test > b/mysql-test/suite/galera/t/galera_prepared_statement.test > index 3bee097..debc00e 100644 > --- a/mysql-test/suite/galera/t/galera_prepared_statement.test > +++ b/mysql-test/suite/galera/t/galera_prepared_statement.test > @@ -38,8 +38,33 @@ ALTER TABLE t1 DROP COLUMN f1; > --error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD > EXECUTE st1; > > +# MDEV-10812 > +# On Closing Prepare Stamemnt, When wsrep_conflict_state is ABORTED > +# It causes wrong response to be sent on Clients. > + > +# First Create a Deadlock > --connection node_1 > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t5(a int primary key); > +INSERT INTO t5 VALUES(1),(2),(3); > +START TRANSACTION ; > +PREPARE st5 FROM 'SELECT * FROM t5'; > +EXECUTE st5; > +update t5 set a=a+100; > +EXECUTE st5; > + > +--sleep 2 > +--connection node_2 > +update t5 set a=a+100; > + > +--sleep 2 > +--connection node_1 > +# here we have deadlock > +--disconnect node_1 > + > +--connection node_2 > DROP TABLE t1; > DROP TABLE t2; > DROP TABLE t3; > DROP TABLE t4; > +DROP TABLE t5; > The test always hits command != COM_QUIT assert if I revert : + if (thd->wsrep_conflict_state == ABORTED && + command != COM_STMT_CLOSE && command != COM_QUIT) So, I modified your test a bit (simplified) to not use PS. --echo # --echo # MDEV-10812: On closing prepare stamemnt, when wsrep_conflict_state --echo # is ABORTED, it causes wrong response to be sent to the client --echo # # First create a deadlock --connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1 SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; CREATE TABLE t1(a INT PRIMARY KEY); INSERT INTO t1 VALUES(1),(2),(3); START TRANSACTION ; UPDATE t1 SET a=a+100; --sleep 2 --connection node_2 UPDATE t1 SET a=a+100; --sleep 2 --connection node_1a # here we get deadlock error --disconnect node_1a --connection node_2 DROP TABLE t1; I think it should be ok. > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 033e88a..d45b196 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > { > wsrep_client_rollback(thd); > } > - if (thd->wsrep_conflict_state== ABORTED) > + /* we let COM_QUIT and STMT_CLOSE execute even if wsrep aborted */ > We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep aborted. + if (thd->wsrep_conflict_state == ABORTED && > + command != COM_STMT_CLOSE && command != COM_QUIT) > { > my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction"); > WSREP_DEBUG("Deadlock error for: %s", thd->query()); > @@ -1918,6 +1920,8 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > > if (WSREP(thd)) > { > + DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE) > + || thd->get_stmt_da()->is_disabled()); > Please add a comment explaining this assert. Thanks, Nirbhay > /* wsrep BF abort in query exec phase */ > mysql_mutex_lock(&thd->LOCK_wsrep_thd); > do_end_of_statement= thd->wsrep_conflict_state != REPLAYING && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] MDEV-11780 Crash with PREPARE + SP out parameter + literal
by Alexander Barkov 24 Jan '17

24 Jan '17

Hello Sergei, Please review a patch for MDEV-11780. Thanks!

2 3

[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