Inconsistant Insert Queries Between Database Drivers

Wow I totally missed when Postgres started using multi-insert statements. Cool. :-)

The behavior of the default driver would be that if you try to insert multiple records in one shot, and one of them fails, they all roll back (assuming transactions are supported). That's what I presume would happen in SQLite.

Conceptually, we do treat multi-insert query objects as a single operation. They should therefore be atomic if possible.

Therefore, IMO we should alter the MySQL and Postgres drivers both to wrap a transaction around the insert query call itself so that the InsertQuery object executes atomically. (And if transactions aren't supported, oh well, nothing we can do at that point.) I'm not sure if it makes sense to do so for SQLite as well. It probably does, but looking at the driver just now, er, I don't see support for multi-value inserts at all.

Josh, can you roll a quick patch for both drivers accordingly?

can we set both drivers to reflect what the generic insert object is doing? multi-querys in transactions rather than a single multi-valued insert?

Er. That's what I just said to do. :-) The expected behavior of a multi-insert operation should be "all or nothing". Transactions inside the query builder are the easiest way to do that. That will not work properly on MySQL without transaction support, however. For that, the answer is "well, sucks to be using MyISAM" because we really can't do anything else with it and it's an error condition to begin with that developers should be avoiding if they're coding properly. (We should probably therefore fork the mulit-insert test depending on transaction status the way we're doing in the transaction overhaul patch, so that we can still get tests to pass properly.)

If it's possible to make MyISAM tables behave the way everything else does (multi-insert is atomic), we should. If it's impossible, then we need to document that caveat somewhere. Actually I think for SQLite it should work properly, as SQLite supports transactions so we can just wrap whatever the actual queries are in a transaction and be done with it.

I defer to David on whether or not it's possible with MyISAM.

@Crell: SQLite behavior on duplicate key errors is fully configurable. By default, it doesn't rollback the transaction in that case, but just abort that query. We can easily change this (I consider this as a bug in the driver).

OK, let's go ahead and tweak the SQLite driver to roll back in that case, for consistency.

@crell in #8:

See http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html#sqlmode_stri... , specifically the distinction between STRICT_ALL_TABLES and STRICT_TRANS_TABLES for non-transactional tables like MyISAM.

I should add that the move to use the mode TRADITIONAL implicitly sets STRICT_ALL_TABLES. See http://drupal.org/node/344575 for the patch related to MySQL's mode.

fiasco also asked for the "real" answer, which is "We cannot guarantee multi-insert atomicity for MyISAM."

I haven't actually run the tests, but assuming they pass, this looks good. The updated test basically ensures MySQL is behaving as good or as bad as we expect given Drupal's configuration. That's the most we can ask of these sorts of tests without resorting to introspection.

uh huh. Comments please? I look at the code and go wtf.

This patch presumes that the multi insert behaviour depends on transaction support and this is not true, it depends on configuration -- and configuration that can be table level ie innodb vs myisam is table level. I vote on removing this test there is no way on earth to figure out what should happen in this case.

OK, let me see if I understand all of the various permutations here. When a multi-insert statement dies on a non-first record:

* MySQL-MyISAM: Commit records up to that point. Lack of transactions means we can't roll back.
* MySQL-InnoDB: Commit nothing.
* PostgreSQL: Commit nothing.
* SQLite: Does not apply as native multi-inserts are not supported; we can emulate them using a transaction and roll back the entire set.

So the only case where behavior is different is MySQL-MyISAM. We cannot check for that directly at present; we can check for transaction support, which usually will parallel InnoDB usage but that's not guaranteed.

So then the alternatives are:

1) Remove the test. I heartily dislike this option as it means we have no check to make sure we don't break multi-insert support entirely. It's unlikely that we'd do so at this point, but still, it reduces code coverage.

2) Assume that MySQL-transactions means InnoDB and MySQL-non-transactions means MyISAM. That's what the patch here is doing, although I agree with chx that more documentation is needed if we go that route.

3) Make it possible to detect the table type and adjust the test accordingly, like we do for transactional and non-transactional branches of tests. This would be ideal, but I can't think of a way to do so without rather silly hard-coding of special cases. Perhaps this is something that could go into the DatabaseSchema_mysql class as an extra method, and then we check for $connection->type() and $connection->schema()->tableType('example')?

Thoughts?

@chx We're doing the same sorts of tests in the transaction layer itself. You could install to MyISAM tables, turn on transaction support, and get failures in the transaction tests. So, we're already assuming elsewhere that, if you enable transaction support, 100% of your tables support transactions. The only alternative is an absurd level of introspection to check that all involved tables are transactional in any given operation. That alternative isn't even that useful because you could be 50% through a transaction and touch a MyISAM table, losing the coherency of your transaction, and we can't predict what tables you might possibly touch. We could try to counter that problem by checking all tables in the database periodically to see that they're transactional, but that also seems excessive.

Lesson: If you want to mix MyISAM and InnoDB tables *and* enable transaction support, you'd better know what you're doing. Fortunately, it's already impossible to use the graphical installer to arrive at such a configuration.

Oh, and I left the patch CNW for documentation reasons.

I think multi insert support can (and is) covered by other tests and this one needs to be removed as its unpredictable what happens on a multi insert, really. http://www.sqlite.org/lang_insert.html for example, it depends completely on the driver and someone can have a different sqlite driver than what we have in core. Also, i think the agreement on innodb was that we still need some tables on myisam...

@Crell Your breakdown of scenarios is correct, though I hesitate to use the word "commit" with MyISAM because it implies transactional-ish operation. Let's go with the rows are "saved" or something similar for clarity when this gets documented.

@chx I should have said "mix the table engines in a way other than we might do by default." Whatever tables we'd keep on MyISAM would be intelligently determined to avoid issues with transaction support.

Here is an implementation of Crell (2) in #18: if the database connection doesn't support transactions, we pass even if the first entry was inserted. Even if the database connection doesn't support transactions, we still pass in the case the db engine exhibit the correct behavior.

looks good but + $this->assertTrue(FALSE, this should be $this->fail

Committed to HEAD. :) Thanks!