Bug #552420 “implicit startTransaction in ALTER TABLE calling rn...” : Bugs : Drizzle

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-03-31:

#1

This must mean that ALTER TABLE code path is NOT calling mysql_lock_tables()?

Revision history for this message

Brian Aker (brianaker) wrote on 2010-03-31: Re: [Bug 552420] Re: implicit startTransaction in ALTER TABLE calling rnd_init to do data copy

#2

Hi!

On Mar 31, 2010, at 9:14 AM, Jay Pipes wrote:

> This must mean that ALTER TABLE code path is NOT calling
> mysql_lock_tables()?

How handy, I just read all of this code :)

I suspect not for temporary tables. There is a codepath for temporary
and not.

We make a temporary table for the new table and only make it "live"
when we are about to do the rename.

Cheers,
-Brian

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-03-31:

#3

k, that gives me something to go on. thx!

Changed in drizzle:
status:	New → Confirmed
importance:	Undecided → High
assignee:	nobody → Jay Pipes (jaypipes)
milestone:	none → 2010-04-07

Revision history for this message

Brian Aker (brianaker) wrote on 2010-03-31:

#4

BTW all of this code is in drizzled/message/alter_table.cc

File creation happens, after that there is a bit of code that sets up
the select/etc.

On Mar 31, 2010, at 10:37 AM, Jay Pipes wrote:

