Drizzle

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

  1. bug682563-commit-all-not-true
  2. Merge into 7.0
Proposed by Stewart Smith
Status: Merged
Approved by: Lee Bieber
Approved revision: 2073
Merged at revision: 2101
Proposed branch: lp:~stewart/drizzle/bug682563-commit-all-not-true
Merge into: lp:drizzle/7.0
Diff against target: 451 lines (+359/-10)
7 files modified
drizzled/transaction_services.cc (+9/-2)
plugin/haildb/haildb_engine.cc (+1/-1)
plugin/innobase/handler/ha_innodb.cc (+2/-3)
plugin/pbxt/src/ha_pbxt.cc (+1/-3)
plugin/storage_engine_api_tester/storage_engine_api_tester.cc (+1/-1)
tests/suite/regression/r/682563.result (+159/-0)
tests/suite/regression/t/682563.test (+186/-0)
To merge this branch: bzr merge lp:~stewart/drizzle/bug682563-commit-all-not-true
Reviewer Review Type Date Requested Status
Drizzle Developers Pending
Review via email: mp+45851@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Brian Aker (brianaker) wrote :

Test case?

Revision history for this message
Stewart Smith (stewart) wrote :

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

Revision history for this message
Stewart Smith (stewart) wrote :

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.

Revision history for this message
Brian Aker (brianaker) wrote :

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

Revision history for this message
Brian Aker (brianaker) wrote :

You need to supply SQL regression tests for this patch.

