Improve performance of search totals calculation

This is a clever idea. Two remarks:

• the LOG(B,X) form exists in both MySQL and PostgreSQL. You can use LOG(10, X) as an alternative to LOG10
• I'm not sure there is much to gain in increasing the batch number more. This needs testing, but 50 or 100 seems more than enough.

fwiw there's contrib support for LOG/LOG10 in sqlite - http://www.sqlite.org/contrib

I don't know about the optimal word limit, but other than that the patch looks good to me. I've also compared results before and after, and counts in search_total were the same.

I would not delay the patch because of the uncertain word limit, which could easily be changed later. This is already a great performance improvement.

When I run the search tests after applying this patch I get:

Unexpected PHP error [You can't specify target table 'simpletest128363search_total' for update in FROM clause query: DELETE FROM simpletest128363search_total WHERE word IN (SELECT t.word AS realword FROM simpletest128363search_total t LEFT JOIN simpletest128363search_index i ON t.word = i.word WHERE i.word IS NULL)] severity [E_USER_WARNING] in [/Users/dries/Sites/drupal-cvs/includes/database.mysqli.inc line 130]

Can somebody reproduce this?

Some code-style and commenting issues with this.

1.) -1 on the magic number, especially in the "$dirty500" variable - couldn't this be made a constant?

2.) SQL queries have WHERE clauses. I've never heard of "an SQL clause". Also, "this prevents" does not specify the alternative case it is preventing - would the big clause this avoids occur when updating them one by one (like now), or all at once?

3.) Please consider running this through the code-cleaner - I see trailing white-space.

4.) "We use the two embedded subqueries to avoid the database 'update in FROM clause' error." This comment doesn't clarify that it is a MySQL quirk, why this fixes it (forcing a temp table) or where to find more information on it. Specifically, on mysql.com.Workarounds should be documented much more clearly.

Marked #312390: Performance: search_update_totals() slow with large number of nodes as duplicate. Here's Wesley's / BrianV's last patch from that issue for reference.

Subscribe.

Re-roll of the patch at #11.

Changes
- Converted to DBTNG ;)
- Renamed $dirty500 to $dirty_spliced
- Left the 500. I don't think changing that to an constant would help, if at all, it should be an variable_get(). Not sure about that and I'm leaving that decision to others...
- Tried to improve the comments

Search tests pass locally on MySQL, but I haven't tested Postgresql nor sqlite.

Not critical. Please read and understand Priority levels of Issues, thanks.

#17: update_total_optimization.patch queued for re-testing.

Any hope for a backport to drupal6?

I definitely don't think the 500 should be hard-wired. Use a variable. Someone will want to set it to a different value and they'll be forced to hack core, which would be annoying.

A few other comments on the code/style/comments:

a)

+  // Update the search totals in chunks of 500 words at a time
+  // to avoid too large sql queries.

2nd line is innacurate -- what you are avoiding is running too many queries, not too large of queries. Also, if you are going to use the acronym SQL it needs to be capitalized (though I think it can just be omitted).

b)

+  // rows which fail to join. MySQL doesn’t allow referring to a table that is
+  // targeted for update/delete in a FROM clause, using two nested subqueries to work
+  // around that limitation.
+  db_delete('search_total')
+    ->where('word IN (SELECT word FROM (SELECT t.word AS realword FROM {search_total} t LEFT JOIN {search_index} i ON t.word = i.word WHERE i.word IS NULL) t1)')
+    ->execute();

The comment has a minor grammatical problem here (should probably say "so we use" instead of "using")... But shouldn't that whole thing be rewritten using DBTNG instead of individual SELECTs? And why do you need to alias the subquery as t1 -- doesn't look like that table alias is being used anywhere?

2nd line is innacurate -- what you are avoiding is running too many queries, not too large of queries. Also, if you are going to use the acronym SQL it needs to be capitalized (though I think it can just be omitted).

No, it's correct. We explain why we group them by 500 instead of executing all of them in a single query. As you've understood it "wrong", how would you sugggest to write it so that it's purpose is clear?

The comment has a minor grammatical problem here (should probably say "so we use" instead of "using")... But shouldn't that whole thing be rewritten using DBTNG instead of individual SELECTs? And why do you need to alias the subquery as t1 -- doesn't look like that table alias is being used anywhere?

I don't think so. That would mean that we'd have to create two additional db_select() objects and don't gain anything, since you can alter the where string if you really want to (don't see a use case for that).

