Optimize comment_save()

OK the reason that test fails, is because if you switch from flat to threaded mode, old comments won't be ordered correctly. I would love it if we could get around this, but will split that to another issue. Threading is why we can never have nice things :(

To make up for this disappointment, I added variable for disabling the maintenance of {node_comment_statistics}, devel_generate and migrate could definitely use this, core can probably use it for bulk updates / deletes of comments too, athough that's not implemented yet.

Updated stats.

Executed 4309 queries in 1979.12 ms.

One other thing, we appear to do search_touch_node() twice on inserting a comment, needs looking into, doesn't have to be done here.

Moved the variable check inside _comment_update_node_statistics() so it works for node inserts too.

#4: comment_save.patch queued for re-testing.

I think we should assume we don't need it, certainly we don't have any such equivalent anywhere else.

Opened #757956: Stop calculating threads for flat comments for the threading calculation, this patch is fairly trivial otherwise apart from not knowing why field cache clear was put in in the first place, so better to do that one separately.

Talked to Moshe in irc about the thread calculation, we decided it's enough to let calling code set it, on normal inserts that query should be quite fast anyway, and while we let people change comment threading settings on the fly there's no nice way to code around it.

We missed one here. comment_node_insert() doesn't check for the variable. ideally comment_update_node_statistics() would do a db_merge() so we didn't have the code duplication here, but for now here's patch to add the variable check.

IMO setting a variable is more self-documenting, and much less of a hack, than having to rewrite a tagged query to make it a null op. We have several variables which allow you to unset stupid stuff in core ({node_comment_statistics} being one of the worst, since it's only really used by site-killing forum module queries) which can't be bypassed via the hook system, it's not great pattern, but it's better than forcing people to maintain custom patches to avoid their sites falling over (or migrations taking extra hours).

It was also committed with #11 already, this patch is just a follow-up.

One other option I can see here which would allow this to be disabled, would be committing #692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements. Then moving all _comment_update_node_statistics() calls to isolated hook implementations. It would then be possible with hook_module_implements_alter() to unset() those hook implementations entirely and force them not to be run. That would again make this a proper no-op rather than trying to force it with a query rewrite. I've not tested this though, and we'd run into problems if more stuff gets added to the hook implementations, although I don't think that's a problem at the moment.

There are tonnes of features in core which you can turn on and off by setting a variable - pretty much anything under /admin - page caching, block caching, gzip compression, various content type and comment settings etc.

The only difference here is that the feature is sufficiently obscure and developer facing that it's not worth a UI for, (or in this case even a settings.php entry).

There are several issues at play here, none which this patch attempts to solve, or that can be solved for Drupal 7:

The design of comment and tracker modules (and default per-table field storage for that matter) is extremely poor for sites with even a few thousand nodes or comments. Tracker and forum modules now have some code to ameliorate the effects of this, but they require maintaining index tables, which means additional writes. taxonomy allows you to turn maintenance of this index table off (also with a variable), in case you have a better option at hand (like mongodb field storage). node_comment_statistics is older, and doesn't.

Maintaining those index tables is a net win on the vast majority of sites, but when doing something like a migration or trying to avoid SQL altogether, you don't want to update a comment count 1 million times for 100,000 nodes, you want to update it all at the end.

Drupal has plenty of mechanisms for doing additional actions on certain events, but very few mechanisms for disabling things. Pluggable includes and variable checks is pretty much all we have at the moment, callbacks + alter hooks in some cases too.

Well the idea here is once you're done with the migration, you rebuild it programmatically. devel.module has a helper function to do just that.

Added an additional comment.

That comment's better that the duplicate one I added. So let's go for that, RTBC again.