http://dev.mysql.com/doc/refman/5.1/en/innodb-parameters.html#sysvar_inn...

I ran some benchmarks deleting and creating 50 nodes for different values of innodb_flush_log_at_trx_commit.

0 - 2.87s
1 - 20.72s
2 - 3.31s

So - drupal is 10 times slower with innodb_flush_log_at_trx_commit == 1. This is also the default setting!

This patch reports this condition in the status report.

Files: 
CommentFileSizeAuthor
#56 innodb.patch1.31 KBmoshe weitzman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,838 pass(es). View
#47 innodb_flush_log_at_trx_commit_warning-817398-47.patch1.32 KBmoshe weitzman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,656 pass(es). View
#45 innodb_flush_log_at_trx_commit_warning-817398-45.patch1.21 KBMatt V.
#2 innodb.patch1.38 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 20,593 pass(es). View
innodb.patch1.36 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

Status: Needs review » Needs work

The last submitted patch, innodb.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 20,593 pass(es). View

Status: Needs review » Needs work

The last submitted patch, innodb.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review

works for me...

jbrown’s picture

#2: innodb.patch queued for re-testing.

lotyrin’s picture

Priority: Critical » Normal
Status: Needs review » Needs work
Issue tags: +Performance, +MySQL, +innoDB

http://dev.mysql.com/doc/refman/5.0/en/innodb-parameters.html#sysvar_inn...

"If the value of innodb_flush_log_at_trx_commit is 0, the log buffer is written out to the log file once per second and the flush to disk operation is performed on the log file, but nothing is done at a transaction commit."

"The default value of 1 is the value required for ACID compliance."

Sure, creating 50 nodes was 10 times slower, but on your average live site how many times more often are you making SELECTs rather than INSERTs? The performance difference becomes minimal. Also, it's likely not really any faster, since the transaction still needs to be written out (at the end of each second), it just doesn't cause the client to wait for the write. Try creating 50000 nodes, and see what the performance difference is.

Sacrificing ACID compliance for a minimal (or even ANY) performance gain isn't something I suggest Drupal tell its users to do. If anything we should give them a yellow warning in the cases that innodb isn't configured to flush the log for each committed transaction.

Lowering priority, this doesn't break any major functionality of a site.
Marking as needs work, so we can get some more opinions in on this, otherwise it'd be won't fix/by design.

Damien Tournoud’s picture

Component: database system » documentation

Database configuration is not the responsibility of core. Tips to improve the performance of your SQL server for Drupal belong in the documentation. I believe there is already plenty of that everywhere, so I'm tempted to won't fix this.

jhodgdon’s picture

Title: innodb_flush_log_at_trx_commit == 1 devastates performance » Document that innodb_flush_log_at_trx_commit == 1 devastates performance
Project: Drupal core » Documentation
Version: 7.x-dev »
Component: documentation » New documentation
Category: bug » feature
Status: Needs work » Active

This would probably belong in a Handbook page rather than in the documentation embedded in Drupal.

I suggest if you want to write something, putting it as a sub-page of:
http://drupal.org/node/2601 (server tuning considerations)

jbrown’s picture

Title: Document that innodb_flush_log_at_trx_commit == 1 devastates performance » Alert admins when innodb_flush_log_at_trx_commit == 1 is devastating db write performance
Project: Documentation » Drupal core
Version: » 7.x-dev
Component: New documentation » database system
Priority: Normal » Critical
Status: Active » Needs review
Issue tags: +Usability

@lotyrin - you are mistaken.

Creating 50000 nodes isn't any faster. Writing / flushing once per second is considerably faster as many transactions from different processes can be written out per disk rotation - potentially achieving the disk's full write speed.

With innodb_flush_log_at_trx_commit == 1, $query->execute() does not return until the database write has been committed to disk. This takes 2 disk flushes (if using xa transactions - default). So the maximum number of database writes that can occur on a 7200 rpm disk is 60 per second. This is not very many. Many site activities cause db writes. As a site is exposed to more and more load this will start to hurt very quickly.

Installation time is also greatly affected: #656894: Installing Drupal with InnoDB takes 4 times longer than MyISAM.

ACID compliance is hardly ever suitable for a Drupal site. The cost is so high and the benefit is so minor. It might be useful for an e-commerce site.

This is a usability issue. Core devs know about this setting, but a very considerable number of people are going to have an unnecessarily sluggish experience of D7. People are unfamiliar with InnoDB. They need a lot of hand holding. This status message will raise much-needed awareness of the issue.

I think it is an extremely important that admins are alerted to this, rather than just leaving them to form an incorrect opinion about the performance of Drupal and complain loudly when their site starts to crawl under load.

One of the major (unfounded) criticisms of Drupal is that it is slow. We don't want this opinion to become further cemented.

