- developers - lists.mariadb.org

Re: [Maria-developers] c76ac1e6de8: MDEV-24089 support oracle syntax: rownum
by Sergei Golubchik 13 May '21

13 May '21

Hi, Michael! On Apr 12, Michael Widenius wrote: > revision-id: c76ac1e6de8 (mariadb-10.5.2-551-gc76ac1e6de8) > parent(s): a507eb17708 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 19:42:24 +0200 > message: > diff --git a/mysql-test/main/rownum.test b/mysql-test/main/rownum.test > new file mode 100644 > index 00000000000..8d89eed2f86 > --- /dev/null > +++ b/mysql-test/main/rownum.test > @@ -0,0 +1,542 @@ > +--source include/have_sequence.inc > +# > +# Test of basic rownum() functionallity > +# Test are done with Aria to ensure that row order is stable what does that mean? > +# > + > +CREATE OR REPLACE TABLE t1(a int, b int) engine=aria; > +CREATE OR REPLACE TABLE t2(a int, b int) engine=aria; > +insert into t1 values (1,10),(2,20),(3,30); > +insert into t2 values (2,21),(3,31),(4,41); > + > +--echo # > +--echo # Simple selects > +--echo # > + > +select a,b,rownum() from t1; > +select a,b,rownum() from t1 where rownum() < 2; > +select a,b from t1 where rownum() <= 2; > +select a,b from t1 where rownum() > 2; > + > +--echo # > +--echo # Subqueries > +--echo # > + > +select t1.a,rownum(),t3.a,t3.t2_rownum from t1, (select t2.a,rownum() as t2_rownum from t2 where rownum() <=2) t3; > +select t1.a, (select t2.b from t2 where t1.a=t2.a and rownum() <= 1) 'b' from t1; > +select t1.a, t3.a from t1, (select * from t2 where rownum() <= 2) t3; > +select * from (select tt.*,rownum() as id from (select * from t1) tt) t3 where id>2; > + > +--echo # > +--echo # Joins > +--echo # > + > +select t1.a,t1.b,t2.a,t2.b,rownum() from t1,t2 where rownum() <= 4; > +select *,rownum() from t1,t2 where t1.a=t2.a and rownum()<=2; > +select * from t1 left join t2 on (t2.a=t1.a and rownum()=0); > +select * from t1 left join t2 on (t2.a=t1.a and rownum()>1); > +select * from t1 left join t2 on (t2.a=t1.a and rownum()<1); > +select * from t1 left join t2 on (t2.a=t1.a and rownum()=1); > +select * from t1 left join t2 on (t2.a=t1.a and rownum()>=1); > + > +--echo # > +--echo # Union > +--echo # > + > +select * from t1 where rownum() <=2 union select * from t2 where rownum()<= 1; > + > +--echo # > +--echo # Order by > +--echo # > + > +select * from t1 where rownum() <= 2 order by a desc; > +explain select * from t1 where rownum() <= 2 order by a desc; > +select t1.a,t1.b,rownum() from t1 where rownum() <= 2 order by a desc; > +explain select t1.a,t1.b,rownum() from t1 where rownum() <= 2 order by a desc; > +select *,rownum() from t1,t2; > +select *,rownum() from t1,t2 order by t2.a desc, t1.a desc; > +select * from (select * from t1 order by a desc) as t where rownum() <= 2; > +select * from t1,t2 where t1.a=t2.a and rownum()<=2 order by t1.a,t2.a; > + > +--echo # > +--echo # Having > +--echo # > + > +select t1.a, sum(t1.b), rownum() as 'r' from t1 group by t1.a having r <= 2; > +--error ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE > +select * from t1 having rownum() <= 2; > +select t1.a, sum(t1.b), rownum() from t1 group by t1.a; > +--error ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE > +select t1.a, sum(t1.b), rownum() as 'r' from t1 group by t1.a having rownum() <= 2; > + > +--echo # > +--echo # Sum functions > +--echo # > + > +select max(rownum()),min(rownum()) from t1; > +select sum(rownum()),avg(rownum()) from t1; > + > +--echo # > +--echo # Group by > +--echo # > + > +select t1.a,sum(t1.b) from t1 where rownum() < 2 group by t1.a; > +select t1.a,sum(t2.b) from t1 JOIN t2 ON (t1.a=t2.a) where rownum() <= 2 group by t1.a; > +select * from (select t1.a,sum(t2.b) from t1 JOIN t2 ON (t1.a=t2.a) group by t1.a) as t where rownum() <= 1; > +select t1.a,sum(rownum()),count(*) from t1 where rownum() <= 2 group by t1.a; > +select * from (select t1.a,sum(t1.b) from t1 group by t1.a) as t3 where rownum() < 2; > + > +create table t3 (a int) engine=myisam; > +insert into t3 values (3),(5),(5),(3); > +select a, max(rownum()) from t3 group by a; > +drop table t3; > + > +CREATE TABLE t3 ( > + a int(11) DEFAULT NULL, > + b varchar(1024) DEFAULT NULL > +); > +insert into t3 select mod(seq*3,20)+1, repeat(char(33+mod(seq,90)),mod(seq,10)*100) from seq_1_to_23; > +SELECT sq.a,length(sq.f) FROM (SELECT a, GROUP_CONCAT(b,b) AS f FROM t3 GROUP BY a ORDER BY a desc) as sq WHERE ROWNUM() <= 10; > +drop table t3; > + > +--echo # > +--echo # Prepared statements > +--echo # > + > +PREPARE stmt1 from "select a,b,rownum() from t1 where rownum() <= 2"; > +execute stmt1; > +execute stmt1; > +deallocate prepare stmt1; > + > +--echo # > +--echo # Views > +--echo # > + > +create view v1 as select t1.a,rownum() from t1; > +select * from v1; > +select t1.a,v1.* from t1,v1 where t1.a=v1.a; > +drop view v1; > + > +CREATE TABLE t3 (a INT); > +INSERT INTO t3 VALUES (1),(2),(3); > +CREATE VIEW v1 AS SELECT a FROM t3 WHERE ROWNUM() <= 2; > +SELECT * FROM v1; > +drop view v1; > +drop table t3; > + > +--echo # > +--echo # Reserved words > +--echo # > + > +create table t4 (a int, rownum int); > +insert into t4 (a,rownum) values (1,2); > +select t4.a,t4.rownum from t4; > +drop table t4; > + > +--echo # > +--echo # Test Oracle mode > +--echo # > + > +set SQL_MODE=ORACLE; > +select t1.a,rownum from t1 where rownum<=2; > +select t1.a,rownum() from t1 where rownum()<=2; > +--error ER_PARSE_ERROR > +create table t4 (a int, rownum int); > + > +DELIMITER |; > +DECLARE > + CURSOR c_cursor > + IS select a,b,rownum from t1 where rownum <= 2; > + v_a t1.a%TYPE; > + v_b t1.b%TYPE; > + v_rn t1.a%TYPE; > +BEGIN > + OPEN c_cursor; > + FETCH c_cursor INTO v_a, v_b, v_rn; > + WHILE c_cursor%FOUND LOOP > + SELECT concat(v_a,'--',v_b,'--',v_rn); > + FETCH c_cursor INTO v_a, v_b, v_rn; > + END LOOP; > + CLOSE c_cursor; > +END;| > +DELIMITER ;| > + > +--error ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE > +select a, rownum from t1 group by a, rownum having rownum < 3; > +select a, rownum from t1 group by a, rownum having "rownum" < 3; what's the difference between these two? Why the first is an error and the second is not? > +select a, rownum as r from t1 group by a, rownum having r < 3; And the this one. Why is it not an error either? > + > +set SQL_MODE=DEFAULT; > + > +--echo # Cleanup > + > +drop table t1,t2; > + > +--echo # > +--echo # INSERT > +--echo # > + > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,rownum()),(2,rownum()),(3,rownum()); > +select * from t1; > +drop table t1; > + > +--echo # > +--echo # INSERT DELAYED > +--echo # > + > +create table t1 (a int not null primary key, b int); > +insert delayed into t1 values (1,rownum()),(2,rownum()),(3,rownum()); > +select * from t1; > +drop table t1; > + > +--echo # > +--echo # INSERT ... RETURNING > +--echo # > + > +create or replace table t1 (a int); > +insert into t1 values (1),(2) returning a, rownum(); > +drop table t1; > + > +--echo # > +--echo # UPDATE > +--echo # > + > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,2),(3,3); > +update t1 set b=0; > +update t1 set b=rownum()+1; > +select * from t1; > + > +update t1 set b=0; > +update t1 set b=rownum() where a < 10 and rownum() < 2; > +select * from t1; > +drop table t1; > + > +create table t1 (a int); > +insert into t1 values (10),(20),(30); > +update t1 set a = rownum(); > +select * from t1; > +update t1 set a = rownum(); > +select * from t1; > +drop table t1; > + > +--echo # > +--echo # DELETE > +--echo # > + > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,0),(3,0); > +delete from t1 where a < 10 and rownum() < 2; > +select * from t1; > +drop table t1; > + > +--echo # > +--echo # MULTI-TABLE-DELETE > +--echo # > + > +create table t1 (a int not null primary key); > +insert into t1 values (1),(2),(3); > +create table t2 (a int not null primary key); > +insert into t2 values (1),(2),(3); > + > +delete t1,t2 from t1,t2 where t1.a=t2.a and rownum() <= 2; > +select * from t1; > +select * from t2; > +drop table t1,t2; > + > +--echo # > +--echo # MULTI-TABLE-UPDATE > + > +CREATE TABLE t1 (ID INT); > +CREATE TABLE t2 (ID INT, > + s1 TEXT, s2 TEXT, s3 VARCHAR(10), s4 TEXT, s5 VARCHAR(10)); > + > +INSERT INTO t1 VALUES (1),(2); > +INSERT INTO t2 VALUES (1,'test', 'test', 'test', 'test', 'test'), > + (2,'test', 'test', 'test', 'test', 'test'); > + > +SELECT * FROM t1 LEFT JOIN t2 USING(ID); > +UPDATE t1 LEFT JOIN t2 USING(ID) SET s1 = 'changed'; > +select * from t2; > +update t2 set s1=""; > +UPDATE t1 LEFT JOIN t2 USING(ID) SET s1 = 'changed' where rownum() <=1; > +select * from t2; > +drop table t1,t2; > + > +--echo # > +--echo # LOAD DATA > +--echo # > + > +create table t1 (a int, b int, c int); > +load data infile '../../std_data/loaddata7.dat' into table t1 fields terminated by ',' lines terminated by "\r\n" (a,b) set c=rownum(); > +select * from t1; > +drop table t1; > + > +--echo # > +--echo # LIMIT OPTIMIZATION > +--echo # > + > +create table t1 (a int); > +insert into t1 select seq from seq_1_to_100; > + > +flush status; > +select * from t1 where rownum() <= 3; > +show status like "Rows_read"; > +flush status; > +flush status; > +select * from t1 where rownum() <= 4 and rownum() <= 3; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() < 4 and a > 10; > +show status like "Rows_read"; > +flush status; > +select * from t1 where 3 >= rownum(); > +show status like "Rows_read"; > +flush status; > +select * from t1 where 4 > rownum() and a > 20; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() = 1 and a > 10; > +show status like "Rows_read"; > +flush status; > +select * from t1 where a > 30 && 1 = rownum(); > +show status like "Rows_read"; > +flush status; > + > +--echo # No limit optimization > + > +select * from t1 where rownum() > 10; > +show status like "Rows_read"; > +flush status; > +select * from t1 where 10 < rownum(); > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() >= 10; > +show status like "Rows_read"; > +flush status; > +select * from t1 where 10 < rownum(); > +show status like "Rows_read"; > +flush status; > +select * from t1 where 10 <= rownum(); > +show status like "Rows_read"; > +flush status; > +select * from t1 where 2 = rownum(); > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() = 2; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() <= 0; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() < 10 limit 4, 4; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() < 10 order by a; > +show status like "Rows_read"; > +flush status; > + > + > +--echo # rownum and limit > + > +select * from t1 where rownum() < 4 limit 10; > +show status like "Rows_read"; > +flush status; > +select * from t1 where rownum() < 10 limit 4; > +show status like "Rows_read"; > + > +drop table t1; > + > +--echo # > +--echo # Rownum examples from Woqutech > +--echo # > + > +set SQL_MODE=ORACLE; > +create table t1 (c1 int ,c2 varchar(20)) engine=myisam; > +insert into t1 values (1, 'aaa'),(2, 'bbb'),(3, 'ccc'),(4, 'ddd'),(5, 'eee'); > +update t1 set c2 = 'xxx' where rownum = 2; > +select * from t1 where c2='xxx'; > +update t1 set c2 = 'xxx' where rownum < 3; > +select * from t1 where c2='xxx'; > +delete from t1 where rownum = 2; > +select count(*) from t1; > +delete from t1 where rownum < 3; > +select count(*) from t1; > +delete from t1 where c1=rownum ; > +select count(*) from t1; > +delete from t1 where c1=rownum+2 ; > +select count(*) from t1; > +set SQL_MODE=DEFAULT; > +drop table t1; > + > +--echo # > +--echo # Rownum() used in not supported places (returns 0 or gives an error) > +--echo # > + > +set @a=rownum(); > +select @a; > + > +--error ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED > +create or replace table t (a int, b int as (rownum()) virtual); > + > +create table t1 (a int); > +insert into t1 values (3),(1),(5),(8),(4); > +handler t1 open; > +--error ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED > +handler t1 read next where rownum() < 1; > +handler t1 close; > +drop table t1; > + > +# rownum() executed in a function will be run in the function context. > + > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,2),(3,3); > +create function f() returns int return rownum(); > +select a, rownum(), f() from t1; > +drop function f; > +drop table t1; > + > +# rownum() executed in a trigger will be run in the function context. > + > +create or replace table t1 (a int, r int); > +create trigger tr before update on t1 for each row set NEW.r = rownum(); > +insert into t1 (a) values (1),(2); > +select * from t1; > +update t1 set a=a+10; > +select * from t1; > +drop trigger tr; > +drop table t1; > + > +--echo # > +--echo # LIMIT optimisation > +--echo # > + > +create table t1 (a int); > +insert into t1 values (1),(2),(3),(4),(5); > + > +let $query= > +select * from (select a from t1 where a < 1000) as tt where rownum() <= 2; > +flush status; > +eval $query; > +show status like "Rows_read"; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +flush status; > +execute stmt; > +show status like "Rows_read"; > +flush status; > +execute stmt; > +show status like "Rows_read"; > +deallocate prepare stmt; > + > + > +let $query= > +select * from (select a from t1 where a < 1000 group by a) as tt where rownum() <= 2; > +flush status; > +eval $query; > +show status like "Rows_read"; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $query= > +select * from (select a from t1 where a < 1000 group by a order by 1) as tt where rownum() <= 2; > +flush status; > +eval $query; > +show status like "Rows_read"; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $query= > +select * from (select a from t1 where a < 1000 union select 10) as tt where rownum() <= 2; > +flush status; > +eval $query; > +show status like "Rows_read"; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # Other limit > + > +let $query= > +select * from (select a from t1 where a < 1000 group by a order by 1 limit 1) as tt where rownum() <= 2; > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # Other limit less > + > +let $query= > +select * from (select a from t1 where a < 1000 group by a order by 1 limit 10) as tt where rownum() <= 2; > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $query= > +select * from (select a from t1 where a < 1000 union select 10 limit 1) as tt where rownum() <= 2; > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # < rownum > + > +let $query= > +select * from (select a from t1 where a < 1000) as tt where rownum() < 2; > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # Simple expression > + > +let $query= > +select * from (select a from t1 where a < 1000) as tt where rownum() <= 1+1; > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # Simple expression reversed > + > +let $query= > +select * from (select a from t1 where a < 1000) as tt where 1+1 >= rownum(); > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +--echo # expensive (no opt) > + > +let $query= > +select * from (select a from t1 where a < 1000) as tt where (select max(a) from t1) >= rownum(); > +eval $query; > +eval explain extended $query; > +eval prepare stmt from "$query"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +drop table t1; > diff --git a/mysql-test/main/rownum.result b/mysql-test/main/rownum.result > new file mode 100644 > index 00000000000..e7ea92e5dde > --- /dev/null > +++ b/mysql-test/main/rownum.result > @@ -0,0 +1,925 @@ > +CREATE OR REPLACE TABLE t1(a int, b int) engine=aria; > +CREATE OR REPLACE TABLE t2(a int, b int) engine=aria; > +insert into t1 values (1,10),(2,20),(3,30); > +insert into t2 values (2,21),(3,31),(4,41); > +# > +# Simple selects > +# > +select a,b,rownum() from t1; > +a b rownum() > +1 10 1 > +2 20 2 > +3 30 3 > +select a,b,rownum() from t1 where rownum() < 2; > +a b rownum() > +1 10 1 > +select a,b from t1 where rownum() <= 2; > +a b > +1 10 > +2 20 > +select a,b from t1 where rownum() > 2; > +a b > +# > +# Subqueries > +# > +select t1.a,rownum(),t3.a,t3.t2_rownum from t1, (select t2.a,rownum() as t2_rownum from t2 where rownum() <=2) t3; > +a rownum() a t2_rownum > +1 1 2 1 > +1 2 3 2 > +2 3 2 1 > +2 4 3 2 > +3 5 2 1 > +3 6 3 2 > +select t1.a, (select t2.b from t2 where t1.a=t2.a and rownum() <= 1) 'b' from t1; > +a b > +1 NULL > +2 21 > +3 31 > +select t1.a, t3.a from t1, (select * from t2 where rownum() <= 2) t3; > +a a > +1 2 > +1 3 > +2 2 > +2 3 > +3 2 > +3 3 > +select * from (select tt.*,rownum() as id from (select * from t1) tt) t3 where id>2; > +a b id > +3 30 3 > +# > +# Joins > +# > +select t1.a,t1.b,t2.a,t2.b,rownum() from t1,t2 where rownum() <= 4; > +a b a b rownum() > +1 10 2 21 1 > +2 20 2 21 2 > +3 30 2 21 3 > +1 10 3 31 4 > +select *,rownum() from t1,t2 where t1.a=t2.a and rownum()<=2; > +a b a b rownum() > +2 20 2 21 1 > +3 30 3 31 2 > +select * from t1 left join t2 on (t2.a=t1.a and rownum()=0); > +a b a b > +1 10 NULL NULL > +2 20 NULL NULL > +3 30 NULL NULL > +select * from t1 left join t2 on (t2.a=t1.a and rownum()>1); > +a b a b > +1 10 NULL NULL > +2 20 NULL NULL > +3 30 NULL NULL > +select * from t1 left join t2 on (t2.a=t1.a and rownum()<1); > +a b a b > +1 10 NULL NULL > +2 20 NULL NULL > +3 30 NULL NULL > +select * from t1 left join t2 on (t2.a=t1.a and rownum()=1); > +a b a b > +2 20 2 21 > +1 10 NULL NULL > +3 30 NULL NULL > +select * from t1 left join t2 on (t2.a=t1.a and rownum()>=1); > +a b a b > +2 20 2 21 > +3 30 3 31 > +1 10 NULL NULL > +# > +# Union > +# > +select * from t1 where rownum() <=2 union select * from t2 where rownum()<= 1; > +a b > +1 10 > +2 20 > +2 21 > +# > +# Order by > +# > +select * from t1 where rownum() <= 2 order by a desc; > +a b > +2 20 > +1 10 > +explain select * from t1 where rownum() <= 2 order by a desc; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 3 Using where; Using filesort > +select t1.a,t1.b,rownum() from t1 where rownum() <= 2 order by a desc; > +a b rownum() > +2 20 2 > +1 10 1 > +explain select t1.a,t1.b,rownum() from t1 where rownum() <= 2 order by a desc; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 3 Using where; Using temporary; Using filesort > +select *,rownum() from t1,t2; > +a b a b rownum() > +1 10 2 21 1 > +2 20 2 21 2 > +3 30 2 21 3 > +1 10 3 31 4 > +2 20 3 31 5 > +3 30 3 31 6 > +1 10 4 41 7 > +2 20 4 41 8 > +3 30 4 41 9 > +select *,rownum() from t1,t2 order by t2.a desc, t1.a desc; > +a b a b rownum() > +3 30 4 41 9 > +2 20 4 41 8 > +1 10 4 41 7 > +3 30 3 31 6 > +2 20 3 31 5 > +1 10 3 31 4 > +3 30 2 21 3 > +2 20 2 21 2 > +1 10 2 21 1 > +select * from (select * from t1 order by a desc) as t where rownum() <= 2; > +a b > +3 30 > +2 20 > +select * from t1,t2 where t1.a=t2.a and rownum()<=2 order by t1.a,t2.a; > +a b a b > +2 20 2 21 > +3 30 3 31 > +# > +# Having > +# > +select t1.a, sum(t1.b), rownum() as 'r' from t1 group by t1.a having r <= 2; > +a sum(t1.b) r > +1 10 1 > +2 20 2 > +select * from t1 having rownum() <= 2; > +ERROR HY000: Function 'rownum()' cannot be used in the HAVING clause > +select t1.a, sum(t1.b), rownum() from t1 group by t1.a; > +a sum(t1.b) rownum() > +1 10 1 > +2 20 2 > +3 30 3 > +select t1.a, sum(t1.b), rownum() as 'r' from t1 group by t1.a having rownum() <= 2; > +ERROR HY000: Function 'rownum()' cannot be used in the HAVING clause > +# > +# Sum functions > +# > +select max(rownum()),min(rownum()) from t1; > +max(rownum()) min(rownum()) > +3 1 > +select sum(rownum()),avg(rownum()) from t1; > +sum(rownum()) avg(rownum()) > +6 2.0000 > +# > +# Group by > +# > +select t1.a,sum(t1.b) from t1 where rownum() < 2 group by t1.a; > +a sum(t1.b) > +1 10 > +select t1.a,sum(t2.b) from t1 JOIN t2 ON (t1.a=t2.a) where rownum() <= 2 group by t1.a; > +a sum(t2.b) > +2 21 > +3 31 > +select * from (select t1.a,sum(t2.b) from t1 JOIN t2 ON (t1.a=t2.a) group by t1.a) as t where rownum() <= 1; > +a sum(t2.b) > +2 21 > +select t1.a,sum(rownum()),count(*) from t1 where rownum() <= 2 group by t1.a; > +a sum(rownum()) count(*) > +1 1 1 > +2 2 1 > +select * from (select t1.a,sum(t1.b) from t1 group by t1.a) as t3 where rownum() < 2; > +a sum(t1.b) > +1 10 > +create table t3 (a int) engine=myisam; > +insert into t3 values (3),(5),(5),(3); > +select a, max(rownum()) from t3 group by a; > +a max(rownum()) > +3 4 > +5 3 > +drop table t3; > +CREATE TABLE t3 ( > +a int(11) DEFAULT NULL, > +b varchar(1024) DEFAULT NULL > +); > +insert into t3 select mod(seq*3,20)+1, repeat(char(33+mod(seq,90)),mod(seq,10)*100) from seq_1_to_23; > +SELECT sq.a,length(sq.f) FROM (SELECT a, GROUP_CONCAT(b,b) AS f FROM t3 GROUP BY a ORDER BY a desc) as sq WHERE ROWNUM() <= 10; > +a length(sq.f) > +20 600 > +19 1200 > +18 1800 > +17 400 > +16 1000 > +15 1600 > +14 200 > +13 800 > +12 1400 > +11 0 > +drop table t3; > +# > +# Prepared statements > +# > +PREPARE stmt1 from "select a,b,rownum() from t1 where rownum() <= 2"; > +execute stmt1; > +a b rownum() > +1 10 1 > +2 20 2 > +execute stmt1; > +a b rownum() > +1 10 1 > +2 20 2 > +deallocate prepare stmt1; > +# > +# Views > +# > +create view v1 as select t1.a,rownum() from t1; > +select * from v1; > +a rownum() > +1 1 > +2 2 > +3 3 > +select t1.a,v1.* from t1,v1 where t1.a=v1.a; > +a a rownum() > +1 1 1 > +2 2 2 > +3 3 3 > +drop view v1; > +CREATE TABLE t3 (a INT); > +INSERT INTO t3 VALUES (1),(2),(3); > +CREATE VIEW v1 AS SELECT a FROM t3 WHERE ROWNUM() <= 2; > +SELECT * FROM v1; > +a > +1 > +2 > +drop view v1; > +drop table t3; > +# > +# Reserved words > +# > +create table t4 (a int, rownum int); > +insert into t4 (a,rownum) values (1,2); > +select t4.a,t4.rownum from t4; > +a rownum > +1 2 > +drop table t4; > +# > +# Test Oracle mode > +# > +set SQL_MODE=ORACLE; > +select t1.a,rownum from t1 where rownum<=2; > +a rownum > +1 1 > +2 2 > +select t1.a,rownum() from t1 where rownum()<=2; > +a rownum() > +1 1 > +2 2 > +create table t4 (a int, rownum int); > +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'rownum int)' at line 1 > +DECLARE > +CURSOR c_cursor > +IS select a,b,rownum from t1 where rownum <= 2; > +v_a t1.a%TYPE; > +v_b t1.b%TYPE; > +v_rn t1.a%TYPE; > +BEGIN > +OPEN c_cursor; > +FETCH c_cursor INTO v_a, v_b, v_rn; > +WHILE c_cursor%FOUND LOOP > +SELECT concat(v_a,'--',v_b,'--',v_rn); > +FETCH c_cursor INTO v_a, v_b, v_rn; > +END LOOP; > +CLOSE c_cursor; > +END;| > +concat(v_a,'--',v_b,'--',v_rn) > +1--10--1 > +concat(v_a,'--',v_b,'--',v_rn) > +2--20--2 > +select a, rownum from t1 group by a, rownum having rownum < 3; > +ERROR HY000: Function 'rownum()' cannot be used in the HAVING clause > +select a, rownum as r from t1 group by a, rownum having r < 3; > +a r > +1 1 > +2 2 > +select a, rownum from t1 group by a, rownum having "rownum" < 3; > +a rownum > +1 1 > +2 2 > +set SQL_MODE=DEFAULT; > +# Cleanup > +drop table t1,t2; > +# > +# INSERT > +# > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,rownum()),(2,rownum()),(3,rownum()); > +select * from t1; > +a b > +1 1 > +2 2 > +3 3 > +drop table t1; > +# > +# INSERT DELAYED > +# > +create table t1 (a int not null primary key, b int); > +insert delayed into t1 values (1,rownum()),(2,rownum()),(3,rownum()); > +select * from t1; > +a b > +1 1 > +2 2 > +3 3 > +drop table t1; > +# > +# INSERT ... RETURNING > +# > +create or replace table t1 (a int); > +insert into t1 values (1),(2) returning a, rownum(); > +a rownum() > +1 1 > +2 2 > +drop table t1; > +# > +# UPDATE > +# > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,2),(3,3); > +update t1 set b=0; > +update t1 set b=rownum()+1; > +select * from t1; > +a b > +1 2 > +2 3 > +3 4 > +update t1 set b=0; > +update t1 set b=rownum() where a < 10 and rownum() < 2; > +select * from t1; > +a b > +1 1 > +2 0 > +3 0 > +drop table t1; > +create table t1 (a int); > +insert into t1 values (10),(20),(30); > +update t1 set a = rownum(); > +select * from t1; > +a > +1 > +2 > +3 > +update t1 set a = rownum(); > +select * from t1; > +a > +1 > +2 > +3 > +drop table t1; > +# > +# DELETE > +# > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,0),(3,0); > +delete from t1 where a < 10 and rownum() < 2; > +select * from t1; > +a b > +2 0 > +3 0 > +drop table t1; > +# > +# MULTI-TABLE-DELETE > +# > +create table t1 (a int not null primary key); > +insert into t1 values (1),(2),(3); > +create table t2 (a int not null primary key); > +insert into t2 values (1),(2),(3); > +delete t1,t2 from t1,t2 where t1.a=t2.a and rownum() <= 2; > +select * from t1; > +a > +3 > +select * from t2; > +a > +3 > +drop table t1,t2; > +# > +# MULTI-TABLE-UPDATE > +CREATE TABLE t1 (ID INT); > +CREATE TABLE t2 (ID INT, > +s1 TEXT, s2 TEXT, s3 VARCHAR(10), s4 TEXT, s5 VARCHAR(10)); > +INSERT INTO t1 VALUES (1),(2); > +INSERT INTO t2 VALUES (1,'test', 'test', 'test', 'test', 'test'), > +(2,'test', 'test', 'test', 'test', 'test'); > +SELECT * FROM t1 LEFT JOIN t2 USING(ID); > +ID s1 s2 s3 s4 s5 > +1 test test test test test > +2 test test test test test > +UPDATE t1 LEFT JOIN t2 USING(ID) SET s1 = 'changed'; > +select * from t2; > +ID s1 s2 s3 s4 s5 > +1 changed test test test test > +2 changed test test test test > +update t2 set s1=""; > +UPDATE t1 LEFT JOIN t2 USING(ID) SET s1 = 'changed' where rownum() <=1; > +select * from t2; > +ID s1 s2 s3 s4 s5 > +1 changed test test test test > +2 test test test test > +drop table t1,t2; > +# > +# LOAD DATA > +# > +create table t1 (a int, b int, c int); > +load data infile '../../std_data/loaddata7.dat' into table t1 fields terminated by ',' lines terminated by "\r\n" (a,b) set c=rownum(); > +select * from t1; > +a b c > +2 2 1 > +3 3 2 > +4 4 3 > +5 5 4 > +6 6 5 > +drop table t1; > +# > +# LIMIT OPTIMIZATION > +# > +create table t1 (a int); > +insert into t1 select seq from seq_1_to_100; > +flush status; > +select * from t1 where rownum() <= 3; > +a > +1 > +2 > +3 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 3 > +flush status; > +flush status; why twice? > +select * from t1 where rownum() <= 4 and rownum() <= 3; > +a > +1 > +2 > +3 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 3 > +flush status; > +select * from t1 where rownum() < 4 and a > 10; > +a > +11 > +12 > +13 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 13 > +flush status; > +select * from t1 where 3 >= rownum(); > +a > +1 > +2 > +3 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 3 > +flush status; > +select * from t1 where 4 > rownum() and a > 20; > +a > +21 > +22 > +23 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 23 > +flush status; > +select * from t1 where rownum() = 1 and a > 10; > +a > +11 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 11 > +flush status; > +select * from t1 where a > 30 && 1 = rownum(); > +a > +31 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 31 > +flush status; > +# No limit optimization > +select * from t1 where rownum() > 10; > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 You could've optimized it to `limit 0` but I guess it's not a particularly common use case > +flush status; > +select * from t1 where 10 < rownum(); > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where rownum() >= 10; > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where 10 < rownum(); > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where 10 <= rownum(); > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where 2 = rownum(); > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where rownum() = 2; > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where rownum() <= 0; > +a > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +select * from t1 where rownum() < 10 limit 4, 4; can you add a test for `select a, rownum() from t1 limit 4, 4` please ? > +a > +5 > +6 > +7 > +8 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 8 > +flush status; > +select * from t1 where rownum() < 10 order by a; > +a > +1 > +2 > +3 > +4 > +5 > +6 > +7 > +8 > +9 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 100 > +flush status; > +# rownum and limit > +select * from t1 where rownum() < 4 limit 10; > +a > +1 > +2 > +3 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 3 > +flush status; > +select * from t1 where rownum() < 10 limit 4; > +a > +1 > +2 > +3 > +4 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 4 > +drop table t1; > +# > +# Rownum examples from Woqutech > +# > +set SQL_MODE=ORACLE; > +create table t1 (c1 int ,c2 varchar(20)) engine=myisam; > +insert into t1 values (1, 'aaa'),(2, 'bbb'),(3, 'ccc'),(4, 'ddd'),(5, 'eee'); > +update t1 set c2 = 'xxx' where rownum = 2; > +select * from t1 where c2='xxx'; > +c1 c2 > +update t1 set c2 = 'xxx' where rownum < 3; > +select * from t1 where c2='xxx'; > +c1 c2 > +1 xxx > +2 xxx > +delete from t1 where rownum = 2; > +select count(*) from t1; > +count(*) > +5 > +delete from t1 where rownum < 3; > +select count(*) from t1; > +count(*) > +3 > +delete from t1 where c1=rownum ; > +select count(*) from t1; > +count(*) > +3 > +delete from t1 where c1=rownum+2 ; > +select count(*) from t1; > +count(*) > +0 > +set SQL_MODE=DEFAULT; > +drop table t1; > +# > +# Rownum() used in not supported places (returns 0 or gives an error) > +# > +set @a=rownum(); > +select @a; > +@a > +0 > +create or replace table t (a int, b int as (rownum()) virtual); > +ERROR HY000: Function or expression 'rownum()' cannot be used in the GENERATED ALWAYS AS clause of `b` > +create table t1 (a int); > +insert into t1 values (3),(1),(5),(8),(4); > +handler t1 open; > +handler t1 read next where rownum() < 1; > +ERROR HY000: Function or expression 'rownum()' cannot be used in the WHERE clause of `HANDLER` why is that? `handler t1 read next limit 2` works just fine. > +handler t1 close; > +drop table t1; > +create table t1 (a int not null primary key, b int); > +insert into t1 values (1,1),(2,2),(3,3); > +create function f() returns int return rownum(); > +select a, rownum(), f() from t1; > +a rownum() f() > +1 1 0 > +2 2 0 > +3 3 0 > +drop function f; > +drop table t1; > +create or replace table t1 (a int, r int); > +create trigger tr before update on t1 for each row set NEW.r = rownum(); > +insert into t1 (a) values (1),(2); > +select * from t1; > +a r > +1 NULL > +2 NULL > +update t1 set a=a+10; > +select * from t1; > +a r > +11 0 > +12 0 > +drop trigger tr; > +drop table t1; > +# > +# LIMIT optimisation > +# > +create table t1 (a int); > +insert into t1 values (1),(2),(3),(4),(5); > +flush status; > +select * from (select a from t1 where a < 1000) as tt where rownum() <= 2; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 2 > +explain extended select * from (select a from t1 where a < 1000) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 limit 2) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000) as tt where rownum() <= 2"; > +flush status; > +execute stmt; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 2 > +flush status; > +execute stmt; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 2 > +deallocate prepare stmt; > +flush status; > +select * from (select a from t1 where a < 1000 group by a) as tt where rownum() <= 2; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 5 > +explain extended select * from (select a from t1 where a < 1000 group by a) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where; Using temporary; Using filesort > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 group by `test`.`t1`.`a` limit 2) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 group by a) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +flush status; > +select * from (select a from t1 where a < 1000 group by a order by 1) as tt where rownum() <= 2; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 5 > +explain extended select * from (select a from t1 where a < 1000 group by a order by 1) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where; Using temporary; Using filesort > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 group by `test`.`t1`.`a` order by 1 limit 2) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 group by a order by 1) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +flush status; > +select * from (select a from t1 where a < 1000 union select 10) as tt where rownum() <= 2; > +a > +1 > +2 > +show status like "Rows_read"; > +Variable_name Value > +Rows_read 5 > +explain extended select * from (select a from t1 where a < 1000 union select 10) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +3 UNION NULL NULL NULL NULL NULL NULL NULL NULL No tables used > +NULL UNION RESULT <union2,3> ALL NULL NULL NULL NULL NULL NULL > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 union /* select#3 */ select 10 AS `10` limit 2) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 union select 10) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +# Other limit > +select * from (select a from t1 where a < 1000 group by a order by 1 limit 1) as tt where rownum() <= 2; > +a > +1 > +explain extended select * from (select a from t1 where a < 1000 group by a order by 1 limit 1) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where; Using temporary; Using filesort > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 group by `test`.`t1`.`a` order by 1 limit 1) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 group by a order by 1 limit 1) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +execute stmt; > +a > +1 > +deallocate prepare stmt; > +# Other limit less > +select * from (select a from t1 where a < 1000 group by a order by 1 limit 10) as tt where rownum() <= 2; > +a > +1 > +2 > +explain extended select * from (select a from t1 where a < 1000 group by a order by 1 limit 10) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where; Using temporary; Using filesort > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 group by `test`.`t1`.`a` order by 1 limit 2) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 group by a order by 1 limit 10) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +select * from (select a from t1 where a < 1000 union select 10 limit 1) as tt where rownum() <= 2; > +a > +1 > +explain extended select * from (select a from t1 where a < 1000 union select 10 limit 1) as tt where rownum() <= 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 5 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +3 UNION NULL NULL NULL NULL NULL NULL NULL NULL No tables used > +NULL UNION RESULT <union2,3> ALL NULL NULL NULL NULL NULL NULL > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 union /* select#3 */ select 10 AS `10` limit 1) `tt` where rownum() <= 2 > +prepare stmt from "select * from (select a from t1 where a < 1000 union select 10 limit 1) as tt where rownum() <= 2"; > +execute stmt; > +a > +1 > +execute stmt; > +a > +1 > +deallocate prepare stmt; > +# < rownum > +select * from (select a from t1 where a < 1000) as tt where rownum() < 2; > +a > +1 > +explain extended select * from (select a from t1 where a < 1000) as tt where rownum() < 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 limit 1) `tt` where rownum() < 2 > +prepare stmt from "select * from (select a from t1 where a < 1000) as tt where rownum() < 2"; > +execute stmt; > +a > +1 > +execute stmt; > +a > +1 > +deallocate prepare stmt; > +# Simple expression > +select * from (select a from t1 where a < 1000) as tt where rownum() <= 1+1; > +a > +1 > +2 > +explain extended select * from (select a from t1 where a < 1000) as tt where rownum() <= 1+1; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 limit 2) `tt` where rownum() <= <cache>(1 + 1) > +prepare stmt from "select * from (select a from t1 where a < 1000) as tt where rownum() <= 1+1"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +# Simple expression reversed > +select * from (select a from t1 where a < 1000) as tt where 1+1 >= rownum(); > +a > +1 > +2 > +explain extended select * from (select a from t1 where a < 1000) as tt where 1+1 >= rownum(); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 2 100.00 Using where > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000 limit 2) `tt` where <cache>(1 + 1) >= rownum() > +prepare stmt from "select * from (select a from t1 where a < 1000) as tt where 1+1 >= rownum()"; > +execute stmt; > +a > +1 > +2 > +execute stmt; > +a > +1 > +2 > +deallocate prepare stmt; > +# expensive (no opt) > +select * from (select a from t1 where a < 1000) as tt where (select max(a) from t1) >= rownum(); > +a > +1 > +2 > +3 > +4 > +5 > +explain extended select * from (select a from t1 where a < 1000) as tt where (select max(a) from t1) >= rownum(); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY <derived2> ALL NULL NULL NULL NULL 5 100.00 Using where > +3 SUBQUERY t1 ALL NULL NULL NULL NULL 5 100.00 > +2 DERIVED t1 ALL NULL NULL NULL NULL 5 100.00 Using where > +Warnings: > +Note 1003 /* select#1 */ select `tt`.`a` AS `a` from (/* select#2 */ select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`a` < 1000) `tt` where (/* select#3 */ select max(`test`.`t1`.`a`) from `test`.`t1`) >= rownum() > +prepare stmt from "select * from (select a from t1 where a < 1000) as tt where (select max(a) from t1) >= rownum()"; > +execute stmt; > +a > +1 > +2 > +3 > +4 > +5 > +execute stmt; > +a > +1 > +2 > +3 > +4 > +5 > +deallocate prepare stmt; > +drop table t1; > diff --git a/sql/filesort.h b/sql/filesort.h > index 29ae5e20cc6..806a4a8b027 100644 > --- a/sql/filesort.h > +++ b/sql/filesort.h > @@ -44,24 +44,21 @@ class Filesort: public Sql_alloc > /** Number of records to return */ > ha_rows limit; > /** ORDER BY list with some precalculated info for filesort */ > + ha_rows *accepted_rows; /* For ROWNUM */ > SORT_FIELD *sortorder; first, I suspect that you want to put your accepted_rows declaration before the comment that applies to sortorder. second, could you do a bit more than just "For ROWNUM"? why do you need something "for ROWNUM" inside Filesort? > /** select to use for getting records */ > SQL_SELECT *select; > + > /** TRUE <=> free select on destruction */ > bool own_select; > - /** true means we are using Priority Queue for order by with limit. */ > + /** TRUE means we are using Priority Queue for order by with limit. */ > bool using_pq; > - > /* > TRUE means sort operation must produce table rowids. > FALSE means that it halso has an option of producing {sort_key, > addon_fields} pairs. > */ > bool sort_positions; > - > - Filesort_tracker *tracker; > - Sort_keys *sort_keys; > - > /* > TRUE means all the fields of table of whose bitmap read_set is set > need to be read while reading records in the sort buffer. > diff --git a/sql/item_func.h b/sql/item_func.h > index 53c236bbce7..c237fcd9ecc 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -2125,6 +2125,63 @@ class Item_func_rand :public Item_real_func > }; > > > +class Item_func_rownum :public Item_longlong_func > +{ > + /* > + This points to a variable that contains the number of rows > + accpted so far in the result set > + */ > + ha_rows *accepted_rows; > + SELECT_LEX *select; > +public: > + Item_func_rownum(THD *thd); > + longlong val_int() override; > + LEX_CSTRING func_name_cstring() const override > + { > + static LEX_CSTRING name= {STRING_WITH_LEN("rownum") }; > + return name; > + } > + enum Functype functype() const override { return ROWNUM_FUNC; } > + void update_used_tables() override {} > + bool const_item() const override { return 0; } > + void fix_after_optimize(THD *thd) override; > + bool fix_fields(THD* thd, Item **ref) override; > + bool fix_length_and_dec() override > + { > + unsigned_flag= 1; > + used_tables_cache= RAND_TABLE_BIT; > + const_item_cache=0; > + set_maybe_null(); how can rownum be NULL? > + return FALSE; > + } > + void cleanup() override > + { > + Item_longlong_func::cleanup(); > + /* Ensure we don't point to freed memory */ > + accepted_rows= 0; > + } > + bool check_vcol_func_processor(void *arg) override > + { > + return mark_unsupported_function(func_name(), "()", arg, > + VCOL_IMPOSSIBLE); > + } > + bool check_handler_func_processor(void *arg) override > + { > + return mark_unsupported_function(func_name(), "()", arg, > + VCOL_IMPOSSIBLE); first, as I wrote above, I think that there's no logical reason why rownum should not with with handler. but even if it doesn't, it's much better to check for SQLCOM_HA_READ from Item_func_rownum::fix_fields instead of traversing the whole item tree for every single HANDLER READ command looking for Item_func_rownum that nobody (literally, not a single user out of all our millions) will use there. > + } > + Item *get_copy(THD *thd) override > + { > + return this; It looks wrong. We don't have any other Item that returns `this` here. I think rownum cannot be pushed down because of RAND_TABLE_BIT, So better return NULL here, meaning "cannot be pushed down". Instead of `this` meaning "if you push it down, it'll surreptitiously start corrupting the memory" > + } > + /* This function is used in insert, update and delete */ > + void store_pointer_to_row_counter(ha_rows *row_counter) So this isn't a virtual method, and to use it you downcast after checking type() == FUNC_ITEM and functype() == ROWNUM_FUNC. To avoid adding a new virtual method thus blowing up the vtable, I presume. And fix_after_optimize() is virtual. To avoid error-prone downcasting, would be my guess. Can you explain when you use what approach, please? > + { > + accepted_rows= row_counter; > + } > +}; > + > + > class Item_func_sign :public Item_long_func > { > bool check_arguments() const override > diff --git a/sql/item_func.cc b/sql/item_func.cc > index 25714dda03c..1165b87356a 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -7206,3 +7206,72 @@ void Item_func_setval::print(String *str, enum_query_type query_type) > str->append_ulonglong(round); > str->append(')'); > } > + > + > +/* > + Return how many row combinations has accepted so far + 1 > + > + The + 1 is to ensure that 'WHERE ROWNUM <=1' returns one row this is a very weird explanation, it sounds as if it's a heuristical a hack for ROWNUM <=1 to work Why not to write "+1 takes into account the currently computed row" > +*/ > + > +longlong Item_func_rownum::val_int() > +{ > + if (!accepted_rows) > + { > + /* > + Rownum is not properly set up. Probably used in wrong context when > + it should not be used. In this case returning 0 is probably the best > + solution. > + */ DBUG_ASSERT(0); here? > + return 0; > + } > + return (longlong) *accepted_rows+1; > +} > + > + > +Item_func_rownum::Item_func_rownum(THD *thd): > + Item_longlong_func(thd),accepted_rows(0) > +{ > + /* > + Remember the select context. > + Add the function to the list fix_after_optimize in the select context > + so that we can easily initializef all rownum functions with the pointers > + to the row counters. > + */ > + select= thd->lex->current_select; > + select->fix_after_optimize.push_back(this, thd->mem_root); > + > + /* > + Mark that query is using rownum() and ensure that this select is > + not merged with other selects > + */ > + select->with_rownum= 1; > + thd->lex->with_rownum= 1; > + thd->lex->uncacheable(UNCACHEABLE_RAND); Why? ROWNUM doesn't make a query uncacheable, it's perfectly deterministic and repeatable. > + > + /* If this command changes data, mark it as unsafe for statement logging */ > + if (sql_command_flags[thd->lex->sql_command] & > + (CF_UPDATES_DATA | CF_DELETES_DATA)) > + thd->lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_SYSTEM_FUNCTION); Why? It's the same as LIMIT. unsafe without ORDER BY and perfectly safe with. > +} > + > + > +bool Item_func_rownum::fix_fields(THD* thd, Item **ref) > +{ > + if (select->having_fix_field) > + { > + my_error(ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE, MYF(0), > + "rownum()", "HAVING"); > + return 1; > + } > + return Item_longlong_func::fix_fields(thd, ref); > +} > + > +/* > + Store a reference to the variable that contains number of accepted rows > +*/ > + > +void Item_func_rownum::fix_after_optimize(THD *thd) > +{ > + accepted_rows= &select->join->accepted_rows; why do you need both that and fix_rownum_pointers()? I'd think that the latter alone would be sufficient > +} > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index badaaa17716..aecb00563f7 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7973,3 +7973,5 @@ ER_DATA_WAS_COMMITED_UNDER_ROLLBACK > eng "Engine %s does not support rollback. Changes were committed during rollback call" > ER_PK_INDEX_CANT_BE_IGNORED > eng "A primary key cannot be marked as IGNORE" > +ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE > + eng "Function '%s' cannot be used in the %s clause" Is it the first function we've got that cannot be used in HAVING? > diff --git a/sql/sql_derived.cc b/sql/sql_derived.cc > index 35b4294707f..da6c567b87c 100644 > --- a/sql/sql_derived.cc > +++ b/sql/sql_derived.cc > @@ -656,14 +656,22 @@ static > bool mysql_derived_prepare(THD *thd, LEX *lex, TABLE_LIST *derived) > { > SELECT_LEX_UNIT *unit= derived->get_unit(); > - bool res= FALSE; > + SELECT_LEX *first_select; > + bool res= FALSE, keep_row_order; > DBUG_ENTER("mysql_derived_prepare"); > DBUG_PRINT("enter", ("unit: %p table_list: %p alias: '%s'", > unit, derived, derived->alias.str)); > if (!unit) > DBUG_RETURN(FALSE); > > - SELECT_LEX *first_select= unit->first_select(); > + first_select= unit->first_select(); > + /* > + If rownum() is used we have to preserve the insert row order > + to make group by with filesort to work. it'd help if you'd add an example of a SELECT here > + */ > + keep_row_order= (thd->lex->with_rownum && > + (first_select->group_list.elements || > + first_select->order_list.elements)); > > if (derived->is_recursive_with_table() && > !derived->is_with_table_recursive_reference() && > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 806049b4465..5c4dff33914 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -746,6 +746,9 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > DBUG_RETURN(TRUE); > } > + > + if (thd->lex->with_rownum && table_list->lock_type == TL_WRITE_DELAYED) > + table_list->lock_type= TL_WRITE; why? do you have a test case when it's needed? > > if (table_list->lock_type == TL_WRITE_DELAYED) > { > if (open_and_lock_for_insert_delayed(thd, table_list)) > @@ -968,11 +971,16 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > */ > if (returning && > result->send_result_set_metadata(returning->item_list, > - Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > + Protocol::SEND_NUM_ROWS | > + Protocol::SEND_EOF)) > goto values_loop_end; > > THD_STAGE_INFO(thd, stage_update); > thd->decide_logging_format_low(table); > + fix_rownum_pointers(thd, thd->lex->current_select, &info.copied); > + if (returning) > + fix_rownum_pointers(thd, thd->lex->returning(), &info.copied); > + Note that info.copied can be incremented more than once per row (in INSERT...ON DUPLICATE KEY UPDATE, for example). This will probably break rownum logic. > do > { > DBUG_PRINT("info", ("iteration %llu", iteration)); > @@ -2133,8 +2141,14 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > autoinc values (generated inside the handler::ha_write()) and > values updated in ON DUPLICATE KEY UPDATE. > */ > - if (sink && sink->send_data(thd->lex->returning()->item_list) < 0) > - trg_error= 1; > + if (sink) > + { > + /* We have to adjust 'copied' to make ROWNUM() work in RETURNING */ > + info->copied--; > + if (sink->send_data(thd->lex->returning()->item_list) < 0) > + trg_error= 1; > + info->copied++; this is quite silly. Can you simply increment info->copied after this sink->send_data call? > + } > > after_trg_or_ignored_err: > if (key) > diff --git a/sql/sql_limit.h b/sql/sql_limit.h > index a4fcedac14a..62fd0546af0 100644 > --- a/sql/sql_limit.h > +++ b/sql/sql_limit.h > @@ -67,6 +67,15 @@ class Select_limit_counters > { return select_limit_cnt; } > ha_rows get_offset_limit() > { return offset_limit_cnt; } > + > + void set_limit_if_lower(ha_rows limit) > + { > + if (!offset_limit_cnt) I suppose, alternatively, you could've had if (offset_limit_cnt + select_limit_cnt > limit) select_limit_cnt= limit - offset_limit_cnt; > + { > + if (select_limit_cnt > limit) > + select_limit_cnt= limit; > + } > + } > }; > > #endif // INCLUDES_MARIADB_SQL_LIMIT_H > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 2bd2575b882..57ba9df42c0 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -10286,6 +10287,22 @@ function_call_nonkeyword: > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > +/* Start SQL_MODE_ORACLE_SPECIFIC > + | ROWNUM_SYM optional_braces > + { > + $$= new (thd->mem_root) Item_func_rownum(thd); > + if (unlikely($$ == NULL)) > + MYSQL_YYABORT; > + } > +End SQL_MODE_ORACLE_SPECIFIC */ > +/* Start SQL_MODE_DEFAULT_SPECIFIC */ > + | ROWNUM_SYM '(' ')' > + { > + $$= new (thd->mem_root) Item_func_rownum(thd); > + if (unlikely($$ == NULL)) > + MYSQL_YYABORT; > + } > +/* End SQL_MODE_DEFAULT_SPECIFIC */ You have two nearly identical rules compiled conditionally. I can pretty much guarantee that they'll go out of sync eventually. Better do something like /* Start SQL_MODE_ORACLE_SPECIFIC oracle_optional_braces: optional_braces ; End SQL_MODE_ORACLE_SPECIFIC */ /* Start SQL_MODE_DEFAULT_SPECIFIC */ oracle_optional_braces: '(' ')' ; /* End SQL_MODE_DEFAULT_SPECIFIC */ or even | ROWNUM_SYM /* Start SQL_MODE_ORACLE_SPECIFIC optional_braces End SQL_MODE_ORACLE_SPECIFIC */ /* Start SQL_MODE_DEFAULT_SPECIFIC */ '(' ')' /* End SQL_MODE_DEFAULT_SPECIFIC */ { $$= new (thd->mem_root) Item_func_rownum(thd); if (unlikely($$ == NULL)) MYSQL_YYABORT; } > | SUBDATE_SYM '(' expr ',' expr ')' > { > $$= new (thd->mem_root) Item_date_add_interval(thd, $3, $5, > diff --git a/sql/table.cc b/sql/table.cc > index 7f3a6468aa2..100813e1f95 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -9331,6 +9331,10 @@ bool TABLE_LIST::init_derived(THD *thd, bool init_view) > { > /* A subquery might be forced to be materialized due to a side-effect. */ > if (!is_materialized_derived() && first_select->is_mergeable() && > + (unit->outer_select() && !unit->outer_select()->with_rownum) && no merging if *outer select* uses rownum? why? example? > + (!thd->lex->with_rownum || > + (!first_select->group_list.elements && > + !first_select->order_list.elements)) && no merging if no rownum but there's group by or order by? group by - this is obvious, and is already checked earlier in this if() order by - why? > optimizer_flag(thd, OPTIMIZER_SWITCH_DERIVED_MERGE) && > !thd->lex->can_not_use_merged(1) && > !is_recursive_with_table()) > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 4bb0bc661bd..b002e27799b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1789,10 +1803,18 @@ JOIN::optimize_inner() > { > DBUG_ENTER("JOIN::optimize_inner"); > subq_exit_fl= false; > - do_send_rows = (unit->lim.get_select_limit()) ? 1 : 0; > > DEBUG_SYNC(thd, "before_join_optimize"); > > + // rownum used somewhere in query, no limits and it is derived > + if (unlikely(thd->lex->with_rownum && > + select_lex->first_cond_optimization && > + select_lex->master_unit()->derived)) > + optimize_upper_rownum_func(); > + > + do_send_rows = (unit->lim.get_select_limit()) ? 1 : 0; Is there any particular reason why you put that between DEBUG_SYNC(thd, "before_join_optimize"); and THD_STAGE_INFO(thd, stage_optimizing); ? I'd expect them logically to go directly one after the other > + > + > THD_STAGE_INFO(thd, stage_optimizing); > > set_allowed_join_cache_types(); > @@ -14210,7 +14236,24 @@ static ORDER * > remove_const(JOIN *join,ORDER *first_order, COND *cond, > bool change_list, bool *simple_order) > { > - *simple_order= join->rollup.state == ROLLUP::STATE_NONE; > + /* > + We can't do ORDER BY using filesort if the select list contains a non > + deterministic value like RAND() or ROWNUM(). > + For example: > + SELECT RAND() as 'r' from t1 order by r; This one uses temporary for me. Can you put here a correct example please? > + > + If we would first sort the table 't1', the RAND() column would be generated > + during end_send() and the order would be wrong. > + > + Previously we had here also a test of ROLLUP: > + 'join->rollup.state == ROLLUP::STATE_NONE' > + I deleted this because > + the ROLLUP was never enforced because of a bug where the inital value > + of simple_order was ignored. Having ROLLUP tested now when the code > + is fixed causes many test failure and some wrong results so better to > + leave the code as it was related to ROLLUP. No need to explain in a comment why something that is no longer here was removed. Use git for that, commit comments. The code comment should explain the code as it is. > + */ > + *simple_order= !join->rand_table_in_field_list; why do you need rand_table_in_field_list instead of writing *simple_order= join->select_lex->select_list_tables & RAND_TABLE_BIT; ? > if (join->only_const_tables()) > return change_list ? 0 : first_order; // No need to sort > > @@ -29587,6 +29640,273 @@ void unpack_to_base_table_fields(TABLE *table) > (*cp->do_copy)(cp); > } > > +/* > + Call item->fix_after_optimize for all items registered in > + lex->fix_after_optimize > + > + This is needed for items like ROWNUM(), which needs access to structures > + created by the early optimizer pass, like JOIN > +*/ > + > +static void fix_items_after_optimize(THD *thd, SELECT_LEX *select_lex) > +{ > + List_iterator<Item> li(select_lex->fix_after_optimize); > + > + while (Item *item= li++) > + item->fix_after_optimize(thd); > +} > + > + here you could've had a comment, like /* callbacks for process_direct_rownum_comparison() */ > +static bool is_const(Item *item) > +{ > + /* > + Constant expression in WHERE can be also subquery or other expensive > + thing we shoud not evaluate on OPTIMIZATION phase > + */ > + return item->const_item() && !item->is_expensive(); > +} > + > + > +// Check if this is an unexpensive functional expression over basic constants > + > +static bool is_basic_const(Item *item) > +{ > + if (item->basic_const_item()) > + return true; > + if (!item->const_item() || item->is_expensive()) > + return false; > + if (item->type() == Item::FUNC_ITEM) > + { > + Item_func *func= ((Item_func *)item); > + if (func->argument_count() == 0) > + return false; > + bool result= true; > + Item **args= func->arguments(); > + for (uint i= 0; i < func->argument_count() && result; i++) > + result= result && is_basic_const(args[i]); > + return result; > + } > + return false; > +} is_basic_const() is a misleading name, a function of basic consts isn't a basic const itself. what's an example of an expression has is_const() == true, but is_basic_const() == false? > + > + > +inline bool is_simple_rownum_exp(Item *rownum, Item *constant) unused > +{ > + return (constant->basic_const_item() && > + rownum->type() == Item::FUNC_ITEM && > + ((Item_func*) rownum)->functype() == Item_func::ROWNUM_FUNC); > +} > + > + > +static bool temporary_set_limit(THD *thd __attribute__((unused)), > + SELECT_LEX_UNIT *unit, > + ha_rows lim) > +{ > + unit->set_limit_if_lower(lim); why is it temporary? in the PS/SP sense, just for one execution? > + return false; > +} > + > + > +static bool permanent_limit(THD *thd, SELECT_LEX_UNIT *unit, ha_rows lim) > +{ > + SELECT_LEX *gpar= unit->global_parameters(); > + if (gpar->select_limit != 0 && > + // limit can not be an expression but can be parameter > + (!gpar->select_limit->basic_const_item() || > + ((ha_rows)gpar->select_limit->val_int()) < lim)) > + return false; > + > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + > + gpar->select_limit= > + new (thd->mem_root)Item_int(thd, lim, 20 /* max 64 bit length*/ ); we have defines for that, MAX_BIGINT_WIDTH, MY_INT64_NUM_DECIMAL_DIGITS, LONGLONG_BUFFER_SIZE - whatever you prefer. > + if (gpar->select_limit == 0) > + return true; // EOM > + > + unit->set_limit(gpar); > + > + gpar->explicit_limit= true; // to show in EXPLAIN > + > + if (arena) > + thd->restore_active_arena(arena, &backup); > + > + return false; > +} > + > + > +/** > + Check posibility of LIMIT setting by rownum() of upper SELECT and do it > + > + @note Ideal is to convert something like > + SELECT ... > + FROM (SELECT ...) table > + WHERE rownum() < <CONSTANT>; > + to > + SELECT ... > + FROM (SELECT ... LIMIT <CONSTANT>) table > + WHERE rownum() < <CONSTANT>; 1. what if the outer select uses LIMIT, not ROWNUM? why not to push that down too? 2. why are you not removing rownum from the outer select? if you remove rownum and reset with_rownum=false, it'll generally give optimizer more freedom. > + > + @retval true EOM > + @retval false no errors > +*/ > + > +bool JOIN::optimize_upper_rownum_func() > +{ > + DBUG_ASSERT(select_lex->master_unit()->derived); > + > + if (select_lex->master_unit()->first_select() != select_lex) > + return false; // first will set parameter > + > + if (select_lex->master_unit()->global_parameters()->offset_limit != NULL) > + return false; // offset is set, we cannot set limit > + > + SELECT_LEX *outer_select= select_lex->master_unit()->outer_select(); > + /* > + Check that the outer query (the one that has 'WHERE rownum()...') is using > + to see if the can be use it to limit this sub query > + */ > + if (outer_select == NULL || > + !outer_select->with_rownum || > + (outer_select->options & SELECT_DISTINCT) || > + outer_select->table_list.elements != 1 || > + outer_select->where == NULL || > + outer_select->where->type() != Item::FUNC_ITEM) > + return false; > + > + return process_direct_rownum_comparison(thd, unit, outer_select->where, > + &is_basic_const, > + &permanent_limit); > +} > + > +/** > + Test if the predicate compares rownum() with a constant > + > + @return 1 No or invalid rownum() compare > + @return 0 rownum() is compared with a constant. > + In this case *args contains the constant and > + *inv_order constains 1 if the rownum() was the right > + argument, like in 'WHERE 2 >= rownum()'. > +*/ > + > +static bool check_rownum_usage(Item_func *func_item, longlong *limit, > + bool *inv_order, const_probe probe) > +{ > + Item *arg1, *arg2; > + *inv_order= 0; > + DBUG_ASSERT(func_item->argument_count() == 2); > + > + /* 'rownum op const' or 'const op field' */ > + arg1= func_item->arguments()[0]->real_item(); > + if (arg1->type() == Item::FUNC_ITEM && > + ((Item_func*) arg1)->functype() == Item_func::ROWNUM_FUNC) > + { > + arg2= func_item->arguments()[1]->real_item(); > + if ((*probe)(arg2)) > + { > + *limit= arg2->val_int(); > + return *limit <= 0 || (ulonglong) *limit >= HA_POS_ERROR; > + } > + } > + else if ((*probe)(arg1)) > + { > + arg2= func_item->arguments()[1]->real_item(); why not to assign arg2 outside of the if() ? > + if (arg2->type() == Item::FUNC_ITEM && > + ((Item_func*) arg2)->functype() == Item_func::ROWNUM_FUNC) > + { > + *limit= arg1->val_int(); > + *inv_order= 1; > + return *limit <= 0 || (ulonglong) *limit >= HA_POS_ERROR; > + } > + } > + return 1; > +} > + > + > +/* > + Limit optimization for ROWNUM() > + > + Go through the WHERE clause and find out if there are any of the following > + constructs on the top level: > + rownum() <= integer_constant > + rownum() < integer_constant > + rownum() = 1 > + > + If yes, then threat the select as if 'LIMIT integer_constant' would > + have been used > +*/ > + > +static void optimize_rownum(THD *thd, SELECT_LEX_UNIT *unit, Item *cond) > +{ > + DBUG_ENTER("optimize_rownum"); > + > + if (cond->type() == Item::COND_ITEM) > + { > + if (((Item_cond*) cond)->functype() == Item_func::COND_AND_FUNC) > + { > + List_iterator<Item> li(*((Item_cond*) cond)->argument_list()); > + Item *item; > + while ((item= li++)) > + optimize_rownum(thd, unit, item); > + } > + DBUG_VOID_RETURN; > + } > + > + process_direct_rownum_comparison(thd, unit, cond, &is_const, > + &temporary_set_limit); please, explain why it's is_const/temporary_set_limit here and is_basic_const/permanent_limit in optimize_upper_rownum_func(). > + DBUG_VOID_RETURN; > +} > + > + > +static bool process_direct_rownum_comparison(THD *thd, > + SELECT_LEX_UNIT *unit, > + Item *cond, > + const_probe probe, > + set_limit_callback set_limit) > +{ > + DBUG_ENTER("process_direct_rownum_comparison"); > + if (cond->real_type() == Item::FUNC_ITEM) > + { > + Item_func *pred= (Item_func*) cond; > + longlong limit; > + bool inv; > + > + if (pred->argument_count() != 2) > + DBUG_RETURN(false); // Not a compare functions > + if (check_rownum_usage(pred, &limit, &inv, probe)) > + DBUG_RETURN(false); > + > + Item_func::Functype pred_type= pred->functype(); > + > + if (inv && pred_type != Item_func::EQ_FUNC) > + { > + if (pred_type == Item_func::GT_FUNC) // # > rownum() > + pred_type= Item_func::LT_FUNC; > + else if (pred_type == Item_func::GE_FUNC) // # >= rownum() > + pred_type= Item_func::LE_FUNC; > + else > + DBUG_RETURN(false); > + } > + switch (pred_type) { > + case Item_func::LT_FUNC: // rownum() < # > + { > + if (limit <= 0) > + DBUG_RETURN(false); > + DBUG_RETURN((*set_limit)(thd, unit, limit - 1)); > + case Item_func::LE_FUNC: > + DBUG_RETURN((*set_limit)(thd, unit, limit)); > + case Item_func::EQ_FUNC: > + if (limit == 1) > + DBUG_RETURN((*set_limit)(thd, unit, limit)); > + break; > + default: > + break; > + } > + } what if it's rownum() > 5 ? why not to mark it as always false right away? > + } > + DBUG_RETURN(false); > +} > + > > /** > @} (end of group Query_Optimizer) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] a94502ab674: Added typedef decimal_digits_t (uint16) for number of decimals
by Sergei Golubchik 11 May '21

