Lazy and Conditional Issue of BEGIN/COMMIT/ROLLBACK about cakephp

1st Apr 2010, Mark Story said:

---

I have no idea what this ticket is even about. Some more information could help.

1st Apr 2010, Stephen Cuppett said:

---

Sorry. This is not a "this isn't working right" bug. I just noticed that every Model->begin and Model->commit is honored and issued without condition, whether other queries need to be issued against the database during the transaction or not.

This can be optimized to avoid creating transactional overhead on the database, slowing PHP down by sending TCP/IP traffic back and forth to issue the commands, etc. until the first query in between the begin/commit is issued.

Does this make sense?

1st Apr 2010, ADmad said:

---

So how do you propose we see into the future and decide whether a transaction BEGIN is going to be followed by other sql queries or not ?

1st Apr 2010, Stephen Cuppett said:

---

It's a pretty simple state machine for each potential datasource. It's not seeing into the future so much as holding back the query in the framework until it's necessary. (Not sure of the additional or current variables, but here is a simplified example):

1. When ->begin() is called, at some level, a $_beginPending is set somewhere in the framework for the datasource.
   2a) If ->commit() is called now, it's never issued, $_beginPending is cleared.
   2b) If SELECT/INSERT/UPDATE/DELETE or generic ->query() is issued against the datasource, BEGIN is issued as well as the first desired query. $_beginPending is cleared. (must include SELECT, rather than defer BEGIN later, in this set because of some datasource's handling of transaction isolation wrt reads)
2. When ->commit() is called, it can be issued.

This is a little fancier in other ORM mappers (not issuing the COMMIT or ROLLBACK unless something other than SELECT is used and/or another transaction is needed to start), but the above would work great to avoid unnecessary BEGIN/COMMITs like the above query log in the cases like:

function someFunction()
{
if ($someValueIneedToPersist) $this->MyModel->save($this->data);
}

function someOtherFunction()
{
for() {
$this->MyModel->begin();
// other stuff
$this->someFunction();
//other stuff
if ($error) break;
$this->MyModel->commit();
}
$this->MyModel->rollback();
}

1st Apr 2010, ADmad said:

---

Patches are always welcome :)

1st Apr 2010, Stephen Cuppett said:

---

I'll give it a shot, thanks! Just wanted to throw it out there when I saw that stream in my SQL log and had it fresh in my head.

1st Apr 2010, Stephen Cuppett said:

---

Sorry this took me so long, have had some interruptions this afternoon. I have a patch. This is against 1.2.6. It cuts down on the BEGIN/COMMIT thrashing tremendously (such that it only encloses query blocks versus always being issued). It uses the simplified state model I proposed above.

I also found a problem. I added my code and realized none of it was being called. In Model, call__ was intercepting MyModel->begin/commit/rollback such that $db->query was getting begin/commit/rollback calls and then DboSource->fetchAll was getting called. The explicit DboSource transaction methods weren't even being used! (PHP 5.3.1, Postgres)

This patch solves that as well.

1st Apr 2010, Stephen Cuppett said:

---

Sorry this took me so long, have had some interruptions this afternoon. I have a patch. This is against 1.2.6. It cuts down on the BEGIN/COMMIT thrashing tremendously (such that it only encloses query blocks versus always being issued). It uses the simplified state model I proposed above.

I also found another problem. I added my code and realized none of it was being called. In Model, call__() was intercepting MyModel->begin/commit/rollback such that $db->query was getting begin/commit/rollback calls and then DboSource->fetchAll was getting called. The explicit DboSource transaction methods weren't even being used! (PHP 5.3.1, Postgres)

This patch solves that as well.

1st Apr 2010, ADmad said:

---

Any such enhancement to 1.2 branch is unlikely. Can you please make your path against 1.3 (preferably including test cases). You can fork the cakephp1x repo on github and make change there.

1st Apr 2010, Stephen Cuppett said:

---

No problem. 1.2 was the easiest thing I had access to during the day. Will submit a 1.3 one later this evening. Thanks!

In terms of tests, does the test harness have a way to check what queries were run when a particular method is run?

So can I do something like:

MyModel->commit();

MyModel->queryCount() == 1
MyModel->queryCount() == 0

or similar in the test? The biggest thing is making sure something is issued under conditions x,y,z and that nothing is issued under a,b,c...

2nd Apr 2010, Stephen Cuppett said:

---

Here is a patch against 1.3. Also includes tests. Thanks again!

10th Apr 2010, Stephen Cuppett said:

---

This has been pushed to my 1.3 branch:

http://github.com/cuppett/cakephp1x/commits/1.3
24a1b68d05a80b6e33688be9b99fa358f8d6f24f

[lorenzo]

I don't think the benefits of having things overweigh the extra time and effort in maintaining the code, I will keep it in mind for 3.0, though