Default MySQL to transactions ON

+1 to transactions by default

If we want to do that, what is the point of keeping the "transaction support" stuff? We are just introducing a "weak transaction" concept, which is a transaction without guarantee. That sounds a little bit dangerous for me.

I would even go one step further: because transactions only really matter for DML, why not detecting the engine type of the tables part of a transaction, and throw a fat exception if a non-transactional engine modifies a table as part of a transaction?

#4 additional overhead for checking and mostly shared hostings does not support innodb.
For example I could clear some cache table which is myisam inside of transaction and it's no matter that other tables innodb and this transaction could be rolled back

This issue seems like code-split for apc and shared hosts. Anyway if user have no innodb tables or just upgrading from d6 he should have ability to work without exceptions and warnings.

@andypost: I'm very aware of the limitations. But the point of starting a transaction is to have some guarantee (of consistency, notably). We are breaking that pattern with the "weak transactions". We need a way for the calling code to say "I need a transaction, really, I mean it."

Agree exactly - mostly all save operations on fieldable entity should "really need a transaction". But I'm scared that it's not possible with current state of Db and alterable workflow

I'd support this patch -- looks like a good thing, but obviously needs a reroll to make it work.

I switched client #33 to InnoDB because it has 10GB of available RAM and 16 cores to try this. my.cnf got a generous allocation to whatever RAM-based settings might want them, based off some recommendations from Narayan a couple months ago.

The test hovered around 3.1GB of consumed RAM pretty much the entire time and the test overall took about 7 minutes. On MyISAM, it took something like 6.5 minutes. There is a lot of time variation as people mod tests and so forth, but 7 minutes is definitely in the 15 minute window that we prefer a client to take doing a round of tests.

The problem with this is that a 4GB memory requirement is going to make it even harder to find PIFR clients to test patches. Classic Graphics and Damien are the only ones that have that much hardware offered up.

#34, #36, and #50 would have to stay MyISAM or do something else. There are servers queued up from Cirtex and Hangar hosting, but they don't meet this kind of spec either. They didn't even really match the MyISAM requirements. Classic Graphics has another server racked with the same specs, but nobody available to put an OS on it right now.

Bottom line: InnoDB passes. #33 is running it now (until someone asks me to switch it back). #32 probably could, too. Going InnoDB all the way would bring us down to 2 clients.

This might be a silly question, but could we not wrap these assertions in a check for transaction support and storage engine?

Wrapper or not, the bots should still be able to test transactions, which wouldn't be possible with a MyISAM config on every bot. I know I took a downer kind of mood in #14, but the truth is that #32 and #33 together can handle the current load of core patches with probably more downtime than CPU time. I'm mentioning finding more clients because of the talk of opening PIFR up to contrib. I dropped email in the Classic Graphics ticket queue to see if they've got some time to setup a Debian instance on their other free server, which could probably handle contrib's extra load (just guessing).

I'm going to temporarily disable PIFR clients #32 and #34 so I can force a retest of patch #12 on InnoDB (client #33).

This is the right thing to do. We can't let some arcane testing.drupal.org issue get in the way of shipping the best drupal we can ship.

moshe: No one's saying anything about not shipping Drupal with this patch. But the testing issues do definitely need to be sorted before this can be committed.

So why is it green now? It just got lucky and got deekayen's server?

Yeah, I shut off all the other clients so this patch would hit #33, which is running InnoDB now, then I turned #32 and #34 back on.

@Moshe: it's really just a matter of deciding what the bots should be configured to do at this point. We can have both MyISAM and InnoDB bots, and make only one of them required. The issue with having both now is that we don't have enough working clients to do both at the same time, so we have to pick one.

@Angie: I think you're on the right track with your question (#15). It would be nice if tests would pass on MyISAM, InnoDB, PgSQL, and sqlite. #12 would go backwards in that support list.

Here's some of the details on how it uses 3G.