11 May '21

Hi, Michael! On Mar 26, Michael Widenius wrote: > revision-id: a94502ab674 (mariadb-10.5.2-511-ga94502ab674) > parent(s): 2be9b69f4ff > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:19:55 +0200 > message: > > Added typedef decimal_digits_t (uint16) for number of decimals > For fields and Item's uint8 should be good enough. After > discussions with Alexander Barkov we choose uint16 (for now) > as some format functions may accept +256 digits. > > The reason for this patch was to make the storage of decimal > digits simlar. Before this patch decimals was stored/used as > uint8, int and uint. > > Changed most decimal variables and functions to use the new > typedef. > > diff --git a/include/decimal.h b/include/decimal.h > index cab18f99348..f713409ede1 100644 > --- a/include/decimal.h > +++ b/include/decimal.h > @@ -53,10 +53,11 @@ int decimal2double(const decimal_t *from, double *to); > int double2decimal(double from, decimal_t *to); > int decimal_actual_fraction(const decimal_t *from); > int decimal2bin(const decimal_t *from, uchar *to, int precision, int scale); > -int bin2decimal(const uchar *from, decimal_t *to, int precision, int scale); > +int bin2decimal(const uchar *from, decimal_t *to, int precision, > + decimal_digits_t scale); > > -int decimal_size(int precision, int scale); > -int decimal_bin_size(int precision, int scale); > +int decimal_size(int precision, decimal_digits_t scale); > +int decimal_bin_size(int precision, decimal_digits_t scale); this is *very* confusing. "decimal" in the context of an Item mean "number of digits after a dot" "decimal" in decimal.h means the whole number, and after a dot is "scale" decimal_digits_t in decimal.h does *not* mean what you want, I'm reviewing this your patch for the third time (first was in Sept, then in Dec). And only now I finally understood what you mean (I think). Please, please, don't use decimal_digits_t in decimal.h FYI, more confusing terminology thay I might need below "precision" for decimal numbers is the total number of digits in a number "precision" for temporal types means the number of digits after a dot > int decimal_result_size(decimal_t *from1, decimal_t *from2, char op, > int param); > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 5fa8f28ff7a..66ca2bf4537 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -79,6 +79,7 @@ typedef const struct my_collation_handler_st MY_COLLATION_HANDLER; > typedef const struct unicase_info_st MY_UNICASE_INFO; > typedef const struct uni_ctype_st MY_UNI_CTYPE; > typedef const struct my_uni_idx_st MY_UNI_IDX; > +typedef uint16 decimal_digits_t; This is Item (and Field) specific declaration, please put it in field.h or item.h > typedef struct unicase_info_char_st > { > diff --git a/sql/field.cc b/sql/field.cc > index 52074417046..08c168e0e21 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -3281,10 +3281,11 @@ Field *Field_decimal::make_new_field(MEM_ROOT *root, TABLE *new_table, > ** Field_new_decimal > ****************************************************************************/ > > -static uint get_decimal_precision(uint len, uint8 dec, bool unsigned_val) > +static decimal_digits_t get_decimal_precision(uint len, decimal_digits_t dec, > + bool unsigned_val) > { > uint precision= my_decimal_length_to_precision(len, dec, unsigned_val); > - return MY_MIN(precision, DECIMAL_MAX_PRECISION); > + return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION); No, this is wrong (or, rather, inconsistent with your other changes). "precision" for DECIMAL is the total number of digits, not a number of digits after the dot. Judging by your edits in decimal.h you don't want that to be decimal_digits_t. > } > > Field_new_decimal::Field_new_decimal(uchar *ptr_arg, > @@ -10390,7 +10391,8 @@ void Column_definition::create_length_to_internal_length_bit() > void Column_definition::create_length_to_internal_length_newdecimal() > { > DBUG_ASSERT(length < UINT_MAX32); > - uint prec= get_decimal_precision((uint)length, decimals, flags & UNSIGNED_FLAG); > + decimal_digit_t prec= get_decimal_precision((uint)length, decimals, > + flags & UNSIGNED_FLAG); same as above, you should decide whether you want decimal precision to be decimal_digit_t or not. Currently this change contraditcs your decimal.h changes. > pack_length= my_decimal_get_binary_size(prec, decimals); > } > > diff --git a/sql/field.h b/sql/field.h > index 4a4f7cee2a5..5b6a69d0075 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -2331,7 +2332,7 @@ class Field_decimal final :public Field_real { > class Field_new_decimal final :public Field_num { > public: > /* The maximum number of decimal digits can be stored */ > - uint precision; > + decimal_digits_t precision; oops. And here again you use decimal_digits_t for decimal precision. > uint bin_size; > /* > Constructors take max_length of the field as a parameter - not the > diff --git a/sql/item.h b/sql/item.h > index 1087c08869e..6753474f2dd 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1649,14 +1649,14 @@ class Item: public Value_source, > return type_handler()->Item_decimal_precision(this); > } > /* Returns the number of integer part digits only */ > - inline int decimal_int_part() const > - { return my_decimal_int_part(decimal_precision(), decimals); } > + inline decimal_digits_t decimal_int_part() const > + { return (decimal_digits_t) my_decimal_int_part(decimal_precision(), decimals); } and here you use decimal_digits_t for precision-scale. > /* > Returns the number of fractional digits only. > NOT_FIXED_DEC is replaced to the maximum possible number > of fractional digits, taking into account the data type. > */ > - uint decimal_scale() const > + decimal_digits_t decimal_scale() const for example, this is consistent with your other changes. scale is decimal_digits_t, it's clear. > { > return type_handler()->Item_decimal_scale(this); > } > @@ -4853,7 +4853,7 @@ class Item_temporal_literal :public Item_literal > Item_literal(thd) > { > collation= DTCollation_numeric(); > - decimals= dec_arg; > + decimals= (decimal_digits_t) dec_arg; why do you cast? I'd expect dec_arg to be decimal_digits_t already > } > > int save_in_field(Field *field, bool no_conversions) override > diff --git a/sql/item_func.cc b/sql/item_func.cc > index a0ef4020aae..db51639a5af 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -2710,7 +2710,7 @@ my_decimal *Item_func_round::decimal_op(my_decimal *decimal_value) > dec= INT_MIN; > > if (!(null_value= (value.is_null() || args[1]->null_value || > - value.round_to(decimal_value, (uint) dec, > + value.round_to(decimal_value, (int) dec, > truncate ? TRUNCATE : HALF_UP) > 1))) I don't think you need to cast decimal_digits_t (aka uint16) to int explicitly. A compiler can handle it, it's not lossy. > return decimal_value; > return 0; > diff --git a/sql/item_func.h b/sql/item_func.h > index e774d9c53bd..ae94698ff96 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -1383,10 +1383,10 @@ class Item_decimal_typecast :public Item_func > { > my_decimal decimal_value; > public: > - Item_decimal_typecast(THD *thd, Item *a, uint len, uint dec) > + Item_decimal_typecast(THD *thd, Item *a, uint len, decimal_digits_t dec) > :Item_func(thd, a) > { > - decimals= (uint8) dec; > + decimals= (decimal_digits_t) dec; not needed, dec is decimal_digits_t already > collation= DTCollation_numeric(); > fix_char_length(my_decimal_precision_to_length_no_truncation(len, dec, > unsigned_flag)); > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 5a31b39c7b6..1c5e03f34bd 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1218,9 +1218,10 @@ uint32 Type_numeric_attributes::find_max_octet_length(Item **item, uint nitems) > } > > > -int Type_numeric_attributes::find_max_decimal_int_part(Item **item, uint nitems) > +decimal_digits_t Type_numeric_attributes:: > +find_max_decimal_int_part(Item **item, uint nitems) > { > - int max_int_part= 0; > + decimal_digits_t max_int_part= 0; again, "int parts" = precision-scale. it's not clear from your patch whether it should be decimal_digits_t > for (uint i=0 ; i < nitems ; i++) > set_if_bigger(max_int_part, item[i]->decimal_int_part()); > return max_int_part; > @@ -1237,11 +1238,12 @@ Type_numeric_attributes::aggregate_numeric_attributes_decimal(Item **item, > uint nitems, > bool unsigned_arg) > { > - int max_int_part= find_max_decimal_int_part(item, nitems); > + decimal_digits_t max_int_part= find_max_decimal_int_part(item, nitems); same here > decimals= find_max_decimals(item, nitems); > - int precision= MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > + decimal_digits_t precision= (decimal_digits_t) > + MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); and here > max_length= my_decimal_precision_to_length_no_truncation(precision, > - (uint8) decimals, > + decimals, > unsigned_flag); > } > > @@ -6955,20 +6957,20 @@ const Vers_type_handler* Type_handler_blob_common::vers() const > > /***************************************************************************/ > > -uint Type_handler::Item_time_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_time_precision(THD *thd, Item *item) const But here it's correct. "precision" in the temporal context is the same as "scale" in the decimal context and "decimals" in the Item/Field context. So, decimal_digits_t, all right. > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); > } > > > -uint Type_handler::Item_datetime_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_datetime_precision(THD *thd, Item *item) const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); > } > > > -uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > - bool is_time) const > +decimal_digits_t Type_handler_string_result:: > +Item_temporal_precision(THD *thd, Item *item, bool is_time) const > { > StringBuffer<64> buf; > String *tmp; > @@ -7020,7 +7022,7 @@ uint Type_handler_temporal_result:: > > /***************************************************************************/ > > -uint Type_handler_string_result::Item_decimal_precision(const Item *item) const > +decimal_digits_t Type_handler_string_result::Item_decimal_precision(const Item *item) const but here it's "decimal precision", so wrong/inconsistent again. > { > uint res= item->max_char_length(); > /* Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 58df9e3446b: MDEV-22010: use executables MariaDB named in scripts
by Sergei Golubchik 11 May '21

11 May '21

Hi, Rucha! It's ok to push, with a couple of changes, see below On May 11, Rucha Deodhar wrote: > revision-id: 58df9e3446b (mariadb-10.5.2-386-g58df9e3446b) > parent(s): de407e7cb4d > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-04-06 16:40:42 +0530 > message: > > MDEV-22010: use executables MariaDB named in scripts > diff --git a/scripts/CMakeLists.txt b/scripts/CMakeLists.txt > index 8d6a486a29d..6f3393a9c22 100644 > --- a/scripts/CMakeLists.txt > +++ b/scripts/CMakeLists.txt > @@ -142,11 +142,11 @@ IF(UNIX) > # FIND_PROC and CHECK_PID are used by mysqld_safe > IF(CMAKE_SYSTEM_NAME MATCHES "Linux") > SET (FIND_PROC > - "ps wwwp $PID | grep -v mysqld_safe | grep -- $MYSQLD > /dev/null") > + "ps wwwp $PID | grep -v mariadbd-safe | grep -- $MYSQLD > /dev/null") I think you need to filter out both mysqld_safe and mariadbd-safe here. because the user can start this script under either name, you need to ignore both. Like grep -v '$mysqld_safe\|mariadbd-safe$' > ENDIF() > IF(NOT FIND_PROC AND CMAKE_SYSTEM_NAME MATCHES "SunOS") > SET (FIND_PROC > - "ps -p $PID | grep -v mysqld_safe | grep -- $MYSQLD > /dev/null") > + "ps -p $PID | grep -v mariadbd-safe | grep -- $MYSQLD > /dev/null") > ENDIF() > > IF(NOT FIND_PROC) > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh > index 5f183afe8fc..05ce4f31a69 100644 > --- a/scripts/mysql_install_db.sh > +++ b/scripts/mysql_install_db.sh > @@ -613,8 +613,8 @@ then > echo "PLEASE REMEMBER TO SET A PASSWORD FOR THE MariaDB root USER !" > echo "To do so, start the server, then issue the following commands:" > echo > - echo "'$bindir/mysqladmin' -u root password 'new-password'" > - echo "'$bindir/mysqladmin' -u root -h $hostname password 'new-password'" > + echo "'$bindir/mariadb-admin' -u root password 'new-password'" > + echo "'$bindir/mariadb-admin' -u root -h $hostname password 'new-password'" > echo > echo "Alternatively you can run:" > echo "'$bindir/mysql_secure_installation'" ^^^ mariadb-secure-installation Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e7762b3a725: MDEV-8334: Rename utf8 to utf8mb3
by Sergei Golubchik 05 May '21

05 May '21

Hi, Rucha! Looks great! Just one question below re. using global vs session old_behavior variable. On May 05, Rucha Deodhar wrote: > revision-id: e7762b3a725 (mariadb-10.5.2-583-ge7762b3a725) > parent(s): bfedf1eb4b6 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-04-20 12:50:32 +0530 > message: > > MDEV-8334: Rename utf8 to utf8mb3 > > This patch changes the main name of 3 byte character set from utf8 to > utf8mb3. New old_mode UTF8_IS_UTF8MB3 is added and set TRUE by default, > so that utf8 would mean utf8mb3. If not set, utf8 would mean utf8mb4. > diff --git a/libmariadb b/libmariadb > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit b6f8883d9687936a50a7ed79bd9e5af2340efccd > +Subproject commit 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab when rebasing and pushing into 10.6 take care not to rollback C/C changes. That is, only update C/C submodule reference if it's earlier than your 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab > Binary files a/mysql-test/suite/sys_vars/r/character_set_results_basic.result and b/mysql-test/suite/sys_vars/r/character_set_results_basic.result differ > diff --git a/sql/item.cc b/sql/item.cc > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -2359,6 +2359,9 @@ left_is_superset(const DTCollation *left, const DTCollation *right) > > bool DTCollation::aggregate(const DTCollation &dt, uint flags) > { > + > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; if old_behavior is a session variable, then you should use session value here. > if (!my_charset_same(collation, dt.collation)) > { > /* > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -4099,6 +4098,8 @@ static int init_common_variables() > test purposes, to be able to start "mysqld" even if > the requested character set is not available (see bug#18743). > */ > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; ok, here there's no "session" yet, so global value is correct > for (;;) > { > char *next_character_set_name= strchr(default_character_set_name, ','); > diff --git a/sql/set_var.cc b/sql/set_var.cc > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -533,11 +533,12 @@ static my_old_conv old_conv[]= > CHARSET_INFO *get_old_charset_by_name(const char *name) > { > my_old_conv *conv; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; technically, you should use a session value here too. but see the old_conv array, it doesn't have "utf8" anywhere, so it doesn't matter what the flag is, you can use 0 there. > for (conv= old_conv; conv->old_name; conv++) > { > if (!my_strcasecmp(&my_charset_latin1, name, conv->old_name)) > - return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, MYF(0)); > + return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, MYF(utf8_flag)); > } > return NULL; > } > diff --git a/sql/sp.cc b/sql/sp.cc > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -291,7 +291,8 @@ bool load_charset(MEM_ROOT *mem_root, > CHARSET_INFO **cs) > { > LEX_CSTRING cs_name; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; but this needs a session value, I suppose > if (field->val_str_nopad(mem_root, &cs_name)) > { > *cs= dflt_cs; > @@ -324,9 +325,10 @@ bool load_collation(MEM_ROOT *mem_root, > *cl= dflt_cl; > return TRUE; > } > + myf utf8_flag= thd->get_utf8_flag(); Hmm, here you do use a session value. So, I suppose your using global value above was not an oversight, but you intentionally did it. What were your reasons? > > DBUG_ASSERT(cl_name.str[cl_name.length] == 0); > - *cl= get_charset_by_name(cl_name.str, MYF(0)); > + *cl= get_charset_by_name(cl_name.str, MYF(utf8_flag)); > > if (*cl == NULL) > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1051,14 +1052,16 @@ static inline void update_global_memory_status(int64 size) > @retval NULL on error > @retval Pointter to CHARSET_INFO with the given name on success > */ > -static inline CHARSET_INFO * > -mysqld_collation_get_by_name(const char *name, > +inline CHARSET_INFO * static inline, please > +mysqld_collation_get_by_name(const char *name, bool utf8_is_utf8mb3, up to you, but wouldn't it be more convenient to pass the flag here? Then you can invoke it with thd->utf8_flag() > CHARSET_INFO *name_cs= system_charset_info) > { > CHARSET_INFO *cs; > MY_CHARSET_LOADER loader; > + myf utf8_flag= utf8_is_utf8mb3 ? MY_UTF8_IS_UTF8MB3 : 0; > my_charset_loader_init_mysys(&loader); > - if (!(cs= my_collation_get_by_name(&loader, name, MYF(0)))) > + > + if (!(cs= my_collation_get_by_name(&loader, name, MYF(utf8_flag)))) > { > ErrConvString err(name, name_cs); > my_error(ER_UNKNOWN_COLLATION, MYF(0), err.ptr()); > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -10449,7 +10449,10 @@ merge_charset_and_collation(CHARSET_INFO *cs, CHARSET_INFO *cl) > CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs) > { > const char *csname= cs->csname; > - cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(0)); > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MY_UTF8_IS_UTF8MB3 : 0; Why not a session value here? > + cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(utf8_flag)); > if (!cs) > { > char tmp[65]; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2bceae199bb: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 05 May '21

05 May '21

Hi, Brandon! Just a couple of minor issues: On May 05, Brandon Nesterenko wrote: > revision-id: 2bceae199bb (mariadb-10.6.0-24-g2bceae199bb) > parent(s): 4ff4df3232f > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-05-05 01:01:01 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..606b629a4d5 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,37 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > +/** > + Utility function to implicitly change the connection protocol to a > + consistent value given the command line arguments. Additionally, > + warns the user that the protocol has been changed. > + > + Arguments: if you do `git show 2bceae199bb` you'll see git highlighting invisible spaces at line ends. Could you, please, remove them? (everywhere in your commit, not only in the comment above) > + @param [in] host Name of the host to connect to > + @param [in, out] opt_protocol Location of the protocol option > + variable to update > + @param [in] new_protocol New protocol to force > diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c > index fb3103a318d..4f8891817e3 100644 > --- a/client/mysqlcheck.c > +++ b/client/mysqlcheck.c > @@ -285,10 +287,14 @@ static void usage(void) > > static my_bool > get_one_option(const struct my_option *opt, > - const char *argument, > - const char *filename __attribute__((unused))) > + const char *argument, > + const char *filename) indentation went wrong here. (only here. in other files where you removed the __attribute__ it was fine) > { > int orig_what_to_do= what_to_do; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > DBUG_ENTER("get_one_option"); > > switch(opt->id) { > diff --git a/man/mysql.1 b/man/mysql.1 > index 03f23df3660..27a7e4d4d70 100644 > --- a/man/mysql.1 > +++ b/man/mysql.1 > @@ -1199,7 +1199,8 @@ Do not write line numbers for errors\&. Useful when you want to compare result f > \fB\-S \fR\fB\fIpath\fR\fR > .sp > For connections to > -localhost, the Unix socket file to use, or, on Windows, the name of the named pipe to use\&. > +localhost, the Unix socket file to use, or, on Windows, the name of the named pipe to use\&. > +Forces --protocol=socket when specified without other connection properties\&. here and everywhere, for -P and -S: I'd clarify that "... when specified on the command line ..." > .RE > .sp > .RS 4 > diff --git a/mysql-test/main/cli_options_force_protocol.result b/mysql-test/main/cli_options_force_protocol.result > new file mode 100644 > index 00000000000..c69a2b4f578 > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.result > @@ -0,0 +1,25 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +# > +# The following group of tests should produce no warnings > +# > +# exec MYSQL --host=localhost -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# exec MYSQL --port=MASTER_MYPORT --protocol=tcp -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: localhost via TCP/IP > +# exec MYSQL --host=localhost --port=MASTER_MYPORT --protocol=socket -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# exec MYSQL --host=127.0.0.1 --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: 127.0.0.1 via TCP/IP > +# exec MYSQL --host=localhost --socket=MASTER_MYSOCK --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# > +# The remaining tests should produce warnings > +# I now have some reservations about it, see below: > +# exec MYSQL --host=localhost --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +WARNING: Forcing protocol to TCP due to option specification. Please explicitly state intended protocol. > +Connection: localhost via TCP/IP old behavior was "localhost via UNIX socket", you've changed it to TCP/IP and issued a warning. Good so far. > +# exec MYSQL --host=localhost --socket=MASTER_MYSOCK -e "status" 2>&1 | grep "Connection:\|WARNING:" > +WARNING: Forcing protocol to SOCKET due to option specification. Please explicitly state intended protocol. > +Connection: Localhost via UNIX socket here the behavior isn't changed, but you still issue a warning. Is it justified? may be it'd be better only to issue a warning when the behavior changes? Regards, Sergei

1 0

Re: [Maria-developers] 2a5663ae524: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 04 May '21

04 May '21

Hi, Brandon! On May 04, Brandon Nesterenko wrote: > revision-id: 2a5663ae524 (mariadb-10.6.0-18-g2a5663ae524) > parent(s): 9fe681c9e4d > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-05-03 19:47:07 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, > mysqldump, etc) silently ignore connection > property options (e.g., --port and --socket) > when protocol is not explicitly set via the > command-line for localhost connections. > > Fix: > === > If connection properties are specified without a > protocol, override the protocol to be consistent. > For example, if --port is specified, automatically > set protocol=tcp. > > Caveats: > ======= > * When multiple connection properties are > specified, nothing is overridden > * If protocol is is set via the command-line, > its value is used > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..94e1fc8bb08 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,57 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > +/** > + Utility function to implicitly change the connection protocol to a > + consistent value given the command line arguments. Additionally, > + warns the user that the protocol has been changed. > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] opt_protocol Location of the protocol option > + variable to update > + @param [in] new_protocol New protocol to force > +*/ > +static inline void warn_protocol_override(FILE *warn_to, it's always stderr, isn't it? Why did you want it as an argument? > + char *host, > + uint *opt_protocol, > + uint new_protocol) > +{ > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + const char *TCP_NAME = "TCP"; > + const char *SOCKET_NAME = "SOCKET"; > + char *protocol_name; > + > + if(new_protocol == MYSQL_PROTOCOL_TCP) > + protocol_name = (char *) TCP_NAME; better not to hard-code protocol names here, you can do protocol_name= sql_protocol_typelib.type_names[new_protocol-1]; > + else if(new_protocol == MYSQL_PROTOCOL_SOCKET) > + protocol_name = (char *) SOCKET_NAME; > + else > + { > + /* > + This should never be entered, but just in case we are called incorrectly > + */ > + > + fprintf(warn_to, "%s %d %s %d\n", > + "WARNING: Protocol ID ", > + new_protocol, > + " cannot override connection type. " > + "Using the configuration value of ", > + *opt_protocol); > + return; for things that cannot happen code-wise (some code guarantees that it can never happen), better add an assert (DBUG_ASSERT, in fact) and there's no need to do an warning or anything. An assert is like a self-maintaining documentation. Its main purpose is to document that something cannot happen. But unlike your comment it also checks that it doesn't become outdated. so, this could become DBUG_ASSERT(new_protocol == MYSQL_PROTOCOL_SOCKET || new_protocol == MYSQL_PROTOCOL_TCP); protocol_name= sql_protocol_typelib.type_names[new_protocol-1]; > + } > + > + fprintf(warn_to, "%s %s %s\n", > + "WARNING: Forcing protocol to ", > + protocol_name, > + " due to option specification. " > + "Please explicitly state intended protocol."); > + > + *opt_protocol = new_protocol; > + } > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..5ca9b4393ec 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,8 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1164,9 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1176,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, current_host, &opt_protocol, protocol_to_force); > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1728,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_protocol_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1781,6 +1797,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > opt->name)) <= 0) > exit(1); > #endif > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > break; > case OPT_SERVER_ARG: > #ifdef EMBEDDED_LIBRARY > @@ -1872,6 +1896,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > #ifdef __WIN__ > opt_protocol = MYSQL_PROTOCOL_PIPE; > opt_protocol_type= "pipe"; > + > + /* Prioritize pipe if explicit via command line */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > #endif > break; > #include <sslopt-case.h> > @@ -1883,6 +1914,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) that's a strange condition. I believe it's practically identical to protocol_to_force == MYSQL_PROTOCOL_DEFAULT but a lot more confusing. Why not to write it as protocol_to_force == MYSQL_PROTOCOL_DEFAULT ? > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) same here > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > + break; > case 'I': > case '?': > usage(0); > diff --git a/client/mysqladmin.cc b/client/mysqladmin.cc > index e40e82f8038..08ec14815bd 100644 > --- a/client/mysqladmin.cc > +++ b/client/mysqladmin.cc > @@ -54,6 +54,8 @@ static bool sql_log_bin_off= false; > static uint opt_protocol=0; > static myf error_flags; /* flags to pass to my_printf_error, like ME_BELL */ > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > /* > When using extended-status relatively, ex_val_max_len is the estimated > maximum length for any relative value printed by extended-status. The > @@ -241,8 +243,12 @@ static const char *load_default_groups[]= > 0 }; > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > switch(opt->id) { > case 'c': > opt_count_iterations= 1; > @@ -274,6 +280,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > case 'W': > #ifdef __WIN__ > opt_protocol = MYSQL_PROTOCOL_PIPE; > + > + /* Prioritize pipe if explicit via command line */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > #endif > break; > case '#': > @@ -309,6 +322,48 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > sf_leaking_memory= 1; /* no memory leak reports here */ > exit(1); > } > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > break; > } > return 0; > @@ -323,6 +378,10 @@ int main(int argc,char *argv[]) > > MY_INIT(argv[0]); > sf_leaking_memory=1; /* don't report memory leaks on early exits */ > + > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > save_argv = argv; /* Save for free_defaults */ > > @@ -331,6 +390,13 @@ int main(int argc,char *argv[]) > temp_argv= mask_password(argc, &argv); > temp_argc= argc; > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); > + } > + > if (debug_info_flag) > my_end_arg= MY_CHECK_ERROR | MY_GIVE_INFO; > if (debug_check_flag) > diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc > index fd31ab6694e..1ff6048e5ad 100644 > --- a/client/mysqlbinlog.cc > +++ b/client/mysqlbinlog.cc > @@ -98,6 +98,8 @@ static const char *output_prefix= ""; > static char **defaults_argv= 0; > static MEM_ROOT glob_root; > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > #ifndef DBUG_OFF > static const char *default_dbug_option = "d:t:o,/tmp/mariadb-binlog.trace"; > const char *current_dbug_option= default_dbug_option; > @@ -1959,9 +1961,13 @@ static my_time_t convert_str_to_timestamp(const char* str) > > > extern "C" my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > bool tty_password=0; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > switch (opt->id) { > #ifndef DBUG_OFF > case '#': > @@ -2011,6 +2017,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > sf_leaking_memory= 1; /* no memory leak reports here */ > die(); > } > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > break; > #ifdef WHEN_FLASHBACK_REVIEW_READY > case opt_flashback_review: > @@ -2092,6 +2106,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > case OPT_PRINT_ROW_EVENT_POSITIONS: > print_row_event_positions_used= 1; > break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > + break; > case 'v': > if (argument == disabled_my_option) > verbose= 0; > @@ -3049,6 +3097,9 @@ int main(int argc, char** argv) > my_init_time(); // for time functions > tzset(); // set tzname > > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_groups, &argc, &argv); > defaults_argv= argv; > > @@ -3062,6 +3113,13 @@ int main(int argc, char** argv) > > parse_args(&argc, (char***)&argv); > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); > + } > + > if (!argc || opt_version) > { > if (!opt_version) > diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c > index fb3103a318d..54f8d2eb4f3 100644 > --- a/client/mysqlcheck.c > +++ b/client/mysqlcheck.c > @@ -56,6 +56,8 @@ static char *opt_skip_database; > DYNAMIC_ARRAY tables4repair, tables4rebuild, alter_table_cmds; > DYNAMIC_ARRAY views4repair; > static uint opt_protocol=0; > + > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > > enum operations { DO_CHECK=1, DO_REPAIR, DO_ANALYZE, DO_OPTIMIZE, DO_FIX_NAMES }; > const char *operation_name[]= > @@ -289,6 +291,10 @@ get_one_option(const struct my_option *opt, > const char *filename __attribute__((unused))) not __attribute__((unused)) anymore here and in other files too, I won't repeat this comment for every file > { > int orig_what_to_do= what_to_do; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > DBUG_ENTER("get_one_option"); > > switch(opt->id) { > diff --git a/mysql-test/main/cli_options_force_protocol.result b/mysql-test/main/cli_options_force_protocol.result > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.result > @@ -0,0 +1,27 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +# > +# The following tests until the first cat_file should produce no warnings > +# > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost -e "status" | grep "Connection:" oh-oh. You cannot hard-code your local path into the result file. Not the port number either. That's why in my previous review I wrote --echo # --host=localhost --port=MASTER_MYPORT and not --echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" (actually I had --port=$MASTER_MYPORT in my email, but that was wrong, should be no $-sign in --echo :) > +Connection: Localhost via UNIX socket > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --port=16000 --protocol=tcp -e "status" | grep "Connection:" > +Connection: localhost via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 --protocol=socket -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=127.0.0.1 --port=16000 -e "status" | grep "Connection:" > +Connection: 127.0.0.1 via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock --port=16000 -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.cli_options_force_protocol > +# > +# The remaining tests should produce warnings > +# > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 -e "status" | grep "Connection:" > +Connection: localhost via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.cli_options_force_protocol > +WARNING: Forcing protocol to TCP due to option specification. Please explicitly state intended protocol. > +WARNING: Forcing protocol to SOCKET due to option specification. Please explicitly state intended protocol. > diff --git a/mysql-test/main/cli_options_force_protocol.test b/mysql-test/main/cli_options_force_protocol.test > new file mode 100644 > index 00000000000..f91d7833a0d > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.test > @@ -0,0 +1,41 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > +--source include/not_windows.inc > + > +--echo # > +--echo # The following tests until the first cat_file should produce no warnings > +--echo # > + > +--echo # $MYSQL --host=localhost -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > + > +--echo # $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > + > +--echo # > +--echo # The remaining tests should produce warnings > +--echo # > + > +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" > + > +--cat_file $MYSQLTEST_VARDIR/log/current_test I still don't understand why you include current test name into the result file. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 242cc3fc41f: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 03 May '21

