This is a follow up from #669794: Use savepoints for nested transactions

Most linux distros except ubuntu 9.10 ships with a old build-in version of sqlite 3.5.XX

As Crell proposed at http://drupal.org/node/669794#comment-2766184
sqlite needs own implementation of savepoints if version lower 3.6.8

CommentFileSizeAuthor
#10 754192-sqlite-savepoint.patch5.79 KBdamien tournoud
#4 754192-sqlite-savepoint-d7.patch5.66 KBandypost
#3 754192-sqlite-savepoint-d7.patch5.5 KBandypost

Comments

mfer’s picture

mfer commented

Subscribe.

This issue blocks sqlite testing. Once testing in sqlite comes back up there will be MANY follow-up issues. Drupal 7 fails A LOT of tests in sqlite at the moment.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@mfer the first thing is really needed to detect actual sqlite version, I found no better approach then parse phpinfo()

EDIT: SELECT sqlite_version(); works

Testing server could use ubuntu for #543582: Meta issue: fix the remaining SQLite bugs
Also there's http://qa.drupal.org/incombatibility

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 754192-sqlite-savepoint-d7.patch5.5 KB

I got it working on debian lenny (pdo_sqlite 3.3.7)

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 754192-sqlite-savepoint-d7.patch5.66 KB

A bit commented and $this->willRollback should mark whole stack as needed total rollback

dries’s picture

dries commented
Status: Needs review » Reviewed & tested by the community

I reviewed the code and this looks good. I haven't tested the code though, but I'm going to mark this RTBC. I'll commit this unless someone else raises some issues with it.

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Needs review

Please give me a few hours to properly review and test this.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Transaction tests are broken, suppose we need a conditional tests for old and new libs

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?id=6&op=do StatusText: Service unavailable (with message) ResponseText: DatabaseSchemaObjectExistsException: Table <em class="placeholder">variable</em> already exists. in DatabaseSchema->createTable() (line 558 of /var/www/d7/includes/database/schema.inc).

EDIT: also "invalid data" test broken but this another issue

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

The transaction test is probably failing because we fail to rollback a transaction at the correct point. SQLite supports transactional DDL statements, so the following will result in the table not being removed:

$tx = db_transaction();
db_drop_table('variable');
$tx->rollback();
andypost’s picture

andypost
he/him
Primary language Russian
commented

@Damien transactional DDL means that DDL statements works in transaction (does not roll back transaction)?

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 754192-sqlite-savepoint.patch5.79 KB

Hm. Actually the issue we have here is that the willRollback flag is never cleared. This works pretty well for me.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

This passes a transaction test!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

mfer’s picture

mfer commented
Status: Fixed » Reviewed & tested by the community

Looking at http://drupalcode.org/viewvc/drupal/drupal/includes/database/sqlite/data... I don't see this committed, yet.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Oops. Committed now! Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.