linter creates unsafe SQL writes
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	jcrespo
	Oct 5 2017, 10:04 AM

Description

During a routine inspection, I saw DELETE /* BatchedQueryRunner::execute */ FROM linter WHERE linter_cat=12 LIMIT 500

This is an unsafe query, for which the master returns a warning, and unless that is done on purpose, a recipe for full replication breaks. We do not have yet row-based replication, that is why "SQL safe statements" mediawiki recommendation was passed:

Non-deterministic queries and unsafe statements for binlog should be avoided as they would return/write different results in a replication environment.

- https://www.mediawiki.org/wiki/Development_policy#Database_policy

There are some cases in which that could be ok, but I want to be thorough at first. Could we order by PK or something else to make is safe again?

Related Objects

Mentioned Here: T176363: New high-priority Linter category for a subset of misnested tags whose behavior will change with a HTML5 parser

Event Timeline

jcrespo created this task.Oct 5 2017, 10:04 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 5 2017, 10:04 AM

This query is not generated by the extension itself. This is from T176363#3659771 -- is this still an issue?

@Legoktm with "I ran the replication" you mean it is not part of the code (not happening now?). If yes, it is no longer an issue.

In T177466#3660896, @jcrespo wrote:

@Legoktm with "I ran the replication" you mean it is not part of the code (not happening now?). If yes, it is no longer an issue.

That is correct. This is not part of the code.

Thanks. This seem to have been observed on a very delayed replica.

(re-opening just so I can ask a question)

@jcrespo this was a one-off script I ran to clear out some bad data from the database. That said, there's a chance that we'll end up running something similar like this in the future. Would adding ORDER BY linter_id have resolved the issue and made the query safe?

Yes, if it doesn't run slower (check explain) and makes it deterministic, it is highly preferred. Even in some cases where "we are actually going to delete all rows" it can be unsafe ,because we assume all servers have the same rows- something true in theory but with the possibility of not being true. Until we migrate to row-based replication, we should try to do safe queries only (you can check warnings on the servers, not sure if logstash stores those).

Got it - I will keep that in mind for the future.

To give more feedback, in one occasion where doing things in an unsafe way was actually prefered (for performance reasons), we did LIMIT a number of times, but at the end we did a last execution without LIMIT to make sure all rows were deleted (making the queries safe logically). In the general case, ORDER + offset is usually suboptimal, and paging by id is recommended and more performant/more reliable in query time limit.

linter creates unsafe SQL writesClosed, ResolvedPublicActions

Description

Related Objects

Event Timeline

linter creates unsafe SQL writes
Closed, ResolvedPublic
Actions