Fix accidental subqueries created by `pick` adding a LIMIT clause to queries with existing LIMIT
The Discovery of the Problem
The following discussion from !147231 (merged) should be addressed:
-
@mfanGitLab started a discussion: (+9 comments) When I expanded the query you had something weird is happening, the query is doubled!! seems like an existing bug/behaviour.
SELECT "ci_pipelines"."status" FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 278964 AND ("ci_pipelines"."source" IN (1, 2, 3, 4, 5, 6, 7, 8, 10, 11) OR "ci_pipelines"."source" IS NULL) AND ("ci_pipelines"."status" IN ('created','waiting_for_resource','preparing','waiting_for_callback','pending','running','failed','success','canceled','canceling','manual','scheduled')) AND "ci_pipelines"."ref" = 'master' AND "ci_pipelines"."id" IN ( SELECT "ci_pipelines"."id" FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 278964 AND ("ci_pipelines"."source" IN (1, 2, 3, 4, 5, 6, 7, 8, 10, 11) OR "ci_pipelines"."source" IS NULL) AND ("ci_pipelines"."status" IN ('created','waiting_for_resource','preparing','waiting_for_callback','pending','running','failed','success','canceled','canceling','manual','scheduled')) AND "ci_pipelines"."ref" = 'master' ORDER BY "ci_pipelines"."id" DESC LIMIT 100) ORDER BY "ci_pipelines"."id" DESC LIMIT 1
The Cause of the Problem
This change, from pluck.first
to pick
:
# Returns the pipelines in descending order (= newest first), optionally
# limited to a number of references.
#
# ref - The name (or names) of the branch(es)/tag(s) to limit the list of
# pipelines to.
# sha - The commit SHA (or multiple SHAs) to limit the list of pipelines to.
# limit - This limits a backlog search, default to 100.
def self.newest_first(ref: nil, sha: nil, limit: 100)
relation = order(id: :desc)
relation = relation.where(ref: ref) if ref
relation = relation.where(sha: sha) if sha
if limit
ids = relation.limit(limit).select(:id)
relation = relation.where(id: ids)
end
relation
end
def self.latest_status(ref = nil)
- newest_first(ref: ref).pluck(:status).first
+ newest_first(ref: ref).pick(:status)
end
Applies a second limit
clause to the relation being built. The first is LIMIT 100
, applied by default in newest_first
. Rather than reduce the existing limit, adding limit 1 causes a new query to be built around the first, which has been made into a subquery:
As-is, with default limit of 100:
project.all_pipelines.newest_first(ref: project.default_branch).pluck(:status).first
SELECT "ci_pipelines"."status" FROM "ci_pipelines"
WHERE "ci_pipelines"."project_id" = 7
AND "ci_pipelines"."ref" = 'master'
AND "ci_pipelines"."id" IN (
SELECT "ci_pipelines"."id" FROM "ci_pipelines"
WHERE "ci_pipelines"."project_id" = 7
AND "ci_pipelines"."ref" = 'master'
ORDER BY "ci_pipelines"."id" DESC LIMIT 100
)
ORDER BY "ci_pipelines"."id" DESC
Adding an explicit limit of nil
:
project.all_pipelines.newest_first(ref: project.default_branch, limit: nil).pluck(:status).first
SELECT "ci_pipelines"."status" FROM "ci_pipelines"
WHERE "ci_pipelines"."project_id" = 7
AND "ci_pipelines"."ref" = 'master'
ORDER BY "ci_pipelines"."id" DESC
The Proposal
- Acutely, for the usage we discovered, the change is simple. We can still use pick, but need to avoid the initial limit:
def self.latest_status(ref = nil)
- newest_first(ref: ref).pick(:status)
+ newest_first(ref: ref, limit: nil).pick(:status)
end
- We should re-review Resolve Rails/Pick offenses (!93973 - merged) and make sure we didn't accidentally introduce other subqueries as a result of the additional
LIMIT
clause on existing queries
Edited by drew stachon