December 2016 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 12034ae: MDEV-10927: Crash When Using sort_union Optimization
by Sergey Petrunia 16 Dec '16

16 Dec '16

The patch is ok, but there is a problem with the testcase: when I run the testcase on the unmodified 5.5 binary, it passes. When I run the tescase from the MDEV, I get the crash. Please fix the testcase. This will also provide test coverage (which was lacking, and this is why have this bug in the first place). On Fri, Dec 16, 2016 at 11:19:43AM +0530, Varun wrote: > revision-id: 12034aec831d776725a43c7537ab4256efb1c974 (mariadb-5.5.53-25-g12034ae) > parent(s): 14e1f32894cdbe63a614738cfd93e9d0818dedee > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-16 11:03:24 +0530 > message: > > MDEV-10927: Crash When Using sort_union Optimization > > In file sql/filesort.cc,when merge_buffers() is called then > - queue_remove(&queue,0) is called > - For the function queue_remove there is assertion states that the element to be removed should have index >=1 > - this is causing the assertion to fail. > > Fixed by removing the top element. > > --- > mysql-test/r/index_merge_innodb.result | 52 ++++++++++++++++++++++++++++++++++ > mysql-test/t/index_merge_innodb.test | 42 ++++++++++++++++++++++++++- > sql/filesort.cc | 2 +- > 3 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/index_merge_innodb.result b/mysql-test/r/index_merge_innodb.result > index b93d15f..49e0a57 100644 > --- a/mysql-test/r/index_merge_innodb.result > +++ b/mysql-test/r/index_merge_innodb.result > @@ -792,4 +792,56 @@ a b c > 8 m m > 9 d d > DROP TABLE t1; > +# > +# MDEV-10927: Crash When Using sort_union Optimization > +# > +SET optimizer_switch='index_merge_sort_intersection=on'; > +CREATE TABLE IF NOT EXISTS t1 ( > +id int(11) NOT NULL AUTO_INCREMENT, > +contact_id int(11) NOT NULL, > +role_id int(11) NOT NULL, > +company_type_id int(11) NOT NULL, > +company_id int(11) NOT NULL, > +team_id int(11) NOT NULL, > +user_id int(11) NOT NULL, > +PRIMARY KEY (id), > +KEY contact_id (contact_id), > +KEY user_id (user_id), > +KEY company_id (company_id), > +KEY team_id (team_id) USING BTREE > +) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > +(5343408, 23913609, 0, 6, 2321, 0, 0), > +(7841449, 39582328, 1194, 0, 0, 0, 57291), > +(7841507, 28707292, 1194, 0, 0, 0, 57291), > +(7841543, 26376778, 1194, 0, 0, 0, 137571), > +(7841563, 42475686, 1194, 0, 0, 0, 137571), > +(7841573, 35956831, 1194, 0, 0, 0, 447223), > +(7841575, 37975231, 1194, 0, 0, 0, 447223), > +(7843049, 42693923, 1194, 0, 0, 0, 376261), > +(7843600, 42696877, 1194, 0, 0, 0, 376261), > +(12858344, 82851915, 1194, 0, 0, 0, 993011), > +(12858554, 80183799, 1194, 0, 0, 0, 993011), > +(12858803, 82649193, 1194, 0, 0, 0, 1288749), > +(12858878, 82700925, 1194, 0, 0, 0, 1288749), > +(12860248, 28642729, 1194, 0, 0, 0, 1234288), > +(12860258, 28642729, 0, 0, 0, 0, 1234288), > +(12860259, 28642729, 1194, 0, 0, 0, 0); > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +contact_id > +23913609 > +26376778 > +28642729 > +28707292 > +35956831 > +37975231 > +39582328 > +42475686 > +42693923 > +42696877 > +80183799 > +82649193 > +82700925 > +82851915 > +DROP TABLE t1; > set optimizer_switch= @optimizer_switch_save; > diff --git a/mysql-test/t/index_merge_innodb.test b/mysql-test/t/index_merge_innodb.test > index 6a1cb53..f1fdbbd 100644 > --- a/mysql-test/t/index_merge_innodb.test > +++ b/mysql-test/t/index_merge_innodb.test > @@ -172,5 +172,45 @@ WHERE ( tb.b != ta.b OR tb.a = ta.a ) > > DROP TABLE t1; > > -set optimizer_switch= @optimizer_switch_save; > +--echo # > +--echo # MDEV-10927: Crash When Using sort_union Optimization > +--echo # > > +SET optimizer_switch='index_merge_sort_intersection=on'; > + > +CREATE TABLE IF NOT EXISTS t1 ( > + id int(11) NOT NULL AUTO_INCREMENT, > + contact_id int(11) NOT NULL, > + role_id int(11) NOT NULL, > + company_type_id int(11) NOT NULL, > + company_id int(11) NOT NULL, > + team_id int(11) NOT NULL, > + user_id int(11) NOT NULL, > + PRIMARY KEY (id), > + KEY contact_id (contact_id), > + KEY user_id (user_id), > + KEY company_id (company_id), > + KEY team_id (team_id) USING BTREE > + ) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > + > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > + (5343408, 23913609, 0, 6, 2321, 0, 0), > + (7841449, 39582328, 1194, 0, 0, 0, 57291), > + (7841507, 28707292, 1194, 0, 0, 0, 57291), > + (7841543, 26376778, 1194, 0, 0, 0, 137571), > + (7841563, 42475686, 1194, 0, 0, 0, 137571), > + (7841573, 35956831, 1194, 0, 0, 0, 447223), > + (7841575, 37975231, 1194, 0, 0, 0, 447223), > + (7843049, 42693923, 1194, 0, 0, 0, 376261), > + (7843600, 42696877, 1194, 0, 0, 0, 376261), > + (12858344, 82851915, 1194, 0, 0, 0, 993011), > + (12858554, 80183799, 1194, 0, 0, 0, 993011), > + (12858803, 82649193, 1194, 0, 0, 0, 1288749), > + (12858878, 82700925, 1194, 0, 0, 0, 1288749), > + (12860248, 28642729, 1194, 0, 0, 0, 1234288), > + (12860258, 28642729, 0, 0, 0, 0, 1234288), > + (12860259, 28642729, 1194, 0, 0, 0, 0); > +--sorted_result > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +DROP TABLE t1; > +set optimizer_switch= @optimizer_switch_save; > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 5bb5c64..38404b0 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -1411,7 +1411,7 @@ int merge_buffers(SORTPARAM *param, IO_CACHE *from_file, > if (!(error= (int) read_to_buffer(from_file, buffpek, > rec_length))) > { > - queue_remove(&queue,0); > + (void) queue_remove_top(&queue); > reuse_freed_buff(&queue, buffpek, rec_length); > } > else if (error == -1) BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-11558 Split Item_type_holder::display_length into virtual methods in Type_handler
by Alexander Barkov 16 Dec '16