> k, that gives me something to go on. thx!
>
> ** Changed in: drizzle
> Status: New => Confirmed
>
> ** Changed in: drizzle
> Importance: Undecided => High
>
> ** Changed in: drizzle
> Assignee: (unassigned) => Jay Pipes (jaypipes)
>
> ** Changed in: drizzle
> Milestone: None => 2010-04-07
>
> --
> implicit startTransaction in ALTER TABLE calling rnd_init to do data
> copy
> https://bugs.launchpad.net/bugs/552420
> You received this bug notification because you are a member of
> Drizzle-
> developers, which is subscribed to Drizzle.
>
> Status in A Lightweight SQL Database for Cloud and Web: Confirmed
>
> Bug description:
> in an ALTER TABLE, your engine is (currently) expected to be able to
> start a transaction in rnd_init().
>
> more info coming (as in, "remove these two lines from
> embedded_innodb and you'll see it crash)
>
>

Revision history for this message

Stewart Smith (stewart) wrote on 2010-04-01:

#5

On Wed, 31 Mar 2010 17:37:26 -0000, Jay Pipes <email address hidden> wrote:
> k, that gives me something to go on. thx!

If you want a simple way to know if you've fixed it, you can grab my
embedded-innodb branch (specifically the auto-increment one) and use it.

just remove the transaction starting code from rnd-init and then run the
auto_increment test in embedded_innodb suite.

(i'm pushing this up now)

InnoDB conviniently segfaults for you (with an error message!) if you try
and use a NULL pointer as a transaction :)

on a side note, have we gotten rid of the need for ::store_lock to be
starting a transaction yet?
--
Stewart Smith

Revision history for this message

Brian Aker (brianaker) wrote on 2010-04-09:

#6

Hi!

On Apr 8, 2010, at 4:22 PM, Jay Pipes wrote:

> more info coming (as in, "remove these two lines from
> embedded_innodb and you'll see it crash)

I just love comments like that one. True... yet...

Cheers,
-Brian

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-04-22:

#7

Stewart, what's the status on this one? Got anything more for me that would be a small repeatable test case? Thanks.

Revision history for this message

Stewart Smith (stewart) wrote on 2010-04-27:

#8

=== modified file 'plugin/embedded_innodb/embedded_innodb_engine.cc'
--- plugin/embedded_innodb/embedded_innodb_engine.cc 2010-04-27 13:04:21 +0000
+++ plugin/embedded_innodb/embedded_innodb_engine.cc 2010-04-27 23:05:12 +0000
@@ -1621,12 +1621,6 @@ int EmbeddedInnoDBCursor::rnd_init(bool)
{
ib_trx_t transaction;

- if(*get_trx(current_session) == NULL)
- {
- EmbeddedInnoDBEngine *innodb_engine= static_cast<EmbeddedInnoDBEngine*>(engine);
- innodb_engine->doStartTransaction(current_session, START_TRANS_NO_OPTIONS);
- }
-
transaction= *get_trx(ha_session());

ib_cursor_attach_trx(cursor, transaction);

and then run any ALTER TABLE on an embedded_innodb table. (test-run --suite=embedded_innodb should do it)

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-04-28:

#9

The problem lies in this block of code in /drizzled/statement/alter_table.cc (lines 981-1000):

<code>
  /* Create a temporary table with the new format */
  /**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to be
    filled.
  */
  TableIdentifier new_table_as_temporary(original_table_identifier.getSchemaName(),
                                         tmp_name,
                                         create_proto.type() != message::Table::TEMPORARY ? message::Table::INTERNAL :
                                         message::Table::TEMPORARY);

error= create_temporary_table(session, new_table_as_temporary, create_info, create_proto, alter_info);

  if (error != 0)
  {
    return true;
  }

/* Open the table so we need to copy the data to it. */
new_table= open_alter_table(session, table, new_table_as_temporary);

</code>

The reason it is problematic in relation to this bug is the comment here:

/**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to be
    filled.
*/

Unfortunately, the only two ways that plugin::TransactionalStorageEngine::doStartTransaction() is called are the following two ways:

1) If it is explicitly called by the kernel (which happens when a START TRANSACTION/BEGIN WORK; SQL statement is issued, or
2) When mysql_lock_tables() is called. The kernel decides in /drizzled/lock.cc which engines are active in a statement and calls plugin::TransactionalStorageEngine::doStartTransaction() for each transactional engine if it has not already started a transaction for the engine

So, clearly, the comment that "Neither of these tables require locks in order to be filled" is a hint here. Indeed, if we trace through the alter_table() method in /drizzled/statement/alter_table.cc, we find that during the copy phase (where the new table is being filled with a copy of the old table's data, that instead of Session::openTableLock() being called (which DOES call mysql_lock_tables()), instead Session::open_temporary_table() is called, which does NOT call mysql_lock_tables().

Therefore, the solution to this is to manually call plugin::TransactionalStorageEngine::doStartTransaction() *somewhere* before the copying of the data from the old table into the new table (when Cursor::rnd_init() is eventually callled...)

I should have a fix for this by end of day today. Just trying to find the "right" place to put the call to doStartTransaction()...

The problem lies in this block of code in /drizzled/statement/alter_table.cc (lines 981-1000):

<code>
  /* Create a temporary table with the new format */
  /**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to  be
    filled.
  */
  TableIdentifier new_table_as_temporary(original_table_identifier.getSchemaName(),
                                         tmp_name,
                                         create_proto.type() != message::Table::TEMPORARY ? message::Table::INTERNAL :
                                         message::Table::TEMPORARY);

error= create_temporary_table(session, new_table_as_temporary, create_info, create_proto, alter_info);

if (error != 0)
  {
    return true;
  }

/* Open the table so we need to copy the data to it. */
  new_table= open_alter_table(session, table, new_table_as_temporary);

</code>

The reason it is problematic in relation to this bug is the comment here:

/**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to  be
    filled.
*/

Unfortunately, the only two ways that plugin::TransactionalStorageEngine::doStartTransaction() is called are the following two ways:

1) If it is explicitly called by the kernel (which happens when a START TRANSACTION/BEGIN WORK; SQL statement is issued, or
2) When mysql_lock_tables() is called.   The kernel decides in /drizzled/lock.cc which engines are active in a statement and calls plugin::TransactionalStorageEngine::doStartTransaction() for each transactional engine if it has not already started a transaction for the engine

So, clearly, the comment that "Neither of these tables require locks in order to be filled" is a hint here.  Indeed, if we trace through the alter_table() method in /drizzled/statement/alter_table.cc, we find that during the copy phase (where the new table is being filled with a copy of the old table's data, that instead of Session::openTableLock() being called (which DOES call mysql_lock_tables()), instead Session::open_temporary_table() is called, which does NOT call mysql_lock_tables().

Therefore, the solution to this is to manually call plugin::TransactionalStorageEngine::doStartTransaction() *somewhere* before the copying of the data from the old table into the new table (when Cursor::rnd_init() is eventually callled...)

I should have a fix for this by end of day today.  Just trying to find the "right" place to put the call to doStartTransaction()...

Revision history for this message

Brian Aker (brianaker) wrote on 2010-04-28:

#10

Download full text (3.9 KiB)

BTW just as useful (or possibly useful) info, these tables are very special.

If you are coming from a temporary table we just create another temporary table and switch the names at the end (and there is no recovery).

From a normal table, you create an INTERNAL_TABLE which half exists in the system. It is for all purposes a normal table except that it bypasses a number of systems along the way (like locking). During an alter table this table can become lost during a crash.

Unfortunately the code paths for TEMP tables and normal tables if different still.

On Apr 28, 2010, at 9:41 AM, Jay Pipes wrote:

> The problem lies in this block of code in
> /drizzled/statement/alter_table.cc (lines 981-1000):
>
> <code>
> /* Create a temporary table with the new format */
> /**
> @note we make an internal temporary table unless the table is a temporary table. In last
> case we just use it as is. Neither of these tables require locks in order to be
> filled.
> */
> TableIdentifier new_table_as_temporary(original_table_identifier.getSchemaName(),
> tmp_name,
> create_proto.type() != message::Table::TEMPORARY ? message::Table::INTERNAL :
> message::Table::TEMPORARY);
>
> error= create_temporary_table(session, new_table_as_temporary,
> create_info, create_proto, alter_info);
>
> if (error != 0)
> {
> return true;
> }
>
> /* Open the table so we need to copy the data to it. */
> new_table= open_alter_table(session, table, new_table_as_temporary);
>
> </code>
>
> The reason it is problematic in relation to this bug is the comment
> here:
>
> /**
> @note we make an internal temporary table unless the table is a temporary table. In last
> case we just use it as is. Neither of these tables require locks in order to be
> filled.
> */
>
> Unfortunately, the only two ways that
> plugin::TransactionalStorageEngine::doStartTransaction() is called are
> the following two ways:
>
> 1) If it is explicitly called by the kernel (which happens when a START TRANSACTION/BEGIN WORK; SQL statement is issued, or
> 2) When mysql_lock_tables() is called. The kernel decides in /drizzled/lock.cc which engines are active in a statement and calls plugin::TransactionalStorageEngine::doStartTransaction() for each transactional engine if it has not already started a transaction for the engine
>
> So, clearly, the comment that "Neither of these tables require locks in
> order to be filled" is a hint here. Indeed, if we trace through the
> alter_table() method in /drizzled/statement/alter_table.cc, we find that
> during the copy phase (where the new table is being filled with a copy
> of the old table's data, that instead of Session::openTableLock() being
> called (which DOES call mysql_lock_tables()), instead
> Session::open_temporary_table() is called, which does NOT call
> mysql_lock_tables().
>
> Therefore, the solution to this is to manually call
> plugin::TransactionalStorageEngine::doStartTransaction() *somewhere*
> before the copying of the data from the old table into the new table
> (when Cursor::rnd_init() is e...

BTW just as useful (or possibly useful) info, these tables are very special.

If you are coming from a temporary table we just create another temporary table and switch the names at the end (and there is no recovery).

From a normal table, you create an INTERNAL_TABLE which half exists in the system. It is for all purposes a normal table except that it bypasses a number of systems along the way (like locking). During an alter table this table can become lost during a crash.

Unfortunately the code paths for TEMP tables and normal tables if different still.

On Apr 28, 2010, at 9:41 AM, Jay Pipes wrote:

> The problem lies in this block of code in
> /drizzled/statement/alter_table.cc (lines 981-1000):
> 
> <code>
>  /* Create a temporary table with the new format */
>  /**
>    @note we make an internal temporary table unless the table is a temporary table. In last
>    case we just use it as is. Neither of these tables require locks in order to  be
>    filled.
>  */
>  TableIdentifier new_table_as_temporary(original_table_identifier.getSchemaName(),
>                                         tmp_name,
>                                         create_proto.type() != message::Table::TEMPORARY ? message::Table::INTERNAL :
>                                         message::Table::TEMPORARY);
> 
>  error= create_temporary_table(session, new_table_as_temporary,
> create_info, create_proto, alter_info);
> 
>  if (error != 0)
>  {
>    return true;
>  }
> 
>  /* Open the table so we need to copy the data to it. */
>  new_table= open_alter_table(session, table, new_table_as_temporary);
> 
> </code>
> 
> The reason it is problematic in relation to this bug is the comment
> here:
> 
> /**
>    @note we make an internal temporary table unless the table is a temporary table. In last
>    case we just use it as is. Neither of these tables require locks in order to  be
>    filled.
> */
> 
> Unfortunately, the only two ways that
> plugin::TransactionalStorageEngine::doStartTransaction() is called are
> the following two ways:
> 
> 1) If it is explicitly called by the kernel (which happens when a START TRANSACTION/BEGIN WORK; SQL statement is issued, or
> 2) When mysql_lock_tables() is called.   The kernel decides in /drizzled/lock.cc which engines are active in a statement and calls plugin::TransactionalStorageEngine::doStartTransaction() for each transactional engine if it has not already started a transaction for the engine
> 
> So, clearly, the comment that "Neither of these tables require locks in
> order to be filled" is a hint here.  Indeed, if we trace through the
> alter_table() method in /drizzled/statement/alter_table.cc, we find that
> during the copy phase (where the new table is being filled with a copy
> of the old table's data, that instead of Session::openTableLock() being
> called (which DOES call mysql_lock_tables()), instead
> Session::open_temporary_table() is called, which does NOT call
> mysql_lock_tables().
> 
> Therefore, the solution to this is to manually call
> plugin::TransactionalStorageEngine::doStartTransaction() *somewhere*
> before the copying of the data from the old table into the new table
> (when Cursor::rnd_init() is eventually callled...)
> 
> I should have a fix for this by end of day today.  Just trying to find
> the "right" place to put the call to doStartTransaction()...
> 
> -- 
> implicit startTransaction in ALTER TABLE calling rnd_init to do data copy
> https://bugs.launchpad.net/bugs/552420
> You received this bug notification because you are a member of Drizzle-
> developers, which is subscribed to Drizzle.
> 
> Status in A Lightweight SQL Database for Cloud and Web: In Progress
> Status in Drizzle cherry series: In Progress
> 
> Bug description:
> in an ALTER TABLE, your engine is (currently) expected to be able to start a transaction in rnd_init().
> 
> more info coming (as in, "remove these two lines from embedded_innodb and you'll see it crash)
> 
>

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-04-28: Re: [Bug 552420] Re: implicit startTransaction in ALTER TABLE calling rnd_init to do data copy

#11

On Wed, Apr 28, 2010 at 1:00 PM, Brian Aker <email address hidden> wrote:
> BTW just as useful (or possibly useful) info, these tables are very
> special.
>
> If you are coming from a temporary table we just create another
> temporary table and switch the names at the end (and there is no
> recovery).
>
> >From a normal table, you create an INTERNAL_TABLE which half exists in
> the system. It is for all purposes a normal table except that it
> bypasses a number of systems along the way (like locking). During an
> alter table this table can become lost during a crash.
>
> Unfortunately the code paths for TEMP tables and normal tables if
> different still.

Thanks, Brian. Yeah, some of that code is indeed ugly :) But, I think
I've figured out a way to fix this bug at least ;)

-jay

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-04-28:

#12

Download full text (7.7 KiB)

OK, so an update. Stewart, I may need your help on this. I can't get the embedded_innodb test suite to run cleanly. I get 5 test failures:

jpipes@serialcoder:~/repos/drizzle/bug552420/tests$ LD_PRELOAD=/home/jpipes/repos/libeatmydata/trunk/libeatmydata.so ./dtr --suite=embedded_innodb --force
Logging: ./dtr --suite=embedded_innodb --force
Drizzle Version 2010.04.1511
Using MTR_BUILD_THREAD = -69.4
Using MASTER_MYPORT = 9306
Using MASTER_MYPORT1 = 9307
Using SLAVE_MYPORT = 9308
Using SLAVE_MYPORT1 = 9309
Using SLAVE_MYPORT2 = 9310
Using MC_PORT = 9316
Killing Possible Leftover Processes
Removing Stale Files
Creating Directories
Saving snapshot of installed databases
================================================================================
DEFAULT STORAGE ENGINE: innodb
TEST RESULT TIME (ms)
--------------------------------------------------------------------------------

embedded_innodb.basic_create_nullable [ pass ] 8
embedded_innodb.basic_create_table [ pass ] 3
embedded_innodb.basic_create_with_index [ pass ] 19
embedded_innodb.basic_delete_all_rows [ fail ]
drizzletest: At line 3: query 'delete from t1' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
CREATE TABLE t1 (a int primary key);
insert into t1 values (1),(2),(3);
delete from t1;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/basic_delete_all_rows.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

embedded_innodb.basic_delete_row [ fail ]
drizzletest: At line 4: query 'delete from t1 where a=2' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
CREATE TABLE t1 (a int primary key);
insert into t1 values (1),(2),(3),(4),(5),(6);
select * from t1 order by a;
a
1
2
3
4
5
6
delete from t1 where a=2;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/basic_delete_row.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

embedded_innodb.basic_drop_table [ pass ] 9
embedded_innodb.basic_index_lookup [ pass ] 5
embedded_innodb.basic_innodb_table_proto_table [ pass ] 3
embedded_innodb.basic_innodb_truncate_table [ pass ] 3
embedded_innodb.basic_insert [ pass ] 2
embedded_innodb.basic_insert_multi_column [ pass ] 22
embedded_innodb.basic_insert_pkey_duplicate [ pass ] 2
embedded_innodb.basic_pkey_index_reverse_scan [ pass ] 7
embedded_innodb.basic_pkey_index_scan [ pass ] 2
embedded_innodb.basic_rename_table [ pass ] 3
embedded_innod...

OK, so an update.  Stewart, I may need your help on this.  I can't get the embedded_innodb test suite to run cleanly.  I get 5 test failures:

jpipes@serialcoder:~/repos/drizzle/bug552420/tests$ LD_PRELOAD=/home/jpipes/repos/libeatmydata/trunk/libeatmydata.so ./dtr --suite=embedded_innodb --force
Logging: ./dtr --suite=embedded_innodb --force
Drizzle Version 2010.04.1511
Using MTR_BUILD_THREAD      = -69.4
Using MASTER_MYPORT         = 9306
Using MASTER_MYPORT1        = 9307
Using SLAVE_MYPORT          = 9308
Using SLAVE_MYPORT1         = 9309
Using SLAVE_MYPORT2         = 9310
Using MC_PORT               = 9316
Killing Possible Leftover Processes
Removing Stale Files
Creating Directories
Saving snapshot of installed databases
================================================================================
DEFAULT STORAGE ENGINE: innodb
TEST                                                         RESULT    TIME (ms)
--------------------------------------------------------------------------------

embedded_innodb.basic_create_nullable                        [ pass ]       8
embedded_innodb.basic_create_table                           [ pass ]       3
embedded_innodb.basic_create_with_index                      [ pass ]      19
embedded_innodb.basic_delete_all_rows                        [ fail ]
drizzletest: At line 3: query 'delete from t1' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
CREATE TABLE t1 (a int primary key);
insert into t1 values (1),(2),(3);
delete from t1;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/basic_delete_all_rows.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

embedded_innodb.basic_delete_row                             [ fail ]
drizzletest: At line 4: query 'delete from t1 where a=2' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
CREATE TABLE t1 (a int primary key);
insert into t1 values (1),(2),(3),(4),(5),(6);
select * from t1 order by a;
a
1
2
3
4
5
6
delete from t1 where a=2;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/basic_delete_row.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

embedded_innodb.basic_drop_table                             [ pass ]       9
embedded_innodb.basic_index_lookup                           [ pass ]       5
embedded_innodb.basic_innodb_table_proto_table               [ pass ]       3
embedded_innodb.basic_innodb_truncate_table                  [ pass ]       3
embedded_innodb.basic_insert                                 [ pass ]       2
embedded_innodb.basic_insert_multi_column                    [ pass ]      22
embedded_innodb.basic_insert_pkey_duplicate                  [ pass ]       2
embedded_innodb.basic_pkey_index_reverse_scan                [ pass ]       7
embedded_innodb.basic_pkey_index_scan                        [ pass ]       2
embedded_innodb.basic_rename_table                           [ pass ]       3
embedded_innodb.basic_savepoint                              [ pass ]       3
embedded_innodb.basic_select                                 [ pass ]       2
embedded_innodb.basic_select_multirow                        [ pass ]       2
embedded_innodb.basic_show_tables                            [ pass ]       1
embedded_innodb.basic_statement_rollback                     [ pass ]       2
embedded_innodb.basic_transaction                            [ pass ]       4
embedded_innodb.basic_transaction_autocommit                 [ pass ]      16
embedded_innodb.basic_transaction_rollback                   [ pass ]      13
embedded_innodb.basic_update                                 [ fail ]
drizzletest: At line 3: query 'update t1 set a=2 where a=1' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
create table t1(a int primary key);
insert into t1 values (1);
update t1 set a=2 where a=1;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/basic_update.log

Stopping All Servers
mysql-test-run: WARNING: Forcing kill of process 10217
Restoring snapshot of databases
Resuming Tests

embedded_innodb.basic_with_indexes                           [ pass ]       3
embedded_innodb.config_table_function                        [ pass ]       1
embedded_innodb.cursor_position                              [ pass ]      11
embedded_innodb.index_multicolumn_unique                     [ pass ]      29
embedded_innodb.index_null                                   [ pass ]       9
embedded_innodb.index_varchar                                [ pass ]      11
embedded_innodb.insert_null                                  [ pass ]       6
embedded_innodb.libinnodb_datadict_dump_func                 [ pass ]       3
embedded_innodb.libinnodb_version_func                       [ pass ]       0
embedded_innodb.plugin_load                                  [ pass ]       1
embedded_innodb.rename                                       [ pass ]      14
embedded_innodb.rnd_pos                                      [ fail ]
drizzletest: At line 17: query 'UPDATE t1 SET t1.xstatus_vor = Greatest(t1.xstatus_vor,1) WHERE t1.aufnr =
"40004712" AND t1.plnfl = "000001" AND t1.vornr > "0010" ORDER BY t1.vornr
ASC LIMIT 1' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
CREATE TABLE t1 (
aufnr varchar(12) NOT NULL default '',
plnfl varchar(6) NOT NULL default '',
vornr varchar(4) NOT NULL default '' primary key,
xstatus_vor int NOT NULL default '0'
);
INSERT INTO t1 VALUES ('40004712','000001','0010',9);
INSERT INTO t1 VALUES ('40004712','000001','0020',0);
UPDATE t1 SET t1.xstatus_vor = Greatest(t1.xstatus_vor,1) WHERE t1.aufnr =
"40004712" AND t1.plnfl = "000001" AND t1.vornr > "0010" ORDER BY t1.vornr
ASC LIMIT 1;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/rnd_pos.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

embedded_innodb.secondary_index_scan                         [ pass ]      10
embedded_innodb.status_table_function                        [ fail ]
drizzletest: At line 13: query 'delete from t1' failed: 1031: Table storage engine for 't1' doesn't have this option

The result from queries just before the failure was:
< snip >
have_atomic_builtins
SELECT * FROM DATA_DICTIONARY.INNODB_STATUS WHERE NAME="page_size";
NAME	VALUE
page_size	16384
SELECT (@fsync_count:= VALUE) AS fsyncs FROM DATA_DICTIONARY.INNODB_STATUS WHERE NAME="fsync_req_done";
fsyncs
#
create table t1 (a int primary key);
SELECT (value - @fsync_count) AS fsyncs FROM DATA_DICTIONARY.INNODB_STATUS WHERE NAME="fsync_req_done";
fsyncs
4
insert into t1 values (1);
SELECT (value - @fsync_count) AS fsyncs FROM DATA_DICTIONARY.INNODB_STATUS WHERE NAME="fsync_req_done";
fsyncs
5
insert into t1 values (5),(2),(3),(4);
SELECT (value - @fsync_count) AS fsyncs FROM DATA_DICTIONARY.INNODB_STATUS WHERE NAME="fsync_req_done";
fsyncs
6
delete from t1;

More results from queries before failure can be found in /home/jpipes/repos/drizzle/bug552420/tests/var/log/status_table_function.log

Stopping All Servers
Restoring snapshot of databases
Saving core
Resuming Tests

--------------------------------------------------------------------------------
Stopping All Servers
Failed 5/38 tests, 86.84% were successful.

This is on an unmodified trunk.

Please advise.

Also, I don't see the code you are referring to in EmbeddedInnobaseCursor::rnd_init().  Please let me know if you have some advice.

Thanks,

jay

Revision history for this message

Stewart Smith (stewart) wrote on 2010-04-29: Re: [Bug 552420] Re: implicit startTransaction in ALTER TABLE calling rnd_init to do data copy

#13

On Wed, 28 Apr 2010 19:48:11 -0000, Jay Pipes <email address hidden> wrote:
> OK, so an update. Stewart, I may need your help on this. I can't get
> the embedded_innodb test suite to run cleanly. I get 5 test failures:

Waiting to be merged into trunk... check out my embedded_innodb branch
in the meantime.

--
Stewart Smith

Revision history for this message

Stewart Smith (stewart) wrote on 2010-04-29:

#14

On Wed, 28 Apr 2010 16:41:37 -0000, Jay Pipes <email address hidden> wrote:
> Therefore, the solution to this is to manually call
> plugin::TransactionalStorageEngine::doStartTransaction() *somewhere*
> before the copying of the data from the old table into the new table
> (when Cursor::rnd_init() is eventually callled...)

There has been an "optimisation" in the past for some engines (NDB and
maybe InnoDB) to help reduce the amount of log potentially have to be
replayed (or in NDB case the number of operations records used as well)
to COMMIT every so often in an alter table.

This could possibly be a feature of the upper layer... OR (I think the
better approach) is to maybe have a flag to start transaction saying
that this transaction can be committed every so often by the engine as
an optimisation.

thoughts?

--
Stewart Smith

Revision history for this message

Brian Aker (brianaker) wrote on 2010-04-29:

#15

Hi!

I think a hint from the engine is appropriate. I expect more of alter
to be pushed down to the engine in the future.

Cheers,
--Brian

On Apr 28, 2010, at 6:38 PM, Stewart Smith <email address hidden>
wrote:

> On Wed, 28 Apr 2010 16:41:37 -0000, Jay Pipes <email address hidden>
> wrote:
>> Therefore, the solution to this is to manually call
>> plugin::TransactionalStorageEngine::doStartTransaction() *somewhere*
>> before the copying of the data from the old table into the new table
>> (when Cursor::rnd_init() is eventually callled...)
>
> There has been an "optimisation" in the past for some engines (NDB and
> maybe InnoDB) to help reduce the amount of log potentially have to be
> replayed (or in NDB case the number of operations records used as
> well)
> to COMMIT every so often in an alter table.
>
> This could possibly be a feature of the upper layer... OR (I think the
> better approach) is to maybe have a flag to start transaction saying
> that this transaction can be committed every so often by the engine as
> an optimisation.
>
> thoughts?
>
> --
> Stewart Smith
>
> --
> implicit startTransaction in ALTER TABLE calling rnd_init to do data
> copy
> https://bugs.launchpad.net/bugs/552420
> You received this bug notification because you are a member of
> Drizzle-
> developers, which is subscribed to Drizzle.
>
> Status in A Lightweight SQL Database for Cloud and Web: In Progress
> Status in Drizzle cherry series: In Progress
>
> Bug description:
> in an ALTER TABLE, your engine is (currently) expected to be able to
> start a transaction in rnd_init().
>
> more info coming (as in, "remove these two lines from
> embedded_innodb and you'll see it crash)
>
>

Revision history for this message

Jay Pipes (jaypipes) wrote on 2010-04-29:

#16

OK, I hear you guys, but in this specific bug case, do we want to "fix" things using a hint? I don't believe that's what you're suggesting, right? For this fix, I believe just explicitly starting the transaction in the alter_table() kernel function is what is required.

Pls let me know if I am on the right track. Thanks.

-jay

Revision history for this message

Stewart Smith (stewart) wrote on 2010-04-30:

#17

On Thu, 29 Apr 2010 18:02:05 -0000, Jay Pipes <email address hidden> wrote:
> OK, I hear you guys, but in this specific bug case, do we want to "fix"
> things using a hint? I don't believe that's what you're suggesting,
> right? For this fix, I believe just explicitly starting the transaction
> in the alter_table() kernel function is what is required.

For this bug, yes.

IIRC engines currently queried sql_lex to see if they could do such
things.. which is still gonig to work with just adding start/commit to
alter_table().

> Pls let me know if I am on the right track. Thanks.

so yeah, it is the right track.

--
Stewart Smith

Jay Pipes (jaypipes) on 2010-05-10

Changed in drizzle:
status:	Fix Committed → Fix Released

Jay Pipes (jaypipes) on 2010-05-10

Changed in drizzle:
status:	Fix Released → Fix Committed

Jay Pipes (jaypipes) on 2010-05-11

Changed in drizzle:
milestone:	2010-05-10 → 2010-05-24

Lee Bieber (kalebral-deactivatedaccount) on 2010-05-21

Changed in drizzle:
milestone:	2010-05-24 → none

Stewart Smith (stewart) on 2010-05-25

tags:

added: storage-engine-api

Stewart Smith (stewart) on 2010-06-03

Changed in drizzle:
status:	Fix Committed → Fix Released

Drizzle

implicit startTransaction in ALTER TABLE calling rnd_init to do data copy

Bug Description

Related branches

Other bug subscribers

Remote bug watches

Affects		Status	Importance	Assigned to	Milestone
	Drizzle	Fix Released	High	Jay Pipes
	Cherry	Fix Released	High	Jay Pipes	Drizzle 2010-06-07