03 May '21

Hi, Brandon! On May 03, Brandon Nesterenko wrote: > revision-id: 242cc3fc41f (mariadb-10.6.0-17-g242cc3fc41f) > parent(s): fd8c68c7fe6 > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-04-30 23:17:37 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore the --port option if no host is given or it is localhost. please, try to avoid very long lines in commit comments, they're difficult to read in git console tools. > > Fix: > === > During configuration processing, force protocol to TCP if port was specified via the command line. However, if protocol is additionally specified via the command line, it will be prioritized. > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..6cb1f02ba33 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,85 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > + > +/** > + Changes flags to prepare for (and revert to the previous state) > + from handle_options. > + > + When creating the state, the original state is saved and can be later > + resumed by calling this function and setting do_revert_values to TRUE. > + > + Flags: > + my_defaults_mark_files: propagates the source of a set option > + > + @param [in] do_revert_values behavior should revert when TRUE, prepare > + when FALSE > + */ > +static inline void set_flags_for_option_handling(my_bool do_revert_values) > +{ > + static my_bool prev_mark_files; > + > + if (do_revert_values) > + { > + my_defaults_mark_files = prev_mark_files; > + } > + else > + { > + prev_mark_files = my_defaults_mark_files; > + my_defaults_mark_files = TRUE; > + } > +} this isn't very helpful. my_defaults_mark_files is part of the my_getopt API. It's supposed to be used directly by whatever tool uses my_getopt. Here you've turned one line my_defaults_mark_files= TRUE; into 27 lines, so when I (or somebody reading the code) will see set_flags_for_option_handling(...); he'd have to go to the function definition, read the comment and the function, all 27 lines, understand what they do, and then jump back. I'd rather prefer to read one line with a very clear semantics and no conditionals instead. > + > +/** > + Helper function to prepare state for handle_options (current state is saved) > + */ > +static inline void prepare_option_handling_flags() > +{ > + set_flags_for_option_handling(FALSE); > +} > + > +/** > + Helper function to revert state after handle_options to that from before > + prepare_option_handling_flags was called > + */ > +static inline void revert_option_handling_flags() > +{ > + set_flags_for_option_handling(TRUE); > +} and it keeps going on. if you remove the first function, you won't need these two functions either. Saving old value, restoring it. my_defaults_mark_files affects only my_load_defaults(). It makes no sense to restore it. > +/** > + > + Utility function to force the connection protocol to TCP when > + just the port is specified via the command line. Additionally, > + warns the user that the protocol has been changed (MDEV-14974). > + > + Notes: > + 1) This only takes effect when connecting to localhost > + 2) Windows uses TCP by default > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] protocol_loc Location of the protocol option > + variable to update > +*/ > +static inline void warn_tcp_protocol_override(FILE *warn_to, > + char *host, > + uint *protocol_loc) > +{ > +#ifndef _WIN32 > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + fprintf(warn_to, "WARNING: " > + "Forcing protocol to TCP due to port specification. " > + "Please explicitly state TCP protocol or remove " > + "port if unintended.\n"); > + *protocol_loc = MYSQL_PROTOCOL_TCP; > + } > +#endif > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..980daa14365 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,10 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +#ifndef _WIN32 > +static my_bool port_forcing_tcp_proto = FALSE; > +#endif > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1166,8 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + prepare_option_handling_flags(); Just do my_defaults_mark_files= TRUE; here > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1177,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + revert_option_handling_flags(); and don't revert it. > + if (port_forcing_tcp_proto) > + { > + warn_tcp_protocol_override(stderr, current_host, &opt_protocol); I'm not sure it needs a warning. But I don't have strong arguments against the warning either, so let's keep a warning, if you like it that way. Why everything is not on _WIN32 ? It has named pipes and tcp. Wouldn't it have the same issue with --port? > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1729,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_port_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1780,6 +1797,19 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > else if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib, > opt->name)) <= 0) > exit(1); > +#ifndef _WIN32 > + /* MDEV-14974 > + * > + * Where specifying port implicitly will set the protocol to use TCP, if the > + * protocol is explicitly set after the port, prioritize protocol. > + */ please, see how multi-comments are usually formatted, and follow the same style. Generally there's no need to specify the MDEV, and there's no vertical line of asterisks. > + if(filename[0] == '\0') > + { > + /* Protocol is specified via command line */ > + ignore_port_override = TRUE; > + port_forcing_tcp_proto = FALSE; > + } > +#endif > #endif > break; > case OPT_SERVER_ARG: > @@ -1883,6 +1913,20 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > +#ifndef _WIN32 > + /* MDEV-14974: > + * > + * If port is set via CLI, change protocol to TCP unless protocol has already > + * been specified via CLI or TCP is already used > + */ > + if (filename[0] == '\0' && !ignore_port_override > + && opt_protocol != MYSQL_PROTOCOL_TCP) > + { > + port_forcing_tcp_proto = TRUE; > + } let's also do the same treatment for --socket. If it's specified on the command line - force the socket protocol. What could be the least surprising behavior? I'd think the most intuitive would be, like, WYSIWIG thing - * if port is specified on the command line (but no socket or protocol) - it forces protocol=TCP * if socket is specified on the command line (but no port of protocol) - it forces protocol=SOCKET * if protocol is specified explicitly anywhere on the command line - port and socket lose their magic behavior * if there's no protocol, but both socket and port are specified? - I don't know, perhaps, let's keep the old behavior, no magic? hmm. How does --host affect that? > +#endif > + break; > case 'I': > case '?': > usage(0); > @@ -1916,8 +1960,10 @@ static int get_options(int argc, char **argv) > opt_max_allowed_packet= *mysql_params->p_max_allowed_packet; > opt_net_buffer_length= *mysql_params->p_net_buffer_length; > > + my_defaults_mark_files = TRUE; > if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option))) > return(ho_error); > + my_defaults_mark_files = FALSE; my_defaults_mark_files has no effect on handle_options() > *mysql_params->p_max_allowed_packet= opt_max_allowed_packet; > *mysql_params->p_net_buffer_length= opt_net_buffer_length; > > diff --git a/mysql-test/main/port_force_tcp.result b/mysql-test/main/port_force_tcp.result > new file mode 100644 > index 00000000000..ae976bda6cc > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.result > @@ -0,0 +1,14 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: 127.0.0.1 via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +WARNING: Forcing protocol to TCP due to port specification. Please explicitly state TCP protocol or remove port if unintended. > diff --git a/mysql-test/main/port_force_tcp.test b/mysql-test/main/port_force_tcp.test > new file mode 100644 > index 00000000000..607670d16b0 > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.test > @@ -0,0 +1,20 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > + > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test this will likely fail on windows, you need to source not_windows.inc too. why do you cat current_test after every --exec? better add --echo before every exec, like --echo # --host=localhost --port=$MASTER_MYPORT --exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 242cc3fc41f: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 03 May '21