16 Dec '16

Hello Nirbhay, Can you please review a patch for MDEV-11558? Thanks!

2 1

Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
by Sergei Golubchik 15 Dec '16

15 Dec '16

Hi, Sergey! On Dec 13, Sergey Vojtovich wrote: > revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66) > parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d > committer: Sergey Vojtovich > timestamp: 2016-12-13 16:17:49 +0400 > message: > > MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES > > Implementation of MDEV-7660 introduced unwanted incompatible change: > modifications under LOCK TABLES with autocommit enabled are rolled back on > disconnect. Previously everything was committed, because LOCK TABLES didn't > adjust autocommit setting. Thanks, very helpful comment (that I didn't quote in my reply for brevity)! Few questions below: > diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test > index dea16c8..30fb694 100644 > --- a/mysql-test/t/delayed.test > +++ b/mysql-test/t/delayed.test > @@ -319,7 +319,7 @@ drop table if exists t1; > --enable_warnings > create table t1 (a int, b int); > insert into t1 values (1,1); > -lock table t1 read; > +lock table t1 read local; why? > connect (update,localhost,root,,); > connection update; > --send insert delayed into t1 values (2,2); > diff --git a/sql/mdl.h b/sql/mdl.h > index 7d659af..d77c07c 100644 > --- a/sql/mdl.h > +++ b/sql/mdl.h > @@ -196,6 +196,12 @@ enum enum_mdl_type { > */ > MDL_SHARED_UPGRADABLE, > /* > + A shared metadata lock for cases when we need to read data from table > + and block all concurrent modifications to it (for both data and metadata). > + Used by LOCK TABLES READ statement. > + */ > + MDL_SHARED_READ_ONLY, Hmm, stronger than MDL_SHARED_WRITE? All read locks were before write locks until now. Why is that changed? > + /* > An upgradable shared metadata lock which blocks all attempts to update > table data, allowing reads. > A connection holding this kind of lock can read table metadata and read > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 1ca7e9c..d3f4517 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > to general/slow log would be disabled in read only transactions. > */ > if (table_list->mdl_request.type >= MDL_SHARED_WRITE && > + table_list->mdl_request.type != MDL_SHARED_READ_ONLY && if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO > thd->tx_read_only && > !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK))) > { > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 6dc9192..0d740bb 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd) > static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) > { > Lock_tables_prelocking_strategy lock_tables_prelocking_strategy; > + MDL_deadlock_and_lock_abort_error_handler deadlock_handler; > + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); > uint counter; > TABLE_LIST *table; > > thd->in_lock_tables= 1; > > +retry: > + > if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy)) > goto err; > > - /* > - We allow to change temporary tables even if they were locked for read > - by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > - TABLES time and by the statement which is later executed under LOCK TABLES > - we ensure that for temporary tables we always request a write lock (such > - discrepancy can cause problems for the storage engine). > - We don't set TABLE_LIST::lock_type in this case as this might result in > - extra warnings from THD::decide_logging_format() even though binary logging > - is totally irrelevant for LOCK TABLES. > - */ > for (table= tables; table; table= table->next_global) > - if (!table->placeholder() && table->table->s->tmp_table) > - table->table->reginfo.lock_type= TL_WRITE; > + { > + if (!table->placeholder()) > + { > + if (table->table->s->tmp_table) > + { > + /* > + We allow to change temporary tables even if they were locked for read > + by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > + TABLES time and by the statement which is later executed under LOCK > + TABLES we ensure that for temporary tables we always request a write > + lock (such discrepancy can cause problems for the storage engine). > + We don't set TABLE_LIST::lock_type in this case as this might result > + in extra warnings from THD::decide_logging_format() even though > + binary logging is totally irrelevant for LOCK TABLES. > + */ > + table->table->reginfo.lock_type= TL_WRITE; > + } > + else if (table->mdl_request.type == MDL_SHARED_READ && > + ! table->prelocking_placeholder && > + table->table->file->lock_count() == 0) > + { > + /* > + In case when LOCK TABLE ... READ LOCAL was issued for table with > + storage engine which doesn't support READ LOCAL option and doesn't > + use THR_LOCK locks we need to upgrade weak SR metadata lock acquired > + in open_tables() to stronger SRO metadata lock. > + This is not needed for tables used through stored routines or > + triggers as we always acquire SRO (or even stronger SNRW) metadata > + lock for them. > + */ > + deadlock_handler.init(); > + thd->push_internal_handler(&deadlock_handler); > + > + bool result= thd->mdl_context.upgrade_shared_lock( > + table->table->mdl_ticket, > + MDL_SHARED_READ_ONLY, > + thd->variables.lock_wait_timeout); > + > + thd->pop_internal_handler(); > + > + if (deadlock_handler.need_reopen()) > + { > + /* > + Deadlock occurred during upgrade of metadata lock. > + Let us restart acquring and opening tables for LOCK TABLES. will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again? > + */ > + close_tables_for_reopen(thd, &tables, mdl_savepoint); > + if (thd->open_temporary_tables(tables)) > + goto err; > + goto retry; > + } > + > + if (result) > + goto err; > + } > + } > + } > > if (lock_tables(thd, tables, counter, 0) || > thd->locked_tables_list.init_locked_tables(thd)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

[Maria-developers] MDEV-11514, MDEV-11497, MDEV-11554, MDEV-11555 - IN and CASE type aggregation problems
by Alexander Barkov 15 Dec '16

15 Dec '16

Hello Sanja, Please review a patch fixing a number of problems in IN and CASE. This is a per-requisite for further Type_handler related work, to clean-up the code before moving it to more IN and CASE related methods in Type_handler. Thanks!

1 0

Re: [Maria-developers] [Commits] bbc52df: MDEV-11490 Galera_3nodes test suite does not suppress Warnings.
by Nirbhay Choubey 14 Dec '16

14 Dec '16

Hi Sachin, I see that you have added a lot of warnings to the suite-level suppression list. Does *all* the new warnings show up for all the tests? I'd only add ones that make sense for the entire suite, and add the individuals to the specific test by calling mtr.add_suppression("<pattern>"). Ping me on slack if you need to discuss this further. Best, Nirbhay On Tue, Dec 6, 2016 at 1:53 AM, <sachin.setiya(a)mariadb.com> wrote: > revision-id: bbc52df39b7dedca50943a7cd99ee80b9e2f5b12 > (mariadb-10.1.19-24-gbbc52df) > parent(s): 611f91605adce17df87acf96b5aede0b73d0fc12 > author: SachinSetiya > committer: SachinSetiya > timestamp: 2016-12-06 12:21:16 +0530 > message: > > MDEV-11490 Galera_3nodes test suite does not suppress Warnings. > > Problem:- While running individual tests of Galera_3nodes , > We get warnings like '[Warning] WSREP: Could not open state file > for reading: '. And because of this individual tests fails. > > Solution:- We change suite.pm of Galera_3nodes to supress these warnings. > > --- > mysql-test/suite/galera_3nodes/suite.pm | 34 > ++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/suite/galera_3nodes/suite.pm > b/mysql-test/suite/galera_3nodes/suite.pm > index 39d5acb..74f7dfa 100644 > --- a/mysql-test/suite/galera_3nodes/suite.pm > +++ b/mysql-test/suite/galera_3nodes/suite.pm > @@ -25,7 +25,8 @@ return "No my_print_defaults" unless $epath; > push @::global_suppressions, > ( > qr(WSREP: wsrep_sst_receive_address is set to '127.0.0.1), > - qr(WSREP: Could not open saved state file for reading: ), > + qr(WSREP: Could not open saved state file for reading: .*), > + qr(WSREP: Could not open state file for reading: .*), > qr(WSREP: Gap in state sequence. Need state transfer.), > qr(WSREP: Failed to prepare for incremental state transfer:), > qr(WSREP:.*down context.*), > @@ -33,16 +34,39 @@ push @::global_suppressions, > qr(WSREP: last inactive check more than .* skipping check), > qr(WSREP: SQL statement was ineffective), > qr(WSREP: Releasing seqno [0-9]* before [0-9]* was assigned.), > - qr|WSREP: access file$gvwstate.dat$ failed$No such file or > directory$|, > + qr|WSREP: access file$.*gvwstate.dat$ failed$No such file or > directory$|, > qr(WSREP: Quorum: No node with complete state), > qr(WSREP: Initial position was provided by configuration or SST, > avoiding override), > qr|WSREP: discarding established $time wait$ .*|, > qr(WSREP: There are no nodes in the same segment that will ever be > able to become donors, yet there is a suitable donor outside. Will use that > one.), > qr(WSREP: evs::proto.*), > - qr|WSREP: Ignoring possible split-brain (allowed by configuration) > from view:.*|, > + qr|WSREP: Ignoring possible split-brain $allowed by configuration$ > from view:.*|, > + qr(WSREP: no nodes coming from prim view, prim not possible), > + qr(WSREP: Member .* requested state transfer from .* but it is > impossible to select State Transfer donor: Resource temporarily > unavailable), > + qr(WSREP: user message in state LEAVING), > + qr(WSREP: .* sending install message failed: Transport endpoint is > not connected), > + qr(WSREP: .* sending install message failed: Resource temporarily > unavailable), > + qr(WSREP: Maximum writeset size exceeded by .*), > + qr(WSREP: transaction size exceeded.*), > + qr(WSREP: RBR event .*), > + qr(WSREP: Ignoring error for TO isolated action: .*), > + qr(WSREP: transaction size limit .*), > + qr(WSREP: rbr write fail, .*), > + qr(WSREP: .*Backend not supported: foo.*), > + qr(WSREP: .*Failed to initialize backend using .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Socket type not supported), > + qr(WSREP: failed to open gcomm backend connection: 110: failed to > reach primary view: 110 .*), > + qr(WSREP: .*Failed to open backend connection: -110 .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Connection timed out), > + qr|WSREP: wsrep::connect$.*$ failed: 7|, > + qr(WSREP: SYNC message from member .* in non-primary configuration. > Ignored.), > qr(WSREP: Could not find peer:), > - qr(WSREP: Protocol violation. JOIN message sender .*), > - qr(WSREP: JOIN message from member [0-9]* in non-primary > configuration. Ignored.), > + qr(WSREP: TO isolation failed for: .*), > + qr|WSREP: gcs_caused returned .*|, > + qr|WSREP: Protocol violation. JOIN message sender .* is not in state > transfer $SYNCED$. Message ignored.|, > + qr(WSREP: Action message in non-primary configuration from member > [0-9]*), > ); > > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