tdo:/var/lib/mysql# ls -alh
total 493M
drwxrwxrwt  5 mysql mysql  220 Dec  5 16:42 .
drwxr-xr-x 27 root  root  4.0K Dec  5 00:34 ..
-rw-------  1 mysql mysql    0 Dec  5 00:44 debian-5.0.flag
drwx------  2 mysql mysql 2.9K Dec  5 00:44 drupal
drwx------  2 mysql mysql 3.7K Dec  5 16:49 drupal_checkout
-rw-------  1 mysql mysql 150M Dec  5 16:49 ib_logfile0
-rw-------  1 mysql mysql 150M Dec  5 16:48 ib_logfile1
-rw-------  1 mysql mysql 150M Dec  5 16:49 ib_logfile2
-rw-------  1 mysql mysql  42M Dec  5 16:49 ibdata1
drwx------  2 mysql mysql 1.1K Dec  5 00:44 mysql
-rw-------  1 mysql mysql    7 Dec  5 00:44 mysql_upgrade_info

That's 500M when idle and it's all in RAM. When it ramps up and makes a lot of simpletest tables, it turns into 900M. Then the MySQL process itself sucks up another 900M or so in part because of the thread concurrency option below.

Additional config for InnoDB in /etc/mysql/my.cnf

key_buffer = 128M
innodb_log_file_size = 64M
innodb_log_group_home_dir = /var/lib/mysql
innodb_data_home_dir = /var/lib/mysql
innodb_data_file_path = ibdata1:10M:autoextend
innodb_log_files_in_group = 2
innodb_flush_method = O_DIRECT
innodb_log_buffer_size = 10M
innodb_thread_concurrency = 8
# If you have more than 512MB ram, you can increase the innodb_buffer_pool_size
innodb_buffer_pool_size = 512M
innodb_additional_mem_pool_size = 10M
innodb_file_per_table = 1
# If you have raid5 or raid10 with more disks, you can increase innodb_file_io_threads
innodb_file_io_threads = 1
innodb_flush_log_at_trx_commit=0
innodb_max_dirty_pages_pct = 70

On top of all that, there's a 256M per PHP process limit in Apache, which it uses about 100M times concurrency 8. Add a couple other misc processes in there and variation in memory used per test, and it comes out around 3.1G. MyISAM only requires 700M or so for concurrency 4.

Regardless of where this testing infrastructure discussion ends up, I still think it's valuable to have tests that pass under both MyISAM/InnoDB, transactions on/off, etc. I would support adding some simple helper functions (I guess to DrupalTestCase? or database.inc?) for test authors that encapsulate the logic required to figure this out.

I switched both bots #32 and #33 to run InnoDB, which means I turned off #34 to make sure the tests would return consistent results. Spread the word as necessary.

Quick retest to see if it works on current head

Any ideas how it should work for migration from drupal6 ?

Lot of things changed lately, needs a reroll

Yeah, it stopped doing that when PIFR 2 was deployed. :( See #635334: Patches are not being re-tested automatically. It's fixed upstream, awaiting deployment.

Had a long discussion about this patch with Crell tonight on IRC. One thing to bear in mind is that although we are encouraging the use of InnoDB in D7, and it gets selected by default if it's available during installation, the vast majority of Drupal's userbase is still on MyISAM, and that won't change anytime soon. This means that changing this default isn't going to make sense for at least 90% of our users for the next few years, unless they manually toggle the transaction support flag in their settings.php's $databases array.

Writing the $databases array dynamically during installation, to properly toggle the flag based on the engine type isn't really an option, either, because settings.php is written before the database is connected to, and then consulted on the following request to create the system table, and so on. And PDO, which we can consult before we have DB access, knows about database types (MySQL, PostgreSQL, SQLite), but not about engine types on a particular database. So we'd have to do some kind of trickery like writing settings.php, reading it, attempting to do a transaction, toggling the transaction flag accordingly, and then writing settings.php back *out* again. (This actually still might be worth looking into, though, since IMO it's a bug for core to provide incorrect settings by default.) I suggested maybe using a variable to store the transaction state, but that spreads our DB connection info into multiple sources, and also makes DBTNG reliant on the variable system, which Crell wants to avoid doing.

However, I agree that having innoDB without transaction support doesn't make a lot of sense. And the good news is that even when the default transaction use flag and the transaction support of the database don't match, there shouldn't be a visible effect. The only exception is for specialized scripts that require the use of transactions and rollbacks to function, as opposed to using them for safety nets, but those types of things are typically very custom code where you know the environment you're dealing with.

So anyway, I think we can go ahead with this patch. Let's get 'er done by Friday, though.

wrong directory...
I'll try your patch again...

thanks aspilicious

Thanks! Committed to HEAD.