Merge lp:~stewart/drizzle/bug682563-commit-all-not-true into lp:drizzle/7.0

[Stewart Smith]

TransactionalStorageEngine::doCommit(Session*, bool all) the all was a lie, engines had to check for autocommit and other ick.

This patch fixes that so that you can have a much simpler check in the engine, not having to poke into session flags.

http://hudson.drizzle.org/view/Drizzle-param/job/drizzle-param/703/

[Brian Aker (brianaker)]

Test case?

[Stewart Smith (stewart)]

I'm working on[1] adapting storage_engine_api_tester to record a history of states so that we can check the right thing is still being called. A simple test-run test will then be able to ensure that the right thing is being called at the right time.

[1] will type 'commit' today

[Stewart Smith (stewart)]

https://code.launchpad.net/~stewart/drizzle/seapitester-state-history/+merge/46092

implements it. when this branch is merged with that one, the test results don't change - as in, the correct thing is still being called at the correct time.

[Brian Aker (brianaker)]

As we talked about in IRC, we need a set of SQL tests to make sure this is correct.

[Brian Aker (brianaker)]

You need to supply SQL regression tests for this patch.

[Patrick Crews (patrick-crews)]

Running the test suite and checking lcov numbers shows that the affected lines of code are exercised by the test suite. Did not check changes to pbxt or haildb code as this is supposed to be a cursory check.

Test cases appear satisfactory.

=== modified file 'drizzled/transaction_services.cc'
--- drizzled/transaction_services.cc	2011-01-13 18:43:55 +0000
+++ drizzled/transaction_services.cc	2011-01-17 01:31:15 +0000
@@ -576,6 +576,13 @@
   TransactionContext::ResourceContexts &resource_contexts= trans->getResourceContexts();
 
   bool is_real_trans= normal_transaction || session->transaction.all.getResourceContexts().empty();