Re: [Maria-developers] [Commits] 9bd354e: MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed
by Sergey Petrunia 13 Dec '16

13 Dec '16

Hi Varun, Please find a small note below. Ok to push after it has been addressed. On Tue, Dec 13, 2016 at 05:09:58AM +0530, Varun wrote: > revision-id: 9bd354ef91ae81e0f997f29acdae4f8a2997b3c8 (mariadb-10.1.19-40-g9bd354e) > parent(s): 67b570af506a42f14a8ae30603d52ca9de68cce5 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-13 05:07:02 +0530 > message: > > MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > > In file sql/opt_range.cc,when calculate_cond_selectivity_for_table() is called with optimizer_use_condition_selectivity=4 then > - thd->no_errors is set to 1 > - the original value of thd->no_error is not restored to its original value > - this is causing the assertion to fail in the subsequent queries > > Fixed by restoring the original value of thd->no_errors > > --- > mysql-test/r/selectivity_innodb.result | 23 +++++++++++++++++++++++ > mysql-test/t/selectivity_innodb.test | 28 ++++++++++++++++++++++++++++ > sql/opt_range.cc | 1 + > 3 files changed, 52 insertions(+) > > diff --git a/mysql-test/r/selectivity_innodb.result b/mysql-test/r/selectivity_innodb.result > index 4355812..b57873d 100644 > --- a/mysql-test/r/selectivity_innodb.result > +++ b/mysql-test/r/selectivity_innodb.result > @@ -1670,6 +1670,29 @@ DROP TABLE t1,t2; > # > # End of 10.0 tests > # > +# > +# MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +# > +set optimizer_use_condition_selectivity=4; > +drop table if exists t1, t2; > +Warnings: > +Note 1051 Unknown table 'test.t1' > +Note 1051 Unknown table 'test.t2' > +drop view if exists v1; > +Warnings: > +Note 1051 Unknown table 'test.v1' Please remove 'DROP TABLE IF EXISTS' statements as they are not necessary. The previous test ends with DROP TABLE commands for tables t1, t2. There is no DROP VIEW command, so it is useful that the VIEW is dropped before it is used (just in case the test is run against a server that already has the view). Check out other test files, most of them start with lines like: --disable_warnings drop table if exists t1,t2; --enable_warnings Please also suppress warnings for the DROP VIEW statement. > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > +create view v1 as select * from t1, t2 where d=2; > +insert v1 (a,c) values (NULL, 20); > +ERROR 23000: Column 'a' cannot be null > +drop table t1,t2; > +drop view v1; > +# > +# End of 10.1 tests > +# > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/selectivity_innodb.test b/mysql-test/t/selectivity_innodb.test > index 25aa0ab..bb87106 100644 > --- a/mysql-test/t/selectivity_innodb.test > +++ b/mysql-test/t/selectivity_innodb.test > @@ -138,6 +138,34 @@ DROP TABLE t1,t2; > --echo # End of 10.0 tests > --echo # > > + > +--echo # > +--echo # MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +--echo # > + > + > +set optimizer_use_condition_selectivity=4; > + > +drop table if exists t1, t2; > +drop view if exists v1; > + > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > + > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > + > +create view v1 as select * from t1, t2 where d=2; > +--error ER_BAD_NULL_ERROR > +insert v1 (a,c) values (NULL, 20); > + > +drop table t1,t2; > +drop view v1; > + > +--echo # > +--echo # End of 10.1 tests > +--echo # > + > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index bed57f3..0577ef0 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3112,6 +3112,7 @@ bool calculate_cond_selectivity_for_table(THD *thd, TABLE *table, Item **cond) > } > > free_alloc: > + thd->no_errors= 0; > thd->mem_root= param.old_root; > free_root(&alloc, MYF(0)); > > _______________________________________________ > 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