03 May '21

Hi, Brandon! On May 03, Brandon Nesterenko wrote: > revision-id: 242cc3fc41f (mariadb-10.6.0-17-g242cc3fc41f) > parent(s): fd8c68c7fe6 > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-04-30 23:17:37 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore the --port option if no host is given or it is localhost. please, try to avoid very long lines in commit comments, they're difficult to read in git console tools. > > Fix: > === > During configuration processing, force protocol to TCP if port was specified via the command line. However, if protocol is additionally specified via the command line, it will be prioritized. > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..6cb1f02ba33 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,85 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > + > +/** > + Changes flags to prepare for (and revert to the previous state) > + from handle_options. > + > + When creating the state, the original state is saved and can be later > + resumed by calling this function and setting do_revert_values to TRUE. > + > + Flags: > + my_defaults_mark_files: propagates the source of a set option > + > + @param [in] do_revert_values behavior should revert when TRUE, prepare > + when FALSE > + */ > +static inline void set_flags_for_option_handling(my_bool do_revert_values) > +{ > + static my_bool prev_mark_files; > + > + if (do_revert_values) > + { > + my_defaults_mark_files = prev_mark_files; > + } > + else > + { > + prev_mark_files = my_defaults_mark_files; > + my_defaults_mark_files = TRUE; > + } > +} this isn't very helpful. my_defaults_mark_files is part of the my_getopt API. It's supposed to be used directly by whatever tool uses my_getopt. Here you've turned one line my_defaults_mark_files= TRUE; into 27 lines, so when I (or somebody reading the code) will see set_flags_for_option_handling(...); he'd have to go to the function definition, read the comment and the function, all 27 lines, understand what they do, and then jump back. I'd rather prefer to read one line with a very clear semantics and no conditionals instead. > + > +/** > + Helper function to prepare state for handle_options (current state is saved) > + */ > +static inline void prepare_option_handling_flags() > +{ > + set_flags_for_option_handling(FALSE); > +} > + > +/** > + Helper function to revert state after handle_options to that from before > + prepare_option_handling_flags was called > + */ > +static inline void revert_option_handling_flags() > +{ > + set_flags_for_option_handling(TRUE); > +} and it keeps going on. if you remove the first function, you won't need these two functions either. Saving old value, restoring it. my_defaults_mark_files affects only my_load_defaults(). It makes no sense to restore it. > +/** > + > + Utility function to force the connection protocol to TCP when > + just the port is specified via the command line. Additionally, > + warns the user that the protocol has been changed (MDEV-14974). > + > + Notes: > + 1) This only takes effect when connecting to localhost > + 2) Windows uses TCP by default > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] protocol_loc Location of the protocol option > + variable to update > +*/ > +static inline void warn_tcp_protocol_override(FILE *warn_to, > + char *host, > + uint *protocol_loc) > +{ > +#ifndef _WIN32 > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + fprintf(warn_to, "WARNING: " > + "Forcing protocol to TCP due to port specification. " > + "Please explicitly state TCP protocol or remove " > + "port if unintended.\n"); > + *protocol_loc = MYSQL_PROTOCOL_TCP; > + } > +#endif > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..980daa14365 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,10 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +#ifndef _WIN32 > +static my_bool port_forcing_tcp_proto = FALSE; > +#endif > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1166,8 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + prepare_option_handling_flags(); Just do my_defaults_mark_files= TRUE; here > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1177,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + revert_option_handling_flags(); and don't revert it. > + if (port_forcing_tcp_proto) > + { > + warn_tcp_protocol_override(stderr, current_host, &opt_protocol); I'm not sure it needs a warning. But I don't have strong arguments against the warning either, so let's keep a warning, if you like it that way. Why everything is not on _WIN32 ? It has named pipes and tcp. Wouldn't it have the same issue with --port? > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1729,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_port_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1780,6 +1797,19 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > else if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib, > opt->name)) <= 0) > exit(1); > +#ifndef _WIN32 > + /* MDEV-14974 > + * > + * Where specifying port implicitly will set the protocol to use TCP, if the > + * protocol is explicitly set after the port, prioritize protocol. > + */ please, see how multi-comments are usually formatted, and follow the same style. Generally there's no need to specify the MDEV, and there's no vertical line of asterisks. > + if(filename[0] == '\0') > + { > + /* Protocol is specified via command line */ > + ignore_port_override = TRUE; > + port_forcing_tcp_proto = FALSE; > + } > +#endif > #endif > break; > case OPT_SERVER_ARG: > @@ -1883,6 +1913,20 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > +#ifndef _WIN32 > + /* MDEV-14974: > + * > + * If port is set via CLI, change protocol to TCP unless protocol has already > + * been specified via CLI or TCP is already used > + */ > + if (filename[0] == '\0' && !ignore_port_override > + && opt_protocol != MYSQL_PROTOCOL_TCP) > + { > + port_forcing_tcp_proto = TRUE; > + } let's also do the same treatment for --socket. If it's specified on the command line - force the socket protocol. What could be the least surprising behavior? I'd think the most intuitive would be, like, WYSIWIG thing - * if port is specified on the command line (but no socket or protocol) - it forces protocol=TCP * if socket is specified on the command line (but no port of protocol) - it forces protocol=SOCKET * if protocol is specified explicitly anywhere on the command line - port and socket lose their magic behavior * if there's no protocol, but both socket and port are specified? - I don't know, perhaps, let's keep the old behavior, no magic? hmm. How does --host affect that? > +#endif > + break; > case 'I': > case '?': > usage(0); > @@ -1916,8 +1960,10 @@ static int get_options(int argc, char **argv) > opt_max_allowed_packet= *mysql_params->p_max_allowed_packet; > opt_net_buffer_length= *mysql_params->p_net_buffer_length; > > + my_defaults_mark_files = TRUE; > if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option))) > return(ho_error); > + my_defaults_mark_files = FALSE; my_defaults_mark_files has no effect on handle_options() > *mysql_params->p_max_allowed_packet= opt_max_allowed_packet; > *mysql_params->p_net_buffer_length= opt_net_buffer_length; > > diff --git a/mysql-test/main/port_force_tcp.result b/mysql-test/main/port_force_tcp.result > new file mode 100644 > index 00000000000..ae976bda6cc > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.result > @@ -0,0 +1,14 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: 127.0.0.1 via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +WARNING: Forcing protocol to TCP due to port specification. Please explicitly state TCP protocol or remove port if unintended. > diff --git a/mysql-test/main/port_force_tcp.test b/mysql-test/main/port_force_tcp.test > new file mode 100644 > index 00000000000..607670d16b0 > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.test > @@ -0,0 +1,20 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > + > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test this will likely fail on windows, you need to source not_windows.inc too. why do you cat current_test after every --exec? better add --echo before every exec, like --echo # --host=localhost --port=$MASTER_MYPORT --exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a7032fbb32d: Bug#29363867: LOST CONNECTION TO MYSQL SERVER DURING QUERY
by Sergei Golubchik 27 Apr '21