So much effort is put into making Drupal as fast as possible. This tiny modification means that many Drupal sites will be able to scream like intended without a consultant getting involved.

It is also useful for consultants. When tasked with speeding up a D7 site, the most common thing to be done will be switching off this setting. If it is mentioned on the status page it can be determined very quickly if this has been done yet.

Potentially this sort of thing could be in a performance module in contrib, but I think this should be in core.

Marking this as critical as i don't believe Drupal 7 should be released until this is resolved.

Please discuss.

jbrown’s picture

Category: feature » bug
marcingy’s picture

Component: database system » documentation
Category: bug » task
Priority: Critical » Normal

As per comment in #7 this just needs to be documented explaining how to tune innodb.

jhodgdon’s picture

marcingy: Where do you think this should be documented? If it's in the Handbook, please set this back to the Documentation project, as I did in #8. If it's on api.drupal.org or in code embedded in Drupal, where should it go?

jbrown/marcinggy: Please resolve between the two of you what should be happening here. Changing the status/component back and forth is not really accomplishing anything. Maybe an IRC discussion would be in order?

jbrown’s picture

Component: documentation » database system
Category: task » bug
Priority: Normal » Critical

@marcingy please don't be so rude as to knock my issue out of critical without responding to my points in #9 and before a wider discussion has occurred.

If you don't want to participate then please don't change anything.

This is an issue of immense importance. There is massive potential for damage to Drupal's reputation.

Simply documenting this is totally insufficient. Admins need to be prodded so they know that what they are experiencing is not typical of D7 when configured correctly.

Usability is after all one of the major goals of D7.

marcingy’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: database system » New documentation
Category: bug » task
Priority: Critical » Normal

@jhobgdon my bad yes it is handbook documentation that is required I didn't realise that was distinct from drupal documentation componenet.

And with regrads the other part of the issue I am simply agreeing with @damz in that drupal core should not be responsible for the configuration of databases.

marcingy’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: New documentation » database system
Category: task » bug
Priority: Normal » Critical

Sorry cross post - I didn't mean to reset again I was responding to jhodgdon comments. However the point is that drupal core should not be responsible for database configuration, If it is the question then emerges should we actually test for optimal settings for every potential thing that a host may have decided to do that may have performance implications during the update install process.

jbrown’s picture

In general I agree with you. Core shouldn't attempt to analyse its db configuration for optimal settings - this would make a good contrib module.

My point is that this is an exceptional circumstance. If a malconfiguration is causing a security problem we report it. I think if an extremely common malconfiguration is responsible for significantly reducing performance then we should report it also.

Sorry for being so abrupt with you.

moshe weitzman’s picture

We take care to help users with their php config (see settings.php). We should do same here as this is an exceptional case. I think the patch is a good idea. We should advise folks on a better value or how to determine their value.

tstoeckler’s picture

I don't think documentation in settings.php vs. a runtime error message being displayed is a valid comparison.
Only people who actually care about their PHP configuration look at settings.php (and we have worked hard, and are still working hard for the installer to be so as to actually allow that) and in the same manner only people who care about their DB configuration should read this and not everyone who has the luck of being on a shared host, that doesn't care too much about such things. I don't know if settings.php or a handbook page is the correct place for this information, but hook_requirements() definitely isn't.

dhthwy’s picture

Innodb is complex and requires many things to be fine-tuned & properly managed (not just innodb_flush_log_at_trx_commit) . The fine-tuning is really dependent on specific usage. One of points of using it is to get full ACID compliance. That is why innodb_flush_log_at_trx_commit is set to 1 by default. *Anyone* that chooses Innodb and doesn't learn how to configure/manage it is just *asking* for trouble, whether it's on a Drupal site, Wordpress site, etc. This doesn't reflect poorly on Drupal.

If anything, Drupal should document the differences between Innodb and MyISAM and provide some general guidelines and recommended settings followed by links to other relevant resources with more detailed information as there are heaps of docs already available.

This isn't critical, infact, I'd say it's minor or won't fix.

Damien Tournoud’s picture

Priority: Critical » Normal

Agreed on the non-criticality. innodb_flush_log_at_trx_commit = 2 is generally a good idea, but it's not an assumption that Drupal should make.

To me it's a won't fix in favor of a documentation page about how to tune InnoDB for Drupal.

jhodgdon’s picture

How about if someone writes the supposed page as a sub-page of http://drupal.org/node/2601 (server tuning considerations)?

Then maybe we can figure out what other information people need to be alerted to, or what they should be warned about and where. It's pretty easy, for instance, to add a line to settings.php saying "If you're using innodb, you really need to think about server tuning, see this page".

nnewton’s picture

