fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Unable to lock tables in transaction #1679

Closed mikepmtl closed 9 years ago

mikepmtl commented 10 years ago

Is this not redundant?

public function start_transaction() { $this->query(0, 'SET AUTOCOMMIT=0', false); $this->query(0, 'START TRANSACTION', false); $this->_in_transaction = true; return true; }

Doesn't SET AUTOCOMMMIT=0 Start the transaction?

It becomes an issue if you want to LOCK the table within a transaction. You can't LOCK a table from within a Transaction if you use START TRANSACTION.

From MySQL (5.0) docs. https://dev.mysql.com/doc/refman/5.0/en/lock-tables-and-transactions.html

The correct way to use LOCK TABLES and UNLOCK TABLES with transactional tables, such as InnoDB tables, is to begin a transaction with SET autocommit = 0 (not START TRANSACTION) followed by LOCK TABLES, and to not call UNLOCK TABLES until you commit the transaction explicitly. For example, if you need to write to table t1 and read from table t2, you can do this:

SET autocommit=0; LOCK TABLES t1 WRITE, t2 READ, ...; ... do something with tables t1 and t2 here ... COMMIT; UNLOCK TABLES;

When you call LOCK TABLES, InnoDB internally takes its own table lock, and MySQL takes its own table lock. InnoDB releases its internal table lock at the next commit, but for MySQL to release its table lock, you have to call UNLOCK TABLES. You should not have autocommit = 1, because then InnoDB releases its internal table lock immediately after the call of LOCK TABLES, and deadlocks can very easily happen. InnoDB does not acquire the internal table lock at all if autocommit = 1, to help old applications avoid unnecessary deadlocks.

WanWizard commented 10 years ago

If I understand https://dev.mysql.com/doc/refman/5.0/en/commit.html correctly, running START TRANSACTION automatically disables autocommit. and a COMMIT or ROLLBACK will return the previous setting.

So I would say it's not only redundant, the current code also destroys any custom value you have set, because it's explicitly set to 0 and 1. So I'd say it needs to be removed.

@FrenkyNet any comments? x-check needed with fuelphp\database ?

WanWizard commented 10 years ago

@mikepmtl does this commit address the issue?

WanWizard commented 10 years ago

@mikepmtl ping?

WanWizard commented 9 years ago

Closed due to no response.