- developers - lists.mariadb.org

[Maria-developers] MDEV-13467 review.
by Alexey Botchkov 09 Jan '21

09 Jan '21

Hi, Anel! Sorry for that long delay with this review. Had real difficult weeks of my life :( Now it seems to be over, so getting back to normal. Below is my opinions of the code i see in the branch bb-10.1-anel-MDEV-13467-gis-feature-st_sphere-v2 Hope it's the recent version. + Adapter for functions that takes two or three arguments. +*/ + +class Create_func_arg2_or_3 : public Create_func +{ Do you think we need the separate Create_fund_arg2_or_3 class? If you don't see any other class to inherit from it, i'd recommend to leave the Create_func_distance_sphere alone. No need to have two constructors in the Item_func_sphere_distance too. Just use the list argument as it's done for the Item_func_json_contains (in 10.2). Anyway i belive we should get rid of duplicating code about the 'param1' and 'param2'. They can be popped/checked with no if (arg_count == 2) condition. + double distance= 0.0; + null_value= (args[0]->null_value || args[1]->null_value); + if (null_value) + { + // Returned item must be set to Null + DBUG_ASSERT(maybe_null); don't think it's needed. + return 0; Should be return 0.0 i belive. Or goto handle_errors. Thet goes to all but one 'return' statements in this function. + } > ... > if (!arg1 || !arg2) This last condition should never happen since we have not-null_value. So i'd remove that 'if' section completely. And anyway it should do the 'return' in that branch. + null_value= args[2]->null_value; + if (null_value) + { + return 0; + } seems nicer to me this way if (args[2]->null_value) { null_value= true; goto handle_errors; } + if (!(g1= Geometry::construct(&buffer1, arg1->ptr(), arg1->length())) || + !(g2= Geometry::construct(&buffer2, arg2->ptr(), arg2->length()))) + { + goto handle_errors; we should launch the ER_GIS_INVALID_DATA here. + // Generate error message in case different geometry is used? Yes, i think the explainig message here is good to have. +double Item_func_sphere_distance::spherical_distance(Geometry *g1, + Geometry *g2, + const double sphere_radius) +double Item_func_sphere_distance::spherical_distance_points(Geometry *g1, + Geometry *g2, + const double r) Why these two are member static functions, not simply static? If you plan any 'external' use for them, please add comments about what exacly they do. +double Item_func_sphere_distance::spherical_distance(Geometry *g1, + Geometry *g2, + const double sphere_radius) +{ + double res= 0.0; + switch (g1->get_class_info()->m_type_id) + { + case Geometry::wkb_point: + res= spherical_distance_points(g1, g2, sphere_radius); + break; + + case Geometry::wkb_multipoint: + res= spherical_distance_points(g1, g2, sphere_radius); + break; Why do we need this function? We confirmed already that the type is either wkb_point or wkb_multipolygon, so just can call spherical_distance_points. No? >>>>> +class Item_func_sphere_distance: public Item_real_func +{ + double sphere_radius= 6370986.0; // Default radius equals Earth radius Some compilers don't like this syntax, and we didn't use it yet, so i'd recomment to move this to the constructor or even better to the ::val_real(). +double Gis_point::calculate_haversine(const Geometry *g, + const double sphere_radius) +{ + DBUG_ASSERT(sphere_radius > 0); + double x1r= 0.0; + double x2r= 0.0; + double y1r= 0.0; + double y2r= 0.0; That lot of unnecessary initializations seems excessive. + if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint) + { + const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1 for the type + char point_temp[point_size]; + memset(point_temp+4, Geometry::wkb_point, 1); + memcpy(point_temp+5, static_cast<const Gis_multi_point *>(g)->get_data_ptr()+5, 4); + memcpy(point_temp+4+WKB_HEADER_SIZE, g->get_data_ptr()+4+WKB_HEADER_SIZE, + POINT_DATA_SIZE); + point_temp[point_size-1]= '\0'; + Geometry_buffer gbuff; + Geometry *gg= Geometry::construct(&gbuff, point_temp, point_size-1); There is the Gis_multi_point::geometry_n() function, should be used here to get the point. I don't like that lot of static_cast-s. Isn't it nicer to have just the function of double calculate_harvesine_points(Geometry *p1, Geometry *p2, double radius) double calculate_harvesine_multipoint_point(Geometry *mp, Geometry *p, double radius) double calculate_harvesine_multipoint_multipoint(Geometry *mp1, Geometry *mp2, double radius) which would call each other? The ::get_xy_radian() method looks unneeded to me. It's enough to have the to_radian(double d) function. BTW it's possible to do the multipoint/multipoint calculations faster than n^2. Not necessary to do it right now, just not to remember. Best regards. HF

1 0

[Maria-developers] 608b0ee52ef: MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11
by sujatha 04 Jan '21

04 Jan '21

revision-id: 608b0ee52ef3e854ce14a407e64e936adbbeba23 (mariadb-10.2.31-648-g608b0ee52ef) parent(s): 25db9ffa8bdab8a2f2af3c7f154343dd6c6d238f author: Sujatha committer: Sujatha timestamp: 2021-01-04 15:06:12 +0530 message: MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11 Problem: ======= Upon deleting or updating a row in a parent table (with primary key), if the child table has virtual column and an associated key with ON UPDATE CASCADE/ON DELETE CASCADE, it will result in slave crash. Analysis: ======== Tables which are related through foreign key require prelocking similar to triggers. i.e If a table has triggers/foreign keys we should add all tables and routines used by them to the prelocking set. This prelocking happens during 'open_and_lock_tables' call. Each table being opened is checked for foreign key references. If foreign key reference exists then the child table is opened and it is linked to the table_list. Upon any modification to parent table its corresponding child tables are retried from table_list and they are updated accordingly. This prelocking work fine on master. On slave prelocking works for following cases. - Statement/mixed based replication - In row based replication when trigger execution is enabled through 'slave_run_triggers_for_rbr=YES/LOGGING/ENFORCE' Otherwise it results in an assert/crash, as the parent table will not find the corresponding child table and it will be NULL. Dereferencing NULL pointer leads to slave server exit. Fix: === Introduce a new 'slave_fk_event_map' flag similar to 'trg_event_map'. This flag will ensure that when foreign key is enabled in row based replication all the parent and child tables are prelocked, so that parent is able to locate the child table. Note: This issue is specific to slave, hence only slave needs to be upgraded. --- mysql-test/suite/rpl/r/rpl_row_vcol_crash.result | 380 ++++++++++++++++++++ mysql-test/suite/rpl/t/rpl_row_vcol_crash.test | 425 +++++++++++++++++++++++ sql/log_event.cc | 42 +-- sql/sql_base.cc | 118 ++++--- sql/table.h | 6 +- 5 files changed, 906 insertions(+), 65 deletions(-) diff --git a/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result b/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result new file mode 100644 index 00000000000..f76d8935fa8 --- /dev/null +++ b/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result @@ -0,0 +1,380 @@ +include/master-slave.inc +[connection master] +# +# Test case 1: KEY on a virtual column with ON DELETE CASCADE +# +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1),(2),(3); +CREATE TABLE t2 (id INT NOT NULL PRIMARY KEY, +t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (90,1,NULL); +INSERT INTO t2 VALUES (91,2,default); +DELETE FROM t1 WHERE id=1; +connection slave; +# +# Verify data consistency on slave +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t2,t1; +connection slave; +# +# Test case 2: Verify "ON DELETE CASCADE" for parent->child->child scenario +# Parent table: users +# Child tables: matchmaking_groups, matchmaking_group_users +# Parent table: matchmaking_groups +# Child tables: matchmaking_group_users, matchmaking_group_maps +# +# Deleting a row from parent table should be reflected in +# child tables. +# matchmaking_groups->matchmaking_group_users->matchmaking_group_maps +# users->matchmaking_group_users->matchmaking_group_maps +# +connection master; +CREATE TABLE users (id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, +name VARCHAR(32) NOT NULL DEFAULT '' +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_groups ( +id BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY, +host_user_id INT UNSIGNED NOT NULL UNIQUE, +v_col INT AS (host_user_id+1) VIRTUAL, KEY (v_col), +CONSTRAINT FOREIGN KEY (host_user_id) REFERENCES users (id) +ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_group_users ( +matchmaking_group_id BIGINT UNSIGNED NOT NULL, +user_id INT UNSIGNED NOT NULL, +v_col1 int as (user_id+1) virtual, KEY (v_col1), +PRIMARY KEY (matchmaking_group_id,user_id), +UNIQUE KEY user_id (user_id), +CONSTRAINT FOREIGN KEY (matchmaking_group_id) +REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE, +CONSTRAINT FOREIGN KEY (user_id) +REFERENCES users (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_group_maps ( +matchmaking_group_id BIGINT UNSIGNED NOT NULL, +map_id TINYINT UNSIGNED NOT NULL, +v_col2 INT AS (map_id+1) VIRTUAL, KEY (v_col2), +PRIMARY KEY (matchmaking_group_id,map_id), +CONSTRAINT FOREIGN KEY (matchmaking_group_id) +REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +connection slave; +connection master; +INSERT INTO users VALUES (NULL,'foo'),(NULL,'bar'); +INSERT INTO matchmaking_groups VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_users VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_maps VALUES (10,55,default),(11,66,default); +DELETE FROM matchmaking_groups WHERE id = 10; +connection slave; +# +# No rows should be returned as ON DELETE CASCASE should have removed +# corresponding rows from child tables. There should not any mismatch +# of 'id' field between parent->child. +# +SELECT * FROM matchmaking_group_users WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +matchmaking_group_id user_id v_col1 +SELECT * FROM matchmaking_group_maps WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +matchmaking_group_id map_id v_col2 +# +# Rows with id=11 should be present +# +SELECT * FROM matchmaking_group_users; +matchmaking_group_id user_id v_col1 +11 2 3 +SELECT * FROM matchmaking_group_maps; +matchmaking_group_id map_id v_col2 +11 66 67 +connection master; +DELETE FROM users WHERE id = 2; +connection slave; +# +# No rows should be present in both the child tables +# +SELECT * FROM matchmaking_group_users; +matchmaking_group_id user_id v_col1 +SELECT * FROM matchmaking_group_maps; +matchmaking_group_id map_id v_col2 +connection master; +DROP TABLE matchmaking_group_maps, matchmaking_group_users, matchmaking_groups, users; +connection slave; +# +# Test case 3: KEY on a virtual column with ON UPDATE CASCADE +# +connection master; +CREATE TABLE t1 (a INT NOT NULL PRIMARY KEY, b INT NOT NULL) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 80); +CREATE TABLE t2 (a INT KEY, b INT, +v_col int as (b+1) virtual, KEY (v_col), +CONSTRAINT b FOREIGN KEY (b) REFERENCES t1(a) ON UPDATE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (51, 1, default); +connection slave; +connection master; +UPDATE t1 SET a = 50 WHERE a = 1; +# +# Master: Verify that ON UPDATE CASCADE works fine +# old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +# +SELECT * FROM t2 WHERE b=50; +a b v_col +51 50 51 +connection slave; +# +# Slave: Verify that ON UPDATE CASCADE works fine +# old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +# +SELECT * FROM t2 WHERE b=50; +a b v_col +51 50 51 +connection master; +DROP TABLE t2, t1; +connection slave; +# +# Test case 4: Define triggers on master, their results should be +# replicated as part of row events and they should be +# applied on slave with the default +# slave_run_triggers_for_rbr=NO +# +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +connection slave; +SHOW GLOBAL VARIABLES LIKE 'slave_run_triggers_for_rbr'; +Variable_name Value +slave_run_triggers_for_rbr NO +# +# As two rows are inserted in table 't1', two rows should get inserted +# into table 't2' as part of trigger. +# +include/assert.inc [Table t2 should have two rows.] +connection master; +DROP TABLE t1,t2; +connection slave; +# +# Test case 5: Define triggers + Foreign Keys on master, their results +# should be replicated as part of row events and master +# and slave should be in sync. +# +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t3 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# As two rows are inserted in table 't1', two rows should get inserted +# into table 't3' as part of trigger. +# +include/assert.inc [Table t3 should have two rows.] +# +# Verify ON DELETE CASCASE correctness +# +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +connection master; +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +include/diff_tables.inc [master:test.t3, slave:test.t3] +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case 6: Triggers are present only on slave and +# 'slave_run_triggers_for_rbr=NO' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= NO;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr NO +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), +KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 0 +# +include/assert.inc [Table t3 should have zero rows.] +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +# +# Verify t1, t2 are consistent on slave. +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case 7: Triggers are present only on slave and +# 'slave_run_triggers_for_rbr=YES' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= YES;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr YES +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), +KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 2 +# +include/assert.inc [Table t3 should have two rows.] +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +# +# Verify t1, t2 are consistent on slave. +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case 8: Triggers and Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=NO' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= NO;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr NO +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 0 +# +include/assert.inc [Table t3 should have zero rows.] +connection master; +DELETE FROM t1 WHERE id=2; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have two rows +SELECT * FROM t2; +t1_id v_col +2 3 +3 4 +connection slave; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +t1_id v_col +3 4 +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case 9: Triggers are Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=YES' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= YES;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr YES +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 2 +# +include/assert.inc [Table t3 should have two rows.] +connection master; +DELETE FROM t1 WHERE id=2; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have two rows +SELECT * FROM t2; +t1_id v_col +2 3 +3 4 +connection slave; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +t1_id v_col +3 4 +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +connection master; +DROP TABLE t3,t2,t1; +connection slave; +include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test b/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test new file mode 100644 index 00000000000..84ee14977f3 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test @@ -0,0 +1,425 @@ +# ==== Purpose ==== +# +# Test verifies that, slave doesn't report any assert on UPDATE or DELETE of +# row which tries to update the virtual columns with associated KEYs. +# +# Test scenarios are listed below. +# 1) KEY on a virtual column with ON DELETE CASCADE +# 2) Verify "ON DELETE CASCADE" for parent->child->child scenario +# 3) KEY on a virtual column with ON UPDATE CASCADE +# 4) Define triggers on master, their results should be replicated +# as part of row events and they should be applied on slave with +# the default slave_run_triggers_for_rbr=NO +# 5) Define triggers + Foreign Keys on master, their results should be +# replicated as part of row events and master and slave should be in sync. +# 6) Triggers are present only on slave and 'slave_run_triggers_for_rbr=NO' +# 7) Triggers are present only on slave and 'slave_run_triggers_for_rbr=YES' +# 8) Triggers and Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=NO' +# 9) Triggers are Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=YES' +# +# ==== References ==== +# +# MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11 +# + +--source include/have_binlog_format_row.inc +--source include/have_innodb.inc +--source include/master-slave.inc + + +--echo # +--echo # Test case 1: KEY on a virtual column with ON DELETE CASCADE +--echo # +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1),(2),(3); + +CREATE TABLE t2 (id INT NOT NULL PRIMARY KEY, + t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; + +INSERT INTO t2 VALUES (90,1,NULL); +INSERT INTO t2 VALUES (91,2,default); + +# Following query results in an assert on slave +DELETE FROM t1 WHERE id=1; +--sync_slave_with_master + +--echo # +--echo # Verify data consistency on slave +--echo # +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc + +--connection master +DROP TABLE t2,t1; +--sync_slave_with_master + +--echo # +--echo # Test case 2: Verify "ON DELETE CASCADE" for parent->child->child scenario +--echo # Parent table: users +--echo # Child tables: matchmaking_groups, matchmaking_group_users +--echo # Parent table: matchmaking_groups +--echo # Child tables: matchmaking_group_users, matchmaking_group_maps +--echo # +--echo # Deleting a row from parent table should be reflected in +--echo # child tables. +--echo # matchmaking_groups->matchmaking_group_users->matchmaking_group_maps +--echo # users->matchmaking_group_users->matchmaking_group_maps +--echo # + +--connection master +CREATE TABLE users (id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, + name VARCHAR(32) NOT NULL DEFAULT '' +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_groups ( + id BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY, + host_user_id INT UNSIGNED NOT NULL UNIQUE, + v_col INT AS (host_user_id+1) VIRTUAL, KEY (v_col), + CONSTRAINT FOREIGN KEY (host_user_id) REFERENCES users (id) + ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_group_users ( + matchmaking_group_id BIGINT UNSIGNED NOT NULL, + user_id INT UNSIGNED NOT NULL, + v_col1 int as (user_id+1) virtual, KEY (v_col1), + PRIMARY KEY (matchmaking_group_id,user_id), + UNIQUE KEY user_id (user_id), + CONSTRAINT FOREIGN KEY (matchmaking_group_id) + REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT FOREIGN KEY (user_id) + REFERENCES users (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_group_maps ( + matchmaking_group_id BIGINT UNSIGNED NOT NULL, + map_id TINYINT UNSIGNED NOT NULL, + v_col2 INT AS (map_id+1) VIRTUAL, KEY (v_col2), + PRIMARY KEY (matchmaking_group_id,map_id), + CONSTRAINT FOREIGN KEY (matchmaking_group_id) + REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +--sync_slave_with_master + +--connection master +INSERT INTO users VALUES (NULL,'foo'),(NULL,'bar'); +INSERT INTO matchmaking_groups VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_users VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_maps VALUES (10,55,default),(11,66,default); + +DELETE FROM matchmaking_groups WHERE id = 10; +--sync_slave_with_master + +--echo # +--echo # No rows should be returned as ON DELETE CASCASE should have removed +--echo # corresponding rows from child tables. There should not any mismatch +--echo # of 'id' field between parent->child. +--echo # +SELECT * FROM matchmaking_group_users WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +SELECT * FROM matchmaking_group_maps WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); + +--echo # +--echo # Rows with id=11 should be present +--echo # +SELECT * FROM matchmaking_group_users; +SELECT * FROM matchmaking_group_maps; + +--connection master +DELETE FROM users WHERE id = 2; +--sync_slave_with_master + +--echo # +--echo # No rows should be present in both the child tables +--echo # +SELECT * FROM matchmaking_group_users; +SELECT * FROM matchmaking_group_maps; + +--connection master +DROP TABLE matchmaking_group_maps, matchmaking_group_users, matchmaking_groups, users; +--sync_slave_with_master + +--echo # +--echo # Test case 3: KEY on a virtual column with ON UPDATE CASCADE +--echo # + +--connection master +CREATE TABLE t1 (a INT NOT NULL PRIMARY KEY, b INT NOT NULL) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 80); + +CREATE TABLE t2 (a INT KEY, b INT, + v_col int as (b+1) virtual, KEY (v_col), + CONSTRAINT b FOREIGN KEY (b) REFERENCES t1(a) ON UPDATE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (51, 1, default); +--sync_slave_with_master + +--connection master +UPDATE t1 SET a = 50 WHERE a = 1; + +--echo # +--echo # Master: Verify that ON UPDATE CASCADE works fine +--echo # old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +--echo # +SELECT * FROM t2 WHERE b=50; +--sync_slave_with_master + +--echo # +--echo # Slave: Verify that ON UPDATE CASCADE works fine +--echo # old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +--echo # +SELECT * FROM t2 WHERE b=50; + +--connection master +DROP TABLE t2, t1; +--sync_slave_with_master + +--echo # +--echo # Test case 4: Define triggers on master, their results should be +--echo # replicated as part of row events and they should be +--echo # applied on slave with the default +--echo # slave_run_triggers_for_rbr=NO +--echo # + +# In row-based replication, the binary log contains row changes. It will have +# both the changes made by the statement itself, and the changes made by the +# triggers that were invoked by the statement. Slave server(s) do not need to +# run triggers for row changes they are applying. Hence verify that this +# property remains the same and data should be available as if trigger was +# executed. Please note by default slave_run_triggers_for_rbr=NO. + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +--sync_slave_with_master + +SHOW GLOBAL VARIABLES LIKE 'slave_run_triggers_for_rbr'; +--echo # +--echo # As two rows are inserted in table 't1', two rows should get inserted +--echo # into table 't2' as part of trigger. +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t2 +--let $assert_text= Table t2 should have two rows. +--source include/assert.inc + +--connection master +DROP TABLE t1,t2; +--sync_slave_with_master + +--echo # +--echo # Test case 5: Define triggers + Foreign Keys on master, their results +--echo # should be replicated as part of row events and master +--echo # and slave should be in sync. +--echo # +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t3 VALUES (1); + +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +--echo # +--echo # As two rows are inserted in table 't1', two rows should get inserted +--echo # into table 't3' as part of trigger. +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc + +--echo # +--echo # Verify ON DELETE CASCASE correctness +--echo # +--connection master +DELETE FROM t1 WHERE id=2; +--sync_slave_with_master + +--connection master +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc +--let $diff_tables= master:test.t3, slave:test.t3 +--source include/diff_tables.inc + +DROP TABLE t3,t2,t1; +--sync_slave_with_master + +# +# Test case: Triggers only on slave +# +--write_file $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc PROCEDURE + if ($slave_run_triggers_for_rbr == '') { + --die !!!ERROR IN TEST: you must set $slave_run_triggers_for_rbr + } + +--connection slave +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +--eval SET GLOBAL slave_run_triggers_for_rbr= $slave_run_triggers_for_rbr; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), + KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +--sync_slave_with_master + +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); + +--connection master +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +if ($slave_run_triggers_for_rbr == 'NO') { +--echo # +--echo # Count must be 0 +--echo # +--let $assert_cond= COUNT(*) = 0 FROM t3 +--let $assert_text= Table t3 should have zero rows. +--source include/assert.inc +} +if ($slave_run_triggers_for_rbr == 'YES') { +--echo # +--echo # Count must be 2 +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc +} + +--connection master +DELETE FROM t1 WHERE id=2; +--sync_slave_with_master +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; + +--echo # +--echo # Verify t1, t2 are consistent on slave. +--echo # +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc + +--connection master +DROP TABLE t3,t2,t1; +--sync_slave_with_master +#END OF +PROCEDURE + +--echo # +--echo # Test case 6: Triggers are present only on slave and +--echo # 'slave_run_triggers_for_rbr=NO' +--echo # +--let $slave_run_triggers_for_rbr=NO +--source $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc + +--echo # +--echo # Test case 7: Triggers are present only on slave and +--echo # 'slave_run_triggers_for_rbr=YES' +--echo # +--let $slave_run_triggers_for_rbr=YES +--source $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc +--remove_file $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc + +# +# Test case: Trigger and Foreign Key are present only on slave +# +--write_file $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc PROCEDURE + if ($slave_run_triggers_for_rbr == '') { + --die !!!ERROR IN TEST: you must set $slave_run_triggers_for_rbr + } + +--connection slave +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +--eval SET GLOBAL slave_run_triggers_for_rbr= $slave_run_triggers_for_rbr; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +--sync_slave_with_master + +# Have foreign key and trigger on slave. +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); + +--connection master +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +if ($slave_run_triggers_for_rbr == 'NO') { +--echo # +--echo # Count must be 0 +--echo # +--let $assert_cond= COUNT(*) = 0 FROM t3 +--let $assert_text= Table t3 should have zero rows. +--source include/assert.inc +} +if ($slave_run_triggers_for_rbr == 'YES') { +--echo # +--echo # Count must be 2 +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc +} + +--connection master +DELETE FROM t1 WHERE id=2; +--echo # t1: Should have one row +SELECT * FROM t1; +--echo # t2: Should have two rows +SELECT * FROM t2; +--sync_slave_with_master +--echo # t1: Should have one row +SELECT * FROM t1; +--echo # t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; + +--connection master +DROP TABLE t3,t2,t1; +--sync_slave_with_master +#END OF +PROCEDURE + +--echo # +--echo # Test case 8: Triggers and Foreign Keys are present only on slave and +--echo # 'slave_run_triggers_for_rbr=NO' +--echo # +--let $slave_run_triggers_for_rbr=NO +--source $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc + +--echo # +--echo # Test case 9: Triggers are Foreign Keys are present only on slave and +--echo # 'slave_run_triggers_for_rbr=YES' +--echo # +--let $slave_run_triggers_for_rbr=YES +--source $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc +--remove_file $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc + +--source include/rpl_end.inc diff --git a/sql/log_event.cc b/sql/log_event.cc index c649e1f64fa..c32f31db1f6 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -10718,7 +10718,7 @@ int Rows_log_event::do_add_row_data(uchar *row_data, size_t length) There was the same problem with MERGE MYISAM tables and so here we try to go the same way. */ -static void restore_empty_query_table_list(LEX *lex) +inline void restore_empty_query_table_list(LEX *lex) { if (lex->first_not_own_table()) (*lex->first_not_own_table()->prev_global)= NULL; @@ -10733,6 +10733,8 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) TABLE* table; DBUG_ENTER("Rows_log_event::do_apply_event(Relay_log_info*)"); int error= 0; + LEX *lex= thd->lex; + uint8 new_trg_event_map= get_trg_event_map(); /* If m_table_id == ~0ULL, then we have a dummy event that does not contain any data. In that case, we just remove all tables in the @@ -10823,27 +10825,29 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) DBUG_ASSERT(!debug_sync_set_action(thd, STRING_WITH_LEN(action))); };); - if (slave_run_triggers_for_rbr) - { - LEX *lex= thd->lex; - uint8 new_trg_event_map= get_trg_event_map(); - - /* - Trigger's procedures work with global table list. So we have to add - rgi->tables_to_lock content there to get trigger's in the list. + /* + Trigger's procedures work with global table list. So we have to add + rgi->tables_to_lock content there to get trigger's in the list. - Then restore_empty_query_table_list() restore the list as it was - */ - DBUG_ASSERT(lex->query_tables == NULL); - if ((lex->query_tables= rgi->tables_to_lock)) - rgi->tables_to_lock->prev_global= &lex->query_tables; + Then restore_empty_query_table_list() restore the list as it was + */ + DBUG_ASSERT(lex->query_tables == NULL); + if ((lex->query_tables= rgi->tables_to_lock)) + rgi->tables_to_lock->prev_global= &lex->query_tables; - for (TABLE_LIST *tables= rgi->tables_to_lock; tables; - tables= tables->next_global) + for (TABLE_LIST *tables= rgi->tables_to_lock; tables; + tables= tables->next_global) + { + if (slave_run_triggers_for_rbr) { tables->trg_event_map= new_trg_event_map; lex->query_tables_last= &tables->next_global; } + else if (!WSREP_ON) + { + tables->slave_fk_event_map= new_trg_event_map; + lex->query_tables_last= &tables->next_global; + } } if (open_and_lock_tables(thd, rgi->tables_to_lock, FALSE, 0)) { @@ -11193,8 +11197,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) } /* remove trigger's tables */ - if (slave_run_triggers_for_rbr) - restore_empty_query_table_list(thd->lex); + restore_empty_query_table_list(thd->lex); #if defined(WITH_WSREP) && defined(HAVE_QUERY_CACHE) if (WSREP(thd) && thd->wsrep_exec_mode == REPL_RECV) @@ -11212,8 +11215,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) DBUG_RETURN(error); err: - if (slave_run_triggers_for_rbr) - restore_empty_query_table_list(thd->lex); + restore_empty_query_table_list(thd->lex); rgi->slave_close_thread_tables(thd); DBUG_RETURN(error); } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 8e57ea437b6..b8d18abb50c 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4341,6 +4341,70 @@ bool table_already_fk_prelocked(TABLE_LIST *tl, LEX_STRING *db, return false; } +/** + Extend the table_list to include foreign tables for prelocking. + + @param[in] thd Thread context. + @param[in] prelocking_ctx Prelocking context of the statement. + @param[in] table_list Table list element for table. + @param[in] sp Routine body. + @param[out] need_prelocking Set to TRUE if method detects that prelocking + required, not changed otherwise. + + @retval FALSE Success. + @retval TRUE Failure (OOM). +*/ +inline bool +prepare_fk_prelocking_list(THD *thd, Query_tables_list *prelocking_ctx, + TABLE_LIST *table_list, bool *need_prelocking, + uint8 op) +{ + List <FOREIGN_KEY_INFO> fk_list; + List_iterator<FOREIGN_KEY_INFO> fk_list_it(fk_list); + FOREIGN_KEY_INFO *fk; + Query_arena *arena, backup; + + arena= thd->activate_stmt_arena_if_needed(&backup); + + table_list->table->file->get_parent_foreign_key_list(thd, &fk_list); + if (thd->is_error()) + { + if (arena) + thd->restore_active_arena(arena, &backup); + return TRUE; + } + + *need_prelocking= TRUE; + + while ((fk= fk_list_it++)) + { + // FK_OPTION_RESTRICT and FK_OPTION_NO_ACTION only need read access + static bool can_write[]= { true, false, true, true, false, true }; + thr_lock_type lock_type; + + if ((op & (1 << TRG_EVENT_DELETE) && can_write[fk->delete_method]) + || (op & (1 << TRG_EVENT_UPDATE) && can_write[fk->update_method])) + lock_type= TL_WRITE_ALLOW_WRITE; + else + lock_type= TL_READ; + + if (table_already_fk_prelocked(prelocking_ctx->query_tables, + fk->foreign_db, fk->foreign_table, + lock_type)) + continue; + + TABLE_LIST *tl= (TABLE_LIST *) thd->alloc(sizeof(TABLE_LIST)); + tl->init_one_table_for_prelocking(fk->foreign_db->str, fk->foreign_db->length, + fk->foreign_table->str, fk->foreign_table->length, + NULL, lock_type, false, table_list->belong_to_view, + op, &prelocking_ctx->query_tables_last); + } + if (arena) + thd->restore_active_arena(arena, &backup); + + return FALSE; +} + /** Defines how prelocking algorithm for DML statements should handle table list @@ -4381,55 +4445,21 @@ handle_table(THD *thd, Query_tables_list *prelocking_ctx, add_tables_and_routines_for_triggers(thd, prelocking_ctx, table_list)) return TRUE; } - if (table_list->table->file->referenced_by_foreign_key()) { - List <FOREIGN_KEY_INFO> fk_list; - List_iterator<FOREIGN_KEY_INFO> fk_list_it(fk_list); - FOREIGN_KEY_INFO *fk; - Query_arena *arena, backup; - - arena= thd->activate_stmt_arena_if_needed(&backup); - - table_list->table->file->get_parent_foreign_key_list(thd, &fk_list); - if (thd->is_error()) - { - if (arena) - thd->restore_active_arena(arena, &backup); - return TRUE; - } - - *need_prelocking= TRUE; - - while ((fk= fk_list_it++)) - { - // FK_OPTION_RESTRICT and FK_OPTION_NO_ACTION only need read access - static bool can_write[]= { true, false, true, true, false, true }; - uint8 op= table_list->trg_event_map; - thr_lock_type lock_type; - - if ((op & (1 << TRG_EVENT_DELETE) && can_write[fk->delete_method]) - || (op & (1 << TRG_EVENT_UPDATE) && can_write[fk->update_method])) - lock_type= TL_WRITE_ALLOW_WRITE; - else - lock_type= TL_READ; - - if (table_already_fk_prelocked(prelocking_ctx->query_tables, - fk->foreign_db, fk->foreign_table, - lock_type)) - continue; - - TABLE_LIST *tl= (TABLE_LIST *) thd->alloc(sizeof(TABLE_LIST)); - tl->init_one_table_for_prelocking(fk->foreign_db->str, fk->foreign_db->length, - fk->foreign_table->str, fk->foreign_table->length, - NULL, lock_type, false, table_list->belong_to_view, - op, &prelocking_ctx->query_tables_last); - } - if (arena) - thd->restore_active_arena(arena, &backup); + return (prepare_fk_prelocking_list(thd, prelocking_ctx, table_list, + need_prelocking, + table_list->trg_event_map)); } } + else if (table_list->slave_fk_event_map && + table_list->table->file->referenced_by_foreign_key()) + { + return (prepare_fk_prelocking_list(thd, prelocking_ctx, + table_list, need_prelocking, + table_list->slave_fk_event_map)); + } return FALSE; } diff --git a/sql/table.h b/sql/table.h index 9a864f7ce9f..57706655d9b 100644 --- a/sql/table.h +++ b/sql/table.h @@ -2277,8 +2277,12 @@ struct TABLE_LIST Indicates what triggers we need to pre-load for this TABLE_LIST when opening an associated TABLE. This is filled after the parsed tree is created. + + slave_fk_event_map is filled on the slave side with bitmaps value + representing row-based event operation to help find and prelock + possible FK constrain-related child tables. */ - uint8 trg_event_map; + uint8 trg_event_map, slave_fk_event_map; /* TRUE <=> this table is a const one and was optimized away. */ bool optimized_away;

2 2

[Maria-developers] 3f48bc08b92: MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11
by sujatha 31 Dec '20

31 Dec '20

revision-id: 3f48bc08b92b2734c2af7408c246b96338675205 (mariadb-10.2.31-646-g3f48bc08b92) parent(s): 78292047a4747ccd9210ba36a185a1dbe825de89 author: Sujatha committer: Sujatha timestamp: 2020-12-31 15:43:53 +0530 message: MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11 Problem: ======= Upon deleting or updating a row in a parent table (with primary key), if the child table has virtual column and an associated key with ON UPDATE CASCADE/ON DELETE CASCADE, it will result in slave crash. Analysis: ======== Tables which are related through foreign key require prelocking similar to triggers. i.e If a table has triggers/foreign keys we should add all tables and routines used by them to the prelocking set. This prelocking happens during 'open_and_lock_tables' call. Each table being opened is checked for foreign key references. If foreign key reference exists then the child table is opened and it is linked to the table_list. Upon any modification to parent table its corresponding child tables are retried from table_list and they are updated accordingly. This prelocking work fine on master. On slave prelocking works for following cases. - Statement/mixed based replication - In row based replication when trigger execution is enabled through 'slave_run_triggers_for_rbr=YES/LOGGING/ENFORCE' Otherwise it results in an assert/crash, as the parent table will not find the corresponding child table and it will be NULL. Dereferencing NULL pointer leads to slave server exit. Fix: === Introduce a new 'slave_fk_event_map' flag similar to 'trg_event_map'. This flag will ensure that when foreign key is enabled in row based replication all the parent and child tables are prelocked, so that parent is able to locate the child table. --- mysql-test/suite/rpl/r/rpl_row_vcol_crash.result | 381 +++++++++++++++++++++ mysql-test/suite/rpl/t/rpl_row_vcol_crash.test | 410 +++++++++++++++++++++++ sql/log_event.cc | 44 ++- sql/sql_base.cc | 6 +- sql/table.h | 6 +- 5 files changed, 821 insertions(+), 26 deletions(-) diff --git a/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result b/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result new file mode 100644 index 00000000000..e2a5d9b65e5 --- /dev/null +++ b/mysql-test/suite/rpl/r/rpl_row_vcol_crash.result @@ -0,0 +1,381 @@ +include/master-slave.inc +[connection master] +# +# Test case: KEY on a virtual column with ON DELETE CASCADE +# +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1),(2),(3); +CREATE TABLE t2 (id INT NOT NULL PRIMARY KEY, +t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (90,1,NULL); +INSERT INTO t2 VALUES (91,2,default); +DELETE FROM t1 WHERE id=1; +connection slave; +# +# Verify data consistency on slave +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t2,t1; +connection slave; +# +# Test case: Verify cascading on delete with multiple levels of child +# tables works fine. +# Parent table: users +# Child tables: matchmaking_groups, matchmaking_group_users +# Parent table: matchmaking_groups +# Child tables: matchmaking_group_users, matchmaking_group_maps +# +# Verify ON DELETE CASCASE works fine when a row is deleted +# from parent table users +# matchmaking_groups->matchmaking_group_users->matchmaking_group_maps +# users->matchmaking_group_users->matchmaking_group_maps +# +connection master; +CREATE TABLE users (id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, +name VARCHAR(32) NOT NULL DEFAULT '' +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_groups ( +id BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY, +host_user_id INT UNSIGNED NOT NULL UNIQUE, +v_col INT AS (host_user_id+1) VIRTUAL, KEY (v_col), +CONSTRAINT FOREIGN KEY (host_user_id) REFERENCES users (id) +ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_group_users ( +matchmaking_group_id BIGINT UNSIGNED NOT NULL, +user_id INT UNSIGNED NOT NULL, +v_col1 int as (user_id+1) virtual, KEY (v_col1), +PRIMARY KEY (matchmaking_group_id,user_id), +UNIQUE KEY user_id (user_id), +CONSTRAINT FOREIGN KEY (matchmaking_group_id) +REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE, +CONSTRAINT FOREIGN KEY (user_id) +REFERENCES users (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +CREATE TABLE matchmaking_group_maps ( +matchmaking_group_id BIGINT UNSIGNED NOT NULL, +map_id TINYINT UNSIGNED NOT NULL, +v_col2 INT AS (map_id+1) VIRTUAL, KEY (v_col2), +PRIMARY KEY (matchmaking_group_id,map_id), +CONSTRAINT FOREIGN KEY (matchmaking_group_id) +REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +connection slave; +connection master; +INSERT INTO users VALUES (NULL,'foo'),(NULL,'bar'); +INSERT INTO matchmaking_groups VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_users VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_maps VALUES (10,55,default),(11,66,default); +DELETE FROM matchmaking_groups WHERE id = 10; +connection slave; +# +# No rows should be returned as ON DELETE CASCASE should have removed +# corresponding rows from child tables. There should not any mismatch +# of 'id' field between parent->child. +# +SELECT * FROM matchmaking_group_users WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +matchmaking_group_id user_id v_col1 +SELECT * FROM matchmaking_group_maps WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +matchmaking_group_id map_id v_col2 +# +# Rows with id=11 should be present +# +SELECT * FROM matchmaking_group_users; +matchmaking_group_id user_id v_col1 +11 2 3 +SELECT * FROM matchmaking_group_maps; +matchmaking_group_id map_id v_col2 +11 66 67 +connection master; +DELETE FROM users WHERE id = 2; +connection slave; +# +# No rows should be present in both the child tables +# +SELECT * FROM matchmaking_group_users; +matchmaking_group_id user_id v_col1 +SELECT * FROM matchmaking_group_maps; +matchmaking_group_id map_id v_col2 +connection master; +DROP TABLE matchmaking_group_maps, matchmaking_group_users, matchmaking_groups, users; +connection slave; +# +# Test case: ON UPDATE CASCADE scenario +# +connection master; +CREATE TABLE t1 (a INT NOT NULL PRIMARY KEY, b INT NOT NULL) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 80); +CREATE TABLE t2 (a INT KEY, b INT, +v_col int as (b+1) virtual, KEY (v_col), +CONSTRAINT b FOREIGN KEY (b) REFERENCES t1(a) ON UPDATE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (51, 1, default); +connection slave; +connection master; +UPDATE t1 SET a = 50 WHERE a = 1; +# +# Master: Verify that ON UPDATE CASCADE works fine +# old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +# +SELECT * FROM t2 WHERE b=50; +a b v_col +51 50 51 +connection slave; +# +# Slave: Verify that ON UPDATE CASCADE works fine +# old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +# +SELECT * FROM t2 WHERE b=50; +a b v_col +51 50 51 +connection master; +DROP TABLE t2, t1; +connection slave; +# +# Test case: When triggers are defined on master they should be +# replicated as part of row events and they should be +# applied on slave with the default +# slave_run_tiggers_for_rbr=NO +# +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +connection slave; +SHOW GLOBAL VARIABLES LIKE 'slave_run_triggers_for_rbr'; +Variable_name Value +slave_run_triggers_for_rbr NO +# +# As two rows are inserted in table 't1', two rows should get inserted +# into table 't2' as part of trigger. +# +include/assert.inc [Table t2 should have two rows.] +connection master; +DROP TABLE t1,t2; +connection slave; +# +# Test case: On master create triggers and tables with foreign key +# relation. Upon replication to slave verify their results +# are fine. Master and Slave should be in sync. +# +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t3 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# As two rows are inserted in table 't1', two rows should get inserted +# into table 't3' as part of trigger. +# +include/assert.inc [Table t3 should have two rows.] +# +# Verify ON DELETE CASCASE correctness +# +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +connection master; +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +include/diff_tables.inc [master:test.t3, slave:test.t3] +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case: Triggers are present only on slave and +# 'slave_run_triggers_for_rbr=NO' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= NO;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr NO +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), +KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 0 +# +include/assert.inc [Table t3 should have zero rows.] +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +# +# Verify t1,t2 are consistent on slave. +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case: Triggers are present only on slave and +# 'slave_run_triggers_for_rbr=YES' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= YES;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr YES +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), +KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 2 +# +include/assert.inc [Table t3 should have two rows.] +connection master; +DELETE FROM t1 WHERE id=2; +connection slave; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +# +# Verify t1,t2 are consistent on slave. +# +include/diff_tables.inc [master:test.t1, slave:test.t1] +include/diff_tables.inc [master:test.t2, slave:test.t2] +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case: Triggers and Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=NO' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= NO;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr NO +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 0 +# +include/assert.inc [Table t3 should have zero rows.] +connection master; +DELETE FROM t1 WHERE id=2; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have two rows +SELECT * FROM t2; +t1_id v_col +2 3 +3 4 +connection slave; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +t1_id v_col +3 4 +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +connection master; +DROP TABLE t3,t2,t1; +connection slave; +# +# Test case: Triggers are Foreign Keys are present only on slave and +# 'slave_run_triggers_for_rbr=YES' +# +connection slave; +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +SET GLOBAL slave_run_triggers_for_rbr= YES;; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; +Variable_name Value +slave_run_triggers_for_rbr YES +connection master; +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +connection slave; +CREATE TABLE t2 (t1_id INT NOT NULL, +v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), +CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); +connection master; +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +connection slave; +# +# Count must be 2 +# +include/assert.inc [Table t3 should have two rows.] +connection master; +DELETE FROM t1 WHERE id=2; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have two rows +SELECT * FROM t2; +t1_id v_col +2 3 +3 4 +connection slave; +# t1: Should have one row +SELECT * FROM t1; +id +3 +# t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +t1_id v_col +3 4 +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; +connection master; +DROP TABLE t3,t2,t1; +connection slave; +include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test b/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test new file mode 100644 index 00000000000..2ed7c219aeb --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_row_vcol_crash.test @@ -0,0 +1,410 @@ +# ==== Purpose ==== +# +# Test verifies that, slave doesn't report any assert on UPDATE or DELETE of +# row which tries to update the virtual columns with associated KEYs. +# +# ==== References ==== +# +# MDEV-23033: All slaves crash once in ~24 hours and loop restart with signal 11 +# + +--source include/have_binlog_format_row.inc +--source include/have_innodb.inc +--source include/master-slave.inc + + +--echo # +--echo # Test case: KEY on a virtual column with ON DELETE CASCADE +--echo # +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1),(2),(3); + +CREATE TABLE t2 (id INT NOT NULL PRIMARY KEY, + t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; + +INSERT INTO t2 VALUES (90,1,NULL); +INSERT INTO t2 VALUES (91,2,default); + +# Following query results in an assert on slave +DELETE FROM t1 WHERE id=1; +--sync_slave_with_master + +--echo # +--echo # Verify data consistency on slave +--echo # +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc + +--connection master +DROP TABLE t2,t1; +--sync_slave_with_master + +--echo # +--echo # Test case: Verify cascading on delete with multiple levels of child +--echo # tables works fine. +--echo # Parent table: users +--echo # Child tables: matchmaking_groups, matchmaking_group_users +--echo # Parent table: matchmaking_groups +--echo # Child tables: matchmaking_group_users, matchmaking_group_maps +--echo # +--echo # Verify ON DELETE CASCASE works fine when a row is deleted +--echo # from parent table users +--echo # matchmaking_groups->matchmaking_group_users->matchmaking_group_maps +--echo # users->matchmaking_group_users->matchmaking_group_maps +--echo # + +--connection master +CREATE TABLE users (id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, + name VARCHAR(32) NOT NULL DEFAULT '' +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_groups ( + id BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY, + host_user_id INT UNSIGNED NOT NULL UNIQUE, + v_col INT AS (host_user_id+1) VIRTUAL, KEY (v_col), + CONSTRAINT FOREIGN KEY (host_user_id) REFERENCES users (id) + ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_group_users ( + matchmaking_group_id BIGINT UNSIGNED NOT NULL, + user_id INT UNSIGNED NOT NULL, + v_col1 int as (user_id+1) virtual, KEY (v_col1), + PRIMARY KEY (matchmaking_group_id,user_id), + UNIQUE KEY user_id (user_id), + CONSTRAINT FOREIGN KEY (matchmaking_group_id) + REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT FOREIGN KEY (user_id) + REFERENCES users (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE matchmaking_group_maps ( + matchmaking_group_id BIGINT UNSIGNED NOT NULL, + map_id TINYINT UNSIGNED NOT NULL, + v_col2 INT AS (map_id+1) VIRTUAL, KEY (v_col2), + PRIMARY KEY (matchmaking_group_id,map_id), + CONSTRAINT FOREIGN KEY (matchmaking_group_id) + REFERENCES matchmaking_groups (id) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8; +--sync_slave_with_master + +--connection master +INSERT INTO users VALUES (NULL,'foo'),(NULL,'bar'); +INSERT INTO matchmaking_groups VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_users VALUES (10,1,default),(11,2,default); +INSERT INTO matchmaking_group_maps VALUES (10,55,default),(11,66,default); + +DELETE FROM matchmaking_groups WHERE id = 10; +--sync_slave_with_master + +--echo # +--echo # No rows should be returned as ON DELETE CASCASE should have removed +--echo # corresponding rows from child tables. There should not any mismatch +--echo # of 'id' field between parent->child. +--echo # +SELECT * FROM matchmaking_group_users WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); +SELECT * FROM matchmaking_group_maps WHERE matchmaking_group_id NOT IN (SELECT id FROM matchmaking_groups); + +--echo # +--echo # Rows with id=11 should be present +--echo # +SELECT * FROM matchmaking_group_users; +SELECT * FROM matchmaking_group_maps; + +--connection master +DELETE FROM users WHERE id = 2; +--sync_slave_with_master + +--echo # +--echo # No rows should be present in both the child tables +--echo # +SELECT * FROM matchmaking_group_users; +SELECT * FROM matchmaking_group_maps; + +--connection master +DROP TABLE matchmaking_group_maps, matchmaking_group_users, matchmaking_groups, users; +--sync_slave_with_master + +--echo # +--echo # Test case: ON UPDATE CASCADE scenario +--echo # + +--connection master +CREATE TABLE t1 (a INT NOT NULL PRIMARY KEY, b INT NOT NULL) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 80); + +CREATE TABLE t2 (a INT KEY, b INT, + v_col int as (b+1) virtual, KEY (v_col), + CONSTRAINT b FOREIGN KEY (b) REFERENCES t1(a) ON UPDATE CASCADE +) ENGINE=InnoDB; +INSERT INTO t2 VALUES (51, 1, default); +--sync_slave_with_master + +--connection master +UPDATE t1 SET a = 50 WHERE a = 1; + +--echo # +--echo # Master: Verify that ON UPDATE CASCADE works fine +--echo # old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +--echo # +SELECT * FROM t2 WHERE b=50; +--sync_slave_with_master + +--echo # +--echo # Slave: Verify that ON UPDATE CASCADE works fine +--echo # old_row: (51, 1, 2) ON UPDATE New_row: (51, 50, 51) +--echo # +SELECT * FROM t2 WHERE b=50; + +--connection master +DROP TABLE t2, t1; +--sync_slave_with_master + +--echo # +--echo # Test case: When triggers are defined on master they should be +--echo # replicated as part of row events and they should be +--echo # applied on slave with the default +--echo # slave_run_tiggers_for_rbr=NO +--echo # + +# In row-based replication, the binary log contains row changes. It will have +# both the changes made by the statement itself, and the changes made by the +# triggers that were invoked by the statement. Slave server(s) do not need to +# run triggers for row changes they are applying. Hence verify that this +# property remains the same and data should be available as if trigger was +# executed. Please note by default slave_run_tiggers_for_rbr=NO. + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (1); +INSERT INTO t1 VALUES (2),(3); +--sync_slave_with_master + +SHOW GLOBAL VARIABLES LIKE 'slave_run_triggers_for_rbr'; +--echo # +--echo # As two rows are inserted in table 't1', two rows should get inserted +--echo # into table 't2' as part of trigger. +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t2 +--let $assert_text= Table t2 should have two rows. +--source include/assert.inc + +--connection master +DROP TABLE t1,t2; +--sync_slave_with_master + +--echo # +--echo # Test case: On master create triggers and tables with foreign key +--echo # relation. Upon replication to slave verify their results +--echo # are fine. Master and Slave should be in sync. +--echo # +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t1 FOR EACH ROW INSERT INTO t3 VALUES (1); + +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +--echo # +--echo # As two rows are inserted in table 't1', two rows should get inserted +--echo # into table 't3' as part of trigger. +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc + +--echo # +--echo # Verify ON DELETE CASCASE correctness +--echo # +--connection master +DELETE FROM t1 WHERE id=2; +--sync_slave_with_master + +--connection master +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc +--let $diff_tables= master:test.t3, slave:test.t3 +--source include/diff_tables.inc + +DROP TABLE t3,t2,t1; +--sync_slave_with_master + +# +# Test case: Triggers only on slave +# +--write_file $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc PROCEDURE + if ($slave_run_triggers_for_rbr == '') { + --die !!!ERROR IN TEST: you must set $slave_run_triggers_for_rbr + } + +--connection slave +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +--eval SET GLOBAL slave_run_triggers_for_rbr= $slave_run_triggers_for_rbr; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), + KEY (t1_id), CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +--sync_slave_with_master + +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); + +--connection master +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +if ($slave_run_triggers_for_rbr == 'NO') { +--echo # +--echo # Count must be 0 +--echo # +--let $assert_cond= COUNT(*) = 0 FROM t3 +--let $assert_text= Table t3 should have zero rows. +--source include/assert.inc +} +if ($slave_run_triggers_for_rbr == 'YES') { +--echo # +--echo # Count must be 2 +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc +} + +--connection master +DELETE FROM t1 WHERE id=2; +--sync_slave_with_master +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; + +--echo # +--echo # Verify t1,t2 are consistent on slave. +--echo # +--let $diff_tables= master:test.t1, slave:test.t1 +--source include/diff_tables.inc +--let $diff_tables= master:test.t2, slave:test.t2 +--source include/diff_tables.inc + +--connection master +DROP TABLE t3,t2,t1; +--sync_slave_with_master +#END OF +PROCEDURE + +--echo # +--echo # Test case: Triggers are present only on slave and +--echo # 'slave_run_triggers_for_rbr=NO' +--echo # +--let $slave_run_triggers_for_rbr=NO +--source $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc + +--echo # +--echo # Test case: Triggers are present only on slave and +--echo # 'slave_run_triggers_for_rbr=YES' +--echo # +--let $slave_run_triggers_for_rbr=YES +--source $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc +--remove_file $MYSQLTEST_VARDIR/tmp/trig_on_slave.inc + +# +# Test case: Trigger and Foreign Key are present only on slave +# +--write_file $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc PROCEDURE + if ($slave_run_triggers_for_rbr == '') { + --die !!!ERROR IN TEST: you must set $slave_run_triggers_for_rbr + } + +--connection slave +SET @save_slave_run_triggers_for_rbr= @@GLOBAL.slave_run_triggers_for_rbr; +--eval SET GLOBAL slave_run_triggers_for_rbr= $slave_run_triggers_for_rbr; +SHOW GLOBAL VARIABLES LIKE '%slave_run_triggers_for_rbr%'; + +--connection master +CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY) ENGINE=InnoDB; +SET sql_log_bin=0; +CREATE TABLE t2 (t1_id INT NOT NULL,v_col INT AS (t1_id+1) VIRTUAL) ENGINE=INNODB; +SET sql_log_bin=1; +CREATE TABLE t3 (count INT NOT NULL) ENGINE=InnoDB; +--sync_slave_with_master + +# Have foreign key and trigger on slave. +CREATE TABLE t2 (t1_id INT NOT NULL, + v_col INT AS (t1_id+1) VIRTUAL, KEY (v_col), KEY (t1_id), + CONSTRAINT a FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE +) ENGINE=InnoDB; +CREATE TRIGGER trg AFTER INSERT ON t2 FOR EACH ROW INSERT INTO t3 VALUES (1); + +--connection master +INSERT INTO t1 VALUES (2),(3); +INSERT INTO t2 VALUES (2, default), (3, default); +--sync_slave_with_master + +if ($slave_run_triggers_for_rbr == 'NO') { +--echo # +--echo # Count must be 0 +--echo # +--let $assert_cond= COUNT(*) = 0 FROM t3 +--let $assert_text= Table t3 should have zero rows. +--source include/assert.inc +} +if ($slave_run_triggers_for_rbr == 'YES') { +--echo # +--echo # Count must be 2 +--echo # +--let $assert_cond= COUNT(*) = 2 FROM t3 +--let $assert_text= Table t3 should have two rows. +--source include/assert.inc +} + +--connection master +DELETE FROM t1 WHERE id=2; +--echo # t1: Should have one row +SELECT * FROM t1; +--echo # t2: Should have two rows +SELECT * FROM t2; +--sync_slave_with_master +--echo # t1: Should have one row +SELECT * FROM t1; +--echo # t2: Should have one row on slave due to ON DELETE CASCASE +SELECT * FROM t2; +SET GLOBAL slave_run_triggers_for_rbr= @save_slave_run_triggers_for_rbr; + +--connection master +DROP TABLE t3,t2,t1; +--sync_slave_with_master +#END OF +PROCEDURE + +--echo # +--echo # Test case: Triggers and Foreign Keys are present only on slave and +--echo # 'slave_run_triggers_for_rbr=NO' +--echo # +--let $slave_run_triggers_for_rbr=NO +--source $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc + +--echo # +--echo # Test case: Triggers are Foreign Keys are present only on slave and +--echo # 'slave_run_triggers_for_rbr=YES' +--echo # +--let $slave_run_triggers_for_rbr=YES +--source $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc +--remove_file $MYSQLTEST_VARDIR/tmp/trig_fk_on_slave.inc + +--source include/rpl_end.inc diff --git a/sql/log_event.cc b/sql/log_event.cc index c649e1f64fa..10aa0afbad8 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -10718,7 +10718,7 @@ int Rows_log_event::do_add_row_data(uchar *row_data, size_t length) There was the same problem with MERGE MYISAM tables and so here we try to go the same way. */ -static void restore_empty_query_table_list(LEX *lex) +inline void restore_empty_query_table_list(LEX *lex) { if (lex->first_not_own_table()) (*lex->first_not_own_table()->prev_global)= NULL; @@ -10733,6 +10733,8 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) TABLE* table; DBUG_ENTER("Rows_log_event::do_apply_event(Relay_log_info*)"); int error= 0; + LEX *lex= thd->lex; + uint8 new_trg_event_map= get_trg_event_map(); /* If m_table_id == ~0ULL, then we have a dummy event that does not contain any data. In that case, we just remove all tables in the @@ -10823,27 +10825,25 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) DBUG_ASSERT(!debug_sync_set_action(thd, STRING_WITH_LEN(action))); };); - if (slave_run_triggers_for_rbr) - { - LEX *lex= thd->lex; - uint8 new_trg_event_map= get_trg_event_map(); - - /* - Trigger's procedures work with global table list. So we have to add - rgi->tables_to_lock content there to get trigger's in the list. + /* + Trigger's procedures work with global table list. So we have to add + rgi->tables_to_lock content there to get trigger's in the list. - Then restore_empty_query_table_list() restore the list as it was - */ - DBUG_ASSERT(lex->query_tables == NULL); - if ((lex->query_tables= rgi->tables_to_lock)) - rgi->tables_to_lock->prev_global= &lex->query_tables; + Then restore_empty_query_table_list() restore the list as it was + */ + DBUG_ASSERT(lex->query_tables == NULL); + if ((lex->query_tables= rgi->tables_to_lock)) + rgi->tables_to_lock->prev_global= &lex->query_tables; - for (TABLE_LIST *tables= rgi->tables_to_lock; tables; - tables= tables->next_global) - { + for (TABLE_LIST *tables= rgi->tables_to_lock; tables; + tables= tables->next_global) + { + if (slave_run_triggers_for_rbr) tables->trg_event_map= new_trg_event_map; - lex->query_tables_last= &tables->next_global; - } + else + tables->slave_fk_event_map= new_trg_event_map; + + lex->query_tables_last= &tables->next_global; } if (open_and_lock_tables(thd, rgi->tables_to_lock, FALSE, 0)) { @@ -11193,8 +11193,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) } /* remove trigger's tables */ - if (slave_run_triggers_for_rbr) - restore_empty_query_table_list(thd->lex); + restore_empty_query_table_list(thd->lex); #if defined(WITH_WSREP) && defined(HAVE_QUERY_CACHE) if (WSREP(thd) && thd->wsrep_exec_mode == REPL_RECV) @@ -11212,8 +11211,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) DBUG_RETURN(error); err: - if (slave_run_triggers_for_rbr) - restore_empty_query_table_list(thd->lex); + restore_empty_query_table_list(thd->lex); rgi->slave_close_thread_tables(thd); DBUG_RETURN(error); } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 8e57ea437b6..a7cddbca16a 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4371,7 +4371,8 @@ handle_table(THD *thd, Query_tables_list *prelocking_ctx, /* We rely on a caller to check that table is going to be changed. */ DBUG_ASSERT(table_list->lock_type >= TL_WRITE_ALLOW_WRITE); - if (table_list->trg_event_map) + if (table_list->trg_event_map || + (thd->rgi_slave && thd->is_current_stmt_binlog_format_row())) { if (table_list->table->triggers) { @@ -4405,7 +4406,8 @@ handle_table(THD *thd, Query_tables_list *prelocking_ctx, { // FK_OPTION_RESTRICT and FK_OPTION_NO_ACTION only need read access static bool can_write[]= { true, false, true, true, false, true }; - uint8 op= table_list->trg_event_map; + uint8 op= ((table_list->trg_event_map) ? table_list->trg_event_map : + table_list->slave_fk_event_map); thr_lock_type lock_type; if ((op & (1 << TRG_EVENT_DELETE) && can_write[fk->delete_method]) diff --git a/sql/table.h b/sql/table.h index 9a864f7ce9f..57706655d9b 100644 --- a/sql/table.h +++ b/sql/table.h @@ -2277,8 +2277,12 @@ struct TABLE_LIST Indicates what triggers we need to pre-load for this TABLE_LIST when opening an associated TABLE. This is filled after the parsed tree is created. + + slave_fk_event_map is filled on the slave side with bitmaps value + representing row-based event operation to help find and prelock + possible FK constrain-related child tables. */ - uint8 trg_event_map; + uint8 trg_event_map, slave_fk_event_map; /* TRUE <=> this table is a const one and was optimized away. */ bool optimized_away;

1 0

Re: [Maria-developers] [Commits] cdc305c8dd8: MDEV-19620: Changing join_buffer_size causes different results
by Sergey Petrunia 29 Dec '20

29 Dec '20

Hi Varun, Please find input below. On Mon, Dec 28, 2020 at 02:34:40PM +0530, varun wrote: > revision-id: cdc305c8dd89a726e09e5fe70ff890d06609cbfb (mariadb-10.3.21-309-gcdc305c8dd8) > parent(s): 043bd85a574a88856ab9c6d497e682ed06fe45e9 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2020-12-28 14:12:14 +0530 > message: > > MDEV-19620: Changing join_buffer_size causes different results > > The scenario here is that query refinement phase decides to use a hash join. > When the join buffers are allocated in the JOIN::init_join_caches, for a table > the size exceeds the value for join_buffer_space_limit (which is the limit of the > space available for all join buffers). When this happens then we disallow join > buffering for the table, this is done in the revise_cache_usage and set_join_cache_denial. > > In this issue the hash key is created on an index for which ref access is possible, so > when we disallow hash join then instead of switching to REF access we switch to a table > scan. This is a problem because the equijoin conditions for which a lookup can be made > are not attached to the table(or are not evaluated for the table). This leads to incorrect > results. > > The fix here would be to switch to using a lookup because it was picked by the join planner > to be more efficient than the table scan. > > --- > mysql-test/main/join_cache.result | 138 ++++++++++++++++++++++++++++++++++++++ > mysql-test/main/join_cache.test | 105 +++++++++++++++++++++++++++++ > sql/sql_select.cc | 69 +++++++++++++++---- > sql/sql_select.h | 6 ++ > 4 files changed, 304 insertions(+), 14 deletions(-) > > diff --git a/mysql-test/main/join_cache.result b/mysql-test/main/join_cache.result > index 3d1d91df997..e58503f422f 100644 > --- a/mysql-test/main/join_cache.result > +++ b/mysql-test/main/join_cache.result > @@ -6128,4 +6128,142 @@ EXPLAIN > } > } > drop table t1,t2,t3; > +# > +# MDEV-19620: Changing join_buffer_size causes different results > +# > +SET @save_join_cache_level= @@join_cache_level; > +SET @save_join_buffer_size= @@join_buffer_size; > +SET @save_join_buffer_space_limit= @@join_buffer_space_limit; > +SET join_cache_level = 3; > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)) ENGINE=MyISAM; > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT PRIMARY KEY, i3 VARCHAR(300), c3 VARCHAR(500)) ENGINE=MyISAM; > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','p'),(3,'1','q'); > +INSERT INTO t2 VALUES (4,'7','g'),(5,'4','p'),(6,'1','q'); > +INSERT INTO t2 VALUES (16,'7','g'),(17,'4','p'),(28,'1','q'); > +# > +# Hash join + table Scan on t2 > +# > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 ALL NULL NULL NULL NULL 9 Using where > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +i2 c2 pk3 i3 c3 > +1 v NULL NULL NULL > +7 s NULL NULL NULL > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 hash_ALL NULL #hash#$hj 503 test.t1.c2 9 Using where; Using join buffer (flat, BNLH join) > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +i2 c2 pk3 i3 c3 > +1 v NULL NULL NULL > +7 s NULL NULL NULL > +# > +# HASH join + ref access on t2 > +# > +ALTER TABLE t2 ADD KEY k1(c3); > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 ref k1 k1 503 test.t1.c2 2 Using where > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +i2 c2 pk3 i3 c3 > +1 v NULL NULL NULL > +7 s NULL NULL NULL > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 hash_ALL k1 #hash#k1 503 test.t1.c2 9 Using where; Using join buffer (flat, BNLH join) > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +i2 c2 pk3 i3 c3 > +1 v NULL NULL NULL > +7 s NULL NULL NULL > +# > +# Hash join + index scan on t2 > +# > +ALTER TABLE t2 DROP KEY k1; > +ALTER TABLE t2 ADD KEY k1(i3,c3); > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 index NULL k1 806 NULL 9 Using where; Using index > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +c3 i3 i2 c2 > +NULL NULL 1 v > +NULL NULL 7 s > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 hash_index NULL #hash#$hj:k1 503:806 test.t1.c2 9 Using where; Using index; Using join buffer (flat, BNLH join) > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +c3 i3 i2 c2 > +NULL NULL 1 v > +NULL NULL 7 s > +DROP TABLE t1,t2; > +# > +# Hash join + range scan on t2 > +# > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)); > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT PRIMARY KEY, i3 VARCHAR(300), c3 VARCHAR(500), INDEX(i3,c3)); > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','s'),(3,'1','q'); > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 Using where > +1 SIMPLE t2 range i3 i3 303 NULL 2 Using index condition; Using where > +SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +i2 c2 pk3 i3 c3 > +7 s 2 4 s > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 Using where > +1 SIMPLE t2 hash_range i3 #hash#$hj:i3 503:303 test.t1.c2 2 Using where; Using join buffer (flat, BNLH join) > +SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +i2 c2 pk3 i3 c3 > +7 s 2 4 s > +DROP TABLE t1,t2; > +# > +# Hash join + eq ref access on t2 > +# > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)); > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT, i3 VARCHAR(300), c3 VARCHAR(500) PRIMARY KEY); > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','s'),(3,'1','q'); > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 eq_ref PRIMARY PRIMARY 502 test.t1.c2 1 Using where > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +c3 i3 i2 c2 > +NULL NULL 1 v > +s 4 7 s > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t2 hash_ALL PRIMARY #hash#PRIMARY 502 test.t1.c2 3 Using where; Using join buffer (flat, BNLH join) > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +c3 i3 i2 c2 > +NULL NULL 1 v > +s 4 7 s > +DROP TABLE t1,t2; > +SET @save_join_cache_level= @@join_cache_level; > +SET @save_join_buffer_size= @@join_buffer_size; > +SET @save_join_buffer_space_limit= @@join_buffer_space_limit; > set @@optimizer_switch=@save_optimizer_switch; > diff --git a/mysql-test/main/join_cache.test b/mysql-test/main/join_cache.test > index 91339c2cb21..6670c62516b 100644 > --- a/mysql-test/main/join_cache.test > +++ b/mysql-test/main/join_cache.test > @@ -4054,5 +4054,110 @@ where > > drop table t1,t2,t3; > > +--echo # > +--echo # MDEV-19620: Changing join_buffer_size causes different results > +--echo # > + > +SET @save_join_cache_level= @@join_cache_level; > +SET @save_join_buffer_size= @@join_buffer_size; > +SET @save_join_buffer_space_limit= @@join_buffer_space_limit; > +SET join_cache_level = 3; > + > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)) ENGINE=MyISAM; > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT PRIMARY KEY, i3 VARCHAR(300), c3 VARCHAR(500)) ENGINE=MyISAM; > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','p'),(3,'1','q'); > +INSERT INTO t2 VALUES (4,'7','g'),(5,'4','p'),(6,'1','q'); > +INSERT INTO t2 VALUES (16,'7','g'),(17,'4','p'),(28,'1','q'); > + > +--echo # > +--echo # Hash join + table Scan on t2 > +--echo # > + > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > + > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > + > +--echo # > +--echo # HASH join + ref access on t2 > +--echo # > + > +ALTER TABLE t2 ADD KEY k1(c3); > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > + > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > +SELECT * FROM t1 LEFT JOIN t2 ON (c3=c2); > + > +--echo # > +--echo # Hash join + index scan on t2 > +--echo # > +ALTER TABLE t2 DROP KEY k1; > +ALTER TABLE t2 ADD KEY k1(i3,c3); > + > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > + > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > + > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +DROP TABLE t1,t2; > + > +--echo # > +--echo # Hash join + range scan on t2 > +--echo # > + > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)); > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT PRIMARY KEY, i3 VARCHAR(300), c3 VARCHAR(500), INDEX(i3,c3)); > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','s'),(3,'1','q'); > + > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > + > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > + > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > +SELECT * FROM t1 LEFT JOIN t2 ON (c3 = c2) WHERE i3 >= '4'; > + > +DROP TABLE t1,t2; > + > +--echo # > +--echo # Hash join + eq ref access on t2 > +--echo # > + > +CREATE TABLE t1 (i2 VARCHAR(500), c2 VARCHAR(500)); > +INSERT INTO t1 VALUES ('1','v'),('7','s'); > +CREATE TABLE t2 (pk3 INT, i3 VARCHAR(300), c3 VARCHAR(500) PRIMARY KEY); > +INSERT INTO t2 VALUES (1,'7','g'),(2,'4','s'),(3,'1','q'); > + > +set join_buffer_size=1024; > +set join_buffer_space_limit=2048; > + > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > + > +set join_buffer_space_limit=262144; > +EXPLAIN SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +SELECT c3,i3, i2,c2 FROM t1 LEFT JOIN t2 ON (c3 = c2); > +DROP TABLE t1,t2; > + > +SET @save_join_cache_level= @@join_cache_level; > +SET @save_join_buffer_size= @@join_buffer_size; > +SET @save_join_buffer_space_limit= @@join_buffer_space_limit; > + > # The following command must be the last one in the file > set @@optimizer_switch=@save_optimizer_switch; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 203422f0b43..8b7deb98dc6 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -111,6 +111,7 @@ static bool best_extension_by_limited_search(JOIN *join, > uint prune_level, > uint use_cond_selectivity); > static uint determine_search_depth(JOIN* join); > +static void pick_table_access_method(JOIN_TAB *tab); > C_MODE_START > static int join_tab_cmp(const void *dummy, const void* ptr1, const void* ptr2); > static int join_tab_cmp_straight(const void *dummy, const void* ptr1, const void* ptr2); > @@ -10081,6 +10082,7 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, > j->ref.disable_cache= FALSE; > j->ref.null_ref_part= NO_REF_PART; > j->ref.const_ref_part_map= 0; > + j->ref.not_null_keyparts= 0; > keyuse=org_keyuse; > > store_key **ref_key= j->ref.key_copy; > @@ -10173,23 +10175,12 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, > } > } /* not ftkey */ > *ref_key=0; // end_marker > + j->ref.not_null_keyparts= not_null_keyparts; > if (j->type == JT_FT) > DBUG_RETURN(0); > - ulong key_flags= j->table->actual_key_flags(keyinfo); > if (j->type == JT_CONST) > j->table->const_table= 1; > - else if (!((keyparts == keyinfo->user_defined_key_parts && > - ( > - (key_flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME || > - /* Unique key and all keyparts are NULL rejecting */ > - ((key_flags & HA_NOSAME) && keyparts == not_null_keyparts) > - )) || > - /* true only for extended keys */ > - (keyparts > keyinfo->user_defined_key_parts && > - MY_TEST(key_flags & HA_EXT_NOSAME) && > - keyparts == keyinfo->ext_key_parts) > - ) || > - null_ref_key) > + else if (!j->is_eq_ref_access()|| null_ref_key) > { > /* Must read with repeat */ > j->type= null_ref_key ? JT_REF_OR_NULL : JT_REF; > @@ -11582,11 +11573,25 @@ void set_join_cache_denial(JOIN_TAB *join_tab) > don't do join buffering for the first table in sjm nest. > */ > join_tab[-1].next_select= sub_select; > - if (join_tab->type == JT_REF && join_tab->is_ref_for_hash_join()) > + if ((join_tab->type == JT_REF || join_tab->type == JT_HASH) && > + join_tab->is_ref_for_hash_join()) > { > join_tab->type= JT_ALL; > join_tab->ref.key_parts= 0; Do we ever get here if join_tab->type == JT_REF? I think we don't. If this is so, please remove it. > } > + > + if (join_tab->type == JT_HASH && !join_tab->is_ref_for_hash_join()) > + { > + join_tab->type= join_tab->is_eq_ref_access() ? JT_EQ_REF : JT_REF; > + pick_table_access_method(join_tab); > + } Also please add a comment explaining that the join optimizer's choice was ref access and we fall back to that. > + > + if (join_tab->type == JT_HASH_NEXT) > + { > + join_tab->type = JT_NEXT; > + DBUG_ASSERT(join_tab->ref.key_parts == 0); > + } > + > join_tab->join->return_tab= join_tab; > } > } > @@ -27954,6 +27959,42 @@ void JOIN_TAB::partial_cleanup() > } > > > +/* > + @brief > + Check if the access method for the table is EQ_REF access or not > + > + @retval > + TRUE EQ_REF access > + FALSE Otherwise > +*/ > +bool JOIN_TAB::is_eq_ref_access() > +{ > + > + KEY *keyinfo; > + if (!is_hash_join_key_no(ref.key)) > + keyinfo= table->key_info + ref.key; > + else > + keyinfo= hj_key; This code now looks as if it was possible that hash join is used and also eq_ref is used. Is this really possible? I've tried catching an example of this with MTR and I wasn't able to do so. Maybe we should just return false if hash join is used and also a comment about this? > + > + uint keyparts= ref.key_parts; > + ulong key_flags= table->actual_key_flags(keyinfo); > + if ( (keyparts == keyinfo->user_defined_key_parts && > + ( > + (key_flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME || > + /* Unique key and all keyparts are NULL rejecting */ > + ((key_flags & HA_NOSAME) && keyparts == ref.not_null_keyparts) > + ) > + ) || > + /* true only for extended keys */ > + (keyparts > keyinfo->user_defined_key_parts && > + MY_TEST(key_flags & HA_EXT_NOSAME) && > + keyparts == keyinfo->ext_key_parts) > + ) > + return true; > + return false; > +} > + > + > /** > @} (end of group Query_Optimizer) > */ BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