I have to agree with Damien. This setting is actually very well known among those with experience configuring InnoDB, but its definitely not in the list of settings that someone who doesn't know much about InnoDB should be setting wildly. Especially considering its a global setting and there could be other apps pointing to the DB server. There are even situations where I have explicitly set it to 1 for various reasons. Core shouldn't be in the business of giving database tuning advice.

killes@www.drop.org’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: database system » New documentation
Status: Needs review » Needs work
Issue tags: -Performance, -Usability +Needs Documentation

moving

larsdesigns’s picture

I agree with moshe #17. Drupal should make an effort to protect performance and this patch would seem to be a great idea.

XiaN Vizjereij’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: New documentation » database system
Priority: Normal » Critical
Status: Needs work » Active

Although i don't like bumping a topic where all the core guru's already said it should be up to the user to figure that out.

I'm pretty sure you will get stomped with post's about sloppy core performance when Drupal 7 hits. I googled a bit in preparation for the new drupal and stumbled upon this problem and the "fixes" suggested out there.

Common sense through these posts is, that it absolutly WRECKS the db performance if set to 1 ( about factor 10x slower is the average reported value ). And i'm 100% sure that ~99% of the drupal users out there never even heard about this setting or even know to fix it.

I think its def. qualified to submit this patch, as it adds a valuable information to the status page, that can otherwise only be found in a hour long google session .. but only if you know what to search for ofc.

Note : Run a SHOW VARIABLES LIKE 'innodb_flush_log_at_trx_commit' query to get the current value ( just a note to myself :P )

chx’s picture

Priority: Critical » Normal

But there are SO many ways to slow down your site up to killing it. Put your theme on an NFS mount, it's almost guaranteed that your site will blow up in nice ways. Set up MaxClients so that there are more Apache clients than your RAM can support and begin to swap. Heck, I am sure there are at least half a dozen InnoDB / MySQL buffer settings that will kill even your read performance. If you dont believe me, I can ask David, Khalid and Narayan to give you excellent advice to have an excruciatingly slow site. What about a contrib that deals with these and gives you ideas to not have that?

XiaN Vizjereij’s picture

Well .. how many of those settings are default to a value that slows drupal write down by 10x ?

Damien Tournoud’s picture

Status: Active » Closed (won't fix)

Everything has been said already.

moshe weitzman’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

True. I'd prefer that a committer decide next step. We do this for PHP configuration already.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

As just discussed over in #926636: Drupal install error on PostgreSQL 9.0 database, a hard-check on a setting that may not always be wrong (like register_globals is always wrong) is a bad idea. It's not something someone can change without server root anyway, so showing it to all users is rather self-defeating.

We knew when we switched to InnoDB as the default engine that it would be a problem if a shared host is running an untuned InnoDB. I was actually very reluctant to make that change for that very reason. However, we did it and we can't undo it now.

This sort of functionality belongs in a performance tuning module of some kind, which would be a great addition and is a good place to include both this and various other tuning recommendations for PHP, Apache, MySQL/PostgreSQL, etc. I recommend DSlow as a possible module name if we want something catchy. :-)

I'd be open to a patch that adds mention/discussion of this issue to INSTALL.mysql.txt, but not to the UI that users who have no ability to do anything about it will see.

Moshe: Really, there's nothing RTBC about this issue. At all.

nnewton’s picture

I'd like to re-iterate that this should be worded carefully even in the README. In certain setups tuning innodb_flush_log_at_trx_commit this way is a bug. Performance is all well and good, but we are thinking about giving advice that invalidates InnoDB's ACID support and has a significant impact on replication durability during a crash.

A performance tuning module would be quite cool though.....

jhodgdon’s picture

Is this written up somewhere in the Handbook that we can just link to?

EDIT/Added: It seems to me that this is way too much information to put into the MySQL installation file, and instead we should write it up in the Handbook with all the pros and cons and how-tos, and link to the page.

chx’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: database system » New documentation
Category: bug » task
Status: Needs work » Active

We need to mention somewhere, wish I knew where, that Drupal 7 is uses InnoDB and you really should read http://www.mysqlperformanceblog.com/2007/11/01/innodb-performance-optimi... about performance.

jhodgdon’s picture

http://drupal.org/node/51263 has some info on MySQL performance. Can someone review and/or add to that page/section?

Gábor Hojtsy’s picture

Component: New documentation » Correction/Clarification
mstrelan’s picture

I've run in to this issue when upgrading a D6 site to D7 and opened this issue - #1576748: Batch size is too high in taxonomy_update_7005(). My upgrade was interrupted consistently on taxonomy_update_7005 due to this innodb setting.

satchownz’s picture

I have seriously been crashing my laptop trying to update to D7 all day. I've also been searching the web like crazy to find a solution to the taxonomy_update_7005 system hang.

