Page MenuHomePhabricator

Bad Wikidata query format in DatabaseTermInLangIdsResolver::selectTermsViaJoin
Closed, ResolvedPublic

Description

At T246159 we can observe a really badly formatted query:

SELECT /* Wikibase\Lib\Store\Sql\Terms\DatabaseTermInLangIdsResolver::selectTermsViaJoin */
       wbtl_id, wbtl_type_id, wbxl_language, wbx_text, wbit_item_id
FROM `wbt_term_in_lang`, `wbt_text_in_lang`, `wbt_text`, `wbt_item_terms`
WHERE (wbtl_text_in_lang_id=wbxl_id) AND (wbxl_text_id=wbx_id) AND wbit_item_id IN (...) AND wbxl_language IN ('hu', 'en') AND (`wbit_term_in_lang_id` = wbtl_id)

Compare this with the equivalent:

SELECT /* Wikibase\Lib\Store\Sql\Terms\DatabaseTermInLangIdsResolver::selectTermsViaJoin */
       wbtl_id, wbtl_type_id, wbxl_language, wbx_text, wbit_item_id
  FROM wbt_term_in_lang
  JOIN wbt_text_in_lang
    ON wbtl_text_in_lang_id = wbxl_id
  JOIN wbt_text
    ON wbxl_text_id = wbx_id
  JOIN wbt_item_terms
    ON wbit_term_in_lang_id = wbtl_id
 WHERE wbit_item_id IN (...) AND wbxl_language IN ('hu', 'en')

(ignore the quotation and justification, plus I don't know by heart the right columns of each table). It is funny that a method called selectTermsViaJoin didn't contain any join! :-)

While this could be disregarded as a minor issue, I would like to direct your attention to this issue for 2 reasons:

  • A badly formatted query like this has been the direct cause of outages at wikimedia database infrastructure. By convention (outside of Wikimedia, but specially on Wikimedia) implicit JOINS are highly discouraged. I don't know how even the wikimedia abstraction layer allows them.

JOINS should be explicit and be used with its ON clause. In the past, the removal of an ON clause but not removal from the implicit list of tables to join caused overload and an outage. As a preventive measure, this was considered a bad practice and discouraged. This is also a standard among any serious professional SQL work. This is hard to read, to the point I could not work with the query. JOIN ... ON is way more legible. You know with a single view which tables are involved and differenciate filtering from joining comparisons, preventing mistakes when changing the query.

  • This issue would have been caught immediately by DBAs or other developers on review and asked to be changed immediately. This leads to the more important issue that review process may be broken because not even the most obvious formatting issue has been corrected on review. I am not looking to assign any blame here, I am asking to review the reviewing process for database layer changes so obvious mistakes like these are caught on time.

acceptance criteria

  • the sql should explicitly JOIN and ON keywords instead of doing the join by some implicit SQL-Magic

Event Timeline

jcrespo created this task.Feb 26 2020, 3:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 26 2020, 3:02 PM
jcrespo updated the task description. (Show Details)Feb 26 2020, 3:03 PM
jcrespo updated the task description. (Show Details)
Addshore moved this task from Incoming to Needs Tech Work on the Wikidata-Campsite board.
Addshore added a subscriber: Addshore.
Addshore triaged this task as Medium priority.Apr 28 2020, 2:55 PM
Addshore moved this task from Needs Tech Work to Prioritized Tech on the Wikidata-Campsite board.

If this query patterns is not desired then perhaps this should not be possible via the mediawiki database abstraction?
I created T251286: Investigate restricting the ability to create sql queries with bad patterns in mediawiki for that.

We will get this query pattern fixed in Wikibase\Lib\Store\Sql\Terms\DatabaseTermInLangIdsResolver::selectTermsViaJoin

Addshore renamed this task from Bad Wikidata query format, review process broken? to Bad Wikidata query format in DatabaseTermInLangIdsResolver::selectTermsViaJoin.Apr 28 2020, 2:57 PM
Addshore updated the task description. (Show Details)Jul 21 2020, 1:43 PM
Michael updated the task description. (Show Details)Jul 21 2020, 1:44 PM

Change 615588 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Rework query format of DatabaseTermInLangIdsResolver::selectTermsViaJoin

https://gerrit.wikimedia.org/r/615588

With this change the query changes to something like this:

SELECT wbtl_id,wbtl_type_id,wbxl_language,wbx_text,wbpt_property_id FROM `wbt_term_in_lang` JOIN `wbt_text_in_lang` ON ((wbtl_text_in_lang_id=wbxl_id)) JOIN `wbt_text` ON ((wbxl_text_id=wbx_id)) JOIN `wbt_property_terms` ON ((`wbpt_term_in_lang_id` = wbtl_id)) WHERE wbpt_property_id = 22 AND wbtl_type_id = 1 AND wbxl_language = 'en'