Re: [Maria-developers] [Commits] 27af988: MDEV-11479 Improved wsrep_dirty_reads
by Vicențiu Ciorbaru 13 Dec '16

13 Dec '16

Hi Sachin! Comments inline: On Sun, 4 Dec 2016 at 08:36 <sachin.setiya(a)mariadb.com> wrote: > revision-id: 27af98805c27c8e0c41dc8c19bf80aefff4c6d3d > (mariadb-galera-5.5.53-3-g27af988) > parent(s): 72fd15f7c31aa3e3705ae1b005a3247a985c5bb3 > author: SachinSetiya > committer: SachinSetiya > NIT: To get these to display correctly you should have a space between Sachin and Setiya. > timestamp: 2016-12-04 12:05:05 +0530 > message: > > MDEV-11479 Improved wsrep_dirty_reads > > Tasks:- > Changes in wsrep_dirty_reads variable > 1.) Global + Session scope (Current: session-only) > 2.) Allow all commands that do not change data (besides SELECT) > 3.) Allow prepared Statements that do not change data > 4.) Works with wsrep_sync_wait enabled > You also seemed to have made the variable available as a command line option. Please specify that as well. > --- > .../suite/galera/r/galera_var_dirty_reads.result | 29 > +++++++++++++++++++--- > .../suite/galera/t/galera_var_dirty_reads.test | 25 > ++++++++++++++++++- > .../sys_vars/r/wsrep_dirty_reads_basic.result | 19 ++++++++++++-- > .../suite/sys_vars/t/wsrep_dirty_reads_basic.test | 13 ++++++++-- > sql/sql_parse.cc | 18 ++++++++++---- > sql/sys_vars.cc | 4 +-- > sql/wsrep_mysqld.cc | 3 ++- > sql/wsrep_mysqld.h | 1 + > 8 files changed, 96 insertions(+), 16 deletions(-) > > diff --git a/mysql-test/suite/galera/r/galera_var_dirty_reads.result > b/mysql-test/suite/galera/r/galera_var_dirty_reads.result > index 6a2aa1e..d009c1b 100644 > --- a/mysql-test/suite/galera/r/galera_var_dirty_reads.result > +++ b/mysql-test/suite/galera/r/galera_var_dirty_reads.result > @@ -28,9 +28,32 @@ SELECT @@max_allowed_packet; > SELECT 2+2 from DUAL; > 2+2 > 4 > -SELECT sysdate() from DUAL; > -sysdate() > -2016-10-28 23:13:06 > +SET @@session.wsrep_dirty_reads=ON; > +SELECT * FROM t1; > +i > +1 > +prepare stmt_show from 'select 1'; > +prepare stmt_select from 'select * from t1'; > +prepare stmt_insert from 'insert into t1 values(1)'; > +execute stmt_show; > +1 > +1 > +execute stmt_select; > +i > +1 > +execute stmt_insert; > +ERROR 08S01: WSREP has not yet prepared node for application use > +select * from t1; > +i > +1 > +execute stmt_show; > +1 > +1 > +execute stmt_select; > +i > +1 > +execute stmt_insert; > +ERROR 08S01: WSREP has not yet prepared node for application use > SELECT * FROM t1; > i > 1 > You're not really testing the global aspect of the variable here. You're only testing the sesssion behaviour of it. Also, test with it both ON and OFF (when setting it globally). Here's a sample that tests both session and global behaviour: --enable_connect_log SELECT @@expensive_subquery_limit; SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; create user user1; create user user2; --connect (con1, localhost, user1,,) SELECT @@expensive_subquery_limit; SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; connection default; SELECT @@expensive_subquery_limit; SET @@global.expensive_subquery_limit=400; SELECT @@expensive_subquery_limit; --connect (con2, localhost, user1,,) SELECT @@expensive_subquery_limit; connection default; drop user user1; drop user user2; ************************************ SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 100 SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 create user user1; create user user2; connect con1, localhost, user1,,; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 100 SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 connection default; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 SET @@global.expensive_subquery_limit=400; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 connect con2, localhost, user1,,; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 400 connection default; drop user user1; drop user user2; Notice how the last connection has the default value 400 as that's the global value. For your use case, you'd also have to execute the prepared statements to see if it works (execute select_stmt). > diff --git a/mysql-test/suite/galera/t/galera_var_dirty_reads.test > b/mysql-test/suite/galera/t/galera_var_dirty_reads.test > index dd7bc88..44931aa 100644 > --- a/mysql-test/suite/galera/t/galera_var_dirty_reads.test > +++ b/mysql-test/suite/galera/t/galera_var_dirty_reads.test > @@ -44,7 +44,30 @@ SET @@session.wsrep_dirty_reads=OFF; > SELECT 2; > SELECT @@max_allowed_packet; > SELECT 2+2 from DUAL; > -SELECT sysdate() from DUAL; > + > +#Query which does not change data should be allowed MDEV-11479 > +SET @@session.wsrep_dirty_reads=ON; > +SELECT * FROM t1; > + > +#Prepared statement creation should be allowed MDEV-11479 > +prepare stmt_show from 'select 1'; > +prepare stmt_select from 'select * from t1'; > +prepare stmt_insert from 'insert into t1 values(1)'; > + > +#Only prepare statement which does not change data should be allowed > +execute stmt_show; > +execute stmt_select; > +--error ER_UNKNOWN_COM_ERROR > +execute stmt_insert; + > +#wsrep_dirty_read should work when wsrep_sync_wait is 1 or non zero > +#because we already are disconnected , So It does not make any sense > +#to wait for other nodes > I don't think you're testing this correctly. See my comment on the if statement which is supposed to govern this. > +select * from t1; > +execute stmt_show; > +execute stmt_select; > +--error ER_UNKNOWN_COM_ERROR > +execute stmt_insert; > > --disable_query_log > --eval SET @@global.wsrep_cluster_address = '$wsrep_cluster_address_saved' > diff --git a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > index d2a62d6..649ac83 100644 > --- a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > +++ b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > @@ -5,12 +5,13 @@ > SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads; > # default > SELECT @@global.wsrep_dirty_reads; > -ERROR HY000: Variable 'wsrep_dirty_reads' is a SESSION variable > +@@global.wsrep_dirty_reads > +0 > SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > 0 > > -# scope and valid values > +# valid values for session > SET @@session.wsrep_dirty_reads=OFF; > SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > @@ -24,6 +25,20 @@ SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > 0 > > +# valid values for global > +SET @@global.wsrep_dirty_reads=OFF; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +0 > +SET @@global.wsrep_dirty_reads=ON; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +1 > +SET @@global.wsrep_dirty_reads=default; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +0 > + > # invalid values > SET @@session.wsrep_dirty_reads=NULL; > ERROR 42000: Variable 'wsrep_dirty_reads' can't be set to the value of > 'NULL' > Test invalid values for global scope as well. It should not change obviously but since we're writing tests, might as well do that too. > diff --git a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > index a47524f..000c4c4 100644 > --- a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > +++ b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > @@ -8,12 +8,12 @@ > SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads; > > --echo # default > ---error ER_INCORRECT_GLOBAL_LOCAL_VAR > + > SELECT @@global.wsrep_dirty_reads; > SELECT @@session.wsrep_dirty_reads; > > --echo > ---echo # scope and valid values > +--echo # valid values for session > SET @@session.wsrep_dirty_reads=OFF; > SELECT @@session.wsrep_dirty_reads; > SET @@session.wsrep_dirty_reads=ON; > @@ -22,6 +22,15 @@ SET @@session.wsrep_dirty_reads=default; > SELECT @@session.wsrep_dirty_reads; > > --echo > +--echo # valid values for global > +SET @@global.wsrep_dirty_reads=OFF; > +SELECT @@global.wsrep_dirty_reads; > +SET @@global.wsrep_dirty_reads=ON; > +SELECT @@global.wsrep_dirty_reads; > +SET @@global.wsrep_dirty_reads=default; > +SELECT @@global.wsrep_dirty_reads; > + > +--echo > --echo # invalid values > --error ER_WRONG_VALUE_FOR_VAR > SET @@session.wsrep_dirty_reads=NULL; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index eb4f714..a56d694 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2393,10 +2393,11 @@ mysql_execute_command(THD *thd) > */ > if (thd->variables.wsrep_on && !thd->wsrep_applier && !wsrep_ready && > lex->sql_command != SQLCOM_SET_OPTION && > - !(thd->variables.wsrep_dirty_reads && > - lex->sql_command == SQLCOM_SELECT) && > - !(lex->sql_command == SQLCOM_SELECT && > - !all_tables) && > + !(thd->variables.wsrep_dirty_reads && > + (sql_command_flags[lex->sql_command] & > + CF_CHANGES_DATA) == 0) && > Put this line on one line only, the single & is confusing, or better, turn it into a small inline function, such as command_changes_data() > + !(lex->sql_command == SQLCOM_SELECT && > + !all_tables) && > Can't tell for certain but this condition might not be necessary if you have the previous one. What happens when you have a stored procedure that inserts into a table? Does the CF_CHANGES_DATA flag filter out the query or not? ex: select function_with_side_efect(1); This seems like a good test case that I'm not sure is tested for this variable anyway. Please try it. > !wsrep_is_show_query(lex->sql_command)) > { > #if DIRTY_HACK > @@ -2512,7 +2513,14 @@ mysql_execute_command(THD *thd) > case SQLCOM_SHOW_INDEX_STATS: > case SQLCOM_SELECT: > #ifdef WITH_WSREP > - if (WSREP_CLIENT(thd) && wsrep_sync_wait(thd)) goto error; > + /* > + wsrep_dirty_read should work when wsrep_sync_wait is 1 or non-zero > + because we already are disconnected. So, it does not make any sense > + to wait for other nodes > + */ > > This here is not correct. This will not execute the select if wsrep_dirty_reads is true and wsrep_ready is true and wsrep_sync_wait(thd) is non-zero. if (WSREP_CLIENT(thd) && thd->variables.wsrep_dirty_reads(wsrep_ready > + if (WSREP_CLIENT(thd) && (wsrep_ready || > !thd->variables.wsrep_dirty_reads) > + && wsrep_sync_wait(thd)) > + goto error case SQLCOM_SHOW_VARIABLES: > case SQLCOM_SHOW_CHARSETS: > case SQLCOM_SHOW_COLLATIONS: > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index bdcedff..0d864f4 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3967,8 +3967,8 @@ static Sys_var_mybool Sys_wsrep_restart_slave( > > static Sys_var_mybool Sys_wsrep_dirty_reads( > "wsrep_dirty_reads", "Do not reject SELECT queries even when the > node " > - "is not ready.", SESSION_ONLY(wsrep_dirty_reads), NO_CMD_LINE, > - DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > + "is not ready.", SESSION_VAR(wsrep_dirty_reads), + CMD_LINE(OPT_ARG), DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > We might need a guard here since global variables can be changed from multiple places. Not 100% sure about this but that's my intuition. Need to look this up. > #endif /* WITH_WSREP */ > > diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc > index 48114fc..f21ff92 100644 > --- a/sql/wsrep_mysqld.cc > +++ b/sql/wsrep_mysqld.cc > @@ -65,7 +65,8 @@ my_bool wsrep_restart_slave_activated = 0; // node has > dropped, and slave > // restart will be needed > my_bool wsrep_slave_UK_checks = 0; // slave thread does UK checks > my_bool wsrep_slave_FK_checks = 0; // slave thread does FK checks > - > +bool wsrep_dirty_reads = 0; // is dirty reads allowed for > + // disconnectd/non major node > Typo: disconnectd -> disconnected. I really hate how the whole file is set up with TRUE/FALSE/0 for either bool or my_bool. It's so inconsistent. I'd prefer 'true' here to complicate stuff further. :) To be "consistent" I'd make it a my_bool, but we should clean up all of this mess at one point. > /* > Set during the creation of first wsrep applier and rollback threads. > Since these threads are critical, abort if the thread creation fails. > diff --git a/sql/wsrep_mysqld.h b/sql/wsrep_mysqld.h > index 3fb5189..9b9179a 100644 > --- a/sql/wsrep_mysqld.h > +++ b/sql/wsrep_mysqld.h > @@ -85,6 +85,7 @@ extern ulong wsrep_max_ws_size; > extern ulong wsrep_max_ws_rows; > extern const char* wsrep_notify_cmd; > extern my_bool wsrep_certify_nonPK; > +extern bool wsrep_dirty_reads; > Remember to update the type here too. > extern long wsrep_max_protocol_version; > extern long wsrep_protocol_version; > extern ulong wsrep_forced_binlog_format;