Revision history for this message
Patrick Crews (patrick-crews) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
576 TransactionContext::ResourceContexts &resource_contexts= trans->getResourceContexts();576 TransactionContext::ResourceContexts &resource_contexts= trans->getResourceContexts();
577577
578 bool is_real_trans= normal_transaction || session->transaction.all.getResourceContexts().empty();578 bool is_real_trans= normal_transaction || session->transaction.all.getResourceContexts().empty();
579 bool all= normal_transaction;
580
581 /* If we're in autocommit then we have a real transaction to commit
582 (except if it's BEGIN)
583 */
584 if (! session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
585 all= true;
579586
580 if (resource_contexts.empty() == false)587 if (resource_contexts.empty() == false)
581 {588 {
@@ -590,7 +597,7 @@
590597
591 if (resource->participatesInXaTransaction())598 if (resource->participatesInXaTransaction())
592 {599 {
593 if ((err= resource_context->getXaResourceManager()->xaCommit(session, normal_transaction)))600 if ((err= resource_context->getXaResourceManager()->xaCommit(session, all)))
594 {601 {
595 my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);602 my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
596 error= 1;603 error= 1;
@@ -602,7 +609,7 @@
602 }609 }
603 else if (resource->participatesInSqlTransaction())610 else if (resource->participatesInSqlTransaction())
604 {611 {
605 if ((err= resource_context->getTransactionalStorageEngine()->commit(session, normal_transaction)))612 if ((err= resource_context->getTransactionalStorageEngine()->commit(session, all)))
606 {613 {
607 my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);614 my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
608 error= 1;615 error= 1;
609616
=== 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 @@
409 ib_err_t err;409 ib_err_t err;
410 ib_trx_t *transaction= get_trx(session);410 ib_trx_t *transaction= get_trx(session);
411411
412 if (all || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))412 if (all)
413 {413 {
414 err= ib_trx_commit(*transaction);414 err= ib_trx_commit(*transaction);
415415
416416
=== 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 @@
2571 trx_search_latch_release_if_reserved(trx);2571 trx_search_latch_release_if_reserved(trx);
2572 }2572 }
25732573
2574 if (all2574 if (all)
2575 || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))) {2575 {
2576
2577 /* We were instructed to commit the whole transaction, or2576 /* We were instructed to commit the whole transaction, or
2578 this is an SQL statement end and autocommit is on */2577 this is an SQL statement end and autocommit is on */
25792578
25802579
=== 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 @@
5723 return xt_ha_pbxt_thread_error_for_mysql(thd, xt_ha_thd_to_self(thd), false);5723 return xt_ha_pbxt_thread_error_for_mysql(thd, xt_ha_thd_to_self(thd), false);
5724}5724}
57255725
5726int PBXTStorageEngine::doCommit(drizzled::Session* thd, bool)5726int PBXTStorageEngine::doCommit(drizzled::Session* thd, bool real_commit)
5727{5727{
5728 int err = 0;5728 int err = 0;
5729 XTThreadPtr self = (XTThreadPtr) *thd->getEngineData(pbxt_hton);5729 XTThreadPtr self = (XTThreadPtr) *thd->getEngineData(pbxt_hton);
57305730
5731 bool real_commit = !session_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN);
5732
5733 XT_PRINT1(self, "PBXTStorageEngine::doCommit(real_commit = %s)\n", real_commit ? "true" : "false");5731 XT_PRINT1(self, "PBXTStorageEngine::doCommit(real_commit = %s)\n", real_commit ? "true" : "false");
57345732
5735 if (real_commit && self) {5733 if (real_commit && self) {
57365734
=== 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 @@
490490
491int SEAPITester::doCommit(Session *session, bool all)491int SEAPITester::doCommit(Session *session, bool all)
492{492{
493 if (all || (!session_test_options(session, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))493 if (all)
494 {494 {
495 ENGINE_NEW_STATE("COMMIT");495 ENGINE_NEW_STATE("COMMIT");
496 ENGINE_NEW_STATE("::SEAPITester()");496 ENGINE_NEW_STATE("::SEAPITester()");
497497
=== 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 @@
1A
2CREATE TABLE t1 (a int);
3SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
4BEGIN;
5INSERT INTO t1 values (1);
6B
7SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
8BEGIN;
9SELECT * from t1;
10a
11A
12INSERT INTO t1 values (2);
13B
14SELECT * from t1;
15a
16COMMIT;
17BEGIN;
18SELECT * FROM t1;
19a
20A
21COMMIT;
22B
23SELECT * from t1 order by a;
24a
251
262
27COMMIT;
28BEGIN;
29SELECT * from t1 order by a;
30a
311
322
33DROP TABLE t1;
34SET AUTOCOMMIT= 0;
35SHOW STATUS LIKE 'Handler_commit%';
36Variable_name Value
37Handler_commit 0
38BEGIN;
39SHOW STATUS LIKE 'Handler_commit%';
40Variable_name Value
41Handler_commit 0
42DROP SCHEMA IF EXISTS boundaries;
43Warnings:
44Note 1008 Can't drop schema 'boundaries'; schema doesn't exist
45SHOW STATUS LIKE 'Handler_commit%';
46Variable_name Value
47Handler_commit 1
48COMMIT;
49SHOW STATUS LIKE 'Handler_commit%';
50Variable_name Value
51Handler_commit 1
52SET AUTOCOMMIT= 1;
53SHOW STATUS LIKE 'Handler_commit%';
54Variable_name Value
55Handler_commit 1
56BEGIN;
57DROP SCHEMA IF EXISTS boundaries;
58Warnings:
59Note 1008 Can't drop schema 'boundaries'; schema doesn't exist
60COMMIT;
61SHOW STATUS LIKE 'Handler_commit%';
62Variable_name Value
63Handler_commit 2
64CREATE TABLE commit_test (a int);
65SHOW STATUS LIKE 'Handler_commit%';
66Variable_name Value
67Handler_commit 2
68INSERT into commit_test VALUES (10);
69SHOW STATUS LIKE 'Handler_commit%';
70Variable_name Value
71Handler_commit 2
72INSERT into commit_test VALUES (10), (20);
73SHOW STATUS LIKE 'Handler_commit%';
74Variable_name Value
75Handler_commit 2
76INSERT into commit_test VALUES (10);
77SHOW STATUS LIKE 'Handler_commit%';
78Variable_name Value
79Handler_commit 2
80BEGIN;
81INSERT into commit_test VALUES (10);
82SHOW STATUS LIKE 'Handler_commit%';
83Variable_name Value
84Handler_commit 2
85COMMIT;
86SHOW STATUS LIKE 'Handler_commit%';
87Variable_name Value
88Handler_commit 3
89BEGIN;
90INSERT into commit_test VALUES (10);
91SHOW STATUS LIKE 'Handler_commit%';
92Variable_name Value
93Handler_commit 3
94ROLLBACK;
95SHOW STATUS LIKE 'Handler_commit%';
96Variable_name Value
97Handler_commit 3
98BEGIN;
99INSERT into commit_test VALUES (10);
100SHOW STATUS LIKE 'Handler_commit%';
101Variable_name Value
102Handler_commit 3
103COMMIT;
104SHOW STATUS LIKE 'Handler_commit%';
105Variable_name Value
106Handler_commit 4
107SET AUTOCOMMIT= 0;
108INSERT into commit_test VALUES (10);
109INSERT into commit_test VALUES (10);
110SHOW STATUS LIKE 'Handler_commit%';
111Variable_name Value
112Handler_commit 4
113drop table commit_test;
114SHOW STATUS LIKE 'Handler_commit%';
115Variable_name Value
116Handler_commit 5
117DROP TABLE IF EXISTS t1_trx, t1_non_trx;
118SET AUTOCOMMIT= 0;
119CREATE TABLE t1_trx (
120k VARCHAR(10) NOT NULL
121, v VARCHAR(10) NOT NULL
122, PRIMARY KEY (k)
123) ENGINE=InnoDB;
124CREATE TEMPORARY TABLE t1_non_trx (
125k VARCHAR(10) NOT NULL
126, v VARCHAR(10) NOT NULL
127, PRIMARY KEY (k)
128) ENGINE=MyISAM;
129START TRANSACTION;
130INSERT INTO t1_trx VALUES ('key1','value1');
131INSERT INTO t1_trx VALUES ('key2','value2');
132INSERT INTO t1_non_trx VALUES ('key1','value1');
133INSERT INTO t1_non_trx VALUES ('key2','value2');
134ROLLBACK;
135Warnings:
136Warning 1196 Some non-transactional changed tables couldn't be rolled back
137Expected warning about non-trx data changes not being rolled back
138SELECT * FROM t1_trx;
139k v
140SELECT * FROM t1_non_trx;
141k v
142key1 value1
143key2 value2
144START TRANSACTION;
145INSERT INTO t1_trx VALUES ('key1','value1');
146INSERT INTO t1_trx VALUES ('key2','value2');
147SELECT t1_trx.k, t1_trx.v
148FROM t1_trx
149INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
150k v
151key1 value1
152key2 value2
153ROLLBACK;
154SELECT t1_trx.k, t1_trx.v
155FROM t1_trx
156INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
157k v
158DROP TABLE t1_trx;
159DROP TABLE t1_non_trx;
0160
=== 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 @@
1connect (a,localhost,root,,);
2connect (b,localhost,root,,);
3
4connection a;
5--echo A
6CREATE TABLE t1 (a int);
7SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
8BEGIN;
9INSERT INTO t1 values (1);
10
11connection b;
12--echo B
13SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
14BEGIN;
15SELECT * from t1;
16
17connection a;
18--echo A
19INSERT INTO t1 values (2);
20
21connection b;
22--echo B
23SELECT * from t1;
24COMMIT;
25BEGIN;
26SELECT * FROM t1;
27
28connection a;
29--echo A
30COMMIT;
31
32connection b;
33--echo B
34SELECT * from t1 order by a;
35COMMIT;
36BEGIN;
37SELECT * from t1 order by a;
38
39DROP TABLE t1;
40
41disconnect a;
42disconnect b;
43connection default;
44# This is a test of various SQL statements
45# and looks at the statement and transaction
46# boundaries (start/end) to ensure they are sane
47
48SET AUTOCOMMIT= 0;
49
50# Expect 0 commit count since nothing
51# has yet happened...
52
53SHOW STATUS LIKE 'Handler_commit%';
54
55BEGIN;
56
57# Expect 0 commit count since nothing
58# has yet been committed...
59
60SHOW STATUS LIKE 'Handler_commit%';
61
62DROP SCHEMA IF EXISTS boundaries;
63
64# Expect 1 commit count since above DROP SCHEMA
65# will implicitly call COMMIT.
66#
67# When we get transactional DDL, should be 0.
68SHOW STATUS LIKE 'Handler_commit%';
69
70COMMIT;
71
72# Expect 1 commit count since
73# an explicit call to COMMIT was made
74# even though nothing was changed...
75
76SHOW STATUS LIKE 'Handler_commit%';
77
78
79## Enable AUOTOCOMMIT
80#
81SET AUTOCOMMIT= 1;
82
83SHOW STATUS LIKE 'Handler_commit%';
84BEGIN;
85DROP SCHEMA IF EXISTS boundaries;
86COMMIT;
87
88SHOW STATUS LIKE 'Handler_commit%';
89CREATE TABLE commit_test (a int);
90SHOW STATUS LIKE 'Handler_commit%';
91INSERT into commit_test VALUES (10);
92SHOW STATUS LIKE 'Handler_commit%';
93INSERT into commit_test VALUES (10), (20);
94SHOW STATUS LIKE 'Handler_commit%';
95INSERT into commit_test VALUES (10);
96SHOW STATUS LIKE 'Handler_commit%';
97
98BEGIN;
99
100INSERT into commit_test VALUES (10);
101SHOW STATUS LIKE 'Handler_commit%';
102
103COMMIT;
104
105SHOW STATUS LIKE 'Handler_commit%';
106BEGIN;
107
108INSERT into commit_test VALUES (10);
109SHOW STATUS LIKE 'Handler_commit%';
110
111ROLLBACK;
112
113SHOW STATUS LIKE 'Handler_commit%';
114BEGIN;
115
116
117INSERT into commit_test VALUES (10);
118SHOW STATUS LIKE 'Handler_commit%';
119
120COMMIT;
121SHOW STATUS LIKE 'Handler_commit%';
122
123SET AUTOCOMMIT= 0;
124INSERT into commit_test VALUES (10);
125INSERT into commit_test VALUES (10);
126
127
128SHOW STATUS LIKE 'Handler_commit%';
129drop table commit_test;
130SHOW STATUS LIKE 'Handler_commit%';
131# Tests a number of things related to transactions:
132#
133# 1. Interaction of more than one engine in a transaction
134# 2. Correct commit and rollback behaviour
135# 3. XA protocol communication and recovery
136
137--disable_warnings
138DROP TABLE IF EXISTS t1_trx, t1_non_trx;
139--enable_warnings
140
141SET AUTOCOMMIT= 0;
142
143CREATE TABLE t1_trx (
144 k VARCHAR(10) NOT NULL
145, v VARCHAR(10) NOT NULL
146, PRIMARY KEY (k)
147) ENGINE=InnoDB;
148
149CREATE TEMPORARY TABLE t1_non_trx (
150 k VARCHAR(10) NOT NULL
151, v VARCHAR(10) NOT NULL
152, PRIMARY KEY (k)
153) ENGINE=MyISAM;
154
155START TRANSACTION;
156
157INSERT INTO t1_trx VALUES ('key1','value1');
158INSERT INTO t1_trx VALUES ('key2','value2');
159
160INSERT INTO t1_non_trx VALUES ('key1','value1');
161INSERT INTO t1_non_trx VALUES ('key2','value2');
162
163ROLLBACK;
164
165--echo Expected warning about non-trx data changes not being rolled back
166
167SELECT * FROM t1_trx;
168SELECT * FROM t1_non_trx;
169
170START TRANSACTION;
171
172INSERT INTO t1_trx VALUES ('key1','value1');
173INSERT INTO t1_trx VALUES ('key2','value2');
174
175SELECT t1_trx.k, t1_trx.v
176FROM t1_trx
177INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
178
179ROLLBACK;
180
181SELECT t1_trx.k, t1_trx.v
182FROM t1_trx
183INNER JOIN t1_non_trx ON t1_trx.k = t1_non_trx.k;
184
185DROP TABLE t1_trx;
186DROP TABLE t1_non_trx;

Subscribers

People subscribed via source and target branches