+  bool all= normal_transaction;
+
+  /* If we're in autocommit then we have a real transaction to commit
+     (except if it's BEGIN)
+  */
+  if (! session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
+    all= true;
 
   if (resource_contexts.empty() == false)
   {
@@ -590,7 +597,7 @@
 
       if (resource->participatesInXaTransaction())
       {
-        if ((err= resource_context->getXaResourceManager()->xaCommit(session, normal_transaction)))
+        if ((err= resource_context->getXaResourceManager()->xaCommit(session, all)))
         {
           my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
           error= 1;
@@ -602,7 +609,7 @@
       }
       else if (resource->participatesInSqlTransaction())
       {
-        if ((err= resource_context->getTransactionalStorageEngine()->commit(session, normal_transaction)))
+        if ((err= resource_context->getTransactionalStorageEngine()->commit(session, all)))
         {
           my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
           error= 1;

=== modified file 'plugin/haildb/haildb_engine.cc'
--- plugin/haildb/haildb_engine.cc	2011-01-10 09:41:44 +0000
+++ plugin/haildb/haildb_engine.cc	2011-01-17 01:31:15 +0000
@@ -409,7 +409,7 @@
   ib_err_t err;
   ib_trx_t *transaction= get_trx(session);
 
-  if (all || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))
+  if (all)
   {
     err= ib_trx_commit(*transaction);
 

=== modified file 'plugin/innobase/handler/ha_innodb.cc'
--- plugin/innobase/handler/ha_innodb.cc	2011-01-15 07:04:48 +0000
+++ plugin/innobase/handler/ha_innodb.cc	2011-01-17 01:31:15 +0000
@@ -2571,9 +2571,8 @@
     trx_search_latch_release_if_reserved(trx);
   }
 
-  if (all
-    || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))) {
-
+  if (all)
+  {
     /* We were instructed to commit the whole transaction, or
     this is an SQL statement end and autocommit is on */
 

=== modified file 'plugin/pbxt/src/ha_pbxt.cc'
--- plugin/pbxt/src/ha_pbxt.cc	2010-12-24 07:15:43 +0000
+++ plugin/pbxt/src/ha_pbxt.cc	2011-01-17 01:31:15 +0000
@@ -5723,13 +5723,11 @@
 	return xt_ha_pbxt_thread_error_for_mysql(thd, xt_ha_thd_to_self(thd), false);
 }
 
-int PBXTStorageEngine::doCommit(drizzled::Session* thd, bool)
+int PBXTStorageEngine::doCommit(drizzled::Session* thd, bool real_commit)
 {
 	int err = 0;
 	XTThreadPtr self = (XTThreadPtr) *thd->getEngineData(pbxt_hton);
 
-	bool real_commit = !session_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN);
-	
 	XT_PRINT1(self, "PBXTStorageEngine::doCommit(real_commit = %s)\n", real_commit ? "true" : "false");
 
 	if (real_commit && self) {

=== modified file 'plugin/storage_engine_api_tester/storage_engine_api_tester.cc'
--- plugin/storage_engine_api_tester/storage_engine_api_tester.cc	2011-01-13 05:06:12 +0000
+++ plugin/storage_engine_api_tester/storage_engine_api_tester.cc	2011-01-17 01:31:15 +0000
@@ -490,7 +490,7 @@
 
 int SEAPITester::doCommit(Session *session, bool all)
 {
-  if (all     || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))
+  if (all)
   {
     ENGINE_NEW_STATE("COMMIT");
     ENGINE_NEW_STATE("::SEAPITester()");

=== added file 'tests/suite/regression/r/682563.result'
--- tests/suite/regression/r/682563.result	1970-01-01 00:00:00 +0000
+++ tests/suite/regression/r/682563.result	2011-01-17 01:31:15 +0000
@@ -0,0 +1,159 @@
+A
+CREATE TABLE t1 (a int);
+SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN;
+INSERT INTO t1 values (1);
+B
+SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN;
+SELECT * from t1;
+a
+A
+INSERT INTO t1 values (2);
+B
+SELECT * from t1;
+a
+COMMIT;
+BEGIN;
+SELECT * FROM t1;
+a
+A
+COMMIT;
+B
+SELECT * from t1 order by a;
+a
+1
+2
+COMMIT;
+BEGIN;
+SELECT * from t1 order by a;
+a
+1
+2
+DROP TABLE t1;
+SET AUTOCOMMIT= 0;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	0
+BEGIN;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	0
+DROP SCHEMA IF EXISTS boundaries;
+Warnings:
+Note	1008	Can't drop schema 'boundaries'; schema doesn't exist
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	1
+COMMIT;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	1
+SET AUTOCOMMIT= 1;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	1
+BEGIN;
+DROP SCHEMA IF EXISTS boundaries;
+Warnings:
+Note	1008	Can't drop schema 'boundaries'; schema doesn't exist
+COMMIT;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+CREATE TABLE commit_test (a int);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+INSERT into commit_test VALUES (10), (20);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+BEGIN;
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	2
+COMMIT;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	3
+BEGIN;
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	3
+ROLLBACK;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	3
+BEGIN;
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	3
+COMMIT;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	4
+SET AUTOCOMMIT= 0;
+INSERT into commit_test VALUES (10);
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	4
+drop table commit_test;
+SHOW STATUS LIKE 'Handler_commit%';
+Variable_name	Value
+Handler_commit	5
+DROP TABLE IF EXISTS t1_trx, t1_non_trx;
+SET AUTOCOMMIT= 0;
+CREATE TABLE t1_trx (
+k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=InnoDB;
+CREATE TEMPORARY TABLE t1_non_trx (
+k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=MyISAM;
+START TRANSACTION;
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+INSERT INTO t1_non_trx VALUES ('key1','value1');
+INSERT INTO t1_non_trx VALUES ('key2','value2');
+ROLLBACK;
+Warnings:
+Warning	1196	Some non-transactional changed tables couldn't be rolled back
+Expected warning about non-trx data changes not being rolled back
+SELECT * FROM t1_trx;
+k	v
+SELECT * FROM t1_non_trx;
+k	v
+key1	value1
+key2	value2
+START TRANSACTION;
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+k	v
+key1	value1
+key2	value2
+ROLLBACK;
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+k	v
+DROP TABLE t1_trx;
+DROP TABLE t1_non_trx;

=== added file 'tests/suite/regression/t/682563.test'
--- tests/suite/regression/t/682563.test	1970-01-01 00:00:00 +0000
+++ tests/suite/regression/t/682563.test	2011-01-17 01:31:15 +0000
@@ -0,0 +1,186 @@
+connect (a,localhost,root,,);
+connect (b,localhost,root,,);
+
+connection a;
+--echo A
+CREATE TABLE t1 (a int);
+SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN;
+INSERT INTO t1 values (1);
+
+connection b;
+--echo B
+SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN;
+SELECT * from t1;
+
+connection a;
+--echo A
+INSERT INTO t1 values (2);
+
+connection b;
+--echo B
+SELECT * from t1;
+COMMIT;
+BEGIN;
+SELECT * FROM t1;
+
+connection a;
+--echo A
+COMMIT;
+
+connection b;
+--echo B
+SELECT * from t1 order by a;
+COMMIT;
+BEGIN;
+SELECT * from t1 order by a;
+
+DROP TABLE t1;
+
+disconnect a;
+disconnect b;
+connection default;
+# This is a test of various SQL statements
+# and looks at the statement and transaction
+# boundaries (start/end) to ensure they are sane
+
+SET AUTOCOMMIT= 0;
+
+# Expect 0 commit count since nothing 
+# has yet happened...
+
+SHOW STATUS LIKE 'Handler_commit%';
+
+BEGIN;
+
+# Expect 0 commit count since nothing 
+# has yet been committed...
+
+SHOW STATUS LIKE 'Handler_commit%';
+
+DROP SCHEMA IF EXISTS boundaries;
+
+# Expect 1 commit count since above DROP SCHEMA
+# will implicitly call COMMIT.
+#
+# When we get transactional DDL, should be 0.
+SHOW STATUS LIKE 'Handler_commit%';
+
+COMMIT;
+
+# Expect 1 commit count since 
+# an explicit call to COMMIT was made
+# even though nothing was changed...
+
+SHOW STATUS LIKE 'Handler_commit%';
+
+
+## Enable AUOTOCOMMIT
+#
+SET AUTOCOMMIT= 1;
+
+SHOW STATUS LIKE 'Handler_commit%';
+BEGIN;
+DROP SCHEMA IF EXISTS boundaries;
+COMMIT;
+
+SHOW STATUS LIKE 'Handler_commit%';
+CREATE TABLE commit_test (a int);
+SHOW STATUS LIKE 'Handler_commit%';
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+INSERT into commit_test VALUES (10), (20);
+SHOW STATUS LIKE 'Handler_commit%';
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+
+BEGIN;
+
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+
+COMMIT;
+
+SHOW STATUS LIKE 'Handler_commit%';
+BEGIN;
+
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+
+ROLLBACK;
+
+SHOW STATUS LIKE 'Handler_commit%';
+BEGIN;
+
+
+INSERT into commit_test VALUES (10);
+SHOW STATUS LIKE 'Handler_commit%';
+
+COMMIT;
+SHOW STATUS LIKE 'Handler_commit%';
+
+SET AUTOCOMMIT= 0;
+INSERT into commit_test VALUES (10);
+INSERT into commit_test VALUES (10);
+
+
+SHOW STATUS LIKE 'Handler_commit%';
+drop table commit_test;
+SHOW STATUS LIKE 'Handler_commit%';
+# Tests a number of things related to transactions:
+# 
+# 1. Interaction of more than one engine in a transaction
+# 2. Correct commit and rollback behaviour
+# 3. XA protocol communication and recovery
+
+--disable_warnings
+DROP TABLE IF EXISTS t1_trx, t1_non_trx;
+--enable_warnings
+
+SET AUTOCOMMIT= 0;
+
+CREATE TABLE t1_trx (
+  k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=InnoDB;
+
+CREATE TEMPORARY TABLE t1_non_trx (
+  k VARCHAR(10) NOT NULL
+, v VARCHAR(10) NOT NULL
+, PRIMARY KEY (k)
+) ENGINE=MyISAM;
+
+START TRANSACTION;
+
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+
+INSERT INTO t1_non_trx VALUES ('key1','value1');
+INSERT INTO t1_non_trx VALUES ('key2','value2');
+
+ROLLBACK;
+
+--echo Expected warning about non-trx data changes not being rolled back
+
+SELECT * FROM t1_trx;
+SELECT * FROM t1_non_trx;
+
+START TRANSACTION;
+
+INSERT INTO t1_trx VALUES ('key1','value1');
+INSERT INTO t1_trx VALUES ('key2','value2');
+
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+
+ROLLBACK;
+
+SELECT t1_trx.k, t1_trx.v
+FROM t1_trx
+INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
+
+DROP TABLE t1_trx;
+DROP TABLE t1_non_trx;