2 1

Re: [Maria-developers] [Commits] 514b557: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Sergei Golubchik 12 Dec '16

12 Dec '16

Hi, Vicentiu! I think mariadb should support 5.7 user table (without the password column) without any errors. That shouldn't be too difficult, just detect at run-time whether the column "Password" exists and adjust all other column numbers accordingly. On Dec 12, Vicentiu wrote: > revision-id: 514b557d066fcd1a90ccbbcdbaf5fa5fe65311eb (mariadb-10.2.2-138-g514b557) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 04:26:27 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02e74a5: MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Vicențiu Ciorbaru 12 Dec '16

12 Dec '16

Hi Sergei! Can you please review this patch for MDEV-11700? Details presented in the commit message. I'm not sure which solution we want. Possible solutions: A) have the user fix it by first starting the server with --skip-grant-tables to avoid "mysql.user table is damaged", then run mysql_upgrade, to fix the problem. Without this patch, this solution fails at mysql_upgrade. B) Have the server understand MySQL 5.7.6 + table format where the Password column is missing. For 10.2 I've chosen solution A. Although this should probably also be targeted for 10.1? For 10.3, since we're planning on doing some authentication changes, we can implement a variant of B. Thoughts? On Mon, 12 Dec 2016 at 03:50 vicentiu <vicentiu(a)mariadb.org> wrote: > revision-id: 02e74a5614b08cfce505002abc26d4634648308e > (mariadb-10.2.2-138-g02e74a5) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 03:42:11 +0200 > message: > > MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/scripts/mysql_system_tables_fix.sql > b/scripts/mysql_system_tables_fix.sql > index ea1059c..80d0ef7 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -164,6 +164,12 @@ ALTER TABLE user > MODIFY Host char(60) NOT NULL default '', > MODIFY User char(80) NOT NULL default '', > ENGINE=MyISAM, CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin; > + > +# In MySQL 5.7.6 the Password column is removed. Recreate it to preserve > the number > +# of columns MariaDB expects in the user table. > +ALTER TABLE user > + ADD Password char(41) character set latin1 collate latin1_bin NOT NULL > default '' AFTER User; > + > ALTER TABLE user > MODIFY Password char(41) character set latin1 collate latin1_bin NOT > NULL default '', > MODIFY Select_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' > NOT NULL, >

