MySQL Bugs: #21413: Engine table hander used by multiple threads in REPLACE DELAYED

Bug #21413	Engine table hander used by multiple threads in REPLACE DELAYED
Submitted:	2 Aug 2006 13:44	Modified:	31 Mar 2008 18:36
Reporter:	Paul McCullagh (Basic Quality Contributor) (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: MyISAM storage engine	Severity:	S3 (Non-critical)
Version:	4.1	OS:	Any
Assigned to:	Mattias Jonsson	CPU Architecture:	Any
Tags:	DELAYED, engine, handler, qc, REPLACE

Description:
The implementation of the engine open table handlers assume that the handler is only accessed by one thread at any one time. As a result the programmer assumes that it is not necessary to protect any of the handler instance variables.

I have found an exception to this rule in the REPLACE DELAYED statement. The handler function extra(HA_EXTRA_NO_IGNORE_DUP_KEY) is called by 2 independently running threads, possibly simultaniously, as you can see from the call trace below.

This means that if you have an instance variable (e.g. i_ignore_dup_key), then the variable should be incremented on HA_EXTRA_IGNORE_DUP_KEY and decremented on HA_EXTRA_NO_IGNORE_DUP_KEY.

In addition, the variable should be protected.

How to repeat:
Execute the following statements:

drop table if exists t1;

CREATE TABLE t1 (x int not null, y int, primary key (x)) engine=pbxt;

insert into t1 values (1, 3), (4, 1);

replace DELAYED into t1 (x, y) VALUES (4, 2);

select * from t1 order by x;

Handler calls can be traced as follows:

ha_pbxt::external_lock() thd=0x83fe330 tab=0x83ff798 lock=1
ha_pbxt::write_row() thd=0x83fe330 tab=0x83ff798
ha_pbxt::write_row() thd=0x83fe330 tab=0x83ff798
ha_pbxt::external_lock() thd=0x83fe330 tab=0x83ff798 lock=2

ha_pbxt::external_lock() thd=0x846b108 tab=0x83ff798 lock=1
ha_pbxt::extra(HA_EXTRA_IGNORE_DUP_KEY) thd=0x83fe330 tab=0x83ff798
ha_pbxt::extra(HA_EXTRA_IGNORE_DUP_KEY) thd=0x846b108 tab=0x83ff798
ha_pbxt::write_row() thd=0x846b108 tab=0x83ff798
ha_pbxt::extra(HA_EXTRA_NO_IGNORE_DUP_KEY) thd=0x83fe330 tab=0x83ff798
ha_pbxt::update_row thd=0x846b108 tab=0x83ff798
ha_pbxt::extra(HA_EXTRA_NO_IGNORE_DUP_KEY) thd=0x846b108 tab=0x83ff798
ha_pbxt::external_lock() thd=0x846b108 tab=0x83ff798 lock=2

ha_pbxt::external_lock() thd=0x83fe330 tab=0x8471328 lock=0
ha_pbxt::external_lock() thd=0x83fe330 tab=0x8471328 lock=2

Suggested fix:
It is possible to avoid one of the pair of calls to extra(HA_EXTRA_IGNORE_DUP_KEY/HA_EXTRA_NO_IGNORE_DUP_KEY) in sequence above. In particular, the call done by thd=0x83fe330 is not necessary, and should be removed in the case of a DELAYED call.

Alternatively, this exception should be clearly documented.

Handlers are not singletons - every thread that has opened a table has separate handler instances. Every handler, when created, may associate to a 'share' to provide proper per-table or lower-level locking. It is up to every function inside handler to lock all shared structures or operations. 

I suppose addresses of ha_pbxt instances are different - please tell if it is same structure being accessed. 

DELAYED operations though have separate threads that may open handler instances.

I've added DBUG_PRINT lines to the myisam handlers extra() and external_lock() handlers and got the following log for the statements above:

T@11306751: | | | | | | | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | | | | | foo: external_lock: thd is 0x8f476d8, this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | | | foo: external_lock: thd is 0x8f476d8, this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11308758: | | | | foo: external_lock: thd is 0x8f632d8, this is 0x8f492e0, pthread is 1130875824
T@11306751: | | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11306751: | | | | foo: extra: this is 0x8f492e0, pthread is 1130675120
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | foo: extra: this is 0x8f492e0, pthread is 1130875824
T@11308758: | | | foo: external_lock: thd is 0x8f632d8, this is 0x8f492e0, pthread is 1130875824

so the same handler instance is indeed used by two different pthreads here,
but by the same THD instace which probably handles the locking ...

could anyone who's really into this code elaborate on this?

In mysql_insert() there is

   table= delayed_get_table(thd,table_list)

which returns a TABLE object used by a delayed thread.
Later fields of this objects are modified, and table->file handler is used, apparently without a protection against a concurrent access.

I think I see the source of the confusion...

I believe in this instance, it is 'ok'. The errant calls are caused by mysql_prepare_insert() while the 'delayed' thread has been locked by the callee.

current_thd should not be used as a way to inquire the calling context and that table->in_use should be used and typically table->in_use == current_thd.

Nevertheless, I shall commit a cset which fixes the problem.

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/commits/41460

ChangeSet@1.2705, 2008-01-30 13:04:04-08:00, acurtis@xiphis.org +1 -0
  Bug#21413
    "Engine table handler used by multiple threads in REPLACE DELAYED"
    add code to avoid surplus calls to handler's extra() method in case of DELAYED.
    Avoid calling storage engine from a different thread than expected.

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/commits/41587

ChangeSet@1.2705, 2008-02-01 14:45:33-08:00, acurtis@xiphis.org +1 -0
  Bug#21413
    "Engine table handler used by multiple threads in REPLACE DELAYED"
    add code to avoid surplus calls to handler's extra() method in case of DELAYED.
    Doing so avoids calling storage engine from a different thread than expected.

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/commits/41691

ChangeSet@1.2705, 2008-02-05 01:03:50-08:00, antony@pcg5ppc.xiphis.org +1 -0
  Bug#21413
    "Engine table handler used by multiple threads in REPLACE DELAYED"
    When executing a REPLACE DELAYED statement, the storage engine
    ::extra() method was invoked by a different thread than the thread
    which has acquired the handler instance.
  
    Added code to avoid surplus calls to extra() method in case of DELAYED
    which avoids calling storage engine from a different thread than 
    expected.
  
    No test case.

More verbose cset comment as requested by reviewers.

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/commits/44582

ChangeSet@1.2711, 2008-03-28 13:44:59+01:00, istruewing@stella.local +2 -0
  Bug#21413 - Engine table handler used by multiple threads
              in REPLACE DELAYED
  Reverted one change from the bug fix.
  Doing a full TABLE_LIST copy again.

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/commits/44588

ChangeSet@1.2707, 2008-03-28 14:27:22+01:00, mattiasj@witty. +1 -0
  Bug#21413 - Engine table handler used by multiple threads
              in REPLACE DELAYED
      
  post push patch, removing the optimization for
  copying delayed_insert variables.

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/commits/44589

ChangeSet@1.2706, 2008-03-27 01:13:39+01:00, mattiasj@witty. +1 -0
  Recommit of antonys previous commit.
  
  Bug#21413
  "Engine table handler used by multiple threads in REPLACE DELAYED"
  When executing a REPLACE DELAYED statement, the storage engine
  ::extra() method was invoked by a different thread than the thread
  which has acquired the handler instance.
  
  This did not cause problems within the current server and with
  the current storage engines.
  But it has the potential to confuse future storage engines.
  
  Added code to avoid surplus calls to extra() method in case of DELAYED
  which avoids calling storage engine from a different thread than
  expected.
  
  No test case.
  This change does not change behavior in conjunction with current
  storage engines. So it cannot be tested by the regression test suite.

Sorry about the late commit mails, I have recently reinstalled my computer and missed the postfix configuration...

Pushed into 5.0.60

Pushed into 5.1.24-rc

No user-visible changes. No changelog entry needed.

Pushed into 6.0.5-alpha