27 Apr '21

Hi, Sanja! On Apr 27, Oleksandr Byelkin wrote: > revision-id: a7032fbb32d (mariadb-10.2.31-906-ga7032fbb32d) > parent(s): b862377c3e9 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2021-04-27 16:24:43 +0200 > message: > > Bug#29363867: LOST CONNECTION TO MYSQL SERVER DURING QUERY > > The problem is that sharing default expression among set instruction > leads to attempt access result field of function created in > other instruction runtime MEM_ROOT and already freed > (a bit different then MySQL problem). > > Fix is the same as in MySQL (but no optimisation for constant), turn > DECLARE a, b, c type DEFAULT expr; > to > DECLARE a type DEFAULT expr, b type DEFAULT a, c type DEFAULT a; > > diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test > index f13b3fbc281..007dfd1a10b 100644 > --- a/mysql-test/t/sp.test > +++ b/mysql-test/t/sp.test > @@ -10025,4 +10025,22 @@ DROP PROCEDURE p1; > DROP VIEW v1; > DROP TABLE t1; > > + > +--echo # > +--echo # forgot the bug summary (the first line of the commit comment) > +--echo # > + > +delimiter |; > +create function f1() returns bigint return now()-1| > +create procedure p1() > +begin > + declare b, c bigint default f1(); > + select b-c; > +end| > +call p1()| > +drop procedure p1| > +drop function f1| > +delimiter ;| > + > + > --echo #End of 10.2 tests > diff --git a/sql/item.cc b/sql/item.cc > index 42272fe0148..ec9f4ffb993 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -2933,7 +2933,7 @@ bool Item_field::eq(const Item *item, bool binary_cmp) const > > table_map Item_field::used_tables() const > { > - if (field->table->const_table) > + if (!field || !field->table || field->table->const_table) in what cases can field or field->table be NULL here? > return 0; // const item > return (get_depended_from() ? OUTER_REF_TABLE_BIT : field->table->map); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e92037989f7: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 25 Apr '21