Regarding the alias, that is simply required by a query that selects FROM a subquery.

Oh, I thought the original ran one query per word? I still think it does, looking at the patch... So we are avoiding both running 1 query per word (lots of queries) and avoiding having one big query (too large of a query). Maybe the comment should say something about balancing the number of queries and the size of queries, and say that 500 seemed like a good balance, and you can adjust it by setting this variable in your settings.php file?

Regarding my second comment, point taken.

Ok..

- added variable, do we need to document that somewhere else?
- Updated the comments

Thanks, I like that comment better.

AFAIK, we don't currently have a good way to document variables. As it is now, if someone has a need they would at least be able to find it in the code, I guess...

A port to Drupal 6 would be really helpful, please.

#25: update_total_optimization2.patch queued for re-testing.

It looks like this still passes the test bot, and I think the concept and patch have been reviewed adequately. Let's squash this one!

subscribe.

#25: update_total_optimization2.patch queued for re-testing.

I'm not a big fan of this patch because it introduces non-standard SQL stuff (i.e. LOG10) that might not be availble on SQLite or MSSQL. I recommend that we postpone this to Drupal 8 so we can develop a better understanding of the downsides. I don't think this is as important to get in.

I thought the comments in #1 - #3 addressed that - it is using LOG not LOG10.

What about databases like MSSQL and Oracle? It would be good to see support for those in Drupal 7 (in contrib)?

Oracle has log: http://www.techonthenet.com/oracle/functions/log.php

Sigh. It looks like SQL-Server has log10 but not generic log according to this site:
http://www.sql-ref.com/

But Oracle has log but not log10.

Sigh.

Hmmm...

Back to the drawing board, indeed.

sorry, finger slip there.

Here's the practical math: LOGB(X) = LOG(B,X) = LOG(X) / LOG(B)
So, lack of a either of the first functions is not a hindrance as all RDBMS appear to support the LOG(oneparam) form?

This stuff does need fixing, even in D6, as it's causing real trouble in growing live sites. Issues like these don't exhibit gradual degradation, they'll suddenly flip into the realm of annoyance. We just experienced this on our own site. It makes the task stay around for effectively forever - in any case way longer than the cron frequency. That over time means that MySQL connections get used up, RAM on the webserver gets used up, and so on. It's like an internal DoS scenario.

Happy to assist with SQL logic to make this work well.

OK. Sounds like we need a new patch, or a review of the last patch.

Regarding support for other databases mentioned above, I don't think we are going to find a solution to this problem, since the log() function is not supported the same in all databases. Can we make a patch that doesn't involve log functions at all?

Hey guys, we will really appreciate if we can fix this issue for Drupal 6. This is the only slow query I keep on getting in my database. And I don't even have that many nodes. How is that patch coming along?

Right at the moment, we don't have a solution or a patch, even for Drupal 7. Some database servers have the LOG function, and others have LOG10, so there is not a generic way to do a patch that involves logs at all. See #32, #35, #38 above. So far, no one has come up with a suggestion that doesn't involve log functions.

If you want this issue to be fixed, that would be the place to start.

dbtng already swaps out ILIKE for LIKE in postgresql if using the query builder, couldn't the various drivers do the same for LOG vs. LOG10?

#41: Yes, that would be a good idea. However, it appears that SQLite doesn't even have a log function at all without contrib support -- see http://www.sqlite.org/contrib and http://www.sqlite.org/lang_corefunc.html

Since D7 officially supports SQLite, I think it would be a bad idea to introduce an unsupported function in a core search.module query?

For SQLite, you can add functions directly in PHP:

http://api.drupal.org/api/drupal/includes--database--sqlite--database.in...

I did not know that -- cool! So we should be able to proceed using LOG (which exists in all dbs except apparently SQLite and MS SQL Server), with the addition of a patch for SQLite to add the LOG function, and the MS SQL Server folks would also need to add a replacement to their driver (see #37 for the math, and #41 for a comment on how to do this).

What we need for this patch:
- The patch in #25 already uses LOG(10,x), so we're OK on that for MySQL, PostgreSQL.
- Need an addition to the SQLite DB classes (see #43) to add the LOG function.
- Need to notify the maintainers of the SQL Server driver that they need to transform LOG to LOG10 in queries.

This sounds like a workable plan. Someone just needs to write the SQLite part of the patch, which looks straightforward. Maybe some of the people who commented that they wished it was fixed would care to put some time in and do this?

Hey guys, thanks a lot for all the time and effort you guys are putting in. Unfortunately I will be of no help on the programming side of things (still a beginner). jhodgdon, is the patch in #25 stable enough to use on a MySQL database running Drupal 6? This would be great to fix this problem if possible.

Thanks again.

Rick

Sorry, I have no idea if the patch above is suitable for MySQL on Drupal 6. If you wanted to test it on a development version of your site, that would probably be helpful. We'd probably want to know whether the patched Drupal and the unpatched Drupal ended up with the same scores in the search index table, when indexing the same content, and if possible, what the performance gains were (like how long it took to run cron on the patched vs. unpatched site).

subscribe

Just a reminder there are patches for D6 at #312390: Performance: search_update_totals() slow with large number of nodes, which was marked duplicate of this one.

subscribing

POW! SQLite compatibility.

EDIT: Although, seeing as how MSSQL and MySQL support LOG10() and it would be easier for us to do LOG10() under SQLite than LOG() (we could just use PHP's log10() function instead of creating our own callback to juggle the argument order for log() as in the patch), I kind of wonder if we should instead just use LOG10() and tell the Postgres driver people that they're the ones that will have to work around it. Wouldn't it just be as easy as doing str_replace('LOG10(', 'LOG(10, ', $query) ?

Subscribe.

I am getting a PDOException during a cron run. (Regardless of patched or not.)

SQLSTATE[42000]: Syntax error or access violation: 1104 Die Ausführung des SELECT würde zu viele Datensätze untersuchen und wahrscheinlich sehr lange dauern. Bitte WHERE-Klausel überprüfen und gegebenenfalls SET SQL_BIG_SELECTS=1 oder SET SQL_MAX_JOIN_SIZE=# verwenden

This this the German error message for an ER_TOO_BIG_SELECT.

It comes from
$result = db_query("SELECT t.word AS realword, i.word FROM {search_total} t LEFT JOIN {search_index} i ON t.word = i.word WHERE i.word IS NULL", array(), array('target' => 'slave'));
in search_update_totals(). (Full stacktrace)

Let me know if this should go into a second bug report.

Edit: Thank you jhodgdon. Seperate issue is: #1189484: ER_TOO_BIG_SELECT in search_update_totals()

Yes, this is a completely separate issue. Please fill a new report.

Hm. sorry, maybe this is related to the same issue. Anyway, thanks for reporting! The English error message would be:
The SELECT would examine more than MAX_JOIN_SIZE rows; check your WHERE and use SET SQL_BIG_SELECTS=1 or SET SQL_MAX_JOIN_SIZE=# if the SELECT is okay

#55: 257916.patch queued for re-testing.

Rerolled against head.

This patch might solve these related issues.

rerolling #59...

Ok.. Let's try this without the variable_get()....

oops

subscribe.

Please do not add "subscribe" messages to issues. Just click the "Follow" button.

I just filed a meta-issue #2367253: [META] Several problems in search_update_totals() to clarify the similarities and differences between this issue and the others around search_update_totals(). This one is really a task and not a bug report per se.

I just ran into this problem on Drupal 6 after manually emptying a 2+ million record search index but forgetting to truncate the search_totals table. Every time cron ran while rebuilding the index it was trying to delete over 39,000 words from the search_total table that don't exist in the search_index table, one by one in a loop. This is easily fixed by deleting using the left join statement instead of selecting, iterating, and deleting. i.e.:

db_query("DELETE t FROM {search_total} t LEFT JOIN {search_index} i ON t.word = i.word WHERE i.word IS NULL");

It took under 0.3 seconds to get rid of those 39,000 words.

The patches here for D7 and D8, look like they use a DELETE WHERE word IN (SELECT word .. IN( SELECT ..)) which is likely to be much slower and require more temporary tables.

I'll leave this patch here for anyone else on Drupal 6 who wants it.

Thanks! Yeah none of the approaches here have been finalized... however I don't think we can do a DELETE query with a JOIN in all databases. MySQL allows it, but I believe it is not compatible with other databases necessarily. I could be wrong?

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Wanted to bump 1 more time before closing.

Also know search module is up for being removed from core (maybe in 12 or 13) so maybe this is postponed on that.

This really is stale and there has been no response to the last two comments which were posted almost 1 year ago.

I have updated credit and am closing this issue.