That one setting in mysql literally solved everything. I can't believe how much faster everything is. Thank you so much for posting it.

IMO this should be part of the minimum requirements. It made such a huge difference.

Cheers!

giorgio79’s picture

Much appreciated jbrown. My server was getting slaughtered, and I was on the verge of buying a new one when I found this post... :)

This module could be a great place as well to display this notice, although it seems abandoned http://drupal.org/project/dbtuner

janis_lv’s picture

thanks jbrown, you are the man!

LeeHunter’s picture

Title: Alert admins when innodb_flush_log_at_trx_commit == 1 is devastating db write performance » Document best practices for tuning innodb

Rewriting the title to reflect the outstanding task for the docs team.

I've done a little restructuring of the Admin guide section on performance, among other things to create a sub-section on database performance so this new page could be a child of https://drupal.org/node/2021599

LeeHunter’s picture

Issue summary: View changes
Status: Active » Fixed

I'm marking this as fixed since there is a fair bit of information here https://drupal.org/node/51263 including links to other resources on innodb.

Status: Fixed » Closed (fixed)

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

Dries’s picture

I think we need to revisit this. This patch seems pretty helpful.

moshe weitzman’s picture

Status: Closed (fixed) » Needs review

2: innodb.patch queued for re-testing.

Matt V.’s picture

I'm attaching a re-rolled version of the patch.

Matt V.’s picture

Project: Documentation » Drupal core
Version: » 8.x-dev
Component: Correction/Clarification » database system
moshe weitzman’s picture

FileSize
1.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,656 pass(es). View

Now with a link to the database tuning handbook page.

xjm’s picture

Title: Document best practices for tuning innodb » Provide a requirements warning when innodb_flush_log_at_trx_commit is set
+++ b/core/modules/system/system.install
@@ -177,6 +177,25 @@ function system_requirements($phase) {
+        'value' => t('Unsupported'),

I don't think we should call it "Unsupported". It is supported, and some sites may have the usecase to set it.

The rest of the text looks good to me: We're not telling them that they need to change it, but raising that it might be a serious concern and providing the documentation they need to make an informed decision.

jbrown’s picture

This patch was much more important when I wrote it 4 years ago.

The performance problem only occurs with spinning disks and now SSDs are much more commonplace.

I reran the tests on my SSD and got these results for creating 500 nodes:

0: 3.480s
1: 3.986s
2: 3.422s

So the value of innodb_flush_log_at_trx_commit does not make much difference to write performance on SSDs.

The text in the patch should be updated to say something like "This will significantly reduce performance on non-SSD drives, as a disk flush is performed after every database write."

Also the patch in it's current form didn't work on my setup. The array returned from fetchAssoc() has capitalized keys: array(2) { ["Variable_name"]=> string(30) "innodb_flush_log_at_trx_commit" ["Value"]=> string(1) "2" } whereas the patch is checking for keys starting with lowercase letters.

jbrown’s picture

-

chx’s picture

How is this issue alive after #31

I'd like to re-iterate that this should be worded carefully even in the README. In certain setups tuning innodb_flush_log_at_trx_commit this way is a bug. Performance is all well and good, but we are thinking about giving advice that invalidates InnoDB's ACID support and has a significant impact on replication durability during a crash.

Those who work with tuning databases already know this option well; those who don't should be superb careful to touch it. So whom are we targeting with the warning?

jbrown’s picture

With the value set to 0 or 2 it still syncs every second so at most you loose 1 second worth of data.

Now that we have SSDs I think this patch doesn't belong in core.

moshe weitzman’s picture

There are many many many Drupal sites without SSDs, and that will continue for some time. Even Amazon just recently introduced them for AWS and not for all instance types. This is just a helpful warning - it doesn't have to apply to everyone in order to be useful.

nnewton’s picture

I continue to think this is a bad idea. I feel for the hosting providers that are going to be endlessly nagged by users because "the db servers are configured wrong". Setting this to 1 is a valid setting, especially when you know what your doing/have a good RAID setup. (Setting this to 1 with a BBU + write-back)

This is giving advice in an area where "we" (drupal) do not have full information and could hurt Drupal's reputation with hosting companies. Perhaps the wording could be softened.

Damien Tournoud’s picture

Yes, I continue to agree this is a really bad idea.

moshe weitzman’s picture

FileSize
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,838 pass(es). View

I softened the wording as nnewton suggested.

chx’s picture

Those who work with tuning databases already know this option well; those who don't should be superb careful to touch it. So whom are we targeting with the warning?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marvil07’s picture

Status: Needs review » Closed (won't fix)

Comment #57 summaries this really well. After a couple of years of inactivity, I would say it is safe to close this issue.