branches/zip:
Fix the rpl_ddl mysql-test that produces a warnings:
master.err: rpl.rpl_ddl: 080611 16:27:01 [Warning] MySQL is closing a connection that has an active InnoDB transaction. 13 row modifications will roll back.
the bug was introduced in r2256, which adds "prebuilt->trx->active_trans = 1;"
in ::add_index() and ::final_drop_index() but never calls innobase_commit() or
innobase_rollback() to set back active_trans to 0.
Also, per Heikki's suggestion we remove the calls to trans_register_ha() from
::add_index() and ::final_drop_index(). Following is the IM discussion that
took place about this:
--- cut ---
(17:59:05) vasil:
unsigned active_trans:2; /* 1 - if a transaction in MySQL
is active. 2 - if prepare_commit_mutex
was taken */
(17:59:13) vasil: what about 0?
(17:59:26) Heikki: Hmm
(17:59:42) Heikki: Maybe in the other cases :)
(18:01:00) vasil:
2 1369 handler/ha_innodb.cc <<<unknown>>>
if (trx->active_trans == 0) {
3 1372 handler/ha_innodb.cc <<<unknown>>>
trx->active_trans = 1;
(18:01:05) vasil: yes, it can be 0
(18:01:38) Heikki: Maybe no active transaction => the value is 0
(18:02:03) vasil: probably yes
(18:09:09) vasil:
/* The flag trx->active_trans is set to 1 in
1. ::external_lock(),
2. ::start_stmt(),
3. innobase_query_caching_of_table_permitted(),
4. innobase_savepoint(),
5. ::init_table_handle_for_HANDLER(),
6. innobase_start_trx_and_assign_read_view(),
7. ::transactional_table_lock()
and it is only set to 0 in a commit or a rollback.
(18:09:14) vasil: this is no longer true
(18:09:35) vasil: it is also set to 1 in ::final_drop_index
(18:09:56) vasil: and in ::add_index()
(18:12:31) Heikki: Please correct the comment :(
(18:13:01) vasil: it is not only the comment, I am trying to understand what's wrong
(18:13:28) Heikki: What is the symptom?
(18:14:37) vasil: can you find this email and the subsequent one:
From: Vasil Dimov <vasil.dimov@oracle.com>
To: InnoDB Dev <innodb_dev_ww@oracle.com>
Subject: Re: innodb - marko - r2256 - in branches/zip: handler include lock
trx
Date: Wed, 11 Jun 2008 16:33:47 +0300
(18:15:02) vasil: the symptom is
master.err: rpl.rpl_ddl: 080611 16:27:01 [Warning] MySQL is closing a connection that has
+an active InnoDB transaction. 13 row modifications will roll back.
(18:16:22) vasil: My understanding till now is that active_trans is set to 1 in ::add_index() and forgotten like this, never set to 0; I wonder if it should be set to 0 somewhere at the end or not set to 1 at all... or there is more...
(18:16:31) Heikki: Vasil, ok the problem is that Marko forgets to commit the transaction
(18:16:55) Heikki: An active transaction has to be committed or rolled back
(18:17:09) vasil: but i can find this in ::add_index():
/* Commit the data dictionary transaction in order to release
the table locks on the system tables. Unfortunately, this
means that if MySQL crashes while creating a new primary key
inside row_merge_build_indexes(), indexed_table will not be
dropped on crash recovery. Thus, it will become orphaned. */
trx_commit_for_mysql(trx);
(18:17:27) vasil: but this function does not set active_trans to 0
(18:17:43) Heikki: Let me look
(18:19:20) vasil: it is only set to 0 in innobase_commit() and in innobase_rollback()
(18:20:10) Heikki: Ok
(18:20:59) Heikki: Hmm...
(18:21:08) vasil: innobase_commit() calls innobase_commit_low() calls trx_commit_for_mysql()
innobase_commit() sets active_trans=0
(18:21:26) Heikki: active_trans is strictly a ha_innodb.cc variable
(18:21:53) Heikki: It is intended to be used in transactions visible to MySQL
(18:22:25) Heikki: I think Marko should call innobase_commit()
(18:23:51) vasil: but still set active_trans=1 inside :add_index() inside handler/handler0alter.cc?
(18:24:12) Heikki: Hmm
(18:24:56) Heikki: I think the transaction is NOT really visible to MySQL
(18:24:57) vasil:
> Author: marko
> Date: 2008-01-25 16:26:07 +0200 (Fri, 25 Jan 2008)
> New Revision: 2256
>
> Log:
> branches/zip: Fast index creation: Release locks on system tables before
> creating indexes. Lock the user table inside the user transaction.
>
> enum trx_dict_op: Remove TRX_OP_INDEX_MAY_WAIT.
>
> ha_innobase::add_index(): Lock the user tables within prebuilt->trx.
> Commit the data dictionary transaction before creating indexes.
>
> ha_innobase::final_drop_index(): Lock the user table within prebuilt->trx.
(18:26:41) Heikki: Let me look at the old ::create_table()
(18:27:25) Heikki: Ok, in the old code, the transactions are internal to InnoDB, not visible to MySQL
(18:27:46) Heikki: I think it is an error to set active_trans=1 inside :add_index() i
(18:28:11) Heikki: MySQL is not really interested in HOW the engine adds the index. In a transaction or not.
(18:28:26) vasil: this is the change inside drop_index(), that is supposed to "Lock the user table within prebuilt->trx":
(18:28:37) vasil:
@@ -1093,27 +1101,25 @@ ha_innobase::final_drop_index(
/* Create a background transaction for the operations on
the data dictionary tables. */
trx = trx_allocate_for_mysql();
trx_start_if_not_started(trx);
trans_register_ha(user_thd, FALSE, ht);
+ prebuilt->trx->active_trans = 1;
trx->mysql_thd = user_thd;
trx->mysql_query_str = thd_query(user_thd);
/* Flag this transaction as a dictionary operation, so that
- the data dictionary will be locked in crash recovery. Prevent
- warnings if row_merge_lock_table() results in a lock wait,
- i.e., when another transaction is holding a conflicting lock
- on the table, e.g., because of SELECT ... FOR UPDATE. */
- trx_set_dict_operation(trx, TRX_DICT_OP_INDEX_MAY_WAIT);
+ the data dictionary will be locked in crash recovery. */
+ trx_set_dict_operation(trx, TRX_DICT_OP_INDEX);
/* Lock the table exclusively, to ensure that no active
transaction depends on an index that is being dropped. */
err = convert_error_code_to_mysql(
- row_merge_lock_table(trx, prebuilt->table, LOCK_X),
+ row_merge_lock_table(prebuilt->trx, prebuilt->table, LOCK_X),
user_thd);
if (UNIV_UNLIKELY(err)) {
/* Unmark the indexes to be dropped. */
row_mysql_lock_data_dictionary(trx);
@@ -1125,14 +1131,12 @@ ha_innobase::final_drop_index(
}
row_mysql_unlock_data_dictionary(trx);
goto func_exit;
}
- trx_set_dict_operation(trx, TRX_DICT_OP_INDEX);
-
/* Drop indexes marked to be dropped */
row_mysql_lock_data_dictionary(trx);
index = dict_table_get_first_index(prebuilt->table);
@@ -1159,12 +1163,13 @@ ha_innobase::final_drop_index(
dict_table_check_for_dup_indexes(prebuilt->table);
#endif
row_mysql_unlock_data_dictionary(trx);
func_exit:
trx_commit_for_mysql(trx);
+ trx_commit_for_mysql(prebuilt->trx);
/* Flush the log to reduce probability that the .frm files and
the InnoDB data dictionary get out-of-sync if the user runs
with innodb_flush_log_at_trx_commit = 0 */
log_buffer_flush_to_disk();
(18:28:43) Heikki: Let me look
(18:29:05) vasil: this is the change inside drop_index(), that is supposed to "Lock the user table within prebuilt->trx"
(18:29:26) Heikki: trans_register_ha(user_thd, FALSE, ht);
+ prebuilt->trx->active_trans = 1;
(18:29:34) Heikki: I think the above code should be removed
(18:29:53) Heikki: MySQL is not interested in InnoDB's transaction in this case
(18:30:33) Heikki: Alternatively, we should use innobase_commit() instead of trx_commit_...
(18:31:01) vasil: but there are many places in ::add_index() where it could return before reaching the trx_commit...
(18:31:12) vasil: leaving active_traans=1
(18:31:56) Heikki: Does Marko handle the errors with trx_rollback()?
(18:32:08) vasil: let me see..
(18:32:30) Heikki: It is risky to leave error handling to MySQL :(
(18:32:35) vasil:
651 if (UNIV_UNLIKELY(error)) {
652 err_exit:
653 mem_heap_free(heap);
654 trx_general_rollback_for_mysql(trx, FALSE, NULL);
655 trx_free_for_mysql(trx);
656 trx_commit_for_mysql(prebuilt->trx);
657 DBUG_RETURN(error);
658 }
(18:32:43) Heikki: Ok
(18:33:25) Heikki: Please remove the ha_register... and active_trx =1 ....
(18:33:42) Heikki: Then check if it prints something
(18:34:08) Heikki: I believe MySQL is not interested in how the engine does the index build
(18:34:11) vasil: why remove ha_register?
(18:34:28) Heikki: Because there is no transaction visible to MySQL
(18:34:53) vasil: the ha_register call was there from before
(18:35:09) Heikki: Registers that InnoDB takes part in an SQL statement, so that MySQL knows to
roll back the statement if the statement results in an error. This MUST be
called for every SQL statement that may be rolled back by MySQL. Calling this
several times to register the same statement is allowed, too.
(18:36:01) Heikki: Hmm
(18:36:17) Heikki: Obviously, MySQL does NOT call commit at the end
(18:36:36) Heikki: Because then innobase_commit() would set active_trx=0!
(18:37:00) Heikki: Ok, maybe MySQL thinks CREATE INDEX is NOT a transactional statement
(18:37:21) Heikki: Then, .._register_.. should be removed
(18:37:51) Heikki: It is logical this way. If it also passes test, even better
(18:38:21) vasil: ha_registrer in add_index() was added in r1845 and in drop_index() in r2128, let me read the comments..
(18:38:55) vasil:
------------------------------------------------------------------------
r1845 | marko | 2007-09-13 13:31:54 +0300 (Thu, 13 Sep 2007) | 3 lines
branches/zip: Move the code related to fast index creation
(smart ALTER TABLE) from ha_innodb.cc to a separate module, handler0alter.cc.
(18:39:05) Heikki: One thing you must check is that MySQL does write the CREATE INDEX in the binlog, even if we remove the _register_
(18:39:41) Heikki: I assume Marko did not know if MySQL thinks CREATE INDEX is transactional or not
(18:39:49) Heikki: He thought it is transactional
(18:39:53) vasil:
------------------------------------------------------------------------
r2128 | marko | 2007-11-29 12:34:55 +0200 (Thu, 29 Nov 2007) | 5 lines
branches/zip: ha_innobase::final_drop_index(): Allocate a separate transaction
for dropping the index trees, and set the dictionary operation flag, similar
to what ha_innobase::add_index() does. This should ensure correct crash
recovery.
(18:40:10) Heikki: Ok
(18:40:17) vasil: how do you know CREATE INDEX is not transactional?
(18:40:33) Heikki: You cannot roll it back
(18:40:44) vasil: ok
(18:41:03) Heikki: MySQL devs probably thought this way
(18:44:15) vasil: hmm
(18:44:21) vasil: about r1845
(18:45:00) vasil: the comments says "move the code" but it is actually "move and change the code"
(18:45:28) Heikki: Well
(18:45:29) vasil: the code that is deleted from ha_innodb is this:
(18:45:32) vasil:
- update_thd(ha_thd());
-
- heap = mem_heap_create(1024);
-
- /* In case MySQL calls this in the middle of a SELECT query, release
- possible adaptive hash latch to avoid deadlocks of threads. */
- trx_search_latch_release_if_reserved(check_trx_exists(user_thd));
-
- trx = trx_allocate_for_mysql();
- trx_start_if_not_started(trx);
-
- innobase_register_stmt(ht, user_thd);
-
- trx->mysql_thd = user_thd;
(18:45:38) vasil: (in add_index)
(18:45:48) inaam left the room.
(18:45:52) vasil: but the new one in handler0alter is this:
(18:46:16) vasil:
+ update_thd(ha_thd());
+
+ heap = mem_heap_create(1024);
+
+ /* In case MySQL calls this in the middle of a SELECT query, release
+ possible adaptive hash latch to avoid deadlocks of threads. */
+ trx_search_latch_release_if_reserved(check_trx_exists(user_thd));
+
+ trx = trx_allocate_for_mysql();
+ trx_start_if_not_started(trx);
+
+ trans_register_ha(user_thd, FALSE, ht);
+
+ trx->mysql_thd = user_thd;
(18:46:48) vasil: innobase_register_stmt(ht, user_thd); was substituted with trans_register_ha(user_thd, FALSE, ht); during the move
(18:46:49) Heikki: I do not see how MySQL could call it in the middle of a SELECT query!
(18:46:59) Heikki: Yes
(18:47:21) Heikki: Did Marko change innobase_register_stmt() code too?
(18:48:05) vasil: not in this commit r1845
(18:48:54) Heikki: Anyway, please test a version where you remove the register thing
(18:49:08) vasil: and active_trans=1?
(18:49:15) Heikki: Remove that, too
(18:49:35) Heikki: innobase_register_stmt(
/*===================*/
handlerton* hton, /* in: Innobase hton */
THD* thd) /* in: MySQL thd (connection) object */
{
DBUG_ASSERT(hton == innodb_hton_ptr);
/* Register the statement */
trans_register_ha(thd, FALSE, hton);
}
(18:50:23) Heikki: looks pretty much the same as what Marko has in the later version
(18:50:43) vasil: innobase_register_stmt() was added in add_index() in r1584
(18:50:47) vasil:
r1584 | marko | 2007-06-18 15:46:42 +0300 (Mon, 18 Jun 2007) | 12 lines
branches/zip: ha_innobase::add_index(): Split some assertions.
Remove the variable parent_trx.
Call innobase_register_stmt() in order to work around a MySQL bug
in mysql_alter_table(), which, as of
ChangeSet@1.2482.61.2, 2007-06-07 16:37:15+02:00, joerg@trift2. +8 -0
commits the transaction before calling ha_innobase::add_index().
Without re-registering the statement, the ha_commit_stmt(thd)
in mysql_alter_table() would not invoke innobase_commit.
(18:51:56) Heikki: But it does not seem to invoke it now, because active_trans is left 1!
(18:54:15) Heikki: Apparently, there has been confusion also among MySQL devs whether CREATE INDEX is transactional or not
(18:54:45) vasil: :-(
(18:54:53) Heikki: The most robust way is to handle all commits and rollbacks internally inside InnoDB and not tell MySQL anything about InnoDB's internal transactions
(18:55:01) Heikki: That is my idea
(18:55:20) Heikki: Then MySQL cannot spoil the logic of the code
(18:55:59) vasil: mysql_alter_table() is ~1100 lines
(18:56:08) vasil: and you suggest what?
(18:56:43) Heikki: I suggest removing the ..._register_... thing and active_trans = 1
(18:57:08) Heikki: Those things tell MySQL that there is a transaction going on
(18:57:14) Heikki: Better not to tell
(18:57:29) vasil: and possibly ignoring the above comment in r1584, "Call innobase_register_stmt() in order to work around a MySQL bug
in mysql_alter_table()........."
(18:57:32) vasil: ?
(18:57:36) Heikki: Yes
(18:57:38) vasil: ok
(18:57:44) vasil: same for drop_index?
(18:57:48) Heikki: Yes
(18:58:51) vasil: testing
Index: handler/handler0alter.cc
===================================================================
--- handler/handler0alter.cc (revision 2498)
+++ handler/handler0alter.cc (working copy)
@@ -632,14 +632,14 @@ ha_innobase::add_index(
/* Create a background transaction for the operations on
the data dictionary tables. */
trx = trx_allocate_for_mysql();
trx_start_if_not_started(trx);
- trans_register_ha(user_thd, FALSE, ht);
- prebuilt->trx->active_trans = 1;
+ //trans_register_ha(user_thd, FALSE, ht);
+ //prebuilt->trx->active_trans = 1;
trx->mysql_thd = user_thd;
trx->mysql_query_str = thd_query(user_thd);
innodb_table = indexed_table
= dict_table_get(prebuilt->table->name, FALSE);
@@ -1081,14 +1081,14 @@ ha_innobase::final_drop_index(
/* Create a background transaction for the operations on
the data dictionary tables. */
trx = trx_allocate_for_mysql();
trx_start_if_not_started(trx);
- trans_register_ha(user_thd, FALSE, ht);
- prebuilt->trx->active_trans = 1;
+ //trans_register_ha(user_thd, FALSE, ht);
+ //prebuilt->trx->active_trans = 1;
trx->mysql_thd = user_thd;
trx->mysql_query_str = thd_query(user_thd);
/* Flag this transaction as a dictionary operation, so that
the data dictionary will be locked in crash recovery. */
(18:59:07) vasil: not warnings
(18:59:22) Heikki: Did it create the indexes
(18:59:36) vasil: I did test only removing active_trans=1 and it removed the warnings too
(18:59:38) vasil: dunno
(18:59:54) Heikki: That is the most important thing to check :)
(19:00:10) vasil: :)
(19:00:38) vasil: it is some mysqltest that is 610 lines
(19:00:48) vasil: and it happens at the end
(19:01:02) vasil: lets see how may add index statements...
(19:01:11) vasil: 0
(19:01:26) Heikki: Mostly I am concerned that MySQL might work wrong without that _register_ thing
(19:03:28) vasil: could be
(19:03:49) Heikki: You need to check it still calls ::add_index()
(19:05:15) vasil: well, the change we made is inside ::add_index() how could it result in mysql stopping to call ::add_index()?
(19:05:39) vasil: if it calls it twice...
(19:05:43) Heikki: :-D
(19:08:34) vasil: I guess it must have created the index, because the test is doing SHOW INDEX after CREATE INDEX and after DROP INDEX
(19:08:43) vasil: the test would have failed if the index was not created
(19:09:10) Heikki: You need to check that a query using that index will have the right EXPLAIN and result
(19:15:04) vasil: this is with the patch:
+CREATE TABLE t (a INT) ENGINE=innodb;
+CREATE INDEX ti ON t(a);
+INSERT INTO t VALUES (1), (2), (3);
+EXPLAIN SELECT * FROM t WHERE a=2;
+id select_type table type possible_keys key key_len ref rows Extra
+1 SIMPLE t ref ti ti 5 const 1 Using where; Using index
+SELECT * FROM t WHERE a=2;
+a
+2
(19:15:15) Heikki: :)
(19:15:46) vasil: this is with the original code:
+CREATE TABLE t (a INT) ENGINE=innodb;
+CREATE INDEX ti ON t(a);
+INSERT INTO t VALUES (1), (2), (3);
+EXPLAIN SELECT * FROM t WHERE a=2;
+id select_type table type possible_keys key key_len ref rows Extra
+1 SIMPLE t ref ti ti 5 const 1 Using where; Using index
+SELECT * FROM t WHERE a=2;
+a
+2
(19:15:59) vasil: it is the same
(19:16:06) vasil: do you think this is sufficient?
(19:16:20) Heikki: No, you need to do some manual tests, too
(19:16:28) vasil: this is manual test
(19:16:37) Heikki: Several of them
(19:16:46) vasil: what should they do?
(19:17:13) Heikki: Test tables of different sizes, different indexes
(19:17:27) Heikki: CREATE several indexes at a time
(19:19:14) Heikki: Dinner time -->
--- cut ---
Approved by: Heikki
Showing
Please register or sign in to comment