Problem/Motivation

Running the HHVM unit tests for Drupal 7, I found that the Transaction tests were passing, however with exceptions. Determining the exact tests failing pointed out that it was a group of tests involving DDL statements with transactions, while using a MySQL server.

The crux of the issue was that the Drupal tests were testing for savepoints in MySQL, after running a DDL statement. The MySQL documentation explicitly states that the active transaction is immediately committed when a DDL statement is run, hence the savepoints would no longer exist (raising the said errors.)

Proposed resolution

Later on in the test there is a branch that handles the situation where a database server does not support transactions. I propose wrapping all of the DDL+transaction tests in the block, instead of just testing ROLLBACK.

This also seems strange to me because why would we be testing for DDL+transactions if the server does not support it?

Remaining tasks

There might be some implications in the value supportsTransactionalDDL() returns. According to this test, it might mean that the database supports rolling back after DDL changes.

CommentFileSizeAuthor
#1 drupal-check-database-server-supports-transactional-ddl-2231937-1-D8.patch3.49 KBlowjoel
#1 drupal-check-database-server-supports-transactional-ddl-2231937-1-D7.patch3.4 KBlowjoel

Comments

lowjoel’s picture

lowjoel commented
StatusFileSize
new drupal-check-database-server-supports-transactional-ddl-2231937-1-D7.patch3.4 KB
new drupal-check-database-server-supports-transactional-ddl-2231937-1-D8.patch3.49 KB

The last submitted patch, 1: drupal-check-database-server-supports-transactional-ddl-2231937-1-D7.patch, failed testing.

lowjoel’s picture

lowjoel commented

Uhh, do I need a separate ticket for the 7.x patch? The Test bot seems to be trying to apply the 7.x patch to 8.x

danblack’s picture

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

patch looks good to me.

> do I need a separate ticket for the 7.x patch

No, after the 8.x is committed the target version for this issue can be changed (as I understand it)

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

We will need more information here.

Running the HHVM unit tests for Drupal 7, I found that the Transaction tests were passing, however with exceptions. Determining the exact tests failing pointed out that it was a group of tests involving DDL statements with transactions, while using a MySQL server.

Exceptions? Which ones? During or outside of the tests?

Does HHVM actually support synchronous destructors? This test passes on PHP, so we are looking at a runtime difference here that needs investigation.

The test itself is legit, it's just confirming that the transaction is silently committed. We catch the RELEASE SAVEPOINT exception in Connection::popCommittableTransactions().

lowjoel’s picture

lowjoel commented
Status: Postponed (maintainer needs more info) » Needs review

Yes, HHVM supports synchronous destructors. PHP passes because they aren't raising any exception.

Destructor threw an object exception: exception 'PDOException' with message
    'SQLSTATE[42000] [1305] SAVEPOINT savepoint_1 does not exist' in :
    Stack trace:
    #0 /home/joel/Development/drupal/includes/database/database.inc(2175):
    PDOStatement->execute()
    #1 /home/joel/Development/drupal/includes/database/database.inc(684):
    DatabaseStatementBase->execute()
    #2 /home/joel/Development/drupal/includes/database/mysql/database.inc(173):
    DatabaseConnection->query()
    #3 /home/joel/Development/drupal/includes/database/database.inc(1135):
    DatabaseConnection_mysql->popCommittableTransactions()
    #4 /home/joel/Development/drupal/includes/database/database.inc(1912):
    DatabaseConnection->popTransaction()
    #5
    /home/joel/Development/drupal/modules/simpletest/tests/database_test.test(3570):
    DatabaseTransaction->__destruct()
    #6
    /home/joel/Development/drupal/modules/simpletest/drupal_web_test_case.php(504):
    DatabaseTransactionTestCase->testTransactionWithDdlStatement()
    #7 /home/joel/Development/drupal/scripts/run-tests.sh(369):
    DrupalTestCase->run()
    #8 /home/joel/Development/drupal/scripts/run-tests.sh(22):
    simpletest_script_run_one_test()
    #9 {main}

I don't see popCommittableTransactions catching anything on D7. So this only affects D7, then...

danblack’s picture

danblack commented
Version: 8.x-dev » 7.x-dev
damien tournoud’s picture

damien tournoud commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Postponed (maintainer needs more info)
Issue tags: -Stalking Crell

Drupal 7 has some similar code in DatabaseConnection_mysql::popCommittableTransactions().

This looks like a HHVM compatibility issue. Most likely HHVM doesn't expose MySQL error code 1305 the same way that the real PDO does.

lowjoel’s picture

lowjoel commented

Yup, I didn't see that bit of code. I'm switching my focus back to HHVM, I'll keep this thread posted.

lowjoel’s picture

lowjoel commented

I've traced it down, #2392 for HHVM. Thanks for the help.

lowjoel’s picture

lowjoel commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)