2 2

[Maria-developers] Review for version#2 of MDEV-11297: Add support for LIMIT clause in GROUP_CONCAT
by Sergey Petrunia 11 Dec '16

11 Dec '16

Hi Varun, Nice to see that nearly all comments from the previous review were addressed. Please find my further input below. General questions: How is DISTINCT handled? Syntax like GROUP_CONCAT(DISTINCT col ORDER BY ... LIMIT ...) is allowed by the parser, however I don't see any handling for DISTINCT in your patch. Did you forget to update group_concat_limit.result file? Please check. > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.cc 10.2-varun-mdev11297-noc/sql/item_sum.cc > --- 10.2-varun-mdev11297-orig/sql/item_sum.cc 2016-12-11 12:17:04.023961449 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.cc 2016-12-11 12:18:40.411957365 +0300 > @@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element > String *result= &item->result; > Item **arg= item->args, **arg_end= item->args + item->arg_count_field; > uint old_length= result->length(); > + ulonglong *offset_limit= &item->copy_offset_limit; > + ulonglong *row_limit = &item->copy_row_limit; > + > + if (item->limit_clause && !(*row_limit)) > + return 1; > > if (item->no_appended) > item->no_appended= FALSE; > @@ -3122,20 +3127,23 @@ int dump_leaf_key(void* key_arg, element > > tmp.length(0); > > - for (; arg < arg_end; arg++) > + if (!item->limit_clause || !(*offset_limit)) > { > - String *res; > - /* > - We have to use get_tmp_table_field() instead of > - real_item()->get_tmp_table_field() because we want the field in > - the temporary table, not the original field > - We also can't use table->field array to access the fields > - because it contains both order and arg list fields. > - */ > - if ((*arg)->const_item()) > - res= (*arg)->val_str(&tmp); > - else > + > + for (; arg < arg_end; arg++) > { > + String *res; > + /* > + We have to use get_tmp_table_field() instead of > + real_item()->get_tmp_table_field() because we want the field in > + the temporary table, not the original field > + We also can't use table->field array to access the fields > + because it contains both order and arg list fields. > + */ > + if ((*arg)->const_item()) > + res= (*arg)->val_str(&tmp); > + else > + { > Field *field= (*arg)->get_tmp_table_field(); Please make the code to use the right indentation. > if (field) > { > @@ -3146,9 +3154,19 @@ int dump_leaf_key(void* key_arg, element > } > else > res= (*arg)->val_str(&tmp); > + } > + if (res) > + { > + result->append(*res); > + } > } > - if (res) > - result->append(*res); > + if ((*row_limit)) > + (*row_limit)--; > + } > + else > + { > + item->no_appended= TRUE; If I comment out the above line, the test still passes. Please add test coverage that shows it is needed, or remove it. > + (*offset_limit)--; > } > > item->row_count++; > @@ -3199,7 +3217,8 @@ Item_func_group_concat:: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool distinct_arg, List<Item> *select_list, > const SQL_I_List<ORDER> &order_list, > - String *separator_arg) > + String *separator_arg, bool limit_clause, > + Item *rowLimit, Item *offsetLimit) camelCase or javaStyleVariableNames are against the coding style. Please use something like row_limit_arg if there is no better option. (In general, please try to make your new code fit in with the rest of the code. Did you see camelCase anywhere? If not, it's a sign not to use it). > :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0), > unique_filter(NULL), table(0), > order(0), context(context_arg), > @@ -3208,7 +3227,9 @@ Item_func_group_concat(THD *thd, Name_re > row_count(0), > distinct(distinct_arg), > warning_for_row(FALSE), > - force_copy_fields(0), original(0) > + force_copy_fields(0),row_limit(0), > + offset_limit(0),limit_clause(limit_clause), > + copy_offset_limit(0), copy_row_limit(0),original(0) Please use 'NULL' for pointers, not '0'. > { > Item *item_select; > Item **arg_ptr; > @@ -3250,6 +3271,16 @@ Item_func_group_concat(THD *thd, Name_re > /* orig_args is only used for print() */ > orig_args= (Item**) (order + arg_count_order); > memcpy(orig_args, args, sizeof(Item*) * arg_count); > + > + if (limit_clause) > + { > + if (!(row_limit=(Item*) thd->alloc(sizeof(Item)))) > + return; I'm not sure what the above is supposed to mean? Why allocate sizeof(Item) bytes? > + row_limit= rowLimit; ... especially when the next thing you do is the above assignment, so you lose the value that you've got from the thd->alloc() call? > + if (!(offset_limit= (Item*)thd->alloc(sizeof(Item)))) > + return; > + offset_limit= offsetLimit; The same question as above. > + } > } > > > @@ -3269,7 +3300,9 @@ Item_func_group_concat::Item_func_group_ > warning_for_row(item->warning_for_row), > always_null(item->always_null), > force_copy_fields(item->force_copy_fields), > - original(item) > + row_limit(item->row_limit), offset_limit(item->offset_limit), > + limit_clause(item->limit_clause),copy_offset_limit(item->copy_offset_limit), > + copy_row_limit(item->copy_row_limit), original(item->original) > { > quick_group= item->quick_group; > result.set_charset(collation.collation); > @@ -3382,6 +3415,10 @@ void Item_func_group_concat::clear() > null_value= TRUE; > warning_for_row= FALSE; > no_appended= TRUE; > + if (offset_limit) > + copy_offset_limit= offset_limit->val_int(); > + if (row_limit) > + copy_row_limit= row_limit->val_int(); > if (tree) > reset_tree(tree); > if (unique_filter) > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.h 10.2-varun-mdev11297-noc/sql/item_sum.h > --- 10.2-varun-mdev11297-orig/sql/item_sum.h 2016-12-11 12:17:04.043961448 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.h 2016-12-11 12:18:40.415957365 +0300 > @@ -1583,6 +1583,12 @@ class Item_func_group_concat : public It > bool always_null; > bool force_copy_fields; > bool no_appended; > + Item *row_limit; > + Item *offset_limit; > + bool limit_clause; > + ulonglong copy_offset_limit; > + ulonglong copy_row_limit; > + In previous review I have requested: >> Please add (short) comments describing each new member variable. And you still haven't done that. > /* > Following is 0 normal object and pointer to original one for copy > (to correctly free resources) > @@ -1602,7 +1608,8 @@ protected: > public: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool is_distinct, List<Item> *is_select, > - const SQL_I_List<ORDER> &is_order, String *is_separator); > + const SQL_I_List<ORDER> &is_order, String *is_separator, > + bool limit_clause, Item *row_limit, Item *offset_limit); > > Item_func_group_concat(THD *thd, Item_func_group_concat *item); > ~Item_func_group_concat(); > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 10.2-varun-mdev11297-noc/sql/sql_yacc.yy > --- 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 2016-12-11 12:17:04.107961446 +0300 > +++ 10.2-varun-mdev11297-noc/sql/sql_yacc.yy 2016-12-11 12:18:40.887957345 +0300 > @@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > %type <num> > order_dir lock_option > udf_type opt_local opt_no_write_to_binlog > - opt_temporary all_or_any opt_distinct > + opt_temporary all_or_any opt_distinct opt_glimit_clause > opt_ignore_leaves fulltext_options union_option > opt_not > select_derived_init transaction_access_mode_types > @@ -1844,7 +1844,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > opt_escape > sp_opt_default > simple_ident_nospvar simple_ident_q > - field_or_var limit_option > + field_or_var limit_option glimit_option > part_func_expr > window_func_expr > window_func > @@ -10449,16 +10449,34 @@ sum_expr: > | GROUP_CONCAT_SYM '(' opt_distinct > { Select->in_sum_expr++; } > expr_list opt_gorder_clause > - opt_gconcat_separator > + opt_gconcat_separator opt_glimit_clause > ')' > { > SELECT_LEX *sel= Select; > sel->in_sum_expr--; > - $$= new (thd->mem_root) > - Item_func_group_concat(thd, Lex->current_context(), $3, $5, > - sel->gorder_list, $7); > - if ($$ == NULL) > - MYSQL_YYABORT; > + if (!$8) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, FALSE, 0, 0); > + else > + { > + if (sel->select_limit && sel->offset_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, > + sel->offset_limit); > + else if (sel->select_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, 0); > + } > + if ($$ == NULL) > + MYSQL_YYABORT; > + > + sel->select_limit= NULL; > + sel->offset_limit= NULL; > + sel->explicit_limit= 0; > + > $5->empty(); > sel->gorder_list.empty(); > } > @@ -10680,6 +10698,95 @@ gorder_list: > { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; } > ; What is the difference between glimit_option and limit_option? Looking at the grammar, they look the same. Please either remove glimit_option or explain why do you need it. I can see that limit_clause and glimit_clause have to be different because limit_clause also supports LIMIT ROWS EXAMINED. Actually, there is one thing in limit_clause that I think should be in glimit_clause also. It's this call: Lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_LIMIT); Did you omit it intentionally? Let's meet and discuss it. > +opt_glimit_clause: > + /* empty */ { $$ = 0; } > + | glimit_clause { $$ = 1; } > + ; > + > +glimit_clause_init: > + LIMIT{} > + ; > + > +glimit_clause: > + glimit_clause_init glimit_options{} > + ; > + > +glimit_options: > + glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= 0; > + sel->explicit_limit= 1; > + } > + | glimit_option ',' glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $3; > + sel->offset_limit= $1; > + sel->explicit_limit= 1; > + } > + | glimit_option OFFSET_SYM glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= $3; > + sel->explicit_limit= 1; > + } > + ; > + > +glimit_option: > + ident > + { > + Item_splocal *splocal; > + LEX *lex= thd->lex; > + Lex_input_stream *lip= & thd->m_parser_state->m_lip; > + sp_variable *spv; > + sp_pcontext *spc = lex->spcont; > + if (spc && (spv = spc->find_variable($1, false))) > + { > + splocal= new (thd->mem_root) > + Item_splocal(thd, $1, spv->offset, spv->sql_type(), > + lip->get_tok_start() - lex->sphead->m_tmp_query, > + lip->get_ptr() - lip->get_tok_start()); > + if (splocal == NULL) > + MYSQL_YYABORT; > +#ifndef DBUG_OFF > + splocal->m_sp= lex->sphead; > +#endif > + lex->safe_to_cache_query=0; > + } > + else > + my_yyabort_error((ER_SP_UNDECLARED_VAR, MYF(0), $1.str)); > + if (splocal->type() != Item::INT_ITEM) > + my_yyabort_error((ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0))); > + splocal->limit_clause_param= TRUE; > + $$= splocal; > + } > + | param_marker > + { > + $1->limit_clause_param= TRUE; > + } > + | ULONGLONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | LONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + > in_sum_expr: > opt_all > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0