SELECT wbtl_id,wbtl_type_id,wbxl_language,wbx_text,wbit_item_id FROM `wbt_term_in_lang` JOIN `wbt_text_in_lang` ON ((wbtl_text_in_lang_id=wbxl_id)) JOIN `wbt_text` ON ((wbxl_text_id=wbx_id)) JOIN `wbt_item_terms` ON ((`wbit_term_in_lang_id` = wbtl_id)) WHERE wbit_item_id = 228 AND wbtl_type_id = 1 AND wbxl_language = 'en'
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptJul 23 2020, 12:54 AM
  • This issue would have been caught immediately by DBAs or other developers on review and asked to be changed immediately. This leads to the more important issue that review process may be broken because not even the most obvious formatting issue has been corrected on review. I am not looking to assign any blame here, I am asking to review the reviewing process for database layer changes so obvious mistakes like these are caught on time.

The earliest version of this code seems to date back to me (Ide4f3d1644), and if I recall correctly, I was unsure about how to do a JOIN properly and found the documentation to be extremely confusing. This part of IDatabase::select is probably what led to the current code (emphasis added):

@param string|array $join_conds Join conditions: Optional associative array of table-specific join conditions. In the most common case, this is unnecessary, since the join condition can be in $conds. However, it is useful for doing a LEFT JOIN.

Based on that sentence, I would even today vote against the change Amir just uploaded if it wasn’t for this ticket, because it seems to fly directly in the face of this documentation: why use $join_conds if the documentation says it’s unnecessary except for a LEFT JOIN that we don’t want?

based on that sentence, I would even today vote against the change Amir just uploaded if it wasn’t for this ticket, because it seems to fly directly in the face of this documentation: why use $join_conds if the documentation says it’s unnecessary except for a LEFT JOIN that we don’t want?

That documentation is, IMHO, wrong. And I can bring authoritative references if needed: implicit joins are not ANSI (aka not real "SQL" standard), and are deprecated in several database engines ("considered harmful" XD). MySQL keeps supporting it, as it does with other set of bad practices- and I don't advocate necessarily to remove the option, but I do to discourage it- again based on actual past outages that it has caused on Wikimedia by mistaken deploys, as I mentioned on my initial comment.

Change 615588 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Rework query format of DatabaseTermInLangIdsResolver::selectTermsViaJoin

https://gerrit.wikimedia.org/r/615588

Michael added a subscriber: Michael.Aug 3 2020, 1:51 PM

I'm looking at this ticket in order to verify that it is done. Is there a way to see the actual queries run against the database? Or maybe dump an hour or a day's worth of Wikibase's SQL queries into a file so that it can be searched with regexes to find joins that are still implicitly done?

There is this procedure: https://wikitech.wikimedia.org/wiki/MariaDB/query_performance which could be done for Wikidata by people with root database privileges, however it is quite involved. I will ask if maybe Manuel wants to perform soon such a thing for other main reasons (e.g. MCR changes checking) and maybe this could be checked at the same time.

There is also performance_schema storing digests, but it is unreliable due to the large amount of different query patterns mw/wikibase gets (it gets quickly out of memory after a server gets started). If the query was stored there, it howerver can reliably say when was the last time it was executed.

Note that due to volume, all queries are almost impossible to be checked, only a high sigma of it, probabilistically.

For now, you can check tendril.wikimedia.org where it has list of slow queries: https://tendril.wikimedia.org/report/slow_queries?host=%5Edb&user=wikiuser&schema=wikidatawiki&qmode=eq&query=&hours=1

It doesn't have all queries but it has enough to see if the change is applied or not.

Lucas_Werkmeister_WMDE closed this task as Resolved.Aug 14 2020, 11:20 AM

The query looks like this in Tendril now:

SELECT /* Wikibase\Lib\Store\Sql\Terms\DatabaseTermInLangIdsResolver::selectTermsViaJoin */
wbtl_id, wbtl_type_id, wbxl_language, wbx_text, wbpt_property_id
FROM `wbt_term_in_lang`
JOIN `wbt_text_in_lang` ON ((wbtl_text_in_lang_id=wbxl_id))
JOIN `wbt_text` ON ((wbxl_text_id=wbx_id))
JOIN `wbt_property_terms` ON ((`wbpt_term_in_lang_id` = wbtl_id))
WHERE wbtl_type_id = 1
AND wbxl_language = 'it'

So I think this is done.