Re: [Maria-developers] 33787ea47a3: MDEV-6899: Database Dump has Syntax Error When Importing
by Sergei Golubchik 23 Dec '20

23 Dec '20

Hi, Anel! On Dec 23, Anel Husakovic wrote: > revision-id: 33787ea47a3 (mariadb-10.2.31-588-g33787ea47a3) > parent(s): ceef26cf866 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-11-17 15:11:14 +0100 > message: > > MDEV-6899: Database Dump has Syntax Error When Importing > > --- > mysql-test/r/mysqldump.result | 32 ++++++++++++++++++++++++++++++++ > mysql-test/t/mysqldump.test | 26 ++++++++++++++++++++++++++ > sql/events.cc | 10 ++++++++-- > sql/sql_show.cc | 4 +++- > 4 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/sql/events.cc b/sql/events.cc > index abac2833833..2880113bc75 100644 > --- a/sql/events.cc > +++ b/sql/events.cc > @@ -680,6 +680,12 @@ send_show_create_event(THD *thd, Event_timed *et, Protocol *protocol) > if (et->get_create_event(thd, &show_str)) > DBUG_RETURN(TRUE); > > + LEX_STRING def_show_str= show_str.lex_string(); > + if (!strcmp(&def_show_str.str[def_show_str.length -1], ";")) > + { > + --def_show_str.length; > + def_show_str.str[def_show_str.length]='\0'; > + } No, this is clearly wrong. The bug is that semicolon even gets into the event definition, see other commands in the bug report. You should fix that, not strip it away in SHOW CREATE, which will only hide the bug. Why is the semicolon parsed as a part of the event anyway? > field_list.push_back(new (mem_root) > Item_empty_string(thd, "Event", NAME_CHAR_LEN), > mem_root); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ab7092a415f: MDEV-24179: Assertion `m_status == DA_ERROR || m_status == DA_OK ||
by Sergei Golubchik 21 Dec '20

21 Dec '20

Hi, Rucha! On Dec 21, Rucha Deodhar wrote: > revision-id: ab7092a415f (mariadb-10.2.31-584-gab7092a415f) > parent(s): 9b30212f15e > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-12-18 10:20:20 +0530 > message: > > MDEV-24179: Assertion `m_status == DA_ERROR || m_status == DA_OK || > m_status == DA_OK_BULK' failed in Diagnostics_area::message() > > Analysis: Assertion failure happens because we reach the maximum limit to > examine rows. > Fix: Return the error state. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 5544c765775..73b7a84fae1 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -5046,6 +5046,12 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > continue; > } > > + if (thd->killed == ABORT_QUERY) > + { > + error= 0; > + goto err; > + } > + you already have exactly the same if() few lines below. may be it shouldn't be duplicated, but moved down to the err: label? > DEBUG_SYNC(thd, "before_open_in_get_all_tables"); > if (fill_schema_table_by_open(thd, FALSE, > table, schema_table, Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 889f90365ef: MDEV-23536 : Race condition between KILL and transaction commit
by Sergei Golubchik 20 Dec '20

20 Dec '20

Hi, Jan! On Dec 19, Jan LindstrÃ¶m wrote: > revision-id: 889f90365ef (mariadb-10.2.31-623-g889f90365ef) > parent(s): 74223c33d1a > author: Jan Lindström <jan.lindstrom(a)mariadb.com> > committer: Jan Lindström <jan.lindstrom(a)mariadb.com> > timestamp: 2020-12-18 16:10:39 +0200 > message: > > MDEV-23536 : Race condition between KILL and transaction commit ... > If you look carefully into the above, you can conclude that > thd->free_connection() can be called concurrently with > KILL/thd->awake(). Which is the bug. And it is partially fixed in > THD::~THD(), that is destructor waits for KILL completion: > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7eaafbd9044..46aeb4f2bb3 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1437,6 +1437,31 @@ void THD::cleanup(void) > void THD::free_connection() > { > DBUG_ASSERT(free_connection_done == 0); > +#ifdef WITH_WSREP > + /* > + wsrep_rgi should be protected with LOCK_thd_data mutex and > + mutexing ordering rules state that LOCK_thd_data must be taken > + before LOCK_thd_kill. > + */ > + if (wsrep_rgi && WSREP(this)) mysql_mutex_lock(&LOCK_thd_data); > +#endif > + /* > + Other threads may have a lock on LOCK_thd_kill to ensure that this > + THD is not deleted while they access it. The following mutex_lock > + ensures that no one else is using this THD and it's now safe to > + continue. > + */ > + mysql_mutex_lock(&LOCK_thd_kill); > + mysql_mutex_unlock(&LOCK_thd_kill); Note that THD destructor starts from `assert_not_linked()` - it's important, because if the THD is linked in the list of threads, then someone might start using it after you released LOCK_thd_kill. With the new logic you need to move assert_not_linked to free_connection(). otherwise ok to push > + > +#ifdef WITH_WSREP > + if (rpl_group_info *rgi= wsrep_rgi) > + { > + wsrep_rgi= 0; > + if (WSREP(this)) mysql_mutex_unlock(&LOCK_thd_data); > + delete rgi; > + } > +#endif > my_free(db); > db= NULL; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 4

Re: [Maria-developers] a36f27e200e: Fixes that enables my_new.cc (new wrapper using my_malloc)
by Sergei Golubchik 11 Dec '20

11 Dec '20

Hi, Michael! On Dec 11, Michael Widenius wrote: > revision-id: a36f27e200e (mariadb-10.5.2-275-ga36f27e200e) > parent(s): 9f2bbf84e8d > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Fixes that enables my_new.cc (new wrapper using my_malloc) > > This is not enabled by default as there are leaks in the > server that needs to be fixed first. One can compile > with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the > memory leaks from 'new' The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant. You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with -DHAVE_CXX_NEW=0 this works for me in 10.5 and fails to compile my_new.cc, as it should. Please, only keed your my_new.cc changes and say in the commit message "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'" As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it automatically? Like #ifdef USE_MYSYS_NEW #define SF_REMEMBER_FRAMES 14 #else #define SF_REMEMBER_FRAMES 8 #endif or may be even just always make it 14. It should be easy to enable this feature, right? > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 80fac4b2ccc..f97a6354f19 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -362,10 +362,8 @@ ENDIF() > # Run platform tests > INCLUDE(configure.cmake) > > -# force -DUSE_MYSYS_NEW unless already done by HAVE_CXX_NEW > -IF(NOT HAVE_CXX_NEW) > - ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > -ENDIF() > +# Always use USE_MYSYS_NEW > +ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > > # Find header files from the bundled libraries > # (wolfssl, readline, pcre2, etc) > diff --git a/include/my_global.h b/include/my_global.h > index 86ef5f882f6..8862a397f73 100644 > --- a/include/my_global.h > +++ b/include/my_global.h > @@ -527,6 +527,9 @@ typedef unsigned short ushort; > duplicate declaration of __cxa_pure_virtual, solved by declaring it a > weak symbol. > */ > +#ifndef REALLY_USE_MYSYS_NEW > +#undef USE_MYSYS_NEW > +#endif > #if defined(USE_MYSYS_NEW) && ! defined(DONT_DECLARE_CXA_PURE_VIRTUAL) > C_MODE_START > int __cxa_pure_virtual () __attribute__ ((weak)); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9f2bbf84e8d1: Fixed my_addr_resolve
by Sergei Golubchik 10 Dec '20

10 Dec '20

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 9f2bbf84e8d1 (mariadb-10.5.2-274-g9f2bbf84e8d1) > parent(s): 173116c3a573 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Fixed my_addr_resolve > > At least on openSUSE Leap 15.1 one should not use the dli_fbase offset > provided by dladdr() for addresses in the main program. > Without this patch the stack traces from sf_report_leaked_memory() > are not usable. Sorry, I was not able to repeat it. I built on openSUSE Leap and the stack trace was perfectly usable without your patch. I tried both `kill -ABRT` and an artificially introduced memory leak. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 53a88b2b7032: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
by Sergei Golubchik 04 Dec '20

04 Dec '20

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 53a88b2b7032 (mariadb-10.5.2-272-g53a88b2b7032) > parent(s): 0aab153e05be > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc() > > The proble was that hen using String::alloc() to allocate a string, "hen" ? after a minute of staring I realized that you probably meant "when". (and "problem") > the String ensures that there is space for an extra NULL byte in the > buffer and if not, reallocates the string. This is a problem with the > String::set_int() that calls alloc(21), which forces > an extra malloc/free to happen. > > - We do not anymore re-allocate String if alloc() is called with the > Allocated_length. This reduces number of malloc() allocations, > especially one big re-allocation in Protocol::send_result_Set_metadata() > for almost every query that produced a result to the connnected client. > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE > This can now be done as alloc() doesn't increase buffers if new length is > not bigger than old one. > - c_ptr() is redesigned to be safer (but a bit longer) than before. > - Remove wrong usage of c_ptr_quick() > c_ptr_quick() where used in many cases to get the pointer to the used > buffer, even when it didn't need to be \0 terminated. In this case > ptr() is a better substitute. > Another problem with c_ptr_quick() is that it didn't guarantee that > the string would be \0 terminated. > - item_val_str(), an API function not used currently by the server, > now always returns a null terminated string (before it didn't always > do that). > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old > mixed usage of performance keys caused assert's when String buffers > where shrunk. > - Binary_string::shrink() is simplifed > > diff --git a/sql/item.cc b/sql/item.cc > index 4a26cb6c140c..328fe96018a5 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -4574,7 +4574,7 @@ const String *Item_param::value_query_val_str(THD *thd, String *str) const > break; > } > DBUG_ASSERT(str->length() <= typelen); > - buf= str->c_ptr_quick(); > + buf= (char*) str->ptr(); > ptr= buf + str->length(); > *ptr++= '\''; > ptr+= (uint) my_TIME_to_str(&value.time, ptr, decimals); this is just wrong. One cannot simply write data at the String's end pointer. One must alloc() the space first. > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 8ad14ca260c4..ce72a7af051d 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -126,7 +126,7 @@ partition_info *partition_info::get_clone(THD *thd) > /** > Mark named [sub]partition to be used/locked. > > - @param part_name Partition name to match. > + @param part_name Partition name to match. Must be \0 terminated! > @param length Partition name length. > > @return Success if partition found > @@ -172,9 +172,9 @@ bool partition_info::add_named_partition(const char *part_name, size_t length) > else > bitmap_set_bit(&read_partitions, part_def->part_id); > } > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s", > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s", > part_def->part_id, part_def->is_subpart, > - part_name)); > + length, part_name)); why did you specify a length if the name "must be \0 terminated!" ? (and I agree that it must be) > DBUG_RETURN(false); > } > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 633d969b3dec..2b603380aa6c 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -404,12 +404,16 @@ static int item_value_type(struct st_mysql_value *value) > static const char *item_val_str(struct st_mysql_value *value, > char *buffer, int *length) > { > + size_t org_length= *length; > - String str(buffer, *length, system_charset_info), *res; > + String str(buffer, org_length, system_charset_info), *res; > if (!(res= ((st_item_value_holder*)value)->item->val_str(&str))) > return NULL; > *length= res->length(); > - if (res->c_ptr_quick() == buffer) > + if (res->ptr() == buffer && res->length() < org_length) > + { > + buffer[res->length()]= 0; > return buffer; > + } Okay. This looked like the *only* correct usage of c_ptr_quick(). Agree, better to remove c_ptr_quick(). > > /* > Lets be nice and create a temporary string since the > diff --git a/sql/sql_string.cc b/sql/sql_string.cc > index e2defba434dd..8ab156d0e556 100644 > --- a/sql/sql_string.cc > +++ b/sql/sql_string.cc > @@ -1251,21 +1259,16 @@ bool String::append_semi_hex(const char *s, uint len, CHARSET_INFO *cs) > // Shrink the buffer, but only if it is allocated on the heap. > void Binary_string::shrink(size_t arg_length) > { > - if (!is_alloced()) > - return; > - if (ALIGN_SIZE(arg_length + 1) < Alloced_length) > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length) > + { > + char *new_ptr; > + /* my_realloc() can't fail as new buffer is less than the original one */ > + if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, > + MYF(thread_specific ? > + MY_THREAD_SPECIFIC : 0)))) why do you need an if() if "my_realloc() can't fail" ? (and it really cannot, you're right) > { > - char* new_ptr; > - if (!(new_ptr = (char*)my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, > - MYF(thread_specific ? MY_THREAD_SPECIFIC : 0)))) > - { > - Alloced_length = 0; > - real_alloc(arg_length); > - } > - else > - { > - Ptr = new_ptr; > - Alloced_length = (uint32)arg_length; > - } > + Ptr= new_ptr; > + Alloced_length= (uint32) arg_length; > } > + } > } > diff --git a/sql/sql_string.h b/sql/sql_string.h > index 2ef817ea0adc..01d5211fe850 100644 > --- a/sql/sql_string.h > +++ b/sql/sql_string.h > @@ -599,25 +599,34 @@ class Binary_string: public Static_binary_string > > inline char *c_ptr() > { > - DBUG_ASSERT(!alloced || !Ptr || !Alloced_length || > - (Alloced_length >= (str_length + 1))); > - > - if (!Ptr || Ptr[str_length]) // Should be safe > - (void) realloc(str_length); > + if (unlikely(!Ptr)) > + return (char*) ""; > + /* > + Here we assume that any buffer used to initalize String has > + an end \0 or have at least an accessable character at end. > + This is to handle the case of String("Hello",5) efficently. > + */ > + if (unlikely(!alloced && !Ptr[str_length])) No, this is wrong. Note the difference between String a("Hello", 5) and char hello[5]; String a(buf, 5); Your assumption should only apply to the first case, not to the second. In the first case alloced==Alloced_length==0, in the second case only alloced==0 and Alloced_length==5. So in the if() above you need to look at Alloced_length. > + return Ptr; > + if (str_length < Alloced_length) > + { > + Ptr[str_length]=0; > + return Ptr; > + } > + (void) realloc(str_length+1); /* This will add end \0 */ > return Ptr; > } > + /* Use c_ptr() instead. This will be deleted soon, kept for compatiblity */ > inline char *c_ptr_quick() > { > - if (Ptr && str_length < Alloced_length) > - Ptr[str_length]=0; > - return Ptr; > + return c_ptr_safe(); > } > inline char *c_ptr_safe() > { > if (Ptr && str_length < Alloced_length) > Ptr[str_length]=0; > else > - (void) realloc(str_length); > + (void) realloc(str_length + 1); what's the difference between c_ptr_safe and c_ptr, again? they're very similar, so they'd need a good comment explaining when to use what. > return Ptr; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0