25 Apr '21

Hi, Andrei! Don't be confused by the subject, this is a review of git diff 450c017c2d 2fa526e26e that is of everything, combined. Not just one e92037989f7 commit. On Apr 25, Sujatha wrote: > revision-id: e92037989f7 (mariadb-10.3.26-128-ge92037989f7) > parent(s): 450c017c2d9 > author: Sujatha <sujatha.sivakumar(a)mariadb.com> > committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > timestamp: 2021-04-13 12:26:12 +0300 > message: > > MDEV-21117: refine the server binlog-based recovery for semisync > diff --git a/libmariadb b/libmariadb > index fc431a035a2..e3824422064 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit fc431a035a21ac1d4ef25d9d3cd8c4d7e64a8ee7 > +Subproject commit e38244220646a7e95c9be22576460aa7a4eb715f This is clearly a mistake, you erroneously checked in old libmaridb commit, rolling back a bunch of changes. See your commit 4bc83b2749 > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_engine-master.opt b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine-master.opt > new file mode 100644 > index 00000000000..df675545bf9 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine-master.opt please rename this file to mysql-test/include/have_rocksdb.opt > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.test b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > new file mode 100644 > index 00000000000..2b794d02dd0 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > @@ -0,0 +1,74 @@ > +# ==== Purpose ==== > +# > +# Test verifies the truncation of single binary log file. > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > +--source include/have_innodb.inc > +--source include/have_aria.inc > +# File: binlog_truncate_active_log.inc included in test makes use of > +# 'debug_sync' facility. > +--source include/have_debug_sync.inc you wouldn't need a comment if you'd include have_debug_sync.inc directly into binlog_truncate_active_log.inc. but ok, whatever you like > +--source include/have_binlog_format_statement.inc > + > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > + > +# The following cases are tested: > +# A. 2pc transaction is followed by a blank "zero-engines" one > +# B. 2pc transaction follows the blank one > +# C. Similarly to A, with the XA blank transaction > + > +RESET MASTER; > +CREATE TABLE t (f INT) ENGINE=INNODB; > +CREATE TABLE t2 (f INT) ENGINE=INNODB; > +CREATE TABLE tm (f INT) ENGINE=Aria; could you add a comment, explaining why you're using Aria here. (you wrote that in an email, but please add a comment too) > + > +--echo # Case A. > +# Using 'debug_sync' hold 'query1' execution after 'query1' is flushed and > +# synced to binary log but not yet committed. In an another connection hold > +# 'query2' execution after 'query2' is flushed and synced to binlog. > +# Crash and restart server with --rpl-semi-sync-slave-enabled=1 > +# > +# During recovery of binary log 'query1' status is checked with InnoDB engine, > +# it will be in prepared but not yet commited. All transactions starting from > +# 'query1' onwards will be removed from the binary log. > + > +--let $truncate_gtid_pos = 0-1-6 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $query2 = DELETE FROM t2 WHERE f = 0 /* no such record */ > +--source binlog_truncate_active_log.inc > + > +--echo # Case B. > +# The inverted sequence ends up to truncate only $query2 > +--let $truncate_gtid_pos = 0-1-10 > +--let $query1 = DELETE FROM t2 WHERE f = 0 > +--let $query2 = INSERT INTO t VALUES (20) > +--source binlog_truncate_active_log.inc > + > + > +delimiter |; > +CREATE PROCEDURE sp_blank_xa() > +BEGIN > + XA START 'blank'; > + DELETE FROM t2 WHERE f = 0 /* no such record */; > + XA END 'blank'; > + XA PREPARE 'blank'; > +END| > +delimiter ;| > + > + > +--echo # Case C. > +--let $truncate_gtid_pos = 0-1-14 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $pre_q2 = CALL sp_blank_xa > +--let $query2 = XA COMMIT 'blank' > +--source binlog_truncate_active_log.inc what was truncated here? a comment explains it for cases A and B, but not here. may be it'd make sense to do show binlog events after every restart, just to see the state of the binlog after truncation? > +DROP PROCEDURE sp_blank_xa; > + > +--echo # Cleanup > +DROP TABLE t,t2,tm; > + > +--echo # End of the tests > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > new file mode 100644 > index 00000000000..bbc464066fc > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > @@ -0,0 +1,55 @@ > +connect(master1,localhost,root,,); > +connect(master2,localhost,root,,); > +connect(master3,localhost,root,,); > + > +--connection default > + > +# First to commit few transactions > +INSERT INTO t VALUES (10); > +INSERT INTO tm VALUES (10); > + > +--connection master1 > +# Hold insert after write to binlog and before "run_commit_ordered" in engine > +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master1_ready WAIT_FOR signal_never_arrives"; > +--send_eval $query1 > + > +--connection master2 > +SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > +if ($pre_q2) > +{ > + eval $pre_q2; > +} > +SET DEBUG_SYNC= "commit_before_get_LOCK_after_binlog_sync SIGNAL master2_ready"; > +# To binlog non-xid transactional group which will be truncated all right > +--send_eval $query2 > + > +--connection master3 > +SET DEBUG_SYNC= "now WAIT_FOR master2_ready"; > +SELECT @@global.gtid_binlog_pos as 'Before the crash'; > + > +--connection default > +--source include/kill_mysqld.inc > +--disconnect master1 > +--disconnect master2 > +--disconnect master3 > + > +# > +# Server restart > +# > +--let $restart_parameters= --rpl-semi-sync-slave-enabled=1 > +--source include/start_mysqld.inc > + > +# Check error log for a successful truncate message. > +--let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err > + > +--let SEARCH_FILE=$log_error_ why not to set SEARCH_FILE directly? but ok, as you like > +--let SEARCH_PATTERN=Successfully truncated.*to remove transactions starting from GTID $truncate_gtid_pos > +--replace_regex /FOUND [0-9]+/FOUND #/ can it be found multiple times? Why would binlog be truncated more than once? > +--source include/search_pattern_in_file.inc > + > +SELECT @@global.gtid_binlog_pos as 'After the crash'; > +--echo "One row should be present in table 't'" > +SELECT * FROM t; > + > +# Local cleanup > +DELETE FROM t; > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test > new file mode 100644 > index 00000000000..94837e3c3ea > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test > @@ -0,0 +1,56 @@ > +# ==== Purpose ==== > +# > +# Test verifies truncation of multiple binary logs with multiple transactional > +# storage engines > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > +--source include/have_innodb.inc > +--source include/have_rocksdb.inc > +--source include/have_debug.inc > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_row.inc > + > +--let $old_max_binlog_size= `select @@global.max_binlog_size` > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > +--let $MYSQLD_DATADIR= `SELECT @@datadir` > + > +CREATE TABLE t1 (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=Innodb; > +CREATE TABLE t2 (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=rocksdb; > + > +--let $case = A: neither engine committed => rollback & binlog truncate > +# Hold off engine commits after write to binlog and its rotation. > +# The transaction is killed along with the server after that. > +--let $shutdown_timeout=0 > +--let $debug_sync_action = "commit_after_release_LOCK_log SIGNAL con1_ready WAIT_FOR signal_no_signal" > +--let $restart_parameters = --rpl-semi-sync-slave-enabled=1 > +--let $test_outcome= 1 row should be present in both tables; binlog is truncated; number of binlogs at reconnect - 3 > +--source binlog_truncate_multi_engine.inc > +--echo Proof of the truncated binlog file is readable (two transactions must be seen): > +--exec $MYSQL_BINLOG --short-form --skip-annotate-row-events $MYSQLD_DATADIR/master-bin.000002 > + > +--let $case = B: one engine has committed its transaction branch > +# Hold off after one engine has committed. > +--let $shutdown_timeout=0 > +--let $debug_sync_action = "commit_after_run_commit_ordered SIGNAL con1_ready WAIT_FOR signal_no_signal" > +# Both debug_sync and debug-dbug are required to make sure Engines remember the commit state > +# debug_sync alone will not help. > +--let $restart_parameters = --rpl-semi-sync-slave-enabled=1 --debug-dbug=d,binlog_truncate_partial_commit in the first review I wrote this seems to be a rather crude way of faking a partially committed transaction. better to crash after the first engine has committed, that'd be much more natural. and you replied This simulation aimed at (allows for) more complicated recovery time event sequences. In this case, indeed, crashing by demand is about of the same efforts. I can convert to that. [x] > +--let $test_outcome= 2 rows should be present in both tables; no binlog truncation; one extra binlog file compare with A; number of binlogs at reconnect - 4 > +--source binlog_truncate_multi_engine.inc > + > +--let $case = C: both engines have committed its transaction branch > +# Hold off after both engines have committed. The server is shut down. > +--let $shutdown_timeout= > +--let $restart_parameters = --rpl-semi-sync-slave-enabled=1 > +--let $test_outcome= 2 rows should be present in both tables; no binlog truncation; the same # of binlog files as in B; number of binlogs at reconnect - 4 > +--source binlog_truncate_multi_engine.inc > + > + > + > +DROP TABLE t1, t2; > + > +--echo # End of the tests > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.inc b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.inc > new file mode 100644 > index 00000000000..41ae856dd9d > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.inc > @@ -0,0 +1,54 @@ > +# > +# Loop body of binlog_truncate_multi_engine.test what do you mean "loop body"? > +# Parameters: > +# $debug_sync_action describes debug-sync actions > +# $kill_server 1 when to crash, 0 for regular restart > +# $restart_parameters the caller may simulate partial commit at recovery > +# $test_outcome summary of extected results > +# $MYSQLD_DATADIR > + > +--echo # > +--echo # > +--echo # Case $case > +--echo # > +RESET MASTER; > +FLUSH LOGS; > +SET GLOBAL max_binlog_size= 4096; > + > +connect(con1,localhost,root,,); > +--echo List of binary logs before rotation > +--source include/show_binary_logs.inc > +INSERT INTO t1 VALUES (1, REPEAT("x", 1)); > +INSERT INTO t2 VALUES (1, REPEAT("x", 1)); I'm not sure I understand the point of REPEAT(..., 1) but sure, if you like it that way... :) > +BEGIN; > + INSERT INTO t1 VALUES (2, REPEAT("x", 4100)); > + INSERT INTO t2 VALUES (2, REPEAT("x", 4100)); > + > +--eval SET DEBUG_SYNC= $debug_sync_action > +send COMMIT; > + > +--connection default > +SET DEBUG_SYNC= "now WAIT_FOR con1_ready"; > +--echo List of binary logs after rotation > +--source include/show_binary_logs.inc > + > +--echo # restart the server with $restart_parameters > +--echo # the server is restarted > +--source include/restart_mysqld.inc > + > +--connection default > +--echo # > +--echo # *** Summary: $test_outcome: > +--echo # > +SELECT COUNT(*) FROM t1; > +SELECT COUNT(*) FROM t2; > +SELECT @@GLOBAL.gtid_binlog_state; > +SELECT @@GLOBAL.gtid_binlog_pos; > +--echo List of binary logs at the end of the tests > +--source include/show_binary_logs.inc > +--echo # *** > +# cleanup > +DELETE FROM t1; > +DELETE FROM t2; > +--disconnect con1 > +--echo # > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_log.test b/mysql-test/suite/binlog/t/binlog_truncate_multi_log.test > new file mode 100644 > index 00000000000..3b557bc89b8 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_log.test > @@ -0,0 +1,78 @@ > +# ==== Purpose ==== > +# > +# Test verifies truncation of multiple binary logs. > +# > +# ==== References ==== > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > +--source include/have_innodb.inc > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_row.inc > + > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > + > +SET @@global.max_binlog_size= 4096; > + > +RESET MASTER; > +FLUSH LOGS; > +CREATE TABLE ti (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=Innodb; > +CREATE TABLE tm (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=MyISAM; > + > +connect(master1,localhost,root,,); > +--echo "List of binary logs before rotation" > +--source include/show_binary_logs.inc > + > +# Some load to either non- and transactional egines > +# that should not affect the following recovery: > +INSERT INTO ti VALUES(1,"I am gonna survive"); > +INSERT INTO tm VALUES(1,"me too!"); > + > +# hold on near engine commit > +SET DEBUG_SYNC= "commit_after_release_LOCK_after_binlog_sync SIGNAL master1_ready WAIT_FOR con1_go"; > +--send_eval INSERT INTO ti VALUES (2, REPEAT("x", 4100)) > + > +connect(master2,localhost,root,,); > +# The 2nd trx for recovery, it does not rotate binlog > +SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master2_ready WAIT_FOR master2_go"; > +--send_eval INSERT INTO ti VALUES (3, "not gonna survive") send_eval? what are you evaluating here? > + > +--connection default > +SET DEBUG_SYNC= "now WAIT_FOR master2_ready"; > +--echo "List of binary logs before crash" > +--source include/show_binary_logs.inc > +--echo # The gtid binlog state prior the crash will be truncated at the end of the test > +SELECT @@global.gtid_binlog_state; > + > +--connection default > +--source include/kill_mysqld.inc > +--disconnect master1 > +--disconnect master2 > + > +# > +# Server restart > +# > +--let $restart_parameters= --rpl-semi-sync-slave-enabled=1 > +--source include/start_mysqld.inc > + > +# Check error log for a successful truncate message. > +let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err; > + > +--let SEARCH_FILE=$log_error_ > +--let SEARCH_PATTERN=truncated binlog file:.*master.*000002 > +--replace_regex /FOUND [0-9]+/FOUND #/ can it be found multiple times? Why would binlog be truncated more than once? > +--source include/search_pattern_in_file.inc > + > + > +--echo "One record should be present in table" > +SELECT * FROM ti; > + > +--echo # The truncated gtid binlog state > +SELECT @@global.gtid_binlog_state; > +SELECT @@global.gtid_binlog_pos; > + > +--echo # Cleanup > +DROP TABLE ti; > + > +--echo # End of the tests > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_log_unsafe.test b/mysql-test/suite/binlog/t/binlog_truncate_multi_log_unsafe.test > new file mode 100644 > index 00000000000..38a9c0832f4 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_log_unsafe.test > @@ -0,0 +1,120 @@ > +# ==== Purpose ==== > +# The test verifies attempt to recover by the semisync slave server whose > +# binlog is unsafe for truncation. > +# > +# ==== Implementation ==== > +# 2 binlog files are created with the 1st one destined to be the binlog > +# checkpoint file for recovery. > +# The final group of events is replication unsafe (myisam INSERT). > +# Therefore the semisync slave recovery may not. > +# > +# Steps: > +# 0 - Set max_binlog_size= 4096, to help an insert into a > +# transaction table 'ti' get binlog rotated while the > +# transaction won't be committed, being stopped at > +# a prior to commit debug_sync point > +# 1 - insert into a non-transactional 'tm' table completes with > +# binary logging as well > +# 2 - kill and attempt to restart the server as semisync slave that > +# must produce an expected unsafe-to-recover error > +# 3 - complete the test with a normal restart that successfully finds and > +# commits the transaction in doubt. > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > +# > + > +--source include/have_innodb.inc > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_row.inc > + > +SET @@global.max_binlog_size= 4096; > + > +call mtr.add_suppression("Table '.*tm' is marked as crashed and should be repaired"); > +call mtr.add_suppression("Got an error from unknown thread"); > +call mtr.add_suppression("Checking table: '.*tm'"); > +call mtr.add_suppression("Recovering table: '.*tm'"); > +call mtr.add_suppression("Cannot trim the binary log to file"); > +call mtr.add_suppression("Crash recovery failed"); > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > +call mtr.add_suppression("Found 1 prepared transactions"); > +call mtr.add_suppression("mysqld: Table.*tm.*is marked as crashed"); > +call mtr.add_suppression("Checking table.*tm"); > + > +RESET MASTER; > +FLUSH LOGS; > +CREATE TABLE ti (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=Innodb; > +CREATE TABLE tm (f INT) ENGINE=MYISAM; > + > +--let $row_count = 5 > +--let $i = `select $row_count-2` > +--disable_query_log > +while ($i) > +{ > + --eval INSERT INTO ti VALUES ($i, REPEAT("x", 1)) > + --dec $i > +} > +--enable_query_log > +INSERT INTO tm VALUES(1); > + > +connect(master1,localhost,root,,); > +connect(master2,localhost,root,,); > +connect(master3,localhost,root,,); > + > +--connection master1 > + > +# The 1st trx binlogs, rotate binlog and hold on before committing at engine > +SET DEBUG_SYNC= "commit_after_release_LOCK_after_binlog_sync SIGNAL master1_ready WAIT_FOR master1_go"; > +--send_eval INSERT INTO ti VALUES ($row_count - 1, REPEAT("x", 4100)) > + > +--connection master2 > + > +# The 2nd trx for recovery, it does not rotate binlog > +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master2_ready WAIT_FOR master2_go"; > +--send_eval INSERT INTO ti VALUES ($row_count, REPEAT("x", 1)) > + > +--connection master3 > +SET DEBUG_SYNC= "now WAIT_FOR master2_ready"; > +SET DEBUG_SYNC= "commit_before_get_LOCK_after_binlog_sync SIGNAL master3_ready"; > +--send INSERT INTO tm VALUES (2) > + > +--connection default > +SET DEBUG_SYNC= "now WAIT_FOR master3_ready"; > +--echo # The gtid binlog state prior the crash must be restored at the end of the test; > +SELECT @@global.gtid_binlog_state; > +--source include/kill_mysqld.inc > + > +# > +# Server restarts > +# > +--echo # Failed restart as the semisync slave > +--error 1 > +--exec $MYSQLD_LAST_CMD --rpl-semi-sync-slave-enabled=1 >> $MYSQLTEST_VARDIR/log/mysqld.1.err 2>&1 > + > +--echo # Normal restart > +--source include/start_mysqld.inc > + > +# Check error log for correct messages. > +let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err; > + > +--let SEARCH_FILE=$log_error_ > +--let SEARCH_PATTERN=Cannot trim the binary log to file > +--replace_regex /FOUND [0-9]+/FOUND #/ can it be found multiple times? Why would binlog be truncated more than once? > +--source include/search_pattern_in_file.inc > + > +--echo # Proof that the in-doubt transactions are recovered by the 2nd normal server restart > +--eval SELECT COUNT(*) = $row_count as 'True' FROM ti > +# myisam table may require repair (which is not tested here) > +--disable_warnings > +SELECT COUNT(*) <= 1 FROM tm; > +--enable_warnings > + > +--echo # The gtid binlog state prior the crash is restored now > +SELECT @@GLOBAL.gtid_binlog_state; > +SELECT @@GLOBAL.gtid_binlog_pos; > + > +--echo # Cleanup > +DROP TABLE ti, tm; > +--echo End of test > diff --git a/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.cnf b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.cnf > new file mode 100644 > index 00000000000..f8312bdc5b8 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.cnf > @@ -0,0 +1,11 @@ > +!include suite/rpl/rpl_1slave_base.cnf > +!include include/default_client.cnf > + > + > +[mysqld.1] > +log-slave-updates > +gtid-strict-mode=1 > + > +[mysqld.2] > +log-slave-updates > +gtid-strict-mode=1 generally opt files (rpl_semi_sync_fail_over.opt in this case) are preferred, because mtr will know what options to apply, while cnf files are more opaque > diff --git a/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test > new file mode 100644 > index 00000000000..a8b40d6ed05 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test > @@ -0,0 +1,143 @@ > +# ==== Purpose ==== > +# > +# Test verifies replication failover scenario. > +# > +# ==== Implementation ==== > +# > +# Steps: > +# 0 - Having two servers 1 and 2 enable semi-sync replication with > +# with the master wait 'after_sync'. > +# 1 - Insert a row. While inserting second row simulate > +# a server crash at once the transaction is written to binlog, flushed > +# and synced but the binlog position is not updated. > +# 2 - Post crash-recovery on the old master execute there CHANGE MASTER > +# TO command to connect to server id 2. > +# 3 - The old master new slave server 1 must connect to the new > +# master server 2. > +# 4 - repeat the above to crash the new master and restore in role the old one > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > + > +--source include/have_innodb.inc > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_row.inc > +--let $rpl_topology=1->2 > +--source include/rpl_init.inc why not to source master-slave.inc if you're using a standard master-slave topology anyway? > + > +--connection server_2 > +--source include/stop_slave.inc > + > +--connection server_1 > +RESET MASTER; > +SET @@global.max_binlog_size= 4096; > + > +--connection server_2 > +RESET MASTER; > +SET @@global.max_binlog_size= 4096; > +set @@global.rpl_semi_sync_slave_enabled = 1; > +set @@global.gtid_slave_pos = ""; > +CHANGE MASTER TO master_use_gtid= slave_pos; > +--source include/start_slave.inc > + > + > +--connection server_1 > +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; here I asked "why?" and you replied Actually not need to. There's no crash in the middle of a slave transaction. So it must be a copy-paste leftover. [x] > +set @@global.rpl_semi_sync_master_enabled = 1; > +set @@global.rpl_semi_sync_master_wait_point=AFTER_SYNC; > + > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > +call mtr.add_suppression("1 client is using or hasn.t closed the table properly"); > +call mtr.add_suppression("Table './mtr/test_suppressions' is marked as crashed and should be repaired"); > + > +CREATE TABLE t1 (a INT PRIMARY KEY, b MEDIUMTEXT) ENGINE=Innodb; > +INSERT INTO t1 VALUES (1, 'dummy1'); > + > +# > +# CRASH the original master, and FAILOVER to the new > +# > + > +# value 1 for server id 1 -> 2 failover > +--let $failover_to_slave=1 > +--let $query_to_crash= INSERT INTO t1 VALUES (2, REPEAT("x", 4100)) > +--let $log_search_pattern=truncated binlog file:.*master.*000001 > +--source rpl_semi_sync_crash.inc > + > +--connection server_2 > +--let $rows_so_far=3 > +--eval INSERT INTO t1 VALUES ($rows_so_far, 'dummy3') > +--save_master_pos > +--echo # The gtid state on current master must be equal to ... > +SHOW VARIABLES LIKE 'gtid_binlog_pos'; > + > +--connection server_1 > +--sync_with_master > +--eval SELECT COUNT(*) = $rows_so_far as 'true' FROM t1 > +--echo # ... the gtid states on the slave: > +SHOW VARIABLES LIKE 'gtid_slave_pos'; > +SHOW VARIABLES LIKE 'gtid_binlog_pos'; > + > +--connection server_2 > +# > +# CRASH the new master and FAILOVER back to the original > +# > + > +# value 0 for the reverse server id 2 -> 1 failover > +--let $failover_to_slave=0 > +--let $query_to_crash = INSERT INTO t1 VALUES (4, REPEAT("x", 4100)) > +--let $query2_to_crash= INSERT INTO t1 VALUES (5, REPEAT("x", 4100)) > +--let $log_search_pattern=truncated binlog file:.*slave.*000001 > +--source rpl_semi_sync_crash.inc > + > +--connection server_1 > +--let $rows_so_far=6 > +--eval INSERT INTO t1 VALUES ($rows_so_far, 'Done') > +--save_master_pos > +--echo # The gtid state on current master must be equal to ... > +SHOW VARIABLES LIKE 'gtid_binlog_pos'; > + > +--connection server_2 > +--sync_with_master > +--eval SELECT COUNT(*) = $rows_so_far as 'true' FROM t1 > +--echo # ... the gtid states on the slave: > +SHOW VARIABLES LIKE 'gtid_slave_pos'; > +SHOW VARIABLES LIKE 'gtid_binlog_pos'; > + > + > +--let $diff_tables=server_1:t1, server_2:t1 > +--source include/diff_tables.inc > + > +# > +--echo # Cleanup > +# > +--connection server_1 > +DROP TABLE t1; > +--save_master_pos > + > +--connection server_2 > +--sync_with_master > +--source include/stop_slave.inc > + > +--connection server_1 > +set @@global.rpl_semi_sync_master_enabled = 0; > +set @@global.rpl_semi_sync_slave_enabled = 0; > +set @@global.rpl_semi_sync_master_wait_point=default; > +RESET SLAVE; > +RESET MASTER; > + > +--connection server_2 > +set @@global.rpl_semi_sync_master_enabled = 0; > +set @@global.rpl_semi_sync_slave_enabled = 0; > +set @@global.rpl_semi_sync_master_wait_point=default; > + > +evalp CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1, master_user='root', master_use_gtid=no; > +--source include/start_slave.inc > + > +connection default; > +--enable_reconnect > +--source include/wait_until_connected_again.inc > + > +--source include/rpl_end.inc > diff --git a/sql/handler.h b/sql/handler.h > index fc69d9423b4..05a62ed0021 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -873,6 +874,15 @@ typedef struct xid_t XID; > /* The 'buf' has to have space for at least SQL_XIDSIZE bytes. */ > uint get_sql_xid(XID *xid, char *buf); > > +/* struct for semisync slave binlog truncate recovery */ > +struct xid_recovery_member > +{ > + my_xid xid; > + uint in_engine_prepare; // number of engines that have xid prepared > + bool decided_to_commit; > + std::pair<uint, my_off_t> binlog_coord; // semisync recovery binlog offset wouldn't it be clearer to have a struct with named members? in fact, I'm somewhat surprised there's no such struct for binlog coords already. > +}; > + > /* for recover() handlerton call */ > #define MIN_XID_LIST_SIZE 128 > #define MAX_XID_LIST_SIZE (1024*128) > @@ -4820,7 +4830,8 @@ int ha_commit_one_phase(THD *thd, bool all); > int ha_commit_trans(THD *thd, bool all); > int ha_rollback_trans(THD *thd, bool all); > int ha_prepare(THD *thd); > -int ha_recover(HASH *commit_list); > +int ha_recover(HASH *commit_list, MEM_ROOT *mem_root= NULL); > +uint ha_recover_complete(HASH *commit_list, std::pair<uint, my_off_t> *coord= NULL); is coord a truncation position? > > /* transactions: these functions never call handlerton functions directly */ > int ha_enable_transaction(THD *thd, bool on); > diff --git a/sql/log_event.h b/sql/log_event.h > index 8a342cb5cd3..1036e9a44d4 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -482,6 +482,16 @@ class String; > */ > #define LOG_EVENT_IGNORABLE_F 0x80 > > +/** > + @def LOG_EVENT_ACCEPT_OWN_F > + > + Flag sets by the semisync slave for accepting > + the same server_id ("own") events which the slave must not have > + in its state. Typically such events were never committed by > + their originator (this server) and discared at its semisync-slave recovery. > +*/ > +#define LOG_EVENT_ACCEPT_OWN_F 0x4000 may be, add an assert on all received events that such a flag is not set? it can only be set on events in relay log. also, consider the case when this event is read from a relay log, applied, and then sent to further slaves. In this case this flag must be removed before sending, otherwise they'll mistakenly might apply it if the server_id will match. > + > /** > @def LOG_EVENT_SKIP_REPLICATION_F > > @@ -3357,6 +3367,12 @@ class Gtid_log_event: public Log_event > uint64 commit_id; > uint32 domain_id; > uchar flags2; > + uint flags_extra; // more flags area placed after the regular flags2's one > + /* > + Extra to a "base" engine recoverable engines participating > + in the transaction. Zero, when the base engine only is present. what's a "base engine"? > + */ > + uint8 extra_engines; > > /* Flags2. */ > > diff --git a/sql/handler.cc b/sql/handler.cc > index c0a810a72bc..a46cef6b64c 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1637,9 +1672,17 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans) > DEBUG_SYNC(thd, "commit_one_phase_2"); > if (ha_info) > { > + int err; > + > + if (has_binlog_hton(ha_info) && can you replace has_binlog_hton() with, like, if trx cache is not empty or binlog enabled or something like that? > + (err= binlog_commit(thd, all, > + is_ro_1pc_trans(thd, ha_info, all, is_real_trans)))) > + { > + my_error(ER_ERROR_DURING_COMMIT, MYF(0), err); > + error= 1; > + } > for (; ha_info; ha_info= ha_info_next) > { > - int err; > handlerton *ht= ha_info->ht(); > if ((err= ht->commit(ht, thd, all))) > { > @@ -1962,8 +2008,177 @@ struct xarecover_st > XID *list; > HASH *commit_list; > bool dry_run; > + MEM_ROOT *mem_root; > + bool error; > }; > > +/** > + Inserts a new hash member. > + > + returns a successfully created and inserted @c xid_recovery_member > + into hash @c hash_arg, > + or NULL. > +*/ > +static xid_recovery_member* > +xid_member_insert(HASH *hash_arg, my_xid xid_arg, MEM_ROOT *ptr_mem_root) > +{ > + xid_recovery_member *member= (xid_recovery_member*) > + alloc_root(ptr_mem_root, sizeof(xid_recovery_member)); > + if (!member) > + return NULL; > + > + member->xid= xid_arg; > + member->in_engine_prepare= 1; > + member->decided_to_commit= false; > + > + return my_hash_insert(hash_arg, (uchar*) member) ? NULL : member; > +} > + > +/* > + Inserts a new or updates an existing hash member to increment > + the member's prepare counter. > + > + returns false on success, > + true otherwise. > +*/ > +static bool xid_member_replace(HASH *hash_arg, my_xid xid_arg, > + MEM_ROOT *ptr_mem_root) > +{ > + xid_recovery_member* member; > + if ((member= (xid_recovery_member *) > + my_hash_search(hash_arg, (uchar *)& xid_arg, sizeof(xid_arg)))) > + member->in_engine_prepare++; > + else > + member= xid_member_insert(hash_arg, xid_arg, ptr_mem_root); > + > + return member == NULL; > +} > + > +/* > + Decision to commit returns true, otherwise false for rollback. > + Flagged to commit member is destined to commit. If it is in doubt in case > + A. the caller does not specify coord_ptr (always so in the normal recovery), or > + B. coord_ptr is not NULL (can only be so in the semisync slave case) and its > + offset is greater than that of the member's the decision is rollback. > + If both A,B do not hold - which is the semisync slave recovery case - > + the decision is to rollback. > +*/ > +static bool xarecover_decide(xid_recovery_member* member, > + xid_t x, std::pair<uint, my_off_t> *coord_ptr) > +{ > + return > + member->decided_to_commit ? true : > + !coord_ptr ? false : > + (member->binlog_coord < *coord_ptr ? // semisync slave recovery > + true : false); > +} > + > +struct xarecover_iterate_arg > +{ > + handlerton *hton; > + std::pair<uint, my_off_t> *binlog_coord; > +}; > + > +/* > + Hash iterate function to complete with commit or rollback as either > + has been decided already or decide now (in the semisync recovery) > + via comparison against passed offset. > + Commit when the offset is greater than that of the member. > +*/ > +static my_bool xarecover_do_commit_or_rollback(void *member_arg, > + void *iter_arg) > +{ > + xid_recovery_member *member= (xid_recovery_member*) member_arg; > + handlerton *hton= ((xarecover_iterate_arg*) iter_arg)->hton; > + std::pair<uint, my_off_t> *max_coord_ptr= > + ((xarecover_iterate_arg*) iter_arg)->binlog_coord; > + xid_t x; > + my_bool rc; > + > + x.set(member->xid); > + > + rc= xarecover_decide(member, x, max_coord_ptr) ? > + hton->commit_by_xid(hton, &x) : hton->rollback_by_xid(hton, &x); > + > + DBUG_ASSERT(rc || member->in_engine_prepare > 0); > + > + if (!rc) > + { > + /* > + This block relies on Engine to report XAER_NOTA at > + "complete"_by_xid for unknown xid. > + */ > + member->in_engine_prepare--; > + if (global_system_variables.log_warnings > 2) > + sql_print_warning("%s transaction with xid %llu", may be not a sql_print_warning, but a sql_print_information? it's just an informational message > + member->decided_to_commit ? > + "Committed" : "Rolled back", (ulonglong) member->xid); > + } > + > + return false; > +} > + > +static my_bool xarecover_do_count_in_prepare(void *member_arg, > + void *ptr_count) > +{ > + xid_recovery_member *member= (xid_recovery_member*) member_arg; > + if (member->in_engine_prepare) > + { > + (*(uint*) ptr_count)++; > + if (global_system_variables.log_warnings > 2) > + sql_print_warning("Found prepared transaction with xid %llu", > + (ulonglong) member->xid); > + } > + > + return false; > +} > + > +struct xarecover_complete_arg > +{ > + HASH *commit_list; > + std::pair<uint, my_off_t> *binlog_coord; > +}; > + > +/* > + Completes binlog recovery to invoke a decider function for > + each transaction in doubt. > +*/ > +static my_bool xarecover_binlog_handlerton(THD *unused, > + plugin_ref plugin, > + void *arg) > +{ > + handlerton *hton= plugin_hton(plugin); > + > + if (hton->state == SHOW_OPTION_YES && hton->recover) > + { > + xarecover_iterate_arg iter_arg= > + { > + hton, > + ((xarecover_complete_arg*) arg)->binlog_coord > + }; > + my_hash_iterate(((xarecover_complete_arg*) arg)->commit_list, > + xarecover_do_commit_or_rollback, &iter_arg); > + } > + > + return FALSE; > +} > + > +/* > + Completes binlog recovery to invoke decider functions for > + each handerton. > + Returns the number of transactions remained doubtful. > +*/ > +uint ha_recover_complete(HASH *commit_list, std::pair<uint, my_off_t> *coord) > +{ > + uint count= 0; > + xarecover_complete_arg complete_arg= { commit_list, coord }; > + plugin_foreach(NULL, xarecover_binlog_handlerton, > + MYSQL_STORAGE_ENGINE_PLUGIN, &complete_arg); > + my_hash_iterate(commit_list, xarecover_do_count_in_prepare, &count); wouldn't it be cleaner to do everything in one commit_list scan? for every xid_recovery_member: run plugin_foreach, commit or rollback as needed increment a counter, if still in doubt > + > + return count; > +} > + > static my_bool xarecover_handlerton(THD *unused, plugin_ref plugin, > void *arg) > { > @@ -1973,6 +2188,9 @@ static my_bool xarecover_handlerton(THD *unused, plugin_ref plugin, > > if (hton->state == SHOW_OPTION_YES && hton->recover) > { > +#ifndef DBUG_OFF > + my_xid dbug_xid_list[128] __attribute__((unused)) = {0}; > +#endif What do you use it for? > while ((got= hton->recover(hton, info->list, info->len)) > 0 ) > { > sql_print_information("Found %d prepared transaction(s) in %s", > diff --git a/sql/log.cc b/sql/log.cc > index 8073f09ab88..a90d1e757e8 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2050,14 +2058,17 @@ static int binlog_commit(handlerton *hton, THD *thd, bool all) > Otherwise, we accumulate the changes. > */ > if (likely(!error) && ending_trans(thd, all)) > + { > + cache_mngr->ro_1pc= ro_1pc; > error= binlog_commit_flush_trx_cache(thd, all, cache_mngr); > + cache_mngr->ro_1pc= false; Why do you put it in cache_mngr, instead of passing it down, like you pass `all` ? > + } > > /* > This is part of the stmt rollback. > */ > if (!all) > cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF); > - > THD_STAGE_INFO(thd, org_stage); > DBUG_RETURN(error); > } > @@ -9609,6 +9626,147 @@ int TC_LOG::using_heuristic_recover() > /****** transaction coordinator log for 2pc - binlog() based solution ******/ > #define TC_LOG_BINLOG MYSQL_BIN_LOG > > +/** > + Truncates the current binlog to specified position. Removes the rest of binlogs > + which are present after this binlog file. > + > + @param truncate_file Holds the binlog name to be truncated > + @param truncate_pos Position within binlog from where it needs to > + truncated. > + > + @retval true ok > + @retval false error > + > +*/ > +bool MYSQL_BIN_LOG::truncate_and_remove_binlogs(const char *file_name, > + my_off_t pos, > + rpl_gtid *ptr_gtid) > +{ > + int error= 0; > +#ifdef HAVE_REPLICATION > + LOG_INFO log_info; > + THD *thd= current_thd; > + my_off_t index_file_offset= 0; > + File file= -1; > + MY_STAT s; > + > + if ((error= find_log_pos(&log_info, file_name, 1))) > + { > + sql_print_error("Failed to locate binary log file:%s." > + "Error:%d", file_name, error); > + goto end; > + } > + > + while (!(error= find_next_log(&log_info, 1))) > + { > + if (!index_file_offset) > + { > + index_file_offset= log_info.index_file_start_offset; > + if ((error= open_purge_index_file(TRUE))) > + { > + sql_print_error("Failed to open purge index " > + "file:%s. Error:%d", purge_index_file_name, error); > + goto end; > + } > + } > + if ((error= register_purge_index_entry(log_info.log_file_name))) > + { > + sql_print_error("Failed to copy %s to purge index" > + " file. Error:%d", log_info.log_file_name, error); > + goto end; > + } > + } > + > + if (error != LOG_INFO_EOF) > + { > + sql_print_error("Failed to find the next binlog to " > + "add to purge index register. Error:%d", error); > + goto end; > + } > + > + if (is_inited_purge_index_file()) > + { > + if (!index_file_offset) > + index_file_offset= log_info.index_file_start_offset; > + > + if ((error= sync_purge_index_file())) > + { > + sql_print_error("Failed to flush purge index " > + "file. Error:%d", error); > + goto end; > + } > + > + // Trim index file > + if ((error= > + mysql_file_chsize(index_file.file, index_file_offset, '\n', > + MYF(MY_WME))) || > + (error= > + mysql_file_sync(index_file.file, MYF(MY_WME|MY_SYNC_FILESIZE)))) > + { > + sql_print_error("Failed to trim binlog index " > + "file:%s to offset:%llu. Error:%d", index_file_name, > + index_file_offset, error); > + goto end; > + } > + > + /* Reset data in old index cache */ > + if ((error= reinit_io_cache(&index_file, READ_CACHE, (my_off_t) 0, 0, 1))) > + { > + sql_print_error("Failed to reinit binlog index " > + "file. Error:%d", error); > + goto end; > + } > + > + /* Read each entry from purge_index_file and delete the file. */ > + if ((error= purge_index_entry(thd, NULL, TRUE))) > + { > + sql_print_error("Failed to process registered " > + "files that would be purged."); > + goto end; > + } > + } > + > + DBUG_ASSERT(pos); > + > + if ((file= mysql_file_open(key_file_binlog, file_name, > + O_RDWR | O_BINARY, MYF(MY_WME))) < 0) > + { > + error= 1; > + sql_print_error("Failed to open binlog file:%s for " > + "truncation.", file_name); > + goto end; > + } > + my_stat(file_name, &s, MYF(0)); > + > + /* Change binlog file size to truncate_pos */ > + if ((error= > + mysql_file_chsize(file, pos, 0, MYF(MY_WME))) || > + (error= mysql_file_sync(file, MYF(MY_WME|MY_SYNC_FILESIZE)))) > + { > + sql_print_error("Failed to trim the " > + "binlog file:%s to size:%llu. Error:%d", > + file_name, pos, error); > + goto end; > + } > + else > + { > + char buf[21]; > + > + longlong10_to_str(ptr_gtid->seq_no, buf, 10); > + sql_print_information("Successfully truncated binlog file:%s " > + "to pos:%llu to remove transactions starting from " > + "GTID %u-%u-%s", file_name, pos, > + ptr_gtid->domain_id, ptr_gtid->server_id, buf); > + } > + > +end: > + if (file >= 0) > + mysql_file_close(file, MYF(MY_WME)); Why you don't clean inuse flag here? You used to do it in the previous version of the patch. > + > + error= error || close_purge_index_file(); > +#endif > + return error > 0; > +} > int TC_LOG_BINLOG::open(const char *opt_name) > { > int error= 1; > @@ -10215,34 +10914,50 @@ int TC_LOG_BINLOG::recover(LOG_INFO *linfo, const char *last_log_name, > end_io_cache(&log); > mysql_file_close(file, MYF(MY_WME)); > file= -1; > + /* > + NOTE: reading other binlog's FD is necessary for finding out > + the checksum status of the respective binlog file. > + */ okay, but where do you read other binlog's FD? In the previous patch I reviewed you had case FORMAT_DESCRIPTION_EVENT: read FD and replace fdle. now you don't have it anymore. > + if (find_next_log(linfo, 1)) > + { > + sql_print_error("Error reading binlog files during recovery. " > + "Aborting."); > + goto err2; > + } > } > > +#ifdef HAVE_REPLICATION > + int rc= ctx.next_binlog_or_round(round, last_log_name, > + binlog_checkpoint_name, linfo, this); > + if (rc == -1) > + goto err2; > + else if (rc == 1) > + break; // all rounds done > +#else > if (!strcmp(linfo->log_file_name, last_log_name)) > break; // No more files to do > + round++; > +#endif > + > if ((file= open_binlog(&log, linfo->log_file_name, &errmsg)) < 0) > { > sql_print_error("%s", errmsg); > goto err2; > } > - /* > - We do not need to read the Format_description_log_event of other binlog > - files. It is not possible for a binlog checkpoint to span multiple > - binlog files written by different versions of the server. So we can use > - the first one read for reading from all binlog files. > - */ > - if (find_next_log(linfo, 1)) > - { > - sql_print_error("Error reading binlog files during recovery. Aborting."); > - goto err2; > - } > fdle->reset_crypto(); > - } > + } // end of for > > if (do_xa) > { > - if (ha_recover(&xids)) > - goto err2; > - > + if (binlog_checkpoint_found) > + { > +#ifndef HAVE_REPLICATION > + if (ha_recover_complete(&xids)) so, ha_recover_complete() is for no-semisync no-replication case? basically it should be the old behavior, exactly as before? why do you need ha_recover_complete() then if it didn't exist before? > +#else > + if (ctx.complete(this, xids)) > +#endif > + goto err2; > + } > free_root(&mem_root, MYF(0)); > my_hash_free(&xids); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0