MySQL Bugs: #11238: SELECT ... FOR UPDATE executed as consistent read inside LOCK TABLES

Bug #11238	SELECT ... FOR UPDATE executed as consistent read inside LOCK TABLES
Submitted:	10 Jun 2005 13:45	Modified:	6 Oct 2005 2:15
Reporter:	Heikki Tuuri
Status:	Closed
Category:	Server	Severity:	S1 (Critical)
Version:	4.1, 5.0	OS:	Any (All)
Assigned to:	Bugs System	Target Version:

Description:
Hi!

Vadim found this bug last week. Inside LOCK TABLES, InnoDB executes:

SELECT ... FOR UPDATE;

and

SELECT ... LOCK IN SHARE MODE;

as non-locking consistent reads. This can break application logic, since a consistent
read may return stale data.

The bug is also present in 5.0 stored procedures, even without using LOCK TABLES.

Regards,

Heikki

How to repeat:
See above.

Suggested fix:
MySQL should store to thd->lex information about a lock clause in a SELECT. Then the code
in ha_innobase::start_stmt() could check it. The code currently looks at
thd->lex->lock_option, but that does NOT contain the required information.

Regards,

Heikki

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/27395

Heikki,
 Is it possible to provide a simple test case or test the cset attached to this case?
Antony,

Antony,

the patch just contains a new argument to the ::start_stmt() function. Where are the rest
of changes?

The knowledge about a ... FOR UPDATE or ... LOCK IN SHARE MODE clause must be gathered in
the .yy code. From there it must be passed to the handlers.

Since it is a syntactic thing, I think you should also add the information to the lex
struct, like I suggested.

Regards,

Heikki

Heikki,

I don't believe any further changes are required in the parser - data is stored when the
user does a ... FOR UPDATE or ... LOCK IN SHARE MODE, but the infomation was only used to
check if the locking was compatible and then was discarded. With the changes in the cset,
the when a ...FOR UPDATE is parsed, ::start_stmt should expect to see TL_WRITE as the
lock type, and when a .. LOCK IN SHARE MODE is parsed, ::start_stmt should expect to see
a TL_READ_WITH_SHARED_LOCKS as the provided lock_type... otherwise only should see a
TL_READ when performing a SELECT statement.
However, I would like a testcase I can run which demonstrates failure without this cset,
so that I may ensure that the code is working as I expect.
Regards,

Antony.

Antony,

you are right.

(gdb) bt
#0  ha_innobase::start_stmt(THD*) (this=0x8c43fe0, thd=0x8c42a40)
    at ha_innodb.cc:5843
#1  0x081d2c85 in check_lock_and_start_stmt (thd=0x8c42a40, table=0x8c4d020,
    lock_type=TL_READ_WITH_SHARED_LOCKS) at sql_base.cc:1964
#2  0x081d36cd in lock_tables(THD*, st_table_list*, unsigned) (thd=0x8c42a40,
    tables=0x8c6b868, count=1) at sql_base.cc:2274
#3  0x081d3020 in open_and_lock_tables(THD*, st_table_list*) (thd=0x8c42a40,
    tables=0x8c6b868) at sql_base.cc:2088
#4  0x081a56d6 in mysql_execute_command(THD*) (thd=0x8c42a40)
    at sql_parse.cc:2397
#5  0x081ae1eb in mysql_parse(THD*, char*, unsigned) (thd=0x8c42a40,
    inBuf=0x8c6b760 "select * from a lock in share mode", length=34)
    at sql_parse.cc:5381
#6  0x081a39bf in dispatch_command(enum_server_command, THD*, char*, unsigned)
    (command=COM_QUERY, thd=0x8c42a40,
    packet=0x8c63701 "select * from a lock in share mode", packet_length=35)
    at sql_parse.cc:1674
#7  0x081a31c6 in do_command(THD*) (thd=0x8c42a40) at sql_parse.cc:1477
#8  0x081a22cd in handle_one_connection (arg=0x8c42a40) at sql_parse.cc:1126
#9  0x40062f60 in pthread_start_thread () from /lib/i686/libpthread.so.0
#10 0x400630fe in pthread_start_thread_event () from /lib/i686/libpthread.so.0
#11 0x401f5327 in clone () from /lib/i686/libc.so.6
(gdb)

MySQL does have the necessary lock information in check_lock_and_start_stmt().

Please run also a stored procedure inside gdb, and check that lock_type is right.

Please post the test results here. Then you have the my permission to push the patch for
InnoDB. For the other code, someone else has to review.

Jan Lindstrom can help you to write a test case. Use two connections: one inserts into a
table, another is in the AUTOCOMMIT=0 mode and does LOCK TABLES and various types of
SELECT ...

Regards,

Heikki

Here is one possible test case (somehow I remember that I have already sent this):

create table t1(a int not null, b int, primary key(a)) engine = innodb;
insert into t1 values(1,1),(3,3);
commit;
connect (con1, ...)
connect (con2, ...)
connection con1;
set autocommit=0;
lock tables t1 read;
select * from t1 lock in share mode;
connection con2;
set autocommit=0;
send insert into t1 values(1,1);
connection con1;
sleep 1;
show processlist;
select * from t1 lock in share mode;
commit;
connection con2;
commit;

And something similar for select for update.

Heikki, I don't see a bug here. LOCK TABLES prevents other threads from modifying the
tbale in question, so there could be no"stale" data.

Could you elaboraye in what scenario it could be a bug ?

Sergei, all,

the bug is that SELECT FOR UPDATE should lock the selected rows with X-locks. That is
very different from doing a consistent read which does not lock anything.

I recall Anthony's patch was correct. Has it been pushed now?

I think an automatic test case is secondary. You can test manually.

Regards,

Heikki

Sergei,
you asked for a scenario. Here is one:
User 2 inserts row but does not commit
User 1 does LOCK TABLE, can do a consistent read (SELECT) but not a locking read (SELECT
... LOCK IN SHARE MODE or SELECT ... FOR UPDATE).

This example does not work:

mysql1> insert t1 values (5);
Query OK, 1 row affected (0.00 sec)

mysql2> lock table t1 write;
--- waits

mysql1> commit;
Query OK, 0 rows affected (0.00 sec)

--- mysql2 now
Query OK, 0 rows affected (7.11 sec)
mysql2> 

So, after LOCK TABLE you cannot see stale data

Sergei,

sorry, I forgot that nowadays LOCK TABLES by default also takes an InnoDB table lock.
Then the consistent read cannot see stale data in this case.

But if you set

innodb_table_locks=0

in my.cnf, that is not the case.

Also, someone should test if the bug is present in stored procedures, like I said in the
original bug report. That is where Vadim originally found this bug.

Regards,

Heikki

Sergei,

sorry, I forgot that nowadays LOCK TABLES by default also takes an InnoDB table lock.
Then the consistent read cannot see stale data in this case.

But if you set

innodb_table_locks=0

in my.cnf, that is not the case.

Also, someone should test if the bug is present in stored procedures, like I said in the
original bug report. That is where Vadim originally found this bug.

Regards,

Heikki

Fixed in 5.0.15

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/30584

fixed in 5.0.14 too :)

Noted in 5.0.14 changelog.