From 852e4510fa662c571a42f550278d4abd09e3c5cf Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Wed, 23 Jul 2025 09:34:47 +0700 Subject: [PATCH 01/14] MDEV-26115: Crash when calling stored function in FOR loop argument On handling SP statement `FOR IN lower_bound..func() DO` the instruction sp_instr_set is allocated on sp_head's memory root, whereas an instance of the class Item_func_sp pointed by the data member sp_instr_set::sp_result_field is allocated on runtime memory root. In result, on finishing the first execution of a stored routine the memory allocated for the instance of the class Item_func_sp is released whereas the pointer sp_instr_set::sp_result_field still references the deleted memory. Next time the same stored routine is run dereferencing deallocated memory results in abnormal server termination. To fix the issue, allocate an instance of the class Item_func_sp on sp_head memory root. Do this allocation only once, meaning the Item_func_sp::cleanup doesn't do deletion an instance of the class Item_func_sp and nullifying the data member sp_instr_set::sp_result_field. --- mysql-test/main/sp-bugs2.result | 35 ++++++++++++++++++++++++++++++++ mysql-test/main/sp-bugs2.test | 30 +++++++++++++++++++++++++++ sql/item.cc | 36 ++++++++++++++++----------------- sql/item.h | 5 +++++ sql/item_func.cc | 17 ++++++++++++++++ sql/item_sum.cc | 12 +++++++++-- 6 files changed, 115 insertions(+), 20 deletions(-) diff --git a/mysql-test/main/sp-bugs2.result b/mysql-test/main/sp-bugs2.result index 0a8df79a903..75826c29221 100644 --- a/mysql-test/main/sp-bugs2.result +++ b/mysql-test/main/sp-bugs2.result @@ -59,4 +59,39 @@ call p2('2'); drop table t1; drop procedure p1; drop procedure p2; +# +# MDEV-26115: Crash when calling stored function in FOR loop argument +# +CREATE OR REPLACE FUNCTION cnt() +RETURNS INTEGER NO SQL +BEGIN +RETURN 3; +END; +$ +CREATE OR REPLACE PROCEDURE p1() +NO SQL +BEGIN +DECLARE i INTEGER; +FOR i IN 1..cnt() DO +SELECT 1; +END FOR; +END; +$ +CALL p1(); +1 +1 +1 +1 +1 +1 +CALL p1(); +1 +1 +1 +1 +1 +1 +# Clean up +DROP FUNCTION cnt; +DROP PROCEDURE p1; # End of 10.11 tests diff --git a/mysql-test/main/sp-bugs2.test b/mysql-test/main/sp-bugs2.test index 01c6106c0ca..6b841626aa8 100644 --- a/mysql-test/main/sp-bugs2.test +++ b/mysql-test/main/sp-bugs2.test @@ -67,4 +67,34 @@ drop table t1; drop procedure p1; drop procedure p2; +--echo # +--echo # MDEV-26115: Crash when calling stored function in FOR loop argument +--echo # +--delimiter $ +CREATE OR REPLACE FUNCTION cnt() +RETURNS INTEGER NO SQL +BEGIN + RETURN 3; +END; +$ + +CREATE OR REPLACE PROCEDURE p1() +NO SQL +BEGIN + DECLARE i INTEGER; + FOR i IN 1..cnt() DO + SELECT 1; + END FOR; +END; +$ + +--delimiter ; + +CALL p1(); +CALL p1(); + +--echo # Clean up +DROP FUNCTION cnt; +DROP PROCEDURE p1; + --echo # End of 10.11 tests diff --git a/sql/item.cc b/sql/item.cc index 5d36f37e6d9..89b6a235f71 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -2898,8 +2898,6 @@ Item_sp::func_name_cstring(THD *thd, bool is_package_function) const void Item_sp::cleanup() { - delete sp_result_field; - sp_result_field= NULL; m_sp= NULL; delete func_ctx; func_ctx= NULL; @@ -3071,7 +3069,6 @@ Item_sp::init_result_field(THD *thd, uint max_length, uint maybe_null, DBUG_ENTER("Item_sp::init_result_field"); DBUG_ASSERT(m_sp != NULL); - DBUG_ASSERT(sp_result_field == NULL); /* A Field needs to be attached to a Table. @@ -3085,23 +3082,26 @@ Item_sp::init_result_field(THD *thd, uint max_length, uint maybe_null, dummy_table->s->table_name= empty_clex_str; dummy_table->maybe_null= maybe_null; - if (!(sp_result_field= m_sp->create_result_field(max_length, name, - dummy_table))) - DBUG_RETURN(TRUE); - - if (sp_result_field->pack_length() > sizeof(result_buf)) + if (!sp_result_field) { - void *tmp; - if (!(tmp= thd->alloc(sp_result_field->pack_length()))) - DBUG_RETURN(TRUE); - sp_result_field->move_field((uchar*) tmp); + sp_result_field= m_sp->create_result_field(max_length, name, + dummy_table); + if (!sp_result_field) + DBUG_RETURN(true); + + if (sp_result_field->pack_length() > sizeof(result_buf)) + { + void *tmp; + if (!(tmp= thd->alloc(sp_result_field->pack_length()))) + DBUG_RETURN(TRUE); + sp_result_field->move_field((uchar*) tmp); + } + else + sp_result_field->move_field(result_buf); + + sp_result_field->null_ptr= (uchar *) null_value; + sp_result_field->null_bit= 1; } - else - sp_result_field->move_field(result_buf); - - sp_result_field->null_ptr= (uchar *) null_value; - sp_result_field->null_bit= 1; - DBUG_RETURN(FALSE); } diff --git a/sql/item.h b/sql/item.h index 0938c5dda78..c71ddcc081d 100644 --- a/sql/item.h +++ b/sql/item.h @@ -5815,6 +5815,11 @@ public: Field *sp_result_field; Item_sp(THD *thd, Name_resolution_context *context_arg, sp_name *name_arg); Item_sp(THD *thd, Item_sp *item); + virtual ~Item_sp() + { + delete sp_result_field; + sp_result_field= NULL; + } LEX_CSTRING func_name_cstring(THD *thd, bool is_package_function) const; void cleanup(); bool sp_check_access(THD *thd); diff --git a/sql/item_func.cc b/sql/item_func.cc index f9fd8a72a45..6623d72214e 100644 --- a/sql/item_func.cc +++ b/sql/item_func.cc @@ -6837,12 +6837,29 @@ Item_func_sp::fix_fields(THD *thd, Item **ref) DBUG_RETURN(TRUE); } + Query_arena *arena, backup; + /* + Allocation an instance of Item_func_sp used for initialization of + sp_result_field taken place inside the method init_result_field() is done + on sp_head's mem_root since Item_sp also allocated on this memory root. + + Switching to SP/PS memory root is done explicitly before calling the method + init_result_field() instead doing that inside init_result_field() + since for the case when rollup aggregate function is handled + (@see Item_sum_sp::copy_or_same, @see JOIN::rollup_make_fields) + the runtime arena used for operations, so switching to SP/PS arena for this + case would result in assertion failure on second execution of the same + prepared statement because the memory root be already marked as read only. + */ + arena= thd->activate_stmt_arena_if_needed(&backup); /* We must call init_result_field before Item_func::fix_fields() to make m_sp and result_field members available to fix_length_and_dec(), which is called from Item_func::fix_fields(). */ res= init_result_field(thd, max_length, maybe_null(), &null_value, &name); + if (arena) + thd->restore_active_arena(arena, &backup); if (res) DBUG_RETURN(TRUE); diff --git a/sql/item_sum.cc b/sql/item_sum.cc index 91e22f68a13..7c2d5f08c03 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -1365,8 +1365,16 @@ Item_sum_sp::fix_fields(THD *thd, Item **ref) return TRUE; } - if (init_result_field(thd, max_length, maybe_null(), &null_value, &name)) - return TRUE; + Query_arena *arena, backup; + arena= thd->activate_stmt_arena_if_needed(&backup); + + bool ret= init_result_field(thd, max_length, maybe_null(), + &null_value, &name); + if (arena) + thd->restore_active_arena(arena, &backup); + + if(ret) + return true; for (uint i= 0 ; i < arg_count ; i++) { From 2147951560c013fc084c4c06dcf8e09ec403dd9f Mon Sep 17 00:00:00 2001 From: Satish kumar Date: Fri, 11 Jul 2025 17:04:12 +0530 Subject: [PATCH 02/14] MDEV-37083: Fixed type mismatch in sys views -Replaced incorrect string comparision in views for is_hashed and is_old as these columns are integer and not string Affected Views: -innodb_buffer_stats_by_schema -innodb_buffer_stats_by_table -x_innodb_buffer_stats_by_schema -x_innodb_buffer_stats_by_table --- .../suite/encryption/r/innochecksum.result | 10 +++++++++ .../suite/encryption/t/innochecksum.opt | 2 ++ .../suite/encryption/t/innochecksum.test | 10 +++++++++ .../r/v_innodb_buffer_stats_by_schema.result | 12 +++++++++-- .../r/v_innodb_buffer_stats_by_table.result | 12 +++++++++-- .../t/v_innodb_buffer_stats_by_schema.test | 21 ++++++++++++------- .../t/v_innodb_buffer_stats_by_table.test | 21 ++++++++++++------- .../i_s/innodb_buffer_stats_by_schema.sql | 5 ++--- .../i_s/innodb_buffer_stats_by_table.sql | 6 +++--- .../i_s/x_innodb_buffer_stats_by_table.sql | 2 +- 10 files changed, 74 insertions(+), 27 deletions(-) create mode 100644 mysql-test/suite/encryption/t/innochecksum.opt diff --git a/mysql-test/suite/encryption/r/innochecksum.result b/mysql-test/suite/encryption/r/innochecksum.result index 7c68164e52a..3e5c2a7e9fd 100644 --- a/mysql-test/suite/encryption/r/innochecksum.result +++ b/mysql-test/suite/encryption/r/innochecksum.result @@ -35,4 +35,14 @@ CREATE TABLE t6 (a INT AUTO_INCREMENT PRIMARY KEY, b TEXT) ENGINE=InnoDB; # Run innochecksum on t6 # Restore the original tables # restart +# Trigger AHI and test pages_hashed, pages_old as non zero from sys view +SELECT STRAIGHT_JOIN COUNT(t6a.a) +FROM t6 AS t6a, t6 AS t6b +WHERE t6a.a = t6b.a; +COUNT(t6a.a) +1000 +SELECT SUM(pages_hashed)>0 `1`, SUM(pages_old)>0 `1` +FROM sys.innodb_buffer_stats_by_table; +1 1 +1 1 DROP TABLE t1, t2, t3, t4, t5, t6; diff --git a/mysql-test/suite/encryption/t/innochecksum.opt b/mysql-test/suite/encryption/t/innochecksum.opt new file mode 100644 index 00000000000..c406b6bf1b8 --- /dev/null +++ b/mysql-test/suite/encryption/t/innochecksum.opt @@ -0,0 +1,2 @@ +--innodb-adaptive-hash-index=ON +--innodb-buffer-pool-size=64M diff --git a/mysql-test/suite/encryption/t/innochecksum.test b/mysql-test/suite/encryption/t/innochecksum.test index 516bc0733d9..a9bee0cf50f 100644 --- a/mysql-test/suite/encryption/t/innochecksum.test +++ b/mysql-test/suite/encryption/t/innochecksum.test @@ -300,4 +300,14 @@ if (0 && $have_debug) { # these messages sometimes fail to appear --move_file $MYSQLD_DATADIR/test/t6.ibd.backup $MYSQLD_DATADIR/test/t6.ibd --source include/start_mysqld.inc + +--echo # Trigger AHI and test pages_hashed, pages_old as non zero from sys view + +SELECT STRAIGHT_JOIN COUNT(t6a.a) +FROM t6 AS t6a, t6 AS t6b +WHERE t6a.a = t6b.a; + +SELECT SUM(pages_hashed)>0 `1`, SUM(pages_old)>0 `1` +FROM sys.innodb_buffer_stats_by_table; + DROP TABLE t1, t2, t3, t4, t5, t6; diff --git a/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_schema.result b/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_schema.result index fbfa41f7867..5267cf90e29 100644 --- a/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_schema.result +++ b/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_schema.result @@ -7,7 +7,11 @@ pages bigint(21) NO 0 pages_hashed bigint(21) NO 0 pages_old bigint(21) NO 0 rows_cached decimal(44,0) YES NULL -SELECT * FROM sys.innodb_buffer_stats_by_schema; +CREATE TABLE t1(a INT PRIMARY KEY) ENGINE=InnoDB; +SELECT * FROM sys.innodb_buffer_stats_by_schema +where object_schema = 'test'; +object_schema allocated data pages pages_hashed pages_old rows_cached +test 16.00 KiB 0 bytes 1 0 0 0 DESC sys.x$innodb_buffer_stats_by_schema; Field Type Null Key Default Extra object_schema text YES NULL @@ -17,4 +21,8 @@ pages bigint(21) NO 0 pages_hashed bigint(21) NO 0 pages_old bigint(21) NO 0 rows_cached decimal(44,0) NO 0 -SELECT * FROM sys.x$innodb_buffer_stats_by_schema; +SELECT * FROM sys.x$innodb_buffer_stats_by_schema +where object_schema = 'test'; +object_schema allocated data pages pages_hashed pages_old rows_cached +test 16384 0 1 0 0 0 +DROP TABLE t1; diff --git a/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_table.result b/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_table.result index 27c7d9917a4..80557863b21 100644 --- a/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_table.result +++ b/mysql-test/suite/sysschema/r/v_innodb_buffer_stats_by_table.result @@ -8,7 +8,11 @@ pages bigint(21) NO 0 pages_hashed bigint(21) NO 0 pages_old bigint(21) NO 0 rows_cached decimal(44,0) YES NULL -SELECT * FROM sys.innodb_buffer_stats_by_table; +CREATE TABLE t1(a INT PRIMARY KEY) ENGINE=InnoDB; +SELECT * FROM sys.innodb_buffer_stats_by_table +where object_schema = 'test' and object_name = 't1'; +object_schema object_name allocated data pages pages_hashed pages_old rows_cached +test t1 16.00 KiB 0 bytes 1 0 0 0 DESC sys.x$innodb_buffer_stats_by_table; Field Type Null Key Default Extra object_schema text YES NULL @@ -19,4 +23,8 @@ pages bigint(21) NO 0 pages_hashed bigint(21) NO 0 pages_old bigint(21) NO 0 rows_cached decimal(44,0) NO 0 -SELECT * FROM sys.x$innodb_buffer_stats_by_table; +SELECT * FROM sys.x$innodb_buffer_stats_by_table +where object_schema = 'test' and object_name = 't1'; +object_schema object_name allocated data pages pages_hashed pages_old rows_cached +test t1 16384 0 1 0 0 0 +DROP TABLE t1; diff --git a/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_schema.test b/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_schema.test index f44b87d43ca..a350f057593 100644 --- a/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_schema.test +++ b/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_schema.test @@ -8,16 +8,21 @@ # Ensure structure changes don't slip in DESC sys.innodb_buffer_stats_by_schema; -# Make sure view select does not error, but ignore results ---disable_result_log -SELECT * FROM sys.innodb_buffer_stats_by_schema; ---enable_result_log +# Create an Empty table +CREATE TABLE t1(a INT PRIMARY KEY) ENGINE=InnoDB; +# Make sure view select does not error, and +# is_hashed & is_old is 0 for empty table +SELECT * FROM sys.innodb_buffer_stats_by_schema +where object_schema = 'test'; # Ensure structure changes don't slip in DESC sys.x$innodb_buffer_stats_by_schema; -# Make sure view select does not error, but ignore results ---disable_result_log -SELECT * FROM sys.x$innodb_buffer_stats_by_schema; ---enable_result_log +# Make sure view select does not error, and +# is_hashed & is_old is 0 for empty table +SELECT * FROM sys.x$innodb_buffer_stats_by_schema +where object_schema = 'test'; + +# Drop the table +DROP TABLE t1; diff --git a/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_table.test b/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_table.test index d65c28ff96d..a374dccbec3 100644 --- a/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_table.test +++ b/mysql-test/suite/sysschema/t/v_innodb_buffer_stats_by_table.test @@ -8,16 +8,21 @@ # Ensure structure changes don't slip in DESC sys.innodb_buffer_stats_by_table; -# Make sure view select does not error, but ignore results ---disable_result_log -SELECT * FROM sys.innodb_buffer_stats_by_table; ---enable_result_log +# Create an Empty table +CREATE TABLE t1(a INT PRIMARY KEY) ENGINE=InnoDB; +# Make sure view select does not error and +# is_hashed & is_old is 0 for empty table +SELECT * FROM sys.innodb_buffer_stats_by_table +where object_schema = 'test' and object_name = 't1'; # Ensure structure changes don't slip in DESC sys.x$innodb_buffer_stats_by_table; -# Make sure view select does not error, but ignore results ---disable_result_log -SELECT * FROM sys.x$innodb_buffer_stats_by_table; ---enable_result_log +# Make sure view select does not error and +# is_hashed & is_old is 0 for empty table +SELECT * FROM sys.x$innodb_buffer_stats_by_table +where object_schema = 'test' and object_name = 't1'; + +# Drop the table +DROP TABLE t1; diff --git a/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_schema.sql b/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_schema.sql index 46f7cdd027c..4b4d46c4659 100644 --- a/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_schema.sql +++ b/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_schema.sql @@ -53,8 +53,8 @@ SELECT IF(LOCATE('.', ibp.table_name) = 0, 'InnoDB System', REPLACE(SUBSTRING_IN sys.format_bytes(SUM(IF(ibp.compressed_size = 0, 16384, compressed_size))) AS allocated, sys.format_bytes(SUM(ibp.data_size)) AS data, COUNT(ibp.page_number) AS pages, - COUNT(IF(ibp.is_hashed = 'YES', 1, NULL)) AS pages_hashed, - COUNT(IF(ibp.is_old = 'YES', 1, NULL)) AS pages_old, + COUNT(IF(ibp.is_hashed, 1, NULL)) AS pages_hashed, + COUNT(IF(ibp.is_old, 1, NULL)) AS pages_old, ROUND(SUM(ibp.number_records)/COUNT(DISTINCT ibp.index_name)) AS rows_cached FROM information_schema.innodb_buffer_page ibp WHERE table_name IS NOT NULL @@ -62,4 +62,3 @@ SELECT IF(LOCATE('.', ibp.table_name) = 0, 'InnoDB System', REPLACE(SUBSTRING_IN ORDER BY SUM(IF(ibp.compressed_size = 0, 16384, compressed_size)) DESC; END$$ DELIMITER ; - diff --git a/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_table.sql b/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_table.sql index be104fb5bfc..ca3d8032873 100644 --- a/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_table.sql +++ b/scripts/sys_schema/views/i_s/innodb_buffer_stats_by_table.sql @@ -56,12 +56,12 @@ SELECT IF(LOCATE('.', ibp.table_name) = 0, 'InnoDB System', REPLACE(SUBSTRING_IN sys.format_bytes(SUM(IF(ibp.compressed_size = 0, 16384, compressed_size))) AS allocated, sys.format_bytes(SUM(ibp.data_size)) AS data, COUNT(ibp.page_number) AS pages, - COUNT(IF(ibp.is_hashed = 'YES', 1, NULL)) AS pages_hashed, - COUNT(IF(ibp.is_old = 'YES', 1, NULL)) AS pages_old, + COUNT(IF(ibp.is_hashed, 1, NULL)) AS pages_hashed, + COUNT(IF(ibp.is_old, 1, NULL)) AS pages_old, ROUND(SUM(ibp.number_records)/COUNT(DISTINCT ibp.index_name)) AS rows_cached FROM information_schema.innodb_buffer_page ibp WHERE table_name IS NOT NULL GROUP BY object_schema, object_name ORDER BY SUM(IF(ibp.compressed_size = 0, 16384, compressed_size)) DESC; END$$ -DELIMITER ; \ No newline at end of file +DELIMITER ; diff --git a/scripts/sys_schema/views/i_s/x_innodb_buffer_stats_by_table.sql b/scripts/sys_schema/views/i_s/x_innodb_buffer_stats_by_table.sql index 39d19b059b2..d9e5c98a4ac 100644 --- a/scripts/sys_schema/views/i_s/x_innodb_buffer_stats_by_table.sql +++ b/scripts/sys_schema/views/i_s/x_innodb_buffer_stats_by_table.sql @@ -63,4 +63,4 @@ SELECT IF(LOCATE('.', ibp.table_name) = 0, 'InnoDB System', REPLACE(SUBSTRING_IN GROUP BY object_schema, object_name ORDER BY SUM(IF(ibp.compressed_size = 0, 16384, compressed_size)) DESC; END$$ -DELIMITER ; \ No newline at end of file +DELIMITER ; From 6fd57f478f585249bdca242b1576ac0a7bd7aacf Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Thu, 10 Apr 2025 15:15:51 +1000 Subject: [PATCH 03/14] MDEV-36542 Remove UNINIT_VAR(x)=x under UBSAN Clang processes the "int x=x" code from UNINIT_VAR literally resulting in an uninitialized read and write. This is something we want to avoid. Gcc does the same without emitting warnings. As the UNINIT_VAR was around avoiding compiler false detection, and clang doesn't false detect, is default action is a noop. Static analysers (examined Infer and SonarQube) are clang based and have the same detection. Using a __clang__ instead of WITH_UBSAN would acheived a better result, however reviewer wanted to keep WITH_UBSAN only. LINT_INIT_STRUCT is no longer required, even a gcc-4.8.5 doesn't warn with this construct removed which matches the comment that it was fixed in gcc ~4.7. mysql.cc - all paths in com_go populate buff before use. json: Item_func_json_merge::val_str LINT_INIT(js2) unneeded as usage in the previous statements it is explicitly initialized to NULL. Item_func_json_contains_path::val_bool n_found is guarded by an uninitialized read by mode_one and from gcc-13.3.0 in Ubuntu 24.04 this is detected. As the only remaining use of LINIT_INIT this usage has been applied with the expanded macro with the unused _lint define removed. The LINT_INIT macro is removed. _ma_ck_delete - org_key only valid under share->now_transactional likewise with _ma_ck_write_btree_with_log connect engine never used anything that FORCE_INIT_OF_VARS would change. Reviewer: Monty --- client/mysql.cc | 2 -- include/m_string.h | 6 ------ include/my_global.h | 17 +++++++---------- sql/item_jsonfunc.cc | 12 ++++++++---- storage/connect/CMakeLists.txt | 2 +- storage/maria/ma_delete.c | 2 -- storage/maria/ma_write.c | 2 -- 7 files changed, 16 insertions(+), 27 deletions(-) diff --git a/client/mysql.cc b/client/mysql.cc index 2d6204517af..0781f42f64f 100644 --- a/client/mysql.cc +++ b/client/mysql.cc @@ -3499,8 +3499,6 @@ static int com_go(String *buffer, char *) old_buffer.copy(); } - /* Remove garbage for nicer messages */ - LINT_INIT_STRUCT(buff[0]); remove_cntrl(*buffer); if (buffer->is_empty()) diff --git a/include/m_string.h b/include/m_string.h index 4a2d8c8e587..4461533cd7c 100644 --- a/include/m_string.h +++ b/include/m_string.h @@ -79,12 +79,6 @@ extern const char _dig_vec_lower[]; extern char *strmov_overlapp(char *dest, const char *src); -#if defined(_lint) || defined(FORCE_INIT_OF_VARS) -#define LINT_INIT_STRUCT(var) bzero(&var, sizeof(var)) /* No uninitialize-warning */ -#else -#define LINT_INIT_STRUCT(var) -#endif - /* Prototypes for string functions */ extern void bmove_upp(uchar *dst,const uchar *src,size_t len); diff --git a/include/my_global.h b/include/my_global.h index 481436faf8c..0bc9a9609ca 100644 --- a/include/my_global.h +++ b/include/my_global.h @@ -448,22 +448,19 @@ extern "C" int madvise(void *addr, size_t len, int behav); /* Suppress uninitialized variable warning without generating code. */ -#if defined(__GNUC__) -/* GCC specific self-initialization which inhibits the warning. */ +#if defined(__GNUC__) && !defined(WITH_UBSAN) +/* + GCC specific self-initialization which inhibits the warning. + clang and static analysis will complain loudly about this + so compile those under WITH_UBSAN. +*/ #define UNINIT_VAR(x) x= x -#elif defined(_lint) || defined(FORCE_INIT_OF_VARS) +#elif defined(FORCE_INIT_OF_VARS) #define UNINIT_VAR(x) x= 0 #else #define UNINIT_VAR(x) x #endif -/* This is only to be used when resetting variables in a class constructor */ -#if defined(_lint) || defined(FORCE_INIT_OF_VARS) -#define LINT_INIT(x) x= 0 -#else -#define LINT_INIT(x) -#endif - #if !defined(HAVE_UINT) #undef HAVE_UINT #define HAVE_UINT diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc index e6f5da2bbe0..24ba3b91d4f 100644 --- a/sql/item_jsonfunc.cc +++ b/sql/item_jsonfunc.cc @@ -1728,9 +1728,16 @@ bool Item_func_json_contains_path::val_bool() longlong result; json_path_t p; int n_found; - LINT_INIT(n_found); int array_sizes[JSON_DEPTH_LIMIT]; uint has_negative_path= 0; +#if defined(FORCE_INIT_OF_VARS) + /* + Initialization force not required after gcc 13.3 + where it correctly sees that an uninitialized read + of n_found doesn't occur with mode_one being true. + */ + n_found= 0; +#endif if ((null_value= args[0]->null_value)) return 0; @@ -1770,8 +1777,6 @@ bool Item_func_json_contains_path::val_bool() bzero(p_found, (arg_count-2) * sizeof(bool)); n_found= arg_count - 2; } - else - n_found= 0; /* Just to prevent 'uninitialized value' warnings */ result= 0; while (json_get_path_next(&je, &p) == 0) @@ -2644,7 +2649,6 @@ String *Item_func_json_merge::val_str(String *str) String *js1= args[0]->val_json(&tmp_js1), *js2=NULL; uint n_arg; THD *thd= current_thd; - LINT_INIT(js2); JSON_DO_PAUSE_EXECUTION(thd, 0.0002); diff --git a/storage/connect/CMakeLists.txt b/storage/connect/CMakeLists.txt index 001e201dec8..7afc74bad29 100644 --- a/storage/connect/CMakeLists.txt +++ b/storage/connect/CMakeLists.txt @@ -46,7 +46,7 @@ user_connect.h valblk.h value.h xindex.h xobject.h xtable.h) # # Definitions that are shared for all OSes # -add_definitions( -DMARIADB -DFORCE_INIT_OF_VARS -Dconnect_EXPORTS) +add_definitions( -DMARIADB -Dconnect_EXPORTS) add_definitions( -DHUGE_SUPPORT -DGZ_SUPPORT ) macro(DISABLE_WARNING W) diff --git a/storage/maria/ma_delete.c b/storage/maria/ma_delete.c index b24cfcc6842..489e81121a4 100644 --- a/storage/maria/ma_delete.c +++ b/storage/maria/ma_delete.c @@ -165,8 +165,6 @@ my_bool _ma_ck_delete(MARIA_HA *info, MARIA_KEY *key) MARIA_KEY org_key; DBUG_ENTER("_ma_ck_delete"); - LINT_INIT_STRUCT(org_key); - alloc_on_stack(*info->stack_end_ptr, key_buff, buff_alloced, key->keyinfo->max_store_length); if (!key_buff) diff --git a/storage/maria/ma_write.c b/storage/maria/ma_write.c index 9039c0395c0..b8a152fd9fd 100644 --- a/storage/maria/ma_write.c +++ b/storage/maria/ma_write.c @@ -483,8 +483,6 @@ static my_bool _ma_ck_write_btree_with_log(MARIA_HA *info, MARIA_KEY *key, my_bool transactional= share->now_transactional; DBUG_ENTER("_ma_ck_write_btree_with_log"); - LINT_INIT_STRUCT(org_key); - if (transactional) { /* Save original value as the key may change */ From 0c80ddb519bd06efbd7ccf2a2487b2503cd16db5 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Wed, 11 Jun 2025 15:07:55 +1000 Subject: [PATCH 04/14] MDEV-36542: apply clang -Werror=uninitialized to catch dubious initializations clang has rather correct error detection in its implementation of the -Werror=uninitialized* set of C/CXX flags. Use them by default. --- cmake/maintainer.cmake | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmake/maintainer.cmake b/cmake/maintainer.cmake index 1f40324d5ca..8a13ad47e9d 100644 --- a/cmake/maintainer.cmake +++ b/cmake/maintainer.cmake @@ -68,6 +68,14 @@ IF(CMAKE_C_COMPILER_ID STREQUAL "GNU" AND CMAKE_C_COMPILER_VERSION VERSION_LESS SET(MY_ERROR_FLAGS ${MY_ERROR_FLAGS} -Wno-error=non-virtual-dtor) # gcc bug 7302 ENDIF() +FOREACH(LANG C CXX) + IF(CMAKE_${LANG}_COMPILER_ID MATCHES "Clang") + MY_CHECK_AND_SET_COMPILER_FLAG(-Werror=uninitialized-explicit-init) + MY_CHECK_AND_SET_COMPILER_FLAG(-Werror=uninitialized-const-reference) + SET(CMAKE_${LANG}_FLAGS "${CMAKE_${LANG}_FLAGS} -Werror=uninitialized") + ENDIF() +ENDFOREACH() + IF(MYSQL_MAINTAINER_MODE MATCHES "OFF|WARN") RETURN() ELSEIF(MYSQL_MAINTAINER_MODE MATCHES "AUTO") From 4f9221ae881b1d4b5dcb5921cee1c5200a48017b Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Thu, 12 Jun 2025 14:30:50 +1000 Subject: [PATCH 05/14] MDEV-36542: remove _lint macro which is unused Attribute noreturn functions don't need a return afterwards. aria_pack was missing the noreturn attribute on its my_end function. --- client/mysql.cc | 4 +--- include/my_dbug.h | 4 ++-- include/my_global.h | 8 ++------ sql/sql_class.cc | 4 ++-- sql/sql_list.h | 2 -- sql/sql_plist.h | 2 -- storage/maria/aria_chk.c | 3 --- storage/maria/aria_pack.c | 4 +--- storage/mroonga/ha_mroonga.cpp | 2 +- storage/mroonga/mrn_mysql.h | 2 +- storage/myisam/myisamchk.c | 3 --- storage/myisam/myisampack.c | 3 --- 12 files changed, 10 insertions(+), 31 deletions(-) diff --git a/client/mysql.cc b/client/mysql.cc index 0781f42f64f..b9e643a12f3 100644 --- a/client/mysql.cc +++ b/client/mysql.cc @@ -1387,9 +1387,7 @@ int main(int argc,char *argv[]) if (opt_outfile) end_tee(); mysql_end(0); -#ifndef _lint - DBUG_RETURN(0); // Keep compiler happy -#endif + DBUG_RETURN(0); } sig_handler mysql_end(int sig) diff --git a/include/my_dbug.h b/include/my_dbug.h index 02caadbff48..c72e9bb608d 100644 --- a/include/my_dbug.h +++ b/include/my_dbug.h @@ -24,7 +24,7 @@ #ifdef __cplusplus extern "C" { #endif -#if !defined(DBUG_OFF) && !defined(_lint) +#if !defined(DBUG_OFF) struct _db_stack_frame_ { const char *func; /* function name of the previous stack frame */ @@ -210,7 +210,7 @@ extern void (*my_dbug_assert_failed)(const char *assert_expr, const char* file, #define DBUG_ASSERT(A) do { } while(0) #define IF_DBUG_ASSERT(A,B) B #endif /* DBUG_ASSERT_AS_PRINTF */ -#endif /* !defined(DBUG_OFF) && !defined(_lint) */ +#endif /* !defined(DBUG_OFF) */ #ifdef EXTRA_DEBUG /** diff --git a/include/my_global.h b/include/my_global.h index 0bc9a9609ca..82b6f21f9a5 100644 --- a/include/my_global.h +++ b/include/my_global.h @@ -281,10 +281,6 @@ C_MODE_END #error "Please add -fno-exceptions to CXXFLAGS and reconfigure/recompile" #endif -#if defined(_lint) && !defined(lint) -#define lint -#endif - #ifndef stdin #include #endif @@ -502,7 +498,7 @@ C_MODE_END #endif /* We might be forced to turn debug off, if not turned off already */ -#if (defined(FORCE_DBUG_OFF) || defined(_lint)) && !defined(DBUG_OFF) +#if defined(FORCE_DBUG_OFF) && !defined(DBUG_OFF) # define DBUG_OFF # ifdef DBUG_ON # undef DBUG_ON @@ -524,7 +520,7 @@ typedef int my_socket; /* File descriptor for sockets */ #endif /* Type for functions that handles signals */ #define sig_handler RETSIGTYPE -#if defined(__GNUC__) && !defined(_lint) +#if defined(__GNUC__) typedef char pchar; /* Mixed prototypes can take char */ typedef char puchar; /* Mixed prototypes can take char */ typedef char pbool; /* Mixed prototypes can take char */ diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 1d51d793872..e821a4d4640 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -7699,7 +7699,7 @@ bool THD::binlog_for_noop_dml(bool transactional_table) } -#if defined(DBUG_TRACE) && !defined(_lint) +#if defined(DBUG_TRACE) static const char * show_query_type(THD::enum_binlog_query_type qtype) { @@ -7713,7 +7713,7 @@ show_query_type(THD::enum_binlog_query_type qtype) DBUG_ASSERT(0 <= qtype && qtype < THD::QUERY_TYPE_COUNT); } static char buf[64]; - sprintf(buf, "UNKNOWN#%d", qtype); + snprintf(buf, sizeof(buf), "UNKNOWN#%d", qtype); return buf; } #endif diff --git a/sql/sql_list.h b/sql/sql_list.h index a55f4764145..af8e20d67ae 100644 --- a/sql/sql_list.h +++ b/sql/sql_list.h @@ -836,9 +836,7 @@ public: inline void move_elements_to(I_List* new_owner) { base_ilist::move_elements_to(new_owner); } -#ifndef _lint friend class I_List_iterator; -#endif }; diff --git a/sql/sql_plist.h b/sql/sql_plist.h index 7f75208ca09..0969fef0749 100644 --- a/sql/sql_plist.h +++ b/sql/sql_plist.h @@ -158,10 +158,8 @@ public: typedef I_P_List Base; typedef I_P_List_iterator Iterator; typedef I_P_List_iterator Const_Iterator; -#ifndef _lint friend class I_P_List_iterator; friend class I_P_List_iterator; -#endif }; diff --git a/storage/maria/aria_chk.c b/storage/maria/aria_chk.c index 4bbe513b44b..c35ca3fc569 100644 --- a/storage/maria/aria_chk.c +++ b/storage/maria/aria_chk.c @@ -236,9 +236,6 @@ end: } maria_end(); my_exit(error); -#ifndef _lint - return 0; /* No compiler warning */ -#endif } /* main */ enum options_mc { diff --git a/storage/maria/aria_pack.c b/storage/maria/aria_pack.c index aad2b316c92..62926ec64ef 100644 --- a/storage/maria/aria_pack.c +++ b/storage/maria/aria_pack.c @@ -289,11 +289,9 @@ end: maria_end(); my_end(verbose ? MY_CHECK_ERROR | MY_GIVE_INFO : MY_CHECK_ERROR); exit(error ? 2 : 0); -#ifndef _lint - return 0; /* No compiler warning */ -#endif } +ATTRIBUTE_NORETURN static void my_exit(int error) { free_defaults(default_argv); diff --git a/storage/mroonga/ha_mroonga.cpp b/storage/mroonga/ha_mroonga.cpp index 2dbc3f351cc..fb279c5458e 100644 --- a/storage/mroonga/ha_mroonga.cpp +++ b/storage/mroonga/ha_mroonga.cpp @@ -337,7 +337,7 @@ static int mrn_change_encoding(grn_ctx *ctx, const CHARSET_INFO *charset) return mrn::encoding::set(ctx, charset); } -#if defined DBUG_TRACE && !defined(_lint) +#if defined DBUG_TRACE static const char *mrn_inspect_thr_lock_type(enum thr_lock_type lock_type) { const char *inspected = ""; diff --git a/storage/mroonga/mrn_mysql.h b/storage/mroonga/mrn_mysql.h index 9573caa921d..332a5cbf480 100644 --- a/storage/mroonga/mrn_mysql.h +++ b/storage/mroonga/mrn_mysql.h @@ -59,7 +59,7 @@ #define MRN_DBUG_ENTER_FUNCTION() DBUG_ENTER(__FUNCTION__) -#if !defined(DBUG_OFF) && !defined(_lint) +#if !defined(DBUG_OFF) # define MRN_DBUG_ENTER_METHOD() \ char method_name[MRN_MESSAGE_BUFFER_SIZE]; \ method_name[0] = '\0'; \ diff --git a/storage/myisam/myisamchk.c b/storage/myisam/myisamchk.c index 3e312269b7f..1696ec0c7db 100644 --- a/storage/myisam/myisamchk.c +++ b/storage/myisam/myisamchk.c @@ -124,9 +124,6 @@ int main(int argc, char **argv) my_end(check_param.testflag & T_INFO ? MY_CHECK_ERROR | MY_GIVE_INFO : MY_CHECK_ERROR); rc= (uchar) error; exit(rc); -#ifndef _lint - return 0; /* No compiler warning */ -#endif } /* main */ enum options_mc { diff --git a/storage/myisam/myisampack.c b/storage/myisam/myisampack.c index 77fdaf8e164..1d08b048d1b 100644 --- a/storage/myisam/myisampack.c +++ b/storage/myisam/myisampack.c @@ -248,9 +248,6 @@ int main(int argc, char **argv) free_defaults(default_argv); my_end(verbose ? MY_CHECK_ERROR | MY_GIVE_INFO : MY_CHECK_ERROR); exit(error ? 2 : 0); -#ifndef _lint - return 0; /* No compiler warning */ -#endif } enum options_mp {OPT_CHARSETS_DIR_MP=256}; From 0de58ecbd507ef4a5e97a9b560e5c81896fba614 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Mon, 28 Apr 2025 16:12:24 +1000 Subject: [PATCH 06/14] connect engine: correct two uninitalized variable errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storage/connect/tabxml.cpp:1616:46: warning: ‘*this.XMLCOL::Long’ may be used uninitialized [-Wmaybe-uninitialized] 1616 | Valbuf = (char*)PlugSubAlloc(g, NULL, n * (Long + 1)); In this case we are overriding the class 3 lines earlier. Add some constructs to preserve the value of Long as the old class being replaced with a new subclass. storage/connect/filter.cpp:1594:13: warning: ‘*this.FILTERCMP::FILTERX.FILTERX::FILTER.FILTER::Opc’ is used uninitialized [-Wuninitialized] 1594 | Bt = OpBmp(g, Opc); The construction of FILTERCMP has an Opc(ode) and this should be passed rather than relying on the uninitialized value of the parent class. Also save its value in the class. --- storage/connect/filter.cpp | 5 +++-- storage/connect/filter.h | 2 +- storage/connect/tabxml.cpp | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/storage/connect/filter.cpp b/storage/connect/filter.cpp index cddb8b8c526..9e9a526792f 100644 --- a/storage/connect/filter.cpp +++ b/storage/connect/filter.cpp @@ -1179,7 +1179,7 @@ bool FILTER::Convert(PGLOBAL g, bool having) case OP_GT: case OP_GE: case OP_LT: - case OP_LE: new(this) FILTERCMP(g); break; + case OP_LE: new(this) FILTERCMP(g, Opc); break; case OP_AND: new(this) FILTERAND; break; case OP_OR: new(this) FILTEROR; break; case OP_NOT: new(this) FILTERNOT; break; @@ -1589,8 +1589,9 @@ void FILTER::Prints(PGLOBAL g, char *ps, uint z) /***********************************************************************/ /* FILTERCMP constructor. */ /***********************************************************************/ -FILTERCMP::FILTERCMP(PGLOBAL g) +FILTERCMP::FILTERCMP(PGLOBAL g, OPVAL Opc) { + this->Opc= Opc; Bt = OpBmp(g, Opc); } // end of FILTERCMP constructor diff --git a/storage/connect/filter.h b/storage/connect/filter.h index 8f973b52cd0..1007e9b3bd9 100644 --- a/storage/connect/filter.h +++ b/storage/connect/filter.h @@ -120,7 +120,7 @@ class FILTERX : public FILTER { class FILTERCMP : public FILTERX { public: // Constructor - FILTERCMP(PGLOBAL g); + FILTERCMP(PGLOBAL, OPVAL); // Methods bool Eval(PGLOBAL) override; diff --git a/storage/connect/tabxml.cpp b/storage/connect/tabxml.cpp index bc2ebb6e581..c09c32e65a7 100644 --- a/storage/connect/tabxml.cpp +++ b/storage/connect/tabxml.cpp @@ -1611,7 +1611,9 @@ bool XMLCOL::ParseXpath(PGLOBAL g, bool mode) if (Tdbp->Xpand) n = Tdbp->Limit; + auto oLong = Long; new(this) XMULCOL(Value); // Change the class of this column + Long = oLong; } // endif Inod Valbuf = (char*)PlugSubAlloc(g, NULL, n * (Long + 1)); From bea8c6a3045437370d71fcbaa0ec321061d43db5 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Tue, 29 Jul 2025 13:32:53 +0200 Subject: [PATCH 07/14] MDEV-37198 Killed query with side effects without error mysql_update() aborts if SQL_SELECT::check_quick() fails. SQL_SELECT::check_quick() fails when thd->killed is set. Thus, mysql_update() must also test thd->killed to know how to terminate - normally (impossible where) or abnormally. --- sql/sql_update.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 5821ff7eef6..decd9ccded0 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -612,7 +612,7 @@ int mysql_update(THD *thd, Currently they rely on the user checking DA for errors when unwinding the stack after calling Item::val_xxx(). */ - if (error || thd->is_error()) + if (error || thd->killed || thd->is_error()) { DBUG_RETURN(1); // Error in where } From 67d2d73d1fe622e457bd58002638fa8fc888197e Mon Sep 17 00:00:00 2001 From: Fariha Shaikh Date: Wed, 30 Jul 2025 19:30:27 +0000 Subject: [PATCH 08/14] MDEV-36385 Fix slave_parallel_threads_basic test in view-protocol mode Add explicit column aliases to SELECT statements in slave_parallel_threads_basic to ensure consistent column names across both normal and view-protocol modes. The test sys_vars.slave_parallel_threads_basic.test was failing in view-protocol mode due to different column naming behavior. Without an explicit column alias, view-protocol mode generates an automatic name (Name_exp_1) while normal mode uses the full expression as the column name. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc. --- .../suite/sys_vars/r/slave_parallel_threads_basic.result | 8 ++++---- .../suite/sys_vars/t/slave_parallel_threads_basic.test | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mysql-test/suite/sys_vars/r/slave_parallel_threads_basic.result b/mysql-test/suite/sys_vars/r/slave_parallel_threads_basic.result index 56aa5976f91..ab64e9dfade 100644 --- a/mysql-test/suite/sys_vars/r/slave_parallel_threads_basic.result +++ b/mysql-test/suite/sys_vars/r/slave_parallel_threads_basic.result @@ -1,6 +1,6 @@ SET @save_slave_parallel_threads= @@GLOBAL.slave_parallel_threads; -SELECT IF(COUNT(*) < 20, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) FROM information_schema.processlist WHERE user = "system user"; -IF(COUNT(*) < 20, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) +SELECT IF(COUNT(*) < 20, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) AS ProcessCheck FROM information_schema.processlist WHERE user = "system user"; +ProcessCheck OK SELECT @@GLOBAL.slave_parallel_threads as 'must be 20 because of .cnf'; must be 20 because of .cnf @@ -16,7 +16,7 @@ SET GLOBAL slave_parallel_threads= 10; SELECT @@GLOBAL.slave_parallel_threads; @@GLOBAL.slave_parallel_threads 10 -SELECT IF(COUNT(*) < 10, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) FROM information_schema.processlist WHERE user = "system user"; -IF(COUNT(*) < 10, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) +SELECT IF(COUNT(*) < 10, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) AS ProcessCheck FROM information_schema.processlist WHERE user = "system user"; +ProcessCheck OK SET GLOBAL slave_parallel_threads = @save_slave_parallel_threads; diff --git a/mysql-test/suite/sys_vars/t/slave_parallel_threads_basic.test b/mysql-test/suite/sys_vars/t/slave_parallel_threads_basic.test index b567b7f8854..dc315403546 100644 --- a/mysql-test/suite/sys_vars/t/slave_parallel_threads_basic.test +++ b/mysql-test/suite/sys_vars/t/slave_parallel_threads_basic.test @@ -4,7 +4,7 @@ SET @save_slave_parallel_threads= @@GLOBAL.slave_parallel_threads; # Check that we don't spawn worker threads at server startup, when no # slave is configured (MDEV-5289). -SELECT IF(COUNT(*) < 20, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) FROM information_schema.processlist WHERE user = "system user"; +SELECT IF(COUNT(*) < 20, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) AS ProcessCheck FROM information_schema.processlist WHERE user = "system user"; SELECT @@GLOBAL.slave_parallel_threads as 'must be 20 because of .cnf'; --error ER_INCORRECT_GLOBAL_LOCAL_VAR @@ -16,6 +16,6 @@ SELECT @@GLOBAL.slave_parallel_threads as 'must be 0 because of default'; SET GLOBAL slave_parallel_threads= 10; SELECT @@GLOBAL.slave_parallel_threads; # Check that we don't spawn worker threads when no slave is started. -SELECT IF(COUNT(*) < 10, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) FROM information_schema.processlist WHERE user = "system user"; +SELECT IF(COUNT(*) < 10, "OK", CONCAT("Found too many system user processes: ", COUNT(*))) AS ProcessCheck FROM information_schema.processlist WHERE user = "system user"; SET GLOBAL slave_parallel_threads = @save_slave_parallel_threads; From f8ac3a7c0eedbdb0f33d4b4b85bcf17a69e7f6ff Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Thu, 25 Apr 2024 23:35:44 +0200 Subject: [PATCH 09/14] multi_delete: fix unlikely -> likely --- sql/sql_delete.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index ec8ed6efff3..c1d32fa4042 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -1612,7 +1612,7 @@ int multi_delete::do_table_deletes(TABLE *table, SORT_INFO *sort_info, during ha_delete_row. Also, don't execute the AFTER trigger if the row operation failed. */ - if (unlikely(!local_error)) + if (likely(!local_error)) { deleted++; if (table->triggers && From 2e2b2a046942bb8f743202c946300b9a3b0d58fd Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Wed, 28 Dec 2022 23:05:46 +0300 Subject: [PATCH 10/14] MDEV-15990 Refactor write_record and fix idempotent replication See also MDEV-30046. Idempotent write_row works same as REPLACE: if there is a duplicating record in the table, then it will be deleted and re-inserted, with the same update optimization. The code in Rows:log_event::write_row was basically copy-pasted from write_record. What's done: REPLACE operation was unified across replication and sql. It is now representred as a Write_record class, that holds the whole state, and allows re-using some resources in between the row writes. Replace, IODKU and single insert implementations are split across different methods, reluting in a much cleaner code. The entry point is preserved as a single Write_record::write_record() call. The implementation to call is chosen on the constructor stage. This allowed several optimizations to be done: 1. The table key list is not iterated for every row. We find last unique key in the order of checking once and preserve it across the rows. See last_uniq_key(). 2. ib_handler::referenced_by_foreign_key acquires a global lock. This call was done per row as well. Not all the table config that allows optimized replace is folded into a single boolean field can_optimize. All the fields to check are even stored in a single register on a 64-bit platform. 3. DUP_REPLACE and DUP_UPDATE cases now have one less level of indirection 4. modified_non_trans_tables is checked and set only when it's really needed. 5. Obsolete bitmap manipulations are removed. Also: * Unify replace initialization step across implementations: add prepare_for_replace and finalize_replace * alloca is removed in favor of mem_root allocation. This memory is reused across the rows. * An rpl-related callback is added to the replace branch, meaning that an extra check is made per row replace even for the common case. It can be avoided with templates if considered a problem. --- .../main/long_unique_bugs_replication.result | 48 +- .../main/long_unique_bugs_replication.test | 46 +- sql/handler.cc | 6 - sql/handler.h | 8 +- sql/log_event.h | 21 +- sql/log_event_server.cc | 273 ++--- sql/sql_class.h | 9 +- sql/sql_insert.cc | 1084 +++++++++-------- sql/sql_insert.h | 87 +- sql/sql_load.cc | 29 +- sql/sql_parse.cc | 4 +- sql/sql_table.cc | 5 +- 12 files changed, 873 insertions(+), 747 deletions(-) diff --git a/mysql-test/main/long_unique_bugs_replication.result b/mysql-test/main/long_unique_bugs_replication.result index 39b0ebe26d2..ab6674f886f 100644 --- a/mysql-test/main/long_unique_bugs_replication.result +++ b/mysql-test/main/long_unique_bugs_replication.result @@ -9,20 +9,64 @@ insert into t1 values (2,2); update t1 set a1 = 'd' limit 1; update t1 set a1 = 'd2' where i1= 2; connection slave; +connection slave; +select * from t1; +i1 a1 +1 d +2 d2 connection master; drop table t1; +connection slave; +connection master; # # MDEV-32093 long uniques break old->new replication # -connection slave; create table t1 (id int not null, b1 varchar(255) not null, b2 varchar(2550) not null, unique (id), unique key (b1,b2) using hash) default charset utf8mb3; set global slave_exec_mode=idempotent; binlog 'aRf2ZA8BAAAA/AAAAAABAAAAAAQAMTAuNS4xNS1NYXJpYURCLWxvZwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABpF/ZkEzgNAAgAEgAEBAQEEgAA5AAEGggAAAAICAgCAAAACgoKAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEEwQADQgICAoKCgFRmTlk'; binlog 'bBf2ZBMBAAAANAAAAJgHAAAAAHEAAAAAAAEABHRlc3QAAnQxAAQDDw8IBP0C4h0AeqMD4A==bBf2ZBcBAAAANAAAAMwHAAAAAHEAAAAAAAEABP/wj6QAAAEAYgEAZa6/VU0JAAAANteqUw=='; binlog 'bBf2ZBMBAAAANAAAAJgHAAAAAHEAAAAAAAEABHRlc3QAAnQxAAQDDw8IBP0C4h0AeqMD4A==bBf2ZBcBAAAANAAAAMwHAAAAAHEAAAAAAAEABP/wj6QAAAEAYgEAZa6/VU0JAAAANteqUw=='; binlog 'bBf2ZBMBAAAANAAAAHUkAAAAAHEAAAAAAAEABHRlc3QAAnQxAAQDDw8IBP0C4h0AaTGFIg==bBf2ZBgBAAAASAAAAL0kAAAAAHEAAAAAAAEABP//8I+kAAABAGIBAGWuv1VNCQAAAPBuWwAAAQBiAQBlrr9VTQkAAADxS9Lu'; -drop table t1; +connection slave; +select * from t1; +id b1 b2 +23406 b e +connection master; set global slave_exec_mode=default; +drop table t1; +# +# End of 10.4 tests +# +# Idempotent scenario, which triggers REPLACE code to be used in the +# event, i.e. duplicated record will be deleted and then re-inserted. +create table t1 (i1 int, a1 text, unique key i1 (a1)) engine=myisam; +connection slave; +connection slave; +set @save_slave_exec_mode= @@slave_exec_mode; +set global slave_exec_mode = idempotent; +insert into t1 values (1,1); +connection master; +insert into t1 values (2,1); +connection slave; +connection slave; +select * from t1; +i1 a1 +2 1 +connection master; +insert into t1 values (3,3); +update t1 set a1 = 'd' limit 1; +update t1 set a1 = 'd2' where i1= 3; +connection slave; +connection slave; +select * from t1; +i1 a1 +2 d +3 d2 +set global slave_exec_mode = @save_slave_exec_mode; +connection master; +drop table t1; +connection slave; +connection master; # # End of 10.4 tests # diff --git a/mysql-test/main/long_unique_bugs_replication.test b/mysql-test/main/long_unique_bugs_replication.test index 9c44d13e6a5..f2225cde101 100644 --- a/mysql-test/main/long_unique_bugs_replication.test +++ b/mysql-test/main/long_unique_bugs_replication.test @@ -16,17 +16,21 @@ update t1 set a1 = 'd' limit 1; update t1 set a1 = 'd2' where i1= 2; sync_slave_with_master; - +connection slave; +select * from t1; connection master; drop table t1; +sync_slave_with_master; +connection master; + + --echo # --echo # MDEV-32093 long uniques break old->new replication --echo # # this is techically a bug in replication, but it needs an old master # so we'll run it as a non-replicated test with BINLOG command -sync_slave_with_master; create table t1 (id int not null, b1 varchar(255) not null, b2 varchar(2550) not null, unique (id), unique key (b1,b2) using hash) default charset utf8mb3; set global slave_exec_mode=idempotent; @@ -40,10 +44,46 @@ binlog 'bBf2ZBMBAAAANAAAAJgHAAAAAHEAAAAAAAEABHRlc3QAAnQxAAQDDw8IBP0C4h0AeqMD4A== ### UPDATE t1 WHERE (42127, 'b', 'e', 39952170926) SET (23406, 'b', 'e', 39952170926) binlog 'bBf2ZBMBAAAANAAAAHUkAAAAAHEAAAAAAAEABHRlc3QAAnQxAAQDDw8IBP0C4h0AaTGFIg==bBf2ZBgBAAAASAAAAL0kAAAAAHEAAAAAAAEABP//8I+kAAABAGIBAGWuv1VNCQAAAPBuWwAAAQBiAQBlrr9VTQkAAADxS9Lu'; -drop table t1; +sync_slave_with_master; +select * from t1; +connection master; set global slave_exec_mode=default; +drop table t1; --echo # --echo # End of 10.4 tests --echo # + +--echo # Idempotent scenario, which triggers REPLACE code to be used in the +--echo # event, i.e. duplicated record will be deleted and then re-inserted. +create table t1 (i1 int, a1 text, unique key i1 (a1)) engine=myisam; + +sync_slave_with_master; +connection slave; +set @save_slave_exec_mode= @@slave_exec_mode; +set global slave_exec_mode = idempotent; +insert into t1 values (1,1); +connection master; +insert into t1 values (2,1); +sync_slave_with_master; +connection slave; +select * from t1; +connection master; +insert into t1 values (3,3); +update t1 set a1 = 'd' limit 1; +update t1 set a1 = 'd2' where i1= 3; +sync_slave_with_master; + +connection slave; +select * from t1; +set global slave_exec_mode = @save_slave_exec_mode; +connection master; +drop table t1; +sync_slave_with_master; +connection master; + +--echo # +--echo # End of 10.4 tests +--echo # + --source include/rpl_end.inc diff --git a/sql/handler.cc b/sql/handler.cc index f770296f8df..6ac0ee5b9a4 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -5003,12 +5003,6 @@ uint handler::get_dup_key(int error) DBUG_RETURN(errkey); } -bool handler::has_dup_ref() const -{ - DBUG_ASSERT(lookup_errkey != (uint)-1 || errkey != (uint)-1); - return ha_table_flags() & HA_DUPLICATE_POS || lookup_errkey != (uint)-1; -} - /** Delete all files with extension from bas_ext(). diff --git a/sql/handler.h b/sql/handler.h index eeeee42c2ca..efa4b126d2c 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -3138,7 +3138,6 @@ protected: Table_flags cached_table_flags; /* Set on init() and open() */ ha_rows estimation_rows_to_insert; - handler *lookup_handler; /* Statistics for the query. Updated if handler_stats.active is set */ ha_handler_stats active_handler_stats; void set_handler_stats(); @@ -3147,6 +3146,7 @@ public: uchar *ref; /* Pointer to current row */ uchar *dup_ref; /* Pointer to duplicate row */ uchar *lookup_buffer; + handler *lookup_handler; /* General statistics for the table like number of row, file sizes etc */ ha_statistics stats; @@ -3348,9 +3348,8 @@ public: handler(handlerton *ht_arg, TABLE_SHARE *share_arg) :table_share(share_arg), table(0), estimation_rows_to_insert(0), - lookup_handler(this), - ht(ht_arg), ref(0), lookup_buffer(NULL), handler_stats(NULL), - end_range(NULL), implicit_emptied(0), + ht(ht_arg), ref(0), lookup_buffer(NULL), lookup_handler(this), + handler_stats(NULL), end_range(NULL), implicit_emptied(0), mark_trx_read_write_done(0), check_table_binlog_row_based_done(0), check_table_binlog_row_based_result(0), @@ -3543,7 +3542,6 @@ public: virtual void print_error(int error, myf errflag); virtual bool get_error_message(int error, String *buf); uint get_dup_key(int error); - bool has_dup_ref() const; /** Retrieves the names of the table and the key for which there was a duplicate entry in the case of HA_ERR_FOREIGN_DUPLICATE_KEY. diff --git a/sql/log_event.h b/sql/log_event.h index 3263637ecf6..9403e15e4c6 100644 --- a/sql/log_event.h +++ b/sql/log_event.h @@ -53,6 +53,7 @@ #include "rpl_record.h" #include "rpl_reporting.h" #include "sql_class.h" /* THD */ +#include "sql_insert.h" #endif #include "rpl_gtid.h" @@ -4930,7 +4931,7 @@ public: enum_logged_status logged_status() override { return LOGGED_TABLE_MAP; } bool is_valid() const override { return m_memory != NULL; /* we check malloc */ } - int get_data_size() override { return (uint) m_data_size; } + int get_data_size() override { return (uint) m_data_size; } #ifdef MYSQL_SERVER #ifdef HAVE_REPLICATION bool is_part_of_group() override { return 1; } @@ -5345,7 +5346,6 @@ protected: int find_key(); // Find a best key to use in find_row() int find_row(rpl_group_info *); - int write_row(rpl_group_info *, const bool); int update_sequence(); // Unpack the current row into m_table->record[0], but with @@ -5410,8 +5410,9 @@ private: The member function will return 0 if all went OK, or a non-zero error code otherwise. */ - virtual - int do_before_row_operations(const Slave_reporting_capability *const log) = 0; + virtual + int do_before_row_operations(rpl_group_info *log, + COPY_INFO*, Write_record*) = 0; /* Primitive to clean up after a sequence of row executions. @@ -5489,6 +5490,7 @@ public: #if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) uint8 get_trg_event_map() override; + int incomplete_record_callback(rpl_group_info *rgi); #endif private: @@ -5499,7 +5501,10 @@ private: #endif #if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) - int do_before_row_operations(const Slave_reporting_capability *const) override; + Write_record *m_write_record; + int write_row(rpl_group_info *, bool); + int do_before_row_operations(rpl_group_info *rgi, + COPY_INFO*, Write_record*) override; int do_after_row_operations(const Slave_reporting_capability *const,int) override; int do_exec_row(rpl_group_info *) override; #endif @@ -5587,7 +5592,8 @@ protected: #endif #if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) - int do_before_row_operations(const Slave_reporting_capability *const) override; + int do_before_row_operations(rpl_group_info *rgi, + COPY_INFO*, Write_record*) override; int do_after_row_operations(const Slave_reporting_capability *const,int) override; int do_exec_row(rpl_group_info *) override; #endif /* defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) */ @@ -5672,7 +5678,8 @@ protected: #endif #if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) - int do_before_row_operations(const Slave_reporting_capability *const) override; + int do_before_row_operations(rpl_group_info *rgi, + COPY_INFO*, Write_record*) override; int do_after_row_operations(const Slave_reporting_capability *const,int) override; int do_exec_row(rpl_group_info *) override; #endif diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index b1058dd0b6b..49fbc11c436 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -6133,7 +6133,10 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) Rows_log_event::Db_restore_ctx restore_ctx(this); master_had_triggers= table->master_had_triggers; bool transactional_table= table->file->has_transactions_and_rollback(); - table->file->prepare_for_insert(get_general_type_code() != WRITE_ROWS_EVENT); + this->slave_exec_mode= slave_exec_mode_options; // fix the mode + + table->file->prepare_for_insert(get_general_type_code() != WRITE_ROWS_EVENT + || slave_exec_mode == SLAVE_EXEC_MODE_IDEMPOTENT); /* table == NULL means that this table should not be replicated @@ -6178,10 +6181,10 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) } m_table->mark_columns_per_binlog_row_image(); - this->slave_exec_mode= slave_exec_mode_options; // fix the mode - - // Do event specific preparations - error= do_before_row_operations(rli); + COPY_INFO copy_info; + Write_record write_record; + // Do event specific preparations + error= do_before_row_operations(rgi, ©_info, &write_record); /* Bug#56662 Assertion failed: next_insert_id == 0, file handler.cc @@ -7547,8 +7550,21 @@ bool Write_rows_compressed_log_event::write() #if defined(HAVE_REPLICATION) -int -Write_rows_log_event::do_before_row_operations(const Slave_reporting_capability *const) + +int Write_rows_log_event::incomplete_record_callback(rpl_group_info *rgi) +{ + restore_record(m_table,record[1]); + int error= unpack_current_row(rgi); + if (!error && m_table->s->long_unique_table) + error= m_table->update_virtual_fields(m_table->file, VCOL_UPDATE_FOR_WRITE); + return error; +} + + +int +Write_rows_log_event::do_before_row_operations(rpl_group_info *rgi, + COPY_INFO* copy_info, + Write_record* write_record) { int error= 0; @@ -7622,6 +7638,39 @@ Write_rows_log_event::do_before_row_operations(const Slave_reporting_capability m_table->mark_auto_increment_column(true); } + if (slave_exec_mode == SLAVE_EXEC_MODE_IDEMPOTENT && + (m_table->file->ha_table_flags() & HA_DUPLICATE_POS || + m_table->s->long_unique_table)) + error= m_table->file->ha_rnd_init_with_error(0); + + if (!error) + { + bzero(copy_info, sizeof *copy_info); + copy_info->handle_duplicates= + slave_exec_mode == SLAVE_EXEC_MODE_IDEMPOTENT ? + DUP_REPLACE : DUP_ERROR; + copy_info->table_list= m_table->pos_in_table_list; + + int (*callback)(void *, void*)= NULL; + if (!get_flags(COMPLETE_ROWS_F)) + { + /* + If row is incomplete we will use the record found to fill + missing columns. + */ + callback= [](void *e, void* r)->int { + auto rgi= static_cast(r); + auto event= static_cast(e); + return event->incomplete_record_callback(rgi); + }; + } + new (write_record) Write_record(thd, m_table, copy_info, + m_table->versioned(VERS_TIMESTAMP), + m_table->triggers && do_invoke_trigger(), + NULL, callback, this, rgi); + m_write_record= write_record; + } + return error; } @@ -7663,7 +7712,15 @@ Write_rows_log_event::do_after_row_operations(const Slave_reporting_capability * { m_table->file->print_error(local_error, MYF(0)); } - return error? error : local_error; + int rnd_error= 0; + if (m_table->file->inited) + { + DBUG_ASSERT(slave_exec_mode == SLAVE_EXEC_MODE_IDEMPOTENT); + DBUG_ASSERT(m_table->file->ha_table_flags() & HA_DUPLICATE_POS || + m_table->s->long_unique_table); + rnd_error= m_table->file->ha_rnd_end(); + } + return error? error : local_error ? local_error : rnd_error; } bool Rows_log_event::process_triggers(trg_event_type event, @@ -7686,17 +7743,6 @@ bool Rows_log_event::process_triggers(trg_event_type event, DBUG_RETURN(result); } -/* - Check if there are more UNIQUE keys after the given key. -*/ -static int -last_uniq_key(TABLE *table, uint keyno) -{ - while (++keyno < table->s->keys) - if (table->key_info[keyno].flags & HA_NOSAME) - return 0; - return 1; -} /** Check if an error is a duplicate key error. @@ -7759,22 +7805,20 @@ is_duplicate_key_error(int errcode) */ int -Rows_log_event::write_row(rpl_group_info *rgi, - const bool overwrite) +Write_rows_log_event::write_row(rpl_group_info *rgi, + const bool overwrite) { DBUG_ENTER("write_row"); DBUG_ASSERT(m_table != NULL && thd != NULL); TABLE *table= m_table; // pointer to event's table - int error; - int UNINIT_VAR(keynum); const bool invoke_triggers= (m_table->triggers && do_invoke_trigger()); - auto_afree_ptr key(NULL); prepare_record(table, m_width, true); /* unpack row into table->record[0] */ - if (unlikely((error= unpack_current_row(rgi)))) + int error= unpack_current_row(rgi); + if (unlikely(error)) { table->file->print_error(error, MYF(0)); DBUG_RETURN(error); @@ -7843,178 +7887,11 @@ Rows_log_event::write_row(rpl_group_info *rgi, my_sleep(20000);); if (table->s->sequence) error= update_sequence(); - else while (unlikely(error= table->file->ha_write_row(table->record[0]))) + else { - if (error == HA_ERR_LOCK_DEADLOCK || - error == HA_ERR_LOCK_WAIT_TIMEOUT || - (keynum= table->file->get_dup_key(error)) < 0 || - !overwrite) - { - DBUG_PRINT("info",("get_dup_key returns %d)", keynum)); - /* - Deadlock, waiting for lock or just an error from the handler - such as HA_ERR_FOUND_DUPP_KEY when overwrite is false. - Retrieval of the duplicate key number may fail - - either because the error was not "duplicate key" error - - or because the information which key is not available - */ - table->file->print_error(error, MYF(0)); - DBUG_RETURN(error); - } - /* - We need to retrieve the old row into record[1] to be able to - either update or delete the offending record. We either: + error= m_write_record->write_record(); - - use rnd_pos() with a row-id (available as dupp_row) to the - offending row, if that is possible (MyISAM and Blackhole), or else - - - use index_read_idx() with the key that is duplicated, to - retrieve the offending row. - */ - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - { - DBUG_PRINT("info",("Locating offending record using rnd_pos()")); - - if ((error= table->file->ha_rnd_init_with_error(0))) - { - DBUG_RETURN(error); - } - - error= table->file->ha_rnd_pos(table->record[1], table->file->dup_ref); - if (unlikely(error)) - { - DBUG_PRINT("info",("rnd_pos() returns error %d",error)); - table->file->print_error(error, MYF(0)); - DBUG_RETURN(error); - } - table->file->ha_rnd_end(); - } - else - { - DBUG_PRINT("info",("Locating offending record using index_read_idx()")); - - if (table->file->extra(HA_EXTRA_FLUSH_CACHE)) - { - DBUG_PRINT("info",("Error when setting HA_EXTRA_FLUSH_CACHE")); - DBUG_RETURN(my_errno); - } - - if (key.get() == NULL) - { - key.assign(static_cast(my_alloca(table->s->max_unique_length))); - if (key.get() == NULL) - { - DBUG_PRINT("info",("Can't allocate key buffer")); - DBUG_RETURN(ENOMEM); - } - } - - key_copy((uchar*)key.get(), table->record[0], table->key_info + keynum, - 0); - error= table->file->ha_index_read_idx_map(table->record[1], keynum, - (const uchar*)key.get(), - HA_WHOLE_KEY, - HA_READ_KEY_EXACT); - if (unlikely(error)) - { - DBUG_PRINT("info",("index_read_idx() returns %s", HA_ERR(error))); - table->file->print_error(error, MYF(0)); - DBUG_RETURN(error); - } - } - - /* - Now, record[1] should contain the offending row. That - will enable us to update it or, alternatively, delete it (so - that we can insert the new row afterwards). - */ - if (table->s->long_unique_table) - { - /* same as for REPLACE/ODKU */ - table->move_fields(table->field, table->record[1], table->record[0]); - table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_REPLACE); - table->move_fields(table->field, table->record[0], table->record[1]); - } - - /* - If row is incomplete we will use the record found to fill - missing columns. - */ - if (!get_flags(COMPLETE_ROWS_F)) - { - restore_record(table,record[1]); - error= unpack_current_row(rgi); - if (table->s->long_unique_table) - table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE); - } - - DBUG_PRINT("debug",("preparing for update: before and after image")); - DBUG_DUMP("record[1] (before)", table->record[1], table->s->reclength); - DBUG_DUMP("record[0] (after)", table->record[0], table->s->reclength); - - /* - REPLACE is defined as either INSERT or DELETE + INSERT. If - possible, we can replace it with an UPDATE, but that will not - work on InnoDB if FOREIGN KEY checks are necessary. - - I (Matz) am not sure of the reason for the last_uniq_key() - check as, but I'm guessing that it's something along the - following lines. - - Suppose that we got the duplicate key to be a key that is not - the last unique key for the table and we perform an update: - then there might be another key for which the unique check will - fail, so we're better off just deleting the row and inserting - the correct row. - - Additionally we don't use UPDATE if rbr triggers should be invoked - - when triggers are used we want a simple and predictable execution path. - */ - if (last_uniq_key(table, keynum) && !invoke_triggers && - !table->file->referenced_by_foreign_key()) - { - DBUG_PRINT("info",("Updating row using ha_update_row()")); - error= table->file->ha_update_row(table->record[1], - table->record[0]); - switch (error) { - - case HA_ERR_RECORD_IS_THE_SAME: - DBUG_PRINT("info",("ignoring HA_ERR_RECORD_IS_THE_SAME error from" - " ha_update_row()")); - error= 0; - - case 0: - break; - - default: - DBUG_PRINT("info",("ha_update_row() returns error %d",error)); - table->file->print_error(error, MYF(0)); - } - - DBUG_RETURN(error); - } - else - { - DBUG_PRINT("info",("Deleting offending row and trying to write new one again")); - if (invoke_triggers && - unlikely(process_triggers(TRG_EVENT_DELETE, TRG_ACTION_BEFORE, - TRUE))) - error= HA_ERR_GENERIC; // in case if error is not set yet - else - { - if (unlikely((error= table->file->ha_delete_row(table->record[1])))) - { - DBUG_PRINT("info",("ha_delete_row() returns error %d",error)); - table->file->print_error(error, MYF(0)); - DBUG_RETURN(error); - } - if (invoke_triggers && - unlikely(process_triggers(TRG_EVENT_DELETE, TRG_ACTION_AFTER, - TRUE))) - DBUG_RETURN(HA_ERR_GENERIC); // in case if error is not set yet - } - /* Will retry ha_write_row() with the offending row removed. */ - } + DBUG_RETURN(error ? m_write_record->last_errno() : 0); } if (invoke_triggers && @@ -8663,7 +8540,8 @@ bool Delete_rows_compressed_log_event::write() #if defined(HAVE_REPLICATION) int -Delete_rows_log_event::do_before_row_operations(const Slave_reporting_capability *const) +Delete_rows_log_event::do_before_row_operations(rpl_group_info *rgi, + COPY_INFO*, Write_record*) { /* Increment the global status delete count variable @@ -8793,7 +8671,8 @@ void Update_rows_log_event::init(MY_BITMAP const *cols) #if defined(HAVE_REPLICATION) int -Update_rows_log_event::do_before_row_operations(const Slave_reporting_capability *const) +Update_rows_log_event::do_before_row_operations(rpl_group_info *, + COPY_INFO*, Write_record*) { /* Increment the global status update count variable diff --git a/sql/sql_class.h b/sql/sql_class.h index 4edc4a081b3..b48af3788a7 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -6395,6 +6395,8 @@ public: int send_data(List &items) override; }; +class Write_record; // defined in sql_insert.h + class select_insert :public select_result_interceptor { public: @@ -6402,13 +6404,14 @@ class select_insert :public select_result_interceptor { TABLE_LIST *table_list; TABLE *table; List *fields; + Write_record *write; ulonglong autoinc_value_of_last_inserted_row; // autogenerated or not COPY_INFO info; bool insert_into_view; select_insert(THD *thd_arg, TABLE_LIST *table_list_par, TABLE *table_par, List *fields_par, List *update_fields, List *update_values, enum_duplicates duplic, - bool ignore, select_result *sel_ret_list); + bool ignore, select_result *sel_ret_list, Write_record *write); ~select_insert(); int prepare(List &list, SELECT_LEX_UNIT *u) override; int prepare2(JOIN *join) override; @@ -6442,9 +6445,9 @@ public: Table_specification_st *create_info_par, Alter_info *alter_info_arg, List &select_fields,enum_duplicates duplic, bool ignore, - TABLE_LIST *select_tables_arg): + TABLE_LIST *select_tables_arg, Write_record *write): select_insert(thd_arg, table_arg, NULL, &select_fields, 0, 0, duplic, - ignore, NULL), + ignore, NULL, write), create_info(create_info_par), select_tables(select_tables_arg), alter_info(alter_info_arg), diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 6d0cbf90022..d3c38217bea 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -82,6 +82,7 @@ #include "debug.h" // debug_crash_here #include #include "rpl_rli.h" +#include "scope.h" #ifdef WITH_WSREP #include "wsrep_mysqld.h" /* wsrep_append_table_keys() */ @@ -693,6 +694,30 @@ Field **TABLE::field_to_fill() return triggers && triggers->nullable_fields() ? triggers->nullable_fields() : field; } +int prepare_for_replace(TABLE *table, enum_duplicates handle_duplicates, + bool ignore) +{ + bool create_lookup_handler= handle_duplicates != DUP_ERROR; + if (ignore || handle_duplicates != DUP_ERROR) + { + create_lookup_handler= true; + table->file->extra(HA_EXTRA_IGNORE_DUP_KEY); + if (table->file->ha_table_flags() & HA_DUPLICATE_POS || + table->s->long_unique_table) + { + if (table->file->ha_rnd_init_with_error(false)) + return 1; + } + } + return table->file->prepare_for_insert(create_lookup_handler); +} + +int finalize_replace(TABLE *table, enum_duplicates handle_duplicates, + bool ignore) +{ + return table->file->inited ? table->file->ha_rnd_end() : 0; +} + /** INSERT statement implementation @@ -786,6 +811,8 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, DBUG_RETURN(TRUE); value_count= values->elements; + Write_record write; + if ((res= mysql_prepare_insert(thd, table_list, fields, values, update_fields, update_values, duplic, ignore, &unused_conds, FALSE, &cache_insert_values))) @@ -878,7 +905,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, } its.rewind (); thd->get_stmt_da()->reset_current_row_for_warning(0); - + /* Restore the current context. */ ctx_state.restore_state(context, table_list); @@ -944,18 +971,8 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, if (lock_type != TL_WRITE_DELAYED) #endif /* EMBEDDED_LIBRARY */ { - bool create_lookup_handler= duplic != DUP_ERROR; - if (duplic != DUP_ERROR || ignore) - { - create_lookup_handler= true; - table->file->extra(HA_EXTRA_IGNORE_DUP_KEY); - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - { - if (table->file->ha_rnd_init_with_error(0)) - goto abort; - } - } - table->file->prepare_for_insert(create_lookup_handler); + if (prepare_for_replace(table, info.handle_duplicates, info.ignore)) + DBUG_RETURN(1); /** This is a simple check for the case when the table has a trigger that reads from it, or when the statement invokes a stored function @@ -1027,6 +1044,9 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, THD_STAGE_INFO(thd, stage_update); + + new (&write) Write_record(thd, table, &info, result); + if (duplic == DUP_UPDATE) { restore_record(table,s->default_values); // Get empty record @@ -1194,7 +1214,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, } else #endif - error= write_record(thd, table, &info, result); + error= write.write_record(); if (unlikely(error)) break; info.accepted_rows++; @@ -1254,8 +1274,7 @@ values_loop_end: if (duplic != DUP_ERROR || ignore) { table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY); - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - table->file->ha_rnd_end(); + finalize_replace(table, duplic, ignore); } transactional_table= table->file->has_transactions_and_rollback(); @@ -1891,31 +1910,74 @@ int mysql_prepare_insert(THD *thd, TABLE_LIST *table_list, } - /* Check if there is more uniq keys after field */ - -static int last_uniq_key(TABLE *table, const KEY *key, uint keynr) +/* Check if there is more uniq keys after field */ +static ushort get_last_unique_key(TABLE *table, KEY *key_info) { + for (uint key= table->s->keys; key; --key) + if (key_info[key-1].flags & HA_NOSAME) + return key-1; + return MAX_KEY; +} + +/** + An update optimization can be done in REPLACE only when certain criterial met. + In particular, if the "error key" was the last one to check. If not, there can + be other keys that can fail the uniqueness check after we will delete the row. + + The problem arises with the order of the key checking. It is divided in two + parts: + 1. Higher-level handler::ha_write_row goes first. It checks for long unique + and period overlaps (the latter is not supported by REPLACE yet). + 2. Engine usual unique index check. + However, the order of the keys in TABLE_SHARE::key_info is: + * first, engine unique keys + * then long unique, which is not seen by engine + WITHOUT OVERLAPS key has no particular order + see sort_keys in sql_table.cc + + So we need to wrap this order. + @return last unique key to check for duplication, or MAX_KEY if none. + */ +ushort Write_record::get_last_unique_key() const +{ + if (info->handle_duplicates != DUP_REPLACE) + return MAX_KEY; // Not used. + if (!table->s->key_info) + return MAX_KEY; /* When an underlying storage engine informs that the unique key conflicts are not reported in the ascending order by setting the HA_DUPLICATE_KEY_NOT_IN_ORDER flag, we cannot rely on this information to determine the last key conflict. - + The information about the last key conflict will be used to do a replace of the new row on the conflicting row, rather than doing a delete (of old row) + insert (of new row). - + Hence check for this flag and disable replacing the last row - by returning 0 always. Returning 0 will result in doing + by returning MAX_KEY always. Returning MAX_KEY will result in doing a delete + insert always. */ if (table->file->ha_table_flags() & HA_DUPLICATE_KEY_NOT_IN_ORDER) - return 0; + return MAX_KEY; - while (++keynr < table->s->keys) - if (key[keynr].flags & HA_NOSAME) - return 0; - return 1; + /* + Note, that TABLE_SHARE and TABLE see long uniques differently: + - TABLE_SHARE sees as HA_KEY_ALG_LONG_HASH and HA_NOSAME + - TABLE sees as usual non-unique indexes + */ + return + table->key_info[0].flags & HA_NOSAME ? + /* + We have an in-engine unique, which should be checked last. + Go through the TABLE list, which knows nothing about long uniques. + */ + ::get_last_unique_key(table, table->key_info) : + /* + We can only have long uniques. + Go through the TABLE_SHARE list, where long uniques can be found. + */ + ::get_last_unique_key(table, table->s->key_info); } @@ -1944,19 +2006,353 @@ int vers_insert_history_row(TABLE *table) } /* + Retrieve the old (duplicated) row into record[1] to be able to + either update or delete the offending record. We either: + + - use ha_rnd_pos() with a row-id (available as dup_ref) to the + offending row, if that is possible (MyISAM and Blackhole, also long unique + and application-time periods), or else + + - use ha_index_read_idx_map() with the key that is duplicated, to + retrieve the offending row. + */ +int Write_record::locate_dup_record() +{ + handler *h= table->file; + int error= 0; + if (h->ha_table_flags() & HA_DUPLICATE_POS || h->lookup_errkey != (uint)-1) + { + DBUG_PRINT("info", ("Locating offending record using rnd_pos()")); + + error= h->ha_rnd_pos(table->record[1], h->dup_ref); + if (unlikely(error)) + { + DBUG_PRINT("info", ("rnd_pos() returns error %d",error)); + h->print_error(error, MYF(0)); + } + } + else + { + DBUG_PRINT("info", + ("Locating offending record using ha_index_read_idx_map")); + + if (h->lookup_handler) + h= h->lookup_handler; + + error= h->extra(HA_EXTRA_FLUSH_CACHE); + if (unlikely(error)) + { + DBUG_PRINT("info",("Error when setting HA_EXTRA_FLUSH_CACHE")); + return my_errno; + } + + if (unlikely(key == NULL)) + { + key= static_cast(thd->alloc(table->s->max_unique_length)); + if (key == NULL) + { + DBUG_PRINT("info",("Can't allocate key buffer")); + return ENOMEM; + } + } + + key_copy(key, table->record[0], table->key_info + key_nr, 0); + + error= h->ha_index_read_idx_map(table->record[1], key_nr, key, + HA_WHOLE_KEY, HA_READ_KEY_EXACT); + if (unlikely(error)) + { + DBUG_PRINT("info", ("index_read_idx() returns %d", error)); + h->print_error(error, MYF(0)); + } + } + + if (unlikely(error)) + return error; + + if (table->vfield) + { + /* + We have not yet called update_virtual_fields() + in handler methods for the just read row in record[1]. + */ + table->move_fields(table->field, table->record[1], table->record[0]); + error= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_REPLACE); + table->move_fields(table->field, table->record[0], table->record[1]); + } + return error; +} + +/** + REPLACE is defined as either INSERT or DELETE + INSERT. If + possible, we can replace it with an UPDATE, but that will not + work on InnoDB if FOREIGN KEY checks are necessary. + + Suppose that we got the duplicate key to be a key that is not + the last unique key for the table and we perform an update: + then there might be another key for which the unique check will + fail, so we're better off just deleting the row and trying to insert again. + + Additionally we don't use ha_update_row in following cases: + * when triggers should be invoked, as it may spoil the intermedite NEW_ROW + representation + * when we have referenced keys, as there can be different ON DELETE/ON UPDATE + actions +*/ +int Write_record::replace_row(ha_rows *inserted, ha_rows *deleted) +{ + int error; + while (unlikely(error=table->file->ha_write_row(table->record[0]))) + { + error= prepare_handle_duplicate(error); + if (unlikely(error)) + return restore_on_error(); + + if (incomplete_records_cb && (error= incomplete_records_cb(arg1, arg2))) + return restore_on_error(); + + if (can_optimize && key_nr == last_unique_key) + { + DBUG_PRINT("info", ("Updating row using ha_update_row()")); + if (table->versioned(VERS_TRX_ID)) + table->vers_start_field()->store(0, false); + + error= table->file->ha_update_row(table->record[1], table->record[0]); + + if (likely(!error)) + ++*deleted; + else if (error != HA_ERR_RECORD_IS_THE_SAME) + return on_ha_error(error); + + if (versioned) + { + store_record(table, record[2]); + error= vers_insert_history_row(table); + restore_record(table, record[2]); + if (unlikely(error)) + return on_ha_error(error); + } + + break; + } + else + { + DBUG_PRINT("info", ("Deleting offending row and trying to write" + " new one again")); + + auto *trg = table->triggers; + if (use_triggers && trg->process_triggers(table->in_use, TRG_EVENT_DELETE, + TRG_ACTION_BEFORE, TRUE)) + return restore_on_error(); + if (!versioned) + error = table->file->ha_delete_row(table->record[1]); + else + { + store_record(table, record[2]); + restore_record(table, record[1]); + table->vers_update_end(); + error = table->file->ha_update_row(table->record[1], table->record[0]); + restore_record(table, record[2]); + } + + if (unlikely(error)) + return on_ha_error(error); + ++*deleted; + + if (use_triggers) + { + error= trg->process_triggers(table->in_use, TRG_EVENT_DELETE, + TRG_ACTION_AFTER, TRUE); + if (unlikely(error)) + return restore_on_error(); + } + } + } + + /* + If more than one iteration of the above loop is done, from + the second one the row being inserted will have an explicit + value in the autoinc field, which was set at the first call of + handler::update_auto_increment(). This value is saved to avoid + thd->insert_id_for_cur_row becoming 0. Use this saved autoinc + value. + */ + if (table->file->insert_id_for_cur_row == 0) + table->file->insert_id_for_cur_row = insert_id_for_cur_row; + + return after_insert(inserted); +} + + +int Write_record::insert_on_duplicate_update(ha_rows *inserted, + ha_rows *updated) +{ + int res= table->file->ha_write_row(table->record[0]); + if (likely(!res)) + return after_insert(inserted); + + res=prepare_handle_duplicate(res); + if (unlikely(res)) + return restore_on_error(); + + ulonglong prev_insert_id_for_cur_row= 0; + /* + We don't check for other UNIQUE keys - the first row + that matches, is updated. If update causes a conflict again, + an error is returned + */ + DBUG_ASSERT(table->insert_values != NULL); + store_record(table,insert_values); + restore_record(table,record[1]); + table->reset_default_fields(); + + /* + in INSERT ... ON DUPLICATE KEY UPDATE the set of modified fields can + change per row. Thus, we have to do reset_default_fields() per row. + Twice (before insert and before update). + */ + DBUG_ASSERT(info->update_fields->elements == + info->update_values->elements); + if (fill_record_n_invoke_before_triggers(thd, table, + *info->update_fields, + *info->update_values, + info->ignore, + TRG_EVENT_UPDATE)) + return restore_on_error(); + + bool different_records= (!records_are_comparable(table) || + compare_record(table)); + /* + Default fields must be updated before checking view updateability. + This branch of INSERT is executed only when a UNIQUE key was violated + with the ON DUPLICATE KEY UPDATE option. In this case the INSERT + operation is transformed to an UPDATE, and the default fields must + be updated as if this is an UPDATE. + */ + if (different_records && table->default_field) + table->evaluate_update_default_function(); + + /* CHECK OPTION for VIEW ... ON DUPLICATE KEY UPDATE ... */ + res= info->table_list->view_check_option(table->in_use, info->ignore); + if (unlikely(res)) + { + if (res == VIEW_CHECK_ERROR) + return restore_on_error(); + DBUG_ASSERT(res == VIEW_CHECK_SKIP); + return 0; + } + + table->file->restore_auto_increment(prev_insert_id); + info->touched++; + if (different_records) + { + int error= table->file->ha_update_row(table->record[1], table->record[0]); + + if (unlikely(error) && error != HA_ERR_RECORD_IS_THE_SAME) + { + if (info->ignore && !table->file->is_fatal_error(error, HA_CHECK_ALL)) + { + if (!(thd->variables.old_behavior & + OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE)) + table->file->print_error(error, MYF(ME_WARNING)); + return 0; + } + return on_ha_error(error); + } + + if (error != HA_ERR_RECORD_IS_THE_SAME) + { + ++*updated; + if (table->versioned() && + table->vers_check_update(*info->update_fields)) + { + if (versioned) + { + store_record(table, record[2]); + error= vers_insert_history_row(table); + restore_record(table, record[2]); + if (unlikely(error)) + return on_ha_error(error); + } + ++*inserted; + } + } + /* + If ON DUP KEY UPDATE updates a row instead of inserting + one, it's like a regular UPDATE statement: it should not + affect the value of a next SELECT LAST_INSERT_ID() or + mysql_insert_id(). Except if LAST_INSERT_ID(#) was in the + INSERT query, which is handled separately by + THD::arg_of_last_insert_id_function. + */ + prev_insert_id_for_cur_row= table->file->insert_id_for_cur_row; + insert_id_for_cur_row= table->file->insert_id_for_cur_row= 0; + + ++*inserted; // Conforms the older behavior; + + if (use_triggers + && table->triggers->process_triggers(thd, TRG_EVENT_UPDATE, + TRG_ACTION_AFTER, TRUE)) + return restore_on_error(); + } + + /* + Only update next_insert_id if the AUTO_INCREMENT value was explicitly + updated, so we don't update next_insert_id with the value from the + row being updated. Otherwise reset next_insert_id to what it was + before the duplicate key error, since that value is unused. + */ + if (table->next_number_field_updated) + { + DBUG_ASSERT(table->next_number_field != NULL); + + table->file->adjust_next_insert_id_after_explicit_value( + table->next_number_field->val_int()); + } + else if (prev_insert_id_for_cur_row) + { + table->file->restore_auto_increment(prev_insert_id_for_cur_row); + } + + return send_data(); +} + +bool Write_record::is_fatal_error(int error) +{ + return error == HA_ERR_LOCK_DEADLOCK || + error == HA_ERR_LOCK_WAIT_TIMEOUT || + table->file->is_fatal_error(error, HA_CHECK_ALL); +} + +int Write_record::single_insert(ha_rows *inserted) +{ + DBUG_EXECUTE_IF("rpl_write_record_small_sleep_gtid_100_200", + { + uint64 seq= thd->rgi_slave ? thd->rgi_slave->current_gtid.seq_no : 0; + if (seq == 100 || seq == 200) + my_sleep(20000); + }); + + int error= table->file->ha_write_row(table->record[0]); + if (unlikely(error)) + { + DEBUG_SYNC(thd, "write_row_noreplace"); + if (!info->ignore || is_fatal_error(error)) + return on_ha_error(error); + if (!(thd->variables.old_behavior & + OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE)) + table->file->print_error(error, MYF(ME_WARNING)); + table->file->restore_auto_increment(prev_insert_id); + return 0; + } + return after_insert(inserted); +} + +/** Write a record to table with optional deleting of conflicting records, invoke proper triggers if needed. - SYNOPSIS - write_record() - thd - thread context - table - table to which record should be written - info - COPY_INFO structure describing handling of duplicates - and which is used for counting number of records inserted - and deleted. - sink - result sink for the RETURNING clause - - NOTE + @note Once this record will be written to table after insert trigger will be invoked. If instead of inserting new record we will update old one then both on update triggers will work instead. Similarly both on @@ -1965,466 +2361,156 @@ int vers_insert_history_row(TABLE *table) Sets thd->transaction.stmt.modified_non_trans_table to TRUE if table which is updated didn't have transactions. - RETURN VALUE + @return 0 - success non-0 - error */ - - -int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) +int Write_record::write_record() { - int error, trg_error= 0; - char *key=0; - MY_BITMAP *save_read_set, *save_write_set; - ulonglong prev_insert_id= table->file->next_insert_id; - ulonglong insert_id_for_cur_row= 0; - ulonglong prev_insert_id_for_cur_row= 0; - DBUG_ENTER("write_record"); - + ha_rows inserted= 0, updated= 0; + prev_insert_id= table->file->next_insert_id; info->records++; - save_read_set= table->read_set; - save_write_set= table->write_set; - - DBUG_EXECUTE_IF("rpl_write_record_small_sleep_gtid_100_200", - { - if (thd->rgi_slave && (thd->rgi_slave->current_gtid.seq_no == 100 || - thd->rgi_slave->current_gtid.seq_no == 200)) - my_sleep(20000); - }); - if (info->handle_duplicates == DUP_REPLACE || - info->handle_duplicates == DUP_UPDATE) + ignored_error= false; + int res= 0; + switch(info->handle_duplicates) { - while (unlikely(error=table->file->ha_write_row(table->record[0]))) - { - uint key_nr; - /* - If we do more than one iteration of this loop, from the second one the - row will have an explicit value in the autoinc field, which was set at - the first call of handler::update_auto_increment(). So we must save - the autogenerated value to avoid thd->insert_id_for_cur_row to become - 0. - */ - if (table->file->insert_id_for_cur_row > 0) - insert_id_for_cur_row= table->file->insert_id_for_cur_row; - else - table->file->insert_id_for_cur_row= insert_id_for_cur_row; - bool is_duplicate_key_error; - if (table->file->is_fatal_error(error, HA_CHECK_ALL)) - goto err; - is_duplicate_key_error= - table->file->is_fatal_error(error, HA_CHECK_ALL & ~HA_CHECK_DUP); - if (!is_duplicate_key_error) - { - /* - We come here when we had an ignorable error which is not a duplicate - key error. In this we ignore error if ignore flag is set, otherwise - report error as usual. We will not do any duplicate key processing. - */ - if (info->ignore) - { - table->file->print_error(error, MYF(ME_WARNING)); - goto after_trg_or_ignored_err; /* Ignoring a not fatal error */ - } - goto err; - } - if (unlikely((int) (key_nr = table->file->get_dup_key(error)) < 0)) - { - error= HA_ERR_FOUND_DUPP_KEY; /* Database can't find key */ - goto err; - } - DEBUG_SYNC(thd, "write_row_replace"); + case DUP_ERROR: + res= single_insert(&inserted); + break; + case DUP_UPDATE: + res= insert_on_duplicate_update(&inserted, &updated); + info->updated+= updated; + break; + case DUP_REPLACE: + res= replace_row(&inserted, &updated); + if (versioned) + info->updated+= updated; + else + info->deleted+= updated; + } - /* Read all columns for the row we are going to replace */ - table->use_all_columns(); - /* - Don't allow REPLACE to replace a row when a auto_increment column - was used. This ensures that we don't get a problem when the - whole range of the key has been used. - */ - if (info->handle_duplicates == DUP_REPLACE && - key_nr == table->s->next_number_index && insert_id_for_cur_row > 0) - goto err; - if (table->file->has_dup_ref()) - { - /* - If engine doesn't support HA_DUPLICATE_POS, the handler may init to - INDEX, but dup_ref could also be set by lookup_handled (and then, - lookup_errkey is set, f.ex. long unique duplicate). + info->copied+= inserted; + if (inserted || updated) + notify_non_trans_table_modified(); + return res; +} - In such case, handler would stay uninitialized, so do it here. - */ - bool init_lookup_handler= table->file->lookup_errkey != (uint)-1 && - table->file->inited == handler::NONE; - if (init_lookup_handler && table->file->ha_rnd_init_with_error(false)) - goto err; - DBUG_ASSERT(table->file->inited == handler::RND); - int rnd_pos_err= table->file->ha_rnd_pos(table->record[1], - table->file->dup_ref); +int Write_record::prepare_handle_duplicate(int error) +{ + /* + If we do more than one iteration of the replace loop, from the second one the + row will have an explicit value in the autoinc field, which was set at + the first call of handler::update_auto_increment(). So we must save + the autogenerated value to avoid thd->insert_id_for_cur_row to become + 0. + */ + if (table->file->insert_id_for_cur_row > 0) + insert_id_for_cur_row= table->file->insert_id_for_cur_row; + else + table->file->insert_id_for_cur_row= insert_id_for_cur_row; - if (init_lookup_handler) - table->file->ha_rnd_end(); - if (rnd_pos_err) - goto err; - } - else - { - if (table->file->extra(HA_EXTRA_FLUSH_CACHE)) /* Not needed with NISAM */ - { - error=my_errno; - goto err; - } + if (is_fatal_error(error)) + return on_ha_error(error); - if (!key) - { - if (!(key=(char*) my_safe_alloca(table->s->max_unique_length))) - { - error=ENOMEM; - goto err; - } - } - key_copy((uchar*) key,table->record[0],table->key_info+key_nr,0); - key_part_map keypart_map= (1 << table->key_info[key_nr].user_defined_key_parts) - 1; - if ((error= (table->file->ha_index_read_idx_map(table->record[1], - key_nr, (uchar*) key, - keypart_map, - HA_READ_KEY_EXACT)))) - goto err; - } - if (table->vfield) - { - /* - We have not yet called update_virtual_fields(VOL_UPDATE_FOR_READ) - in handler methods for the just read row in record[1]. - */ - table->move_fields(table->field, table->record[1], table->record[0]); - int verr = table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_REPLACE); - table->move_fields(table->field, table->record[0], table->record[1]); - if (verr) - goto err; - } - if (info->handle_duplicates == DUP_UPDATE) - { - int res= 0; - /* - We don't check for other UNIQUE keys - the first row - that matches, is updated. If update causes a conflict again, - an error is returned - */ - DBUG_ASSERT(table->insert_values != NULL); - store_record(table,insert_values); - restore_record(table,record[1]); - table->reset_default_fields(); - - /* - in INSERT ... ON DUPLICATE KEY UPDATE the set of modified fields can - change per row. Thus, we have to do reset_default_fields() per row. - Twice (before insert and before update). - */ - DBUG_ASSERT(info->update_fields->elements == - info->update_values->elements); - if (fill_record_n_invoke_before_triggers(thd, table, - *info->update_fields, - *info->update_values, - info->ignore, - TRG_EVENT_UPDATE)) - goto before_trg_err; - - bool different_records= (!records_are_comparable(table) || - compare_record(table)); - /* - Default fields must be updated before checking view updateability. - This branch of INSERT is executed only when a UNIQUE key was violated - with the ON DUPLICATE KEY UPDATE option. In this case the INSERT - operation is transformed to an UPDATE, and the default fields must - be updated as if this is an UPDATE. - */ - if (different_records && table->default_field) - table->evaluate_update_default_function(); - - /* CHECK OPTION for VIEW ... ON DUPLICATE KEY UPDATE ... */ - res= info->table_list->view_check_option(table->in_use, info->ignore); - if (res == VIEW_CHECK_SKIP) - goto after_trg_or_ignored_err; - if (res == VIEW_CHECK_ERROR) - goto before_trg_err; - - table->file->restore_auto_increment(prev_insert_id); - info->touched++; - if (different_records) - { - if (unlikely(error=table->file->ha_update_row(table->record[1], - table->record[0])) && - error != HA_ERR_RECORD_IS_THE_SAME) - { - if (info->ignore && - !table->file->is_fatal_error(error, HA_CHECK_ALL)) - { - if (!(thd->variables.old_behavior & - OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE)) - table->file->print_error(error, MYF(ME_WARNING)); - goto after_trg_or_ignored_err; - } - goto err; - } - - if (error != HA_ERR_RECORD_IS_THE_SAME) - { - info->updated++; - if (table->versioned() && - table->vers_check_update(*info->update_fields)) - { - if (table->versioned(VERS_TIMESTAMP)) - { - store_record(table, record[2]); - if ((error= vers_insert_history_row(table))) - { - info->last_errno= error; - table->file->print_error(error, MYF(0)); - trg_error= 1; - restore_record(table, record[2]); - goto after_trg_or_ignored_err; - } - restore_record(table, record[2]); - } - info->copied++; - } - } - else - error= 0; - /* - If ON DUP KEY UPDATE updates a row instead of inserting - one, it's like a regular UPDATE statement: it should not - affect the value of a next SELECT LAST_INSERT_ID() or - mysql_insert_id(). Except if LAST_INSERT_ID(#) was in the - INSERT query, which is handled separately by - THD::arg_of_last_insert_id_function. - */ - prev_insert_id_for_cur_row= table->file->insert_id_for_cur_row; - insert_id_for_cur_row= table->file->insert_id_for_cur_row= 0; - trg_error= (table->triggers && - table->triggers->process_triggers(thd, TRG_EVENT_UPDATE, - TRG_ACTION_AFTER, TRUE)); - info->copied++; - } - - /* - Only update next_insert_id if the AUTO_INCREMENT value was explicitly - updated, so we don't update next_insert_id with the value from the - row being updated. Otherwise reset next_insert_id to what it was - before the duplicate key error, since that value is unused. - */ - if (table->next_number_field_updated) - { - DBUG_ASSERT(table->next_number_field != NULL); - - table->file->adjust_next_insert_id_after_explicit_value(table->next_number_field->val_int()); - } - else if (prev_insert_id_for_cur_row) - { - table->file->restore_auto_increment(prev_insert_id_for_cur_row); - } - goto ok; - } - else /* DUP_REPLACE */ - { - /* - The manual defines the REPLACE semantics that it is either - an INSERT or DELETE(s) + INSERT; FOREIGN KEY checks in - InnoDB do not function in the defined way if we allow MySQL - to convert the latter operation internally to an UPDATE. - We also should not perform this conversion if we have - timestamp field with ON UPDATE which is different from DEFAULT. - Another case when conversion should not be performed is when - we have ON DELETE trigger on table so user may notice that - we cheat here. Note that it is ok to do such conversion for - tables which have ON UPDATE but have no ON DELETE triggers, - we just should not expose this fact to users by invoking - ON UPDATE triggers. - - Note, TABLE_SHARE and TABLE see long uniques differently: - - TABLE_SHARE sees as HA_KEY_ALG_LONG_HASH and HA_NOSAME - - TABLE sees as usual non-unique indexes - */ - bool is_long_unique= table->s->key_info && - table->s->key_info[key_nr].algorithm == - HA_KEY_ALG_LONG_HASH; - if ((is_long_unique ? - /* - We have a long unique. Test that there are no in-engine - uniques and the current long unique is the last long unique. - */ - !(table->key_info[0].flags & HA_NOSAME) && - last_uniq_key(table, table->s->key_info, key_nr) : - /* - We have a normal key - not a long unique. - Test is the current normal key is unique and - it is the last normal unique. - */ - last_uniq_key(table, table->key_info, key_nr)) && - !table->file->referenced_by_foreign_key() && - (!table->triggers || !table->triggers->has_delete_triggers())) - { - /* - Optimized dup handling via UPDATE (and insert history for versioned). - */ - if (table->versioned(VERS_TRX_ID)) - { - DBUG_ASSERT(table->vers_write); - bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); - table->file->column_bitmaps_signal(); - table->vers_start_field()->store(0, false); - } - if (unlikely(error= table->file->ha_update_row(table->record[1], - table->record[0])) && - error != HA_ERR_RECORD_IS_THE_SAME) - goto err; - if (likely(!error)) - { - info->deleted++; - if (!table->file->has_transactions()) - thd->transaction->stmt.modified_non_trans_table= TRUE; - if (table->versioned(VERS_TIMESTAMP) && table->vers_write) - { - store_record(table, record[2]); - error= vers_insert_history_row(table); - restore_record(table, record[2]); - if (unlikely(error)) - goto err; - } - } - else - error= 0; // error was HA_ERR_RECORD_IS_THE_SAME - /* - Since we pretend that we have done insert we should call - its after triggers. - */ - goto after_trg_n_copied_inc; - } - else - { - /* - Normal dup handling via DELETE (or UPDATE to history for versioned) - and repeating the cycle of INSERT. - */ - if (table->triggers && - table->triggers->process_triggers(thd, TRG_EVENT_DELETE, - TRG_ACTION_BEFORE, TRUE)) - goto before_trg_err; - - bool do_delete= !table->versioned(VERS_TIMESTAMP); - if (do_delete) - error= table->file->ha_delete_row(table->record[1]); - else - { - /* Update existing row to history */ - store_record(table, record[2]); - restore_record(table, record[1]); - table->vers_update_end(); - error= table->file->ha_update_row(table->record[1], - table->record[0]); - restore_record(table, record[2]); - if (error == HA_ERR_FOUND_DUPP_KEY || /* Unique index, any SE */ - error == HA_ERR_FOREIGN_DUPLICATE_KEY || /* Unique index, InnoDB */ - error == HA_ERR_RECORD_IS_THE_SAME) /* No index */ - { - /* Such history row was already generated from previous cycles */ - error= table->file->ha_delete_row(table->record[1]); - do_delete= true; - } - } - if (unlikely(error)) - goto err; - if (do_delete) - info->deleted++; - else - info->updated++; - if (!table->file->has_transactions_and_rollback()) - thd->transaction->stmt.modified_non_trans_table= TRUE; - if (table->triggers && - table->triggers->process_triggers(thd, TRG_EVENT_DELETE, - TRG_ACTION_AFTER, TRUE)) - { - trg_error= 1; - goto after_trg_or_ignored_err; - } - /* Let us attempt do write_row() once more */ - } - } - } - + bool is_duplicate_key_error= + table->file->is_fatal_error(error, HA_CHECK_ALL & ~HA_CHECK_DUP); + if (!is_duplicate_key_error) + { /* - If more than one iteration of the above while loop is done, from - the second one the row being inserted will have an explicit - value in the autoinc field, which was set at the first call of - handler::update_auto_increment(). This value is saved to avoid - thd->insert_id_for_cur_row becoming 0. Use this saved autoinc - value. - */ - if (table->file->insert_id_for_cur_row == 0) - table->file->insert_id_for_cur_row= insert_id_for_cur_row; - - /* - Restore column maps if they where replaced during an duplicate key - problem. + We come here when we had an ignorable error which is not a duplicate + key error. In this we ignore error if ignore flag is set, otherwise + report error as usual. We will not do any duplicate key processing. */ - if (table->read_set != save_read_set || - table->write_set != save_write_set) - table->column_bitmaps_set(save_read_set, save_write_set); - } - else if (unlikely((error=table->file->ha_write_row(table->record[0])))) - { - DEBUG_SYNC(thd, "write_row_noreplace"); - if (!info->ignore || - table->file->is_fatal_error(error, HA_CHECK_ALL)) - goto err; - if (!(thd->variables.old_behavior & - OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE)) + if (info->ignore) + { table->file->print_error(error, MYF(ME_WARNING)); - table->file->restore_auto_increment(prev_insert_id); - goto after_trg_or_ignored_err; + ignored_error= true; + return 1; /* Ignoring a not fatal error */ + } + return on_ha_error(error); } -after_trg_n_copied_inc: - info->copied++; - thd->record_first_successful_insert_id_in_cur_stmt(table->file->insert_id_for_cur_row); - trg_error= (table->triggers && - table->triggers->process_triggers(thd, TRG_EVENT_INSERT, - TRG_ACTION_AFTER, TRUE)); + key_nr = table->file->get_dup_key(error); -ok: + if (unlikely(key_nr == std::numeric_limits::max())) + return on_ha_error(HA_ERR_FOUND_DUPP_KEY); /* Database can't find key */ + + DEBUG_SYNC(thd, "write_row_replace"); + + /* Read all columns for the row we are going to replace */ + table->use_all_columns(); + /* + Don't allow REPLACE to replace a row when an auto_increment column + was used. This ensures that we don't get a problem when the + whole range of the key has been used. + */ + if (info->handle_duplicates == DUP_REPLACE && table->next_number_field && + key_nr == table->s->next_number_index && insert_id_for_cur_row > 0) + return on_ha_error(error); + + error= locate_dup_record(); + if (error) + return on_ha_error(error); + + return 0; +} + +inline +void Write_record::notify_non_trans_table_modified() +{ + if (!table->file->has_transactions_and_rollback()) + thd->transaction->stmt.modified_non_trans_table= TRUE; +} + +inline +int Write_record::on_ha_error(int error) +{ + info->last_errno= error; + table->file->print_error(error,MYF(0)); + DBUG_PRINT("info", ("Returning error %d", error)); + return restore_on_error(); +} + +inline +int Write_record::restore_on_error() +{ + table->file->restore_auto_increment(prev_insert_id); + return ignored_error ? 0 : 1; +} + +inline +int Write_record::after_insert(ha_rows *inserted) +{ + ++*inserted; + thd->record_first_successful_insert_id_in_cur_stmt(table->file->insert_id_for_cur_row); + return after_ins_trg() || send_data(); +} + +inline +int Write_record::after_ins_trg() +{ + int res= 0; + if (use_triggers) + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, + TRG_ACTION_AFTER, TRUE); + return res; +} + +inline +int Write_record::send_data() +{ /* We send the row after writing it to the table so that the correct values are sent to the client. Otherwise it won't show autoinc values (generated inside the handler::ha_write()) and values updated in ON DUPLICATE KEY UPDATE. */ - if (sink) - { - if (sink->send_data(thd->lex->returning()->item_list) < 0) - trg_error= 1; - } - -after_trg_or_ignored_err: - if (key) - my_safe_afree(key,table->s->max_unique_length); - if (!table->file->has_transactions_and_rollback()) - thd->transaction->stmt.modified_non_trans_table= TRUE; - DBUG_RETURN(trg_error); - -err: - info->last_errno= error; - table->file->print_error(error,MYF(0)); - -before_trg_err: - table->file->restore_auto_increment(prev_insert_id); - if (key) - my_safe_afree(key, table->s->max_unique_length); - table->column_bitmaps_set(save_read_set, save_write_set); - DBUG_RETURN(1); + return sink ? sink->send_data(thd->lex->returning()->item_list) < 0 : 0; } + /****************************************************************************** Check that there aren't any null_fields ******************************************************************************/ @@ -3715,6 +3801,7 @@ bool Delayed_insert::handle_inserts(void) ulong max_rows; bool using_ignore= 0, using_opt_replace= 0, using_bin_log; delayed_row *row; + Write_record write; DBUG_ENTER("handle_inserts"); /* Allow client to insert new rows */ @@ -3855,7 +3942,9 @@ bool Delayed_insert::handle_inserts(void) VCOL_UPDATE_FOR_WRITE); } - if (unlikely(tmp_error || write_record(&thd, table, &info, NULL))) + new (&write) Write_record(&thd, table, &info); + + if (unlikely(tmp_error || write.write_record())) { info.error_count++; // Ignore errors thread_safe_increment(delayed_insert_errors,&LOCK_delayed_status); @@ -4080,10 +4169,12 @@ select_insert::select_insert(THD *thd_arg, TABLE_LIST *table_list_par, List *update_values, enum_duplicates duplic, bool ignore_check_option_errors, - select_result *result): + select_result *result, + Write_record *write): select_result_interceptor(thd_arg), sel_result(result), table_list(table_list_par), table(table_par), fields(fields_par), + write(write), autoinc_value_of_last_inserted_row(0), insert_into_view(table_list_par && table_list_par->view != 0) { @@ -4264,18 +4355,10 @@ select_insert::prepare(List &values, SELECT_LEX_UNIT *u) #endif thd->cuted_fields=0; - bool create_lookup_handler= info.handle_duplicates != DUP_ERROR; - if (info.ignore || info.handle_duplicates != DUP_ERROR) - { - create_lookup_handler= true; - table->file->extra(HA_EXTRA_IGNORE_DUP_KEY); - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - { - if (table->file->ha_rnd_init_with_error(0)) - DBUG_RETURN(1); - } - } - table->file->prepare_for_insert(create_lookup_handler); + + if (prepare_for_replace(table, info.handle_duplicates, info.ignore)) + DBUG_RETURN(1); + if (info.handle_duplicates == DUP_REPLACE && (!table->triggers || !table->triggers->has_delete_triggers())) table->file->extra(HA_EXTRA_WRITE_CAN_REPLACE); @@ -4290,6 +4373,7 @@ select_insert::prepare(List &values, SELECT_LEX_UNIT *u) table->prepare_triggers_for_insert_stmt_or_event(); table->mark_columns_needed_for_insert(); } + new (write) Write_record(thd, table, &info, sel_result); DBUG_RETURN(res); } @@ -4380,7 +4464,7 @@ int select_insert::send_data(List &values) } } - error= write_record(thd, table, &info, sel_result); + error= write->write_record(); table->auto_increment_field_not_null= FALSE; if (likely(!error)) @@ -4467,9 +4551,8 @@ bool select_insert::prepare_eof() if (likely(!error) && unlikely(thd->is_error())) error= thd->get_stmt_da()->sql_errno(); - if (info.ignore || info.handle_duplicates != DUP_ERROR) - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - table->file->ha_rnd_end(); + if (unlikely(finalize_replace(table, info.handle_duplicates, info.ignore))) + DBUG_RETURN(1); if (error <= 0) { error= table->file->extra(HA_EXTRA_END_ALTER_COPY); @@ -4615,8 +4698,7 @@ void select_insert::abort_result_set() if (thd->locked_tables_mode <= LTM_LOCK_TABLES) table->file->ha_end_bulk_insert(); - if (table->file->inited) - table->file->ha_rnd_end(); + finalize_replace(table, info.handle_duplicates, info.ignore); table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY); table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE); table->file->extra(HA_EXTRA_ABORT_ALTER_COPY); @@ -5076,18 +5158,8 @@ select_create::prepare(List &_values, SELECT_LEX_UNIT *u) restore_record(table,s->default_values); // Get empty record thd->cuted_fields=0; - bool create_lookup_handler= info.handle_duplicates != DUP_ERROR; - if (info.ignore || info.handle_duplicates != DUP_ERROR) - { - create_lookup_handler= true; - table->file->extra(HA_EXTRA_IGNORE_DUP_KEY); - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) - { - if (table->file->ha_rnd_init_with_error(0)) - DBUG_RETURN(1); - } - } - table->file->prepare_for_insert(create_lookup_handler); + if (prepare_for_replace(table, info.handle_duplicates, info.ignore)) + DBUG_RETURN(1); if (info.handle_duplicates == DUP_REPLACE && (!table->triggers || !table->triggers->has_delete_triggers())) table->file->extra(HA_EXTRA_WRITE_CAN_REPLACE); @@ -5107,6 +5179,7 @@ select_create::prepare(List &_values, SELECT_LEX_UNIT *u) table->mark_columns_needed_for_insert(); // Mark table as used table->query_id= thd->query_id; + new (write) Write_record(thd, table, &info, sel_result); DBUG_RETURN(0); } @@ -5504,10 +5577,7 @@ void select_create::abort_result_set() thd->restore_tmp_table_share(saved_tmp_table_share); } - if (table->file->inited && - (info.ignore || info.handle_duplicates != DUP_ERROR) && - (table->file->ha_table_flags() & HA_DUPLICATE_POS)) - table->file->ha_rnd_end(); + finalize_replace(table, info.handle_duplicates, info.ignore); table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY); table->file->extra(HA_EXTRA_WRITE_CANNOT_REPLACE); table->auto_increment_field_not_null= FALSE; diff --git a/sql/sql_insert.h b/sql/sql_insert.h index 176c861cfad..163708d70c5 100644 --- a/sql/sql_insert.h +++ b/sql/sql_insert.h @@ -23,6 +23,7 @@ typedef List List_item; typedef struct st_copy_info COPY_INFO; + int mysql_prepare_insert(THD *thd, TABLE_LIST *table_list, List &fields, List_item *values, List &update_fields, @@ -46,7 +47,10 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, void kill_delayed_threads(void); bool binlog_create_table(THD *thd, TABLE *table, bool replace); bool binlog_drop_table(THD *thd, TABLE *table); - +int prepare_for_replace(TABLE *table, enum_duplicates handle_duplicates, + bool ignore); +int finalize_replace(TABLE *table, enum_duplicates handle_duplicates, + bool ignore); static inline void restore_default_record_for_insert(TABLE *t) { restore_record(t,s->default_values); @@ -54,6 +58,87 @@ static inline void restore_default_record_for_insert(TABLE *t) t->triggers->default_extra_null_bitmap(); } + +class Write_record +{ + THD *thd; + TABLE *table; + COPY_INFO *info; + + ulonglong prev_insert_id; + ulonglong insert_id_for_cur_row= 0; + uchar *key; + ushort key_nr; + + ushort last_unique_key; + bool use_triggers; + bool versioned; + bool can_optimize; + bool ignored_error; + + int (*incomplete_records_cb)(void *arg1, void *arg2); + void *arg1, *arg2; + select_result *sink; + + ushort get_last_unique_key() const; + // FINALIZATION + void notify_non_trans_table_modified(); + int after_insert(ha_rows *inserted); + int after_ins_trg(); + int send_data(); + + int on_ha_error(int error); + int restore_on_error(); + + bool is_fatal_error(int error); + int prepare_handle_duplicate(int error); + int locate_dup_record(); + + int replace_row(ha_rows *inserted, ha_rows *deleted); + int insert_on_duplicate_update(ha_rows *inserted, ha_rows *updated); + int single_insert(ha_rows *inserted); +public: + + /** + @param thd thread context + @param info COPY_INFO structure describing handling of duplicates + and which is used for counting number of records inserted + and deleted. + @param sink result sink for the RETURNING clause + @param table + @param versioned + @param use_triggers + */ + Write_record(THD *thd, TABLE *table, COPY_INFO *info, + bool versioned, bool use_triggers, select_result *sink, + int (*incomplete_records_cb)(void *, void *), + void *arg1, void* arg2): + thd(thd), table(table), info(info), insert_id_for_cur_row(0), + key(NULL), last_unique_key(get_last_unique_key()), + use_triggers(use_triggers), versioned(versioned), + incomplete_records_cb(incomplete_records_cb), arg1(arg1), arg2(arg2), + sink(sink) + { + if (info->handle_duplicates == DUP_REPLACE) + { + bool has_delete_triggers= use_triggers && + table->triggers->has_delete_triggers(); + bool referenced_by_fk= table->file->referenced_by_foreign_key(); + can_optimize= !referenced_by_fk && !has_delete_triggers; + } + } + Write_record(THD *thd, TABLE *table, COPY_INFO *info, + select_result *sink= NULL): + Write_record(thd, table, info, table->versioned(VERS_TIMESTAMP), + table->triggers, sink, NULL, NULL, NULL) + {} + Write_record() = default; // dummy, to allow later (lazy) initializations + + /* Main entry point, see docs in sql_insert.cc */ + int write_record(); + + int last_errno() { return info->last_errno; } +}; #ifdef EMBEDDED_LIBRARY inline void kill_delayed_threads(void) {} #endif diff --git a/sql/sql_load.cc b/sql/sql_load.cc index fa4f8a57607..26b64d99bf4 100644 --- a/sql/sql_load.cc +++ b/sql/sql_load.cc @@ -684,14 +684,9 @@ int mysql_load(THD *thd, const sql_exchange *ex, TABLE_LIST *table_list, thd->abort_on_warning= !ignore && thd->is_strict_mode(); thd->get_stmt_da()->reset_current_row_for_warning(1); - bool create_lookup_handler= handle_duplicates != DUP_ERROR; - if ((table_list->table->file->ha_table_flags() & HA_DUPLICATE_POS)) - { - create_lookup_handler= true; - if ((error= table_list->table->file->ha_rnd_init_with_error(0))) - goto err; - } - table->file->prepare_for_insert(create_lookup_handler); + if (prepare_for_replace(table, info.handle_duplicates, info.ignore)) + DBUG_RETURN(1); + thd_progress_init(thd, 2); fix_rownum_pointers(thd, thd->lex->current_select, &info.copied); if (table_list->table->validate_default_values_of_unset_fields(thd)) @@ -712,8 +707,8 @@ int mysql_load(THD *thd, const sql_exchange *ex, TABLE_LIST *table_list, set_fields, set_values, read_info, *ex->enclosed, skip_lines, ignore); - if (table_list->table->file->ha_table_flags() & HA_DUPLICATE_POS) - table_list->table->file->ha_rnd_end(); + if (unlikely(finalize_replace(table, handle_duplicates, ignore))) + DBUG_RETURN(1); thd_proc_info(thd, "End bulk insert"); if (likely(!error)) @@ -988,6 +983,8 @@ read_fixed_length(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, if ((thd->progress.max_counter= read_info.file_length()) == ~(my_off_t) 0) progress_reports= 0; + Write_record write(thd, table, &info, NULL); + while (!read_info.read_fixed_length()) { if (thd->killed) @@ -1068,7 +1065,7 @@ read_fixed_length(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, DBUG_RETURN(-1); } - err= write_record(thd, table, &info); + err= write.write_record(); table->auto_increment_field_not_null= FALSE; if (err) DBUG_RETURN(1); @@ -1117,6 +1114,8 @@ read_sep_field(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, if ((thd->progress.max_counter= read_info.file_length()) == ~(my_off_t) 0) progress_reports= 0; + Write_record write(thd, table, &info, NULL); + for (;;it.rewind()) { if (thd->killed) @@ -1218,7 +1217,7 @@ read_sep_field(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, DBUG_RETURN(-1); } - err= write_record(thd, table, &info); + err= write.write_record(); table->auto_increment_field_not_null= FALSE; if (err) DBUG_RETURN(1); @@ -1262,7 +1261,9 @@ read_xml_field(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, DBUG_ENTER("read_xml_field"); no_trans_update_stmt= !table->file->has_transactions_and_rollback(); - + + Write_record write(thd, table, &info, NULL); + for ( ; ; it.rewind()) { bool err; @@ -1339,7 +1340,7 @@ read_xml_field(THD *thd, COPY_INFO &info, TABLE_LIST *table_list, DBUG_RETURN(-1); } - err= write_record(thd, table, &info); + err= write.write_record(); table->auto_increment_field_not_null= false; if (err) DBUG_RETURN(1); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 3657f8127d2..758a57a0967 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4799,6 +4799,7 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) select_lex->context.table_list= select_lex->context.first_name_resolution_table= second_table; res= mysql_insert_select_prepare(thd, result); + Write_record write; if (!res && (sel_result= new (thd->mem_root) select_insert(thd, first_table, @@ -4808,7 +4809,8 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) &lex->value_list, lex->duplicates, lex->ignore, - result))) + result, + &write))) { if (lex->analyze_stmt) ((select_result_interceptor*)sel_result)->disable_my_ok_calls(); diff --git a/sql/sql_table.cc b/sql/sql_table.cc index ff6adb49363..089d364e73b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -13063,6 +13063,8 @@ bool Sql_cmd_create_table_like::execute(THD *thd) DEBUG_SYNC(thd, "wsrep_create_table_as_select"); + Write_record write; + /* select_create is currently not re-execution friendly and needs to be created for every execution of a PS/SP. @@ -13074,7 +13076,8 @@ bool Sql_cmd_create_table_like::execute(THD *thd) select_lex->item_list, lex->duplicates, lex->ignore, - select_tables))) + select_tables, + &write))) { /* CREATE from SELECT give its SELECT_LEX for SELECT, From 6353a80ef5cd6a4172ce46c4499b6202b9e69066 Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Tue, 7 Jan 2025 19:37:58 +0100 Subject: [PATCH 11/14] MDEV-15990 REPLACE on a precise-versioned table returns ER_DUP_ENTRY We had a protection against it, by allowing versioned delete if: trx->id != table->vers_start_id() For replace this check fails: replace calls ha_delete_row(record[2]), but table->vers_start_id() returns the value from record[0], which is irrelevant. The same problem hits Field::is_max, which may have checked the wrong record. Fix: * Refactor Field::is_max to optionally accept a pointer as an argument. * Refactor vers_start_id and vers_end_id to always accept a pointer to the record. there is a difference with is_max is that is_max accepts the pointer to the field data, rather than to the record. Method val_int() would be too effortful to refactor to accept the argument, so instead the value in record is fetched directly, like it is done in Field_longlong. --- mysql-test/suite/versioning/r/replace.result | 19 ++++++++++++++++++- mysql-test/suite/versioning/t/replace.test | 17 +++++++++++++++++ sql/field.cc | 12 ++++++------ sql/field.h | 19 ++++++++++++------- sql/handler.cc | 8 ++++++-- sql/log_event_server.cc | 8 +++++--- sql/sql_delete.cc | 3 ++- sql/sql_insert.cc | 13 ------------- sql/sql_insert.h | 3 ++- sql/table.cc | 5 +++++ sql/table.h | 6 +++--- storage/innobase/handler/ha_innodb.cc | 7 ++++--- 12 files changed, 80 insertions(+), 40 deletions(-) diff --git a/mysql-test/suite/versioning/r/replace.result b/mysql-test/suite/versioning/r/replace.result index c64f42ee7cf..7b654a821da 100644 --- a/mysql-test/suite/versioning/r/replace.result +++ b/mysql-test/suite/versioning/r/replace.result @@ -63,9 +63,11 @@ connection default; replace into t1 values (1),(2); connection con1; replace into t1 values (1),(2); -ERROR 23000: Duplicate entry '1' for key 'PRIMARY' drop table t1; # +# MDEV-14794 Limitations which the row end as a part of PK imposes due to +# CURRENT_TIMESTAMP behavior and otherwise +# # MDEV-15330 Server crash or assertion `table->insert_values' failure in write_record upon LOAD DATA # create table t1 (a int, b int, c int, vc int as (c), unique(a), unique(b)) with system versioning; @@ -88,3 +90,18 @@ Note 1831 Duplicate index `data_2`. This is deprecated and will be disallowed in alter table t1 add system versioning; replace into t1 values ('o'), ('o'); drop table t1; +# +# MDEV-15990 REPLACE on a precise-versioned table returns duplicate key +# error (ER_DUP_ENTRY) +# +create or replace table t1 ( +pk int primary key, i int, +row_start bigint unsigned as row start, +row_end bigint unsigned as row end, +period for system_time(row_start, row_end) +) engine=InnoDB with system versioning; +replace into t1 (pk,i) values (1,10),(1,100),(1,1000); +select pk, i, row_end from t1 for system_time all; +pk i row_end +1 1000 18446744073709551615 +drop table t1; diff --git a/mysql-test/suite/versioning/t/replace.test b/mysql-test/suite/versioning/t/replace.test index abae4f3d3cb..def092812f1 100644 --- a/mysql-test/suite/versioning/t/replace.test +++ b/mysql-test/suite/versioning/t/replace.test @@ -78,6 +78,7 @@ replace into t1 values (1),(2); --connection con1 --error ER_DUP_ENTRY replace into t1 values (1),(2); +drop table t1; drop table t1; @@ -116,4 +117,20 @@ alter table t1 add system versioning; replace into t1 values ('o'), ('o'); drop table t1; +--echo # +--echo # MDEV-15990 REPLACE on a precise-versioned table returns duplicate key +--echo # error (ER_DUP_ENTRY) +--echo # + +create or replace table t1 ( + pk int primary key, i int, + row_start bigint unsigned as row start, + row_end bigint unsigned as row end, + period for system_time(row_start, row_end) +) engine=InnoDB with system versioning; +replace into t1 (pk,i) values (1,10),(1,100),(1,1000); +select pk, i, row_end from t1 for system_time all; +drop table t1; + + --source suite/versioning/common_finish.inc diff --git a/sql/field.cc b/sql/field.cc index d6412417bfe..f9fd3f850eb 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -4723,17 +4723,17 @@ void Field_longlong::set_max() int8store(ptr, unsigned_flag ? ULONGLONG_MAX : LONGLONG_MAX); } -bool Field_longlong::is_max() +bool Field_longlong::is_max(const uchar *ptr_arg) const { DBUG_ASSERT(marked_for_read()); if (unsigned_flag) { ulonglong j; - j= uint8korr(ptr); + j= uint8korr(ptr_arg); return j == ULONGLONG_MAX; } longlong j; - j= sint8korr(ptr); + j= sint8korr(ptr_arg); return j == LONGLONG_MAX; } @@ -5792,13 +5792,13 @@ void Field_timestampf::set_max() DBUG_VOID_RETURN; } -bool Field_timestampf::is_max() +bool Field_timestampf::is_max(const uchar *ptr_arg) const { DBUG_ENTER("Field_timestampf::is_max"); DBUG_ASSERT(marked_for_read()); - DBUG_RETURN(mi_sint4korr(ptr) == TIMESTAMP_MAX_VALUE && - mi_sint3korr(ptr + 4) == TIME_MAX_SECOND_PART); + DBUG_RETURN(mi_sint4korr(ptr_arg) == TIMESTAMP_MAX_VALUE && + mi_sint3korr(ptr_arg + 4) == TIME_MAX_SECOND_PART); } my_time_t Field_timestampf::get_timestamp(const uchar *pos, diff --git a/sql/field.h b/sql/field.h index 31d08e2e544..e6e5c4c1ab7 100644 --- a/sql/field.h +++ b/sql/field.h @@ -803,8 +803,9 @@ public: */ virtual void set_max() { DBUG_ASSERT(0); } - virtual bool is_max() + virtual bool is_max(const uchar *ptr_arg) const { DBUG_ASSERT(0); return false; } + bool is_max() const { return is_max(ptr); } uchar *ptr; // Position to field in record @@ -2857,7 +2858,7 @@ public: return unpack_int64(to, from, from_end); } void set_max() override; - bool is_max() override; + bool is_max(const uchar *ptr_arg) const override; ulonglong get_max_int_value() const override { return unsigned_flag ? 0xFFFFFFFFFFFFFFFFULL : 0x7FFFFFFFFFFFFFFFULL; @@ -3437,7 +3438,7 @@ public: return memcmp(a_ptr, b_ptr, pack_length()); } void set_max() override; - bool is_max() override; + bool is_max(const uchar *ptr_arg) const override; my_time_t get_timestamp(const uchar *pos, ulong *sec_part) const override; bool val_native(Native *to) override; uint size_of() const override { return sizeof *this; } @@ -5973,17 +5974,21 @@ bool check_expression(Virtual_column_info *vcol, const LEX_CSTRING *name, #define f_visibility(x) (static_cast ((x) & INVISIBLE_MAX_BITS)) inline -ulonglong TABLE::vers_end_id() const +ulonglong TABLE::vers_end_id(const uchar *record_arg) const { DBUG_ASSERT(versioned(VERS_TRX_ID)); - return static_cast(vers_end_field()->val_int()); + DBUG_ASSERT(dynamic_cast(vers_end_field())); + const uchar *ptr= vers_end_field()->ptr_in_record(record_arg); + return static_cast(sint8korr(ptr)); } inline -ulonglong TABLE::vers_start_id() const +ulonglong TABLE::vers_start_id(const uchar *record_arg) const { DBUG_ASSERT(versioned(VERS_TRX_ID)); - return static_cast(vers_start_field()->val_int()); + DBUG_ASSERT(dynamic_cast(vers_start_field())); + const uchar *ptr= vers_start_field()->ptr_in_record(record_arg); + return static_cast(sint8korr(ptr)); } double pos_in_interval_for_string(CHARSET_INFO *cset, diff --git a/sql/handler.cc b/sql/handler.cc index 6ac0ee5b9a4..7f33cbd791f 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -7551,8 +7551,12 @@ int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) DBUG_ASSERT(this == table->file); if (!table_share->period.unique_keys) return 0; - if (table->versioned() && !table->vers_end_field()->is_max()) - return 0; + if (table->versioned()) + { + Field *end= table->vers_end_field(); + if (!end->is_max(end->ptr_in_record(new_data))) + return 0; + } const bool after_write= ha_table_flags() & HA_CHECK_UNIQUE_AFTER_WRITE; const bool is_update= !after_write && old_data; diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 49fbc11c436..91e51ce27da 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -8720,8 +8720,6 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi) return error; } - const bool history_change= m_table->versioned() ? - !m_table->vers_end_field()->is_max() : false; TABLE_LIST *tl= m_table->pos_in_table_list; uint8 trg_event_map_save= tl->trg_event_map; @@ -8772,8 +8770,12 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi) { if (m_vers_from_plain && m_table->versioned(VERS_TIMESTAMP)) m_table->vers_update_fields(); - if (!history_change && !m_table->vers_end_field()->is_max()) + Field *end= m_table->vers_end_field(); + const uchar *old_ptr= end->ptr_in_record(m_table->record[1]); + + if (end->is_max(old_ptr) && !end->is_max()) { + // This is a versioned delete, and we'll have to invoke ON DELETE actions tl->trg_event_map|= trg2bit(TRG_EVENT_DELETE); } } diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index c1d32fa4042..e5779410177 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -288,7 +288,8 @@ int update_portion_of_time(THD *thd, TABLE *table, inline int TABLE::delete_row() { - if (!versioned(VERS_TIMESTAMP) || !vers_end_field()->is_max()) + if (!versioned(VERS_TIMESTAMP) || + !vers_end_field()->is_max()) return file->ha_delete_row(record[0]); store_record(this, record[1]); diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index d3c38217bea..e4e472a2aaf 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -2114,25 +2114,12 @@ int Write_record::replace_row(ha_rows *inserted, ha_rows *deleted) if (can_optimize && key_nr == last_unique_key) { DBUG_PRINT("info", ("Updating row using ha_update_row()")); - if (table->versioned(VERS_TRX_ID)) - table->vers_start_field()->store(0, false); - error= table->file->ha_update_row(table->record[1], table->record[0]); if (likely(!error)) ++*deleted; else if (error != HA_ERR_RECORD_IS_THE_SAME) return on_ha_error(error); - - if (versioned) - { - store_record(table, record[2]); - error= vers_insert_history_row(table); - restore_record(table, record[2]); - if (unlikely(error)) - return on_ha_error(error); - } - break; } else diff --git a/sql/sql_insert.h b/sql/sql_insert.h index 163708d70c5..414dfb4ff73 100644 --- a/sql/sql_insert.h +++ b/sql/sql_insert.h @@ -124,7 +124,8 @@ public: bool has_delete_triggers= use_triggers && table->triggers->has_delete_triggers(); bool referenced_by_fk= table->file->referenced_by_foreign_key(); - can_optimize= !referenced_by_fk && !has_delete_triggers; + can_optimize= !referenced_by_fk && !has_delete_triggers + && !versioned && !table->versioned(); } } Write_record(THD *thd, TABLE *table, COPY_INFO *info, diff --git a/sql/table.cc b/sql/table.cc index 688e89ae018..5fcb5928865 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -7668,6 +7668,11 @@ static void do_mark_index_columns(TABLE *table, uint index, table->s->primary_key != MAX_KEY && table->s->primary_key != index) do_mark_index_columns(table, table->s->primary_key, bitmap, read); + if (table->versioned(VERS_TRX_ID)) + { + table->vers_start_field()->register_field_in_read_map(); + table->vers_end_field()->register_field_in_read_map(); + } } /* mark columns used by key, but don't reset other fields diff --git a/sql/table.h b/sql/table.h index 86502589ec0..924b1f77518 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1753,7 +1753,7 @@ public: check_assignability_all_visible_fields(values, ignore); } - bool insert_all_rows_into_tmp_table(THD *thd, + bool insert_all_rows_into_tmp_table(THD *thd, TABLE *tmp_table, TMP_TABLE_PARAM *tmp_table_param, bool with_cleanup); @@ -1839,8 +1839,8 @@ public: } - ulonglong vers_start_id() const; - ulonglong vers_end_id() const; + ulonglong vers_start_id(const uchar *ptr) const; + ulonglong vers_end_id(const uchar *ptr) const; #ifdef WITH_PARTITION_STORAGE_ENGINE bool vers_switch_partition(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 4ae491b88af..a4b400a9931 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -8623,7 +8623,7 @@ ha_innobase::update_row( if (error == DB_SUCCESS && m_prebuilt->versioned_write /* Multiple UPDATE of same rows in single transaction create historical rows only once. */ - && trx->id != table->vers_start_id()) { + && trx->id != table->vers_start_id(new_row)) { /* UPDATE is not used by ALTER TABLE. Just precaution as we don't need history generation for ALTER TABLE. */ ut_ad(thd_sql_command(m_user_thd) != SQLCOM_ALTER_TABLE); @@ -8738,8 +8738,9 @@ ha_innobase::delete_row( /* This is a delete */ m_prebuilt->upd_node->is_delete = table->versioned_write(VERS_TRX_ID) - && table->vers_end_field()->is_max() - && trx->id != table->vers_start_id() + && table->vers_end_field()->is_max( + table->vers_end_field()->ptr_in_record(record)) + && trx->id != table->vers_start_id(record) ? VERSIONED_DELETE : PLAIN_DELETE; trx->fts_next_doc_id = 0; From e56f6c585e8478f430dd7be648de6e80da96aab1 Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Sat, 30 Mar 2024 18:34:26 +0100 Subject: [PATCH 12/14] MDEV-15990 handle timestamp-based collisions as well Timestamp-versioned row deletion was exposed to a collisional problem: if current timestamp wasn't changed, then a sequence of row delete+insert could get a duplication error. A row delete would find another conflicting history row and return an error. This is true both for REPLACE and DELETE statements, however in REPLACE, the "optimized" path is usually taken, especially in the tests. There, delete+insert is substituted for a single versioned row update. In the end, both paths end up as ha_update_row + ha_write_row. The solution is to handle a history collision somehow. From the design perspective, the user shouldn't experience history rows loss, unless there's a technical limitation. To the contrary, trxid-based changes should never generate history for the same transaction, see MDEV-15427. If two operations on the same row happened too quickly, so that they happen at the same timestamp, the history row shouldn't be lost. We can still write a history row, though it'll have row_start == row_end. We cannot store more than one such historical row, as this will violate the unique constraint on row_end. So we will have to phisically delete the row if the history row is already available. In this commit: 1. Improve TABLE::delete_row to handle the history collision: if an update results with a duplicate error, delete a row for real. 2. use TABLE::delete_row in a non-optimistic path of REPLACE, where the system-versioned case now belongs entirely. --- mysql-test/suite/versioning/common.inc | 11 ++- mysql-test/suite/versioning/r/foreign.result | 5 +- mysql-test/suite/versioning/r/insert2.result | 6 -- .../suite/versioning/r/partition.result | 4 +- .../suite/versioning/r/replace,trx_id.rdiff | 26 ++++++ mysql-test/suite/versioning/r/replace.result | 58 +++++++++++-- mysql-test/suite/versioning/t/foreign.test | 1 + mysql-test/suite/versioning/t/insert2.test | 8 -- mysql-test/suite/versioning/t/replace.test | 59 +++++++++++-- sql/sql_delete.cc | 86 +++++++++++++++---- sql/sql_insert.cc | 12 +-- sql/table.h | 4 +- 12 files changed, 214 insertions(+), 66 deletions(-) create mode 100644 mysql-test/suite/versioning/r/replace,trx_id.rdiff diff --git a/mysql-test/suite/versioning/common.inc b/mysql-test/suite/versioning/common.inc index 73dacedb9de..6aa467f4588 100644 --- a/mysql-test/suite/versioning/common.inc +++ b/mysql-test/suite/versioning/common.inc @@ -66,6 +66,11 @@ if ($MTR_COMBINATION_TRX_ID) let $sys_datatype_expl= bigint(20) unsigned; let $sys_datatype_expl_uc= BIGINT(20) UNSIGNED; let $sys_datatype_max= 18446744073709551615; + let $current_row= current_row; +} +if ($MTR_COMBINATION_TIMESTAMP) +{ + let $current_row= current_row_ts; } eval create or replace function current_row(sys_trx_end $sys_datatype_expl) @@ -76,7 +81,7 @@ deterministic eval create or replace function current_row_ts(sys_trx_end timestamp(6)) returns int deterministic - return convert_tz(sys_trx_end, '+00:00', @@time_zone) = TIMESTAMP'2038-01-19 03:14:07.999999'; + return convert_tz(sys_trx_end, @@time_zone, '+00:00') = TIMESTAMP'2038-01-19 03:14:07.999999'; delimiter ~~; eval create or replace function check_row(row_start $sys_datatype_expl, row_end $sys_datatype_expl) @@ -87,7 +92,7 @@ begin return "ERROR: row_end < row_start"; elseif row_end = row_start then return "ERROR: row_end == row_start"; - elseif current_row(row_end) then + elseif $current_row(row_end) then return "CURRENT ROW"; end if; return "HISTORICAL ROW"; @@ -99,7 +104,7 @@ eval create or replace function check_row_slave(row_start $sys_datatype_expl, ro returns varchar(255) deterministic begin - if current_row(row_end) then + if $current_row(row_end) then return "CURRENT ROW"; end if; return "HISTORICAL ROW"; diff --git a/mysql-test/suite/versioning/r/foreign.result b/mysql-test/suite/versioning/r/foreign.result index b17deba1c1e..f937cf1024b 100644 --- a/mysql-test/suite/versioning/r/foreign.result +++ b/mysql-test/suite/versioning/r/foreign.result @@ -393,10 +393,7 @@ SET FOREIGN_KEY_CHECKS = ON; SET SESSION SQL_MODE= 'NO_BACKSLASH_ESCAPES'; SET timestamp = 8; LOAD DATA INFILE 't1.data' REPLACE INTO TABLE t1; -Warnings: -Warning 1265 Data truncated for column 'f12' at row 2 -Warning 1265 Data truncated for column 'f12' at row 4 -Warning 1265 Data truncated for column 'f12' at row 7 +ERROR 23000: Cannot add or update a child row: a foreign key constraint fails (`test`.`t1`, CONSTRAINT `t1_ibfk_1` FOREIGN KEY (`f13`) REFERENCES `t2` (`f`) ON DELETE SET NULL) SET timestamp = 9; REPLACE INTO t2 SELECT * FROM t2; DROP TABLE t1, t2; diff --git a/mysql-test/suite/versioning/r/insert2.result b/mysql-test/suite/versioning/r/insert2.result index f5c7b411cdf..be3992e1437 100644 --- a/mysql-test/suite/versioning/r/insert2.result +++ b/mysql-test/suite/versioning/r/insert2.result @@ -78,9 +78,3 @@ create or replace table t1 (pk int primary key) with system versioning; create trigger tr before insert on t1 for each row select 1 into @a; insert into t1 values (1),(2); drop table t1; -create table t1 (pk int primary key, i int) with system versioning; -replace into t1 values (1,10),(1,100),(1,1000); -select pk,i,row_end > '2038-01-01' from t1 for system_time all; -pk i row_end > '2038-01-01' -1 1000 1 -drop table t1; diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result index 4c257426f72..91a4074a6c8 100644 --- a/mysql-test/suite/versioning/r/partition.result +++ b/mysql-test/suite/versioning/r/partition.result @@ -1283,7 +1283,7 @@ where table_name = 't1'; partition_name table_rows p0 150 p1 60 -pn 90 +pn 91 rollback; select partition_name, table_rows from information_schema.partitions @@ -1291,7 +1291,7 @@ where table_name = 't1'; partition_name table_rows p0 0 p1 0 -pn 90 +pn 91 replace into t1 select seq from seq_1_to_10; select partition_name, table_rows from information_schema.partitions diff --git a/mysql-test/suite/versioning/r/replace,trx_id.rdiff b/mysql-test/suite/versioning/r/replace,trx_id.rdiff new file mode 100644 index 00000000000..ff68947c6bb --- /dev/null +++ b/mysql-test/suite/versioning/r/replace,trx_id.rdiff @@ -0,0 +1,26 @@ +--- replace.result ++++ replace.reject +@@ -75,7 +75,6 @@ + replace into t1 (pk,i) values (1,10),(1,100),(1,1000); + select pk, i, check_row(row_start, row_end) from t1 for system_time all; + pk i check_row(row_start, row_end) +-1 10 ERROR: row_end == row_start + 1 1000 CURRENT ROW + drop table t1; + create or replace table t1 ( +@@ -94,7 +93,6 @@ + commit; + select pk, i, check_row(row_start, row_end) from t1 for system_time all; + pk i check_row(row_start, row_end) +-1 3 ERROR: row_end == row_start + 1 300 CURRENT ROW + drop table t1; + # In this case there'll be no unique constraint on a historical row. +@@ -114,8 +112,6 @@ + commit; + select pk, i, check_row(row_start, row_end) from t1 for system_time all; + pk i check_row(row_start, row_end) +-1 3 ERROR: row_end == row_start +-1 30 ERROR: row_end == row_start + 1 300 CURRENT ROW + drop table t1; diff --git a/mysql-test/suite/versioning/r/replace.result b/mysql-test/suite/versioning/r/replace.result index 7b654a821da..44c6ea1fae9 100644 --- a/mysql-test/suite/versioning/r/replace.result +++ b/mysql-test/suite/versioning/r/replace.result @@ -95,13 +95,57 @@ drop table t1; # error (ER_DUP_ENTRY) # create or replace table t1 ( -pk int primary key, i int, -row_start bigint unsigned as row start, -row_end bigint unsigned as row end, +pk int KEY_TYPE, i int, +row_start SYS_DATATYPE as row start invisible, +row_end SYS_DATATYPE as row end invisible, period for system_time(row_start, row_end) -) engine=InnoDB with system versioning; +) with system versioning; replace into t1 (pk,i) values (1,10),(1,100),(1,1000); -select pk, i, row_end from t1 for system_time all; -pk i row_end -1 1000 18446744073709551615 +select pk, i, check_row(row_start, row_end) from t1 for system_time all; +pk i check_row(row_start, row_end) +1 10 ERROR: row_end == row_start +1 1000 CURRENT ROW drop table t1; +create or replace table t1 ( +pk int KEY_TYPE, i int, +row_start SYS_DATATYPE as row start invisible, +row_end SYS_DATATYPE as row end invisible, +period for system_time(row_start, row_end) +) with system versioning; +set timestamp= (select unix_timestamp()); +begin; +insert t1(pk, i) values(1,3); +delete from t1; +insert t1(pk, i) values(1,30); +delete from t1; +insert t1(pk, i) values(1,300); +commit; +select pk, i, check_row(row_start, row_end) from t1 for system_time all; +pk i check_row(row_start, row_end) +1 3 ERROR: row_end == row_start +1 300 CURRENT ROW +drop table t1; +# In this case there'll be no unique constraint on a historical row. +create or replace table t1 ( +pk int, i int, +row_start SYS_DATATYPE as row start invisible, +row_end SYS_DATATYPE as row end invisible, +period for system_time(row_start, row_end) +) with system versioning; +set timestamp= (select unix_timestamp()); +begin; +insert t1(pk, i) values(1,3); +delete from t1; +insert t1(pk, i) values(1,30); +delete from t1; +insert t1(pk, i) values(1,300); +commit; +select pk, i, check_row(row_start, row_end) from t1 for system_time all; +pk i check_row(row_start, row_end) +1 3 ERROR: row_end == row_start +1 30 ERROR: row_end == row_start +1 300 CURRENT ROW +drop table t1; +# +# End of 10.5 tests +# diff --git a/mysql-test/suite/versioning/t/foreign.test b/mysql-test/suite/versioning/t/foreign.test index 1b8bb83f064..55a051c95c9 100644 --- a/mysql-test/suite/versioning/t/foreign.test +++ b/mysql-test/suite/versioning/t/foreign.test @@ -419,6 +419,7 @@ SET FOREIGN_KEY_CHECKS = ON; SET SESSION SQL_MODE= 'NO_BACKSLASH_ESCAPES'; SET timestamp = 8; +--error ER_NO_REFERENCED_ROW_2 LOAD DATA INFILE 't1.data' REPLACE INTO TABLE t1; SET timestamp = 9; REPLACE INTO t2 SELECT * FROM t2; diff --git a/mysql-test/suite/versioning/t/insert2.test b/mysql-test/suite/versioning/t/insert2.test index 527a66e5f21..998bd349c8a 100644 --- a/mysql-test/suite/versioning/t/insert2.test +++ b/mysql-test/suite/versioning/t/insert2.test @@ -78,11 +78,3 @@ create or replace table t1 (pk int primary key) with system versioning; create trigger tr before insert on t1 for each row select 1 into @a; insert into t1 values (1),(2); drop table t1; - -# -# MDEV-14794 Limitations which the row end as a part of PK imposes due to CURRENT_TIMESTAMP behavior and otherwise -# -create table t1 (pk int primary key, i int) with system versioning; -replace into t1 values (1,10),(1,100),(1,1000); -select pk,i,row_end > '2038-01-01' from t1 for system_time all; -drop table t1; diff --git a/mysql-test/suite/versioning/t/replace.test b/mysql-test/suite/versioning/t/replace.test index def092812f1..33600e7d3d9 100644 --- a/mysql-test/suite/versioning/t/replace.test +++ b/mysql-test/suite/versioning/t/replace.test @@ -76,12 +76,12 @@ replace into t1 values (1),(2); replace into t1 values (1),(2); --connection con1 ---error ER_DUP_ENTRY replace into t1 values (1),(2); drop table t1; -drop table t1; - +--echo # +--echo # MDEV-14794 Limitations which the row end as a part of PK imposes due to +--echo # CURRENT_TIMESTAMP behavior and otherwise --echo # --echo # MDEV-15330 Server crash or assertion `table->insert_values' failure in write_record upon LOAD DATA --echo # @@ -122,15 +122,56 @@ drop table t1; --echo # error (ER_DUP_ENTRY) --echo # -create or replace table t1 ( - pk int primary key, i int, - row_start bigint unsigned as row start, - row_end bigint unsigned as row end, +--replace_result $sys_datatype_expl SYS_DATATYPE "$KEY_TYPE" KEY_TYPE +eval create or replace table t1 ( + pk int $KEY_TYPE, i int, + row_start $sys_datatype_expl as row start invisible, + row_end $sys_datatype_expl as row end invisible, period for system_time(row_start, row_end) -) engine=InnoDB with system versioning; +) with system versioning; replace into t1 (pk,i) values (1,10),(1,100),(1,1000); -select pk, i, row_end from t1 for system_time all; +select pk, i, check_row(row_start, row_end) from t1 for system_time all; drop table t1; +--replace_result $sys_datatype_expl SYS_DATATYPE "$KEY_TYPE" KEY_TYPE +eval create or replace table t1 ( + pk int $KEY_TYPE, i int, + row_start $sys_datatype_expl as row start invisible, + row_end $sys_datatype_expl as row end invisible, + period for system_time(row_start, row_end) +) with system versioning; +set timestamp= (select unix_timestamp()); +begin; + insert t1(pk, i) values(1,3); + delete from t1; + insert t1(pk, i) values(1,30); + delete from t1; + insert t1(pk, i) values(1,300); +commit; +select pk, i, check_row(row_start, row_end) from t1 for system_time all; +drop table t1; + +--echo # In this case there'll be no unique constraint on a historical row. +--replace_result $sys_datatype_expl SYS_DATATYPE +eval create or replace table t1 ( + pk int, i int, + row_start $sys_datatype_expl as row start invisible, + row_end $sys_datatype_expl as row end invisible, + period for system_time(row_start, row_end) +) with system versioning; +set timestamp= (select unix_timestamp()); +begin; + insert t1(pk, i) values(1,3); + delete from t1; + insert t1(pk, i) values(1,30); + delete from t1; + insert t1(pk, i) values(1,300); +commit; +select pk, i, check_row(row_start, row_end) from t1 for system_time all; +drop table t1; + +--echo # +--echo # End of 10.5 tests +--echo # --source suite/versioning/common_finish.inc diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index e5779410177..735c2fd0863 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -285,26 +285,80 @@ int update_portion_of_time(THD *thd, TABLE *table, return res; } -inline -int TABLE::delete_row() -{ - if (!versioned(VERS_TIMESTAMP) || - !vers_end_field()->is_max()) - return file->ha_delete_row(record[0]); +/** + Delete a record stored in: + replace= true: record[0] + replace= false: record[1] + + with regard to the treat_versioned flag, which can be false for a versioned + table in case of versioned->versioned replication. + + For a versioned case, we detect a few conditions, under which we should delete + a row instead of updating it to a history row. + This includes: + * History deletion by user; + * History collision, in case of REPLACE or very fast sequence of dmls + so that timestamp doesn't change; + * History collision in the parent table + + A normal delete is processed here as well. +*/ +template +int TABLE::delete_row(bool treat_versioned) +{ + int err= 0; + uchar *del_buf= record[replace ? 1 : 0]; + bool delete_row= !treat_versioned + || in_use->lex->vers_conditions.delete_history + || versioned(VERS_TRX_ID) + || !vers_end_field()->is_max( + vers_end_field()->ptr_in_record(del_buf)); + if (!delete_row) + { + if (replace) + { + store_record(this, record[2]); + restore_record(this, record[1]); + } + else + { + store_record(this, record[1]); + } + vers_update_end(); + err= file->ha_update_row(record[1], record[0]); + if (unlikely(err)) + { + /* + MDEV-23644: we get HA_ERR_FOREIGN_DUPLICATE_KEY iff we already got + history row with same trx_id which is the result of foreign key + action, so we don't need one more history row. + + Additionally, delete the row if versioned record already exists. + This happens on replace, a very fast sequence of inserts and deletes, + or if timestamp is frozen. + */ + delete_row= err == HA_ERR_FOUND_DUPP_KEY + || err == HA_ERR_FOUND_DUPP_UNIQUE + || err == HA_ERR_FOREIGN_DUPLICATE_KEY; + if (!delete_row) + return err; + + if (!replace) + del_buf= record[1]; + } + + if (replace) + restore_record(this, record[2]); + } + + if (delete_row) + err= file->ha_delete_row(del_buf); - store_record(this, record[1]); - vers_update_end(); - int err= file->ha_update_row(record[1], record[0]); - /* - MDEV-23644: we get HA_ERR_FOREIGN_DUPLICATE_KEY iff we already got history - row with same trx_id which is the result of foreign key action, so we - don't need one more history row. - */ - if (err == HA_ERR_FOREIGN_DUPLICATE_KEY) - return file->ha_delete_row(record[0]); return err; } +template int TABLE::delete_row(bool treat_versioned); +template int TABLE::delete_row(bool treat_versioned); /** Implement DELETE SQL word. diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index e4e472a2aaf..9f0c83cbbce 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -2131,16 +2131,8 @@ int Write_record::replace_row(ha_rows *inserted, ha_rows *deleted) if (use_triggers && trg->process_triggers(table->in_use, TRG_EVENT_DELETE, TRG_ACTION_BEFORE, TRUE)) return restore_on_error(); - if (!versioned) - error = table->file->ha_delete_row(table->record[1]); - else - { - store_record(table, record[2]); - restore_record(table, record[1]); - table->vers_update_end(); - error = table->file->ha_update_row(table->record[1], table->record[0]); - restore_record(table, record[2]); - } + + error= table->delete_row(versioned); if (unlikely(error)) return on_ha_error(error); diff --git a/sql/table.h b/sql/table.h index 924b1f77518..cb9c4aadc9a 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1853,7 +1853,9 @@ public: ha_rows *rows_inserted); bool vers_check_update(List &items); static bool check_period_overlaps(const KEY &key, const uchar *lhs, const uchar *rhs); - int delete_row(); + inline int delete_row(){ return delete_row(versioned(VERS_TIMESTAMP)); } + template + int delete_row(bool versioned); /* Used in majority of DML (called from fill_record()) */ bool vers_update_fields(); /* Used in DELETE, DUP REPLACE and insert history row */ From db26bf7ada0736f2aaba3acf2ce42c2675533ed6 Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Sat, 27 Apr 2024 21:04:49 +0200 Subject: [PATCH 13/14] MDEV-15990 versioning: don't allow changes in the past --- mysql-test/suite/versioning/r/misc.result | 24 +++++++++++++++++++++++ mysql-test/suite/versioning/t/misc.test | 19 ++++++++++++++++++ sql/sql_delete.cc | 16 +++++++++++---- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/mysql-test/suite/versioning/r/misc.result b/mysql-test/suite/versioning/r/misc.result index fadc896d185..b7eada49aa0 100644 --- a/mysql-test/suite/versioning/r/misc.result +++ b/mysql-test/suite/versioning/r/misc.result @@ -57,6 +57,30 @@ insert into t2 (a) values (now()),(now()); select * from t2 where a in (select row_start from t1); a drop table t1, t2; +# MDEV-30046 wrong row targeted with "insert ... on duplicate" and "replace" +# Don't allow changes from the past +create or replace table t1 (pk int primary key, i int) with system versioning; +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `pk` int(11) NOT NULL, + `i` int(11) DEFAULT NULL, + PRIMARY KEY (`pk`) +) ENGINE=MyISAM DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci WITH SYSTEM VERSIONING +set timestamp=12345; +insert t1(pk, i) values(1,3); +set timestamp=12344; +delete from t1; +select pk,i from t1 for system_time all; +pk i +set timestamp=12345; +insert t1(pk, i) values(1,3); +set timestamp=12344; +replace t1(pk, i) values(1,4); +select pk,i from t1 for system_time all; +pk i +1 4 +drop table t1; # # End of 10.11 tests # diff --git a/mysql-test/suite/versioning/t/misc.test b/mysql-test/suite/versioning/t/misc.test index fa5012b6efa..5ec9c964dca 100644 --- a/mysql-test/suite/versioning/t/misc.test +++ b/mysql-test/suite/versioning/t/misc.test @@ -46,6 +46,25 @@ insert into t2 (a) values (now()),(now()); select * from t2 where a in (select row_start from t1); drop table t1, t2; +--echo # MDEV-30046 wrong row targeted with "insert ... on duplicate" and "replace" +--echo # Don't allow changes from the past +create or replace table t1 (pk int primary key, i int) with system versioning; +show create table t1; +set timestamp=12345; +insert t1(pk, i) values(1,3); +set timestamp=12344; +delete from t1; + +select pk,i from t1 for system_time all; + +set timestamp=12345; +insert t1(pk, i) values(1,3); +set timestamp=12344; +replace t1(pk, i) values(1,4); + +select pk,i from t1 for system_time all; +drop table t1; + --echo # --echo # End of 10.11 tests --echo # diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 735c2fd0863..71d354e6423 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -308,6 +308,7 @@ int TABLE::delete_row(bool treat_versioned) { int err= 0; uchar *del_buf= record[replace ? 1 : 0]; + bool delete_row= !treat_versioned || in_use->lex->vers_conditions.delete_history || versioned(VERS_TRX_ID) @@ -325,7 +326,14 @@ int TABLE::delete_row(bool treat_versioned) store_record(this, record[1]); } vers_update_end(); - err= file->ha_update_row(record[1], record[0]); + + Field *row_start= vers_start_field(); + Field *row_end= vers_end_field(); + // Don't make history row with negative lifetime + delete_row= row_start->cmp(row_start->ptr, row_end->ptr) > 0; + + if (likely(!delete_row)) + err= file->ha_update_row(record[1], record[0]); if (unlikely(err)) { /* @@ -342,11 +350,11 @@ int TABLE::delete_row(bool treat_versioned) || err == HA_ERR_FOREIGN_DUPLICATE_KEY; if (!delete_row) return err; - - if (!replace) - del_buf= record[1]; } + if (delete_row) + del_buf= record[1]; + if (replace) restore_record(this, record[2]); } From c4b76b984fd321f01c18e2ec84dc67744e8f761e Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Sun, 28 Apr 2024 01:10:36 +0200 Subject: [PATCH 14/14] MDEV-15990 innodb: change DB_FOREIGN_DUPLICATE_KEY to DB_DUPLICATE_KEY during row insert DB_FOREIGN_DUPLICATE_KEY in row_ins_duplicate_error_in_clust was motivated by handling the cascade changes during versioned operations. It was found though, that certain row_update_for_mysql calls could return DB_FOREIGN_DUPLICATE_KEY, even if there's no foreign relations. Change DB_FOREIGN_DUPLICATE_KEY to DB_DUPLICATE_KEY in row_ins_duplicate_error_in_clust. It will be later converted to DB_FOREIGN_DUPLICATE_KEY in row_ins_check_foreign_constraint if needed. Additionally, ha_delete_row should return neither. Ensure it by an assertion. --- storage/innobase/handler/ha_innodb.cc | 3 +++ storage/innobase/row/row0ins.cc | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index a4b400a9931..de144f25142 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -8748,6 +8748,9 @@ ha_innobase::delete_row( ut_ad(!trx->is_bulk_insert()); error = row_update_for_mysql(m_prebuilt); + ut_ad(error != DB_DUPLICATE_KEY); + ut_ad(error != DB_FOREIGN_DUPLICATE_KEY); + #ifdef WITH_WSREP if (error == DB_SUCCESS && trx->is_wsrep() && wsrep_thd_is_local(m_user_thd) diff --git a/storage/innobase/row/row0ins.cc b/storage/innobase/row/row0ins.cc index 134a0815e6a..0c1f5614e4c 100644 --- a/storage/innobase/row/row0ins.cc +++ b/storage/innobase/row/row0ins.cc @@ -2412,7 +2412,7 @@ duplicate: &trx_id_len); ut_ad(trx_id_len == DATA_TRX_ID_LEN); if (trx->id == trx_read_trx_id(trx_id)) { - err = DB_FOREIGN_DUPLICATE_KEY; + err = DB_DUPLICATE_KEY; } } goto func_exit;