Improve speed of l10n_community_get_strings()

It would have one row for sid, language, is_active = 1 and is_suggestion = 0, but without is_suggestion = 0 and leaving the other conditions, there may be multiple outstanding suggestions (regardless of whether there was an is_active = 1 and is_suggestions = 0 too or not).

ts.language = t.language AND t.is_active = 1 may have any number of rows in translations (as many as outstanding suggetions + 1 if there is an approved translation). I think the result of this query is the number of source strings that have at least one active translation OR suggestion in the given language. (silly)

Given all the left joins, though, those do not actually filter this query, so the result is the unique number of sids in the source table, which is constant. l10n_community_get_strings() OTOH supports various filters, and in case one of them is applied, the results would be quite different. Eg. if only strings that have suggestions in this language or if only strings that were translated by a particular uid.

It is true that this default query without filters should not be this complicated for the count query at least, which would speed up the default setup of the translation form at least. However, people usually filter at least by a project/release. Eg. https://localize.drupal.org/translate/drupal8 links to languages filtered to D8 RC2 and only untranslated strings to help people finish those up. For example https://localize.drupal.org/translate/languages/hu/translate?project=dru... for Hungarian.

BTW the tables have these indexes:

SOURCE:
    'primary key' => array('sid'),
    'unique keys' => array(
      'hashkey' => array('hashkey'),
    ),

TRANSLATION:
    'primary key' => array('tid'),
    'indexes' => array(
      'uid_entered' => array('uid_entered'),
      'is_suggestion_is_active_language' => array('is_suggestion', 'is_active', 'language'),
      'sid_language_is_suggestion_is_active' => array('sid', 'language', 'is_suggestion', 'is_active'),
    ),

STATUS FLAG:
    'primary key' => array('sid', 'language'),
    'indexes' => array(
      'sid_language_has_suggestion' => array('sid', 'language', 'has_suggestion'),
      'sid_language_has_translation' => array('sid', 'language', 'has_translation'),
    ),

The UI / API supports filtering by any combination of:

s.sid, s.context (string context), t.uid_entered, l.rid (release), l.pid (project), LIKE on s.value or s.translation, ts.has_translation, ts.has_suggestion, t.is_suggestion

that is pretty rich :)

We can definitely look at what data do we need in the first place if only certain filters are used.

That would not suffice IMHO. Here is a summary of the tables vs. filters:

The translations have non-unique sid because there may be multiple suggestions for a sid (that are is_active). The lines are non-unique because there may be multiple occurrences of the same string in a project / release. In fact this later one is very likely.

So until we need one of the filters that uses the line (release/project) or translation (author, search and is / is not suggestion) tables, we can skip the distinct. Unfortunately it is very typical for a translator to go translate a specific project release. Or a moderator to review suggestions from a specific user or suggestions that contain a specific (incorrect) substring, etc.

We could break the line table to two, sid to project and sid to release if we don't care for usage repetitions, and then we can also abandon the file table (which sits inbetween releases and lines now, but release and project info is denormalized into lines). We would loose some data that we don't actually use now but could, eg. to help translators prioritize projects that are used more in a project vs. one-offs. Or we could have a parallel table structure for quicker lookups for sid to pid and sid to rid, which we keep updated on imports. That would still only help with the project and release filters (which are very typical), not the author or any others. So one could easily run into a temp table then too.

I started a deep look at this by updating some docs first. Also removing the no-pager feature because this function is only ever invoked once and that always uses a pager.

@drumm: so these are ALL the cases where distincts would be needed. At least the other cases may be temp-table free then (assumed, did not test :).

Ok committed that for a limited improvement. Hope more is possible :)

@drumm: so in #10 I removed the non-pager support on this, because it is only used once and that always has a pager:

Gabor:l10n_server gabor$ git grep l10n_community_get_strings
l10n_community/translate.inc:  $strings = l10n_community_get_strings($langcode, $filters, $filters['limit']);
l10n_community/translate.inc:function l10n_community_get_strings($langcode, $filters, $pager) {

So if there is a non-pager version of this query, it would be somewhere else, no?

As for further optimizing the query, I looked at what data is used. In l10n_community_get_strings() if the tid that was found was not a translation, we load the translation if there was one. Because the tid found may be nonexistent or even a "random" one based on a matching suggestion, etc. However, all suggestions are loaded with their respective data in _l10n_community_translate_form_item() in if ($source->has_suggestion). So the output of l10n_community_get_strings() is taken primarily in terms of the sid found and if the tid is an active translation (or was overloaded with one), then that is used to be displayed as the translation. I think we can generally do the loading of the translation data only if needed.

While writing this patch I realized that merely joining the translation table will add duplicates of sids, so not doing the distinct for them generally but only if the translation filters are used still leaves duplicates. So we should not add the translation table when not needed.

This drastically cuts down on the columns queried and only uses 1-1 joins by default unless the line or translation tables are needed to be joined in. Even then it is less columns, so the temp table would be smaller at least :) It also makes sense since we may have found a suggestion in many cases, so their data will be thrown away and the translation loaded in place anyway. So not loading those columns in the first place is fine IMHO for the big table. We can load them for the (max 50) results found.

Duh.

Deployed that and I'm seeing dramatic improvements in search. On staging it looked like almost instant ;) Even with filters that required distinct. Really interested in monitoring data on how much this one helped :)

Depending on those results, we may want to close this :)

Hm, according to answers on http://stackoverflow.com/questions/581521/whats-faster-select-distinct-o... DISTINCT creates a temporary table and uses it for storing duplicates. GROUP BY does the same, but sortes the distinct results afterwards.. So I am curious how does it solve it. But either way we are not interested in anything but the sid, language and translation stat (has_translation, has_suggestion), and either group by or distinct should keep those, because they should be the same regardless of what is joined in.

Maybe that has to do with http://stackoverflow.com/questions/581521/whats-faster-select-distinct-o... that is that the group by would only need to look at the sid which has an index, and the distinct needs to look at the whole row which does not.

Right, we only need a unique sid list, we don't care for uniqueness of other values (that would not even be possible, they are boolean flags). Neither that the combination of values is unique, because the sid itself should already be unique, so the rest should not matter. Do you have a proposed patch for a group by that we can test on staging?

Here is a quick patch. I don't think the D7 query API supports setting an ORDER BY NULL, it requires using a specific field name as per the docs at least (it says it requires adding the field before using the orderby). So not trying that out for now...

Let's do that then.

Added some comments to enlighten future code reviewiers ;)

Deployed. Looking forward to monitoring feedback to confirm :D

It seems that this query was run yesterday.

# Time: 151027 13:00:21
# User@Host: localize_rw[localize_rw] @ www1.drupal.org [140.211.10.18]
# Thread_id: 9135934  Schema: drupal_localize  QC_hit: No
# Query_time: 10.370310  Lock_time: 0.000054  Rows_sent: 1  Rows_examined: 1822041
SET timestamp=1445950821;
SELECT COUNT(*) AS expression
FROM
(SELECT DISTINCT s.sid AS sid, s.value AS value, s.context AS context, t.tid AS tid, t.language AS language, t.translation AS translation, t.uid_entered A
S uid_entered, t.time_entered AS time_entered, t.time_changed AS time_changed, t.is_suggestion AS is_suggestion, t.is_active AS is_active, ts.has_suggesti
on AS has_suggestion, ts.has_translation AS has_translation, u.name AS username, 1 AS expression
FROM
l10n_server_string s
LEFT OUTER JOIN l10n_server_status_flag ts ON s.sid = ts.sid AND ts.language = 'gsw-berne'
LEFT OUTER JOIN l10n_server_translation t ON ts.sid = t.sid AND ts.language = t.language AND t.is_active = 1
LEFT OUTER JOIN users u ON u.uid = t.uid_entered) subquery;

@mlhess: not sure that that query may be generated by the current code of this function. It does indeed come from localize, but I don't think the function can generate such a query anymore. For starters, the query does not join the users table anymore in a count query under ANY circumstances.

It would be great to also quantify the problem between "things could be more optimal" vs. "OMG this brings all the servers down". That would help prioritize :)