Repository navigation
Speed up work package visibility CTE - #26131
oliverguenther wants to merge 3 commits into
Conversation
Following a demo extraction of pg_stat_statements on the community shard, I noticed that `WITH "member_projects" AS ...` made up a lot of the statements records. I let claude analyze the allowed_to CTE on the public anonymized community dump. On that dump, queries containing this CTE (visibility checks and unread notification counts) use about 50 % of the database time and write most of the temp files. It suggests to use IN in both branches, so that each branch returns every work package at most once, and combine them with UNION ALL. Only work packages that are both in a member project and shared directly with the user can now appear twice, which the EXISTS check ignores. The CTE stays MATERIALIZED, so the LIMIT 1 planning issue it works around is not affected. On the local dump, for a user with 263 member project rows, the CTE input drops from 22,617 rows into a HashAggregate to 10,387 rows with no aggregation. The result set is identical. Median query times on the anonymized community dump over six runs: - Visible work package IDs: 38.4 → 21.7 ms - Any visible work package: 25.3 → 8.5 ms - Single work package visibility: 22.1 → 5.3 ms - Unread notification count: 41.9 → 25.6 ms
github-actions
Bot
added
the
ai: Directed 🪄
|
I was looking into this one as well when working on #25984. This can also be further optimized because this loads and materializes a list of all work packages within the allowed projects. This can be optimized again, where we just load the list of visible projects and the handful of shared work package IDs and putting the UNION one level above. |
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
|
I compared the first commit 231cb6e (UNION ALL) and added a second one 090f748 (plucked ids). One would think that plucking + uniq in Ruby would be way worse, but that seems not to be the case:
Tested benchmarks
C was not measured in production. It was the "small instances" strategy of #19876 and was removed Note The anonymous user path is not changed. On community, Benchmark runRunning a benchmark script of the variants on a temporary rails console:
Factor exec and factor wall in the tables below are A divided by D. Benchmark script# Work package visibility benchmark. Read-only: runs in a READ ONLY transaction and rolls back.
# Paste into a Rails console. On a multi-tenant shard, switch to the tenant first.
# Optional: set VISIBILITY_BENCH_USER_IDS = [1, 2, 3] before pasting to choose the users yourself.
(ActiveRecord::Base.logger || Logger.new(nil)).silence do
runs = 5
conn = ActiveRecord::Base.connection
permissions = Authorization.contextual_permissions(:view_work_packages, :work_package, raise_on_unknown: true)
member_union = ->(user) do
Project.unscoped.allowed_to_member_union(user, permissions, entity_types: [nil, WorkPackage.name])
end
split_ctes = {
entity_member_projects: Arel.sql("SELECT * FROM member_projects WHERE entity_id IS NOT NULL"),
project_member_projects: Arel.sql("SELECT * FROM member_projects WHERE entity_id IS NULL")
}
materialized_cte = ->(user, allowed_sql) do
WorkPackage
.with(member_projects: Arel.sql(member_union.(user).to_sql),
**split_ctes,
allowed_by_projects_and_work_packages: Arel.sql("WITH allowed AS MATERIALIZED (#{allowed_sql}) SELECT * FROM allowed"))
.where("EXISTS (SELECT 1 FROM allowed_by_projects_and_work_packages a WHERE a.id = work_packages.id)")
end
pluck_sql = ->(user) do
Project.unscoped
.from(Arel::Nodes::TableAlias.new(Arel::Nodes::Grouping.new(member_union.(user)), "member_projects"))
.select("member_projects.id", "member_projects.entity_id")
.to_sql
end
variants = {
"17.9 CTE" => ->(user) {
materialized_cte.(user, <<~SQL.squish)
SELECT work_packages.id FROM work_packages
JOIN project_member_projects ON project_member_projects.id = work_packages.project_id
UNION
SELECT work_packages.id FROM work_packages
JOIN entity_member_projects ON entity_member_projects.entity_id = work_packages.id
SQL
},
"UNION ALL" => ->(user) {
materialized_cte.(user, <<~SQL.squish)
SELECT work_packages.id FROM work_packages
WHERE work_packages.project_id IN (SELECT id FROM project_member_projects)
UNION ALL
SELECT work_packages.id FROM work_packages
WHERE work_packages.id IN (SELECT entity_id FROM entity_member_projects)
SQL
},
"plucked ids" => ->(user) {
project_rows, work_package_rows = conn.select_rows(pluck_sql.(user)).partition { |_, entity_id| entity_id.nil? }
project_ids = project_rows.map { it.first.to_i }.uniq
work_package_ids = work_package_rows.map { it.last.to_i }.uniq
WorkPackage.where(WorkPackage.arel_table[:project_id].in(project_ids).or(WorkPackage.arel_table[:id].in(work_package_ids)))
}
}
explain = ->(sql) do
result = JSON.parse(conn.select_value("EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) #{sql}")).first
{ exec: result["Execution Time"], plan: result["Planning Time"], temp: result["Plan"]["Temp Written Blocks"].to_i }
end
measure = ->(sql) do
explain.(sql)
samples = Array.new(runs) { explain.(sql) }
median = ->(key) { samples.map { it[key] }.sort[runs / 2] }
{ exec: median.(:exec), plan: median.(:plan), temp: samples.map { it[:temp] }.max }
end
measure_wall = ->(&block) do
clock = -> { Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond) }
timed = -> { started = clock.(); block.(); clock.() - started }
timed.()
Array.new(runs) { timed.() }.sort[runs / 2]
end
pick_users = -> do
base = User.active.where(admin: false)
{
"most memberships" => base.joins(:members).group("users.id").order(Arel.sql("COUNT(*) DESC")).limit(1).pluck("users.id").first,
"most unread notifications" => Notification.where(read_ian: false, recipient_id: base.select(:id))
.group(:recipient_id).order(Arel.sql("COUNT(*) DESC")).limit(1)
.pluck(:recipient_id).first,
"one private project" => base.joins(members: :project).where(members: { entity_type: nil }, projects: { public: false })
.group("users.id").having("COUNT(*) = 1").limit(1).pluck("users.id").first,
"no memberships" => base.where.not(id: Member.select(:user_id))
.order(Arel.sql("last_login_on DESC NULLS LAST")).limit(1).pluck(:id).first
}.compact
end
conn.uncached do ActiveRecord::Base.transaction do
conn.execute("SET TRANSACTION READ ONLY")
conn.execute("SET LOCAL statement_timeout = '60s'")
puts "OpenProject #{OpenProject::VERSION} | #{conn.select_value('SHOW server_version')} | " \
"work_mem #{conn.select_value('SHOW work_mem')} | #{WorkPackage.count} work packages | " \
"median of #{runs} runs, times in ms, temp = max temp blocks written, " \
"wall = scope build + SQL build + query + result loading in this console"
user_ids = defined?(VISIBILITY_BENCH_USER_IDS) ? VISIBILITY_BENCH_USER_IDS.index_by { "user #{it}" } : pick_users.()
user_ids.each do |label, user_id|
user = User.find(user_id)
pluck_cost = measure.(pluck_sql.(user))
member_rows = conn.select_rows(pluck_sql.(user))
project_rows, share_rows = member_rows.partition { |_, entity_id| entity_id.nil? }
plucked_ids = "#{project_rows.map(&:first).uniq.size} project ids, #{share_rows.map(&:last).uniq.size} shared ids"
scopes = variants.transform_values { it.(user) }
visible = scopes["plucked ids"]
visible_count = visible.count
sample_id = visible.order(:id).limit(1).pluck(:id).first
hidden_id = WorkPackage.where.not(id: visible.select(:id)).limit(1).pluck(:id).first
puts "\n### #{label}: user #{user.id}, sees #{visible_count} work packages, " \
"#{Notification.where(recipient_id: user.id, read_ian: false).count} unread notifications, " \
"pluck query #{format('%.2f', pluck_cost[:exec])} ms exec (added to 'plucked ids' below), " \
"#{format('%.2f', pluck_cost[:plan])} ms plan (not added), " \
"#{member_rows.size} member union rows, #{plucked_ids}"
puts format("%-26s %-12s %10s %8s %10s %6s %s", "query", "variant", "exec", "plan", "wall", "temp", "result")
shapes = {
"count" => ->(s) { s.select("COUNT(*)") },
"exists? visible id" => ->(s) { sample_id && s.where(id: sample_id).select("1 AS one").limit(1) },
"exists? hidden id" => ->(s) { hidden_id && s.where(id: hidden_id).select("1 AS one").limit(1) },
"list of 20 by updated_at" => ->(s) { s.select(:id).order(updated_at: :desc, id: :desc).limit(20) },
"unread notification count" => ->(s) {
Notification.where(recipient_id: user.id, read_ian: false, resource_type: "WorkPackage")
.where(resource_id: s.select(:id)).select("COUNT(notifications.id)")
}
}
shapes.each do |shape, build|
results = scopes.filter_map do |variant, scope|
relation = build.(scope)
next unless relation
sql = relation.to_sql
cost = measure.(sql)
cost[:exec] += pluck_cost[:exec] if variant == "plucked ids"
cost[:wall] = measure_wall.() { conn.select_values(build.(variants[variant].(user)).to_sql) }
[variant, cost, conn.select_values(sql)]
end
next if results.empty?
same = results.map(&:last).uniq.one? ? "same" : "DIFFERENT"
results.each do |variant, cost, _|
puts format("%-26s %-12s %10.2f %8.2f %10.2f %6d %s",
shape, variant, cost[:exec], cost[:plan], cost[:wall], cost[:temp], same)
end
end
end
raise ActiveRecord::Rollback
end end
end
nilcommunity (54,185 work packages, PostgreSQL 17.9, `work_mem` 4 MB)Most memberships (sees 43,769, 26 unread notifications, 279 member union rows):
Most unread notifications (sees 29,457, 89,932 unread notifications, 37 member union rows):
One private project (sees 29,465, 38 member union rows) and, in parentheses, no memberships
Most community projects are public, so every sampled user sees more than half of the work Largest SaaS instance (2,469,503 work packages, PostgreSQL 17.9, `work_mem` 4 MB)Broad user (sees 63,706, 47,857 unread notifications, 45 member union rows). This user had both
Narrow user (one private project, sees 432, no unread notifications, 1 member union row):
No memberships (sees 0, 0 member union rows): all variants and shapes took 0.9 to 1.0 ms exec Growing member union (local benchmark only)The production users above had at most 279 member union rows. How would this change when memberships increase for instances with huge number of projects? I ran a local benchmark with synthetic rows: 2 member roles per
Query wall includes planning. Execution stays small, because PostgreSQL hashes the constant
For comparison: a user with thousands of projects most likely sees a large part of all work The same ids as one array literal ( conn = ActiveRecord::Base.connection
clock = -> { Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond) }
median = ->(runs = 5, &block) { Array.new(runs) { started = clock.(); block.(); clock.() - started }.sort[runs / 2] }
explain = ->(sql) { JSON.parse(conn.select_value("EXPLAIN (ANALYZE, FORMAT JSON) #{sql}")).first }
conn.uncached do
[1_000, 10_000, 50_000].each do |memberships|
rows_sql = "SELECT g / 2 AS id, NULL::bigint AS entity_id FROM generate_series(1, #{memberships * 2}) g"
pluck = median.() { conn.select_rows(rows_sql) }
rows = conn.select_rows(rows_sql)
split = median.() { rows.partition { |_, entity_id| entity_id.nil? }.first.map { it.first.to_i }.uniq }
ids = rows.map { it.first.to_i }.uniq
literal_sql = nil
build = median.() do
literal_sql = WorkPackage.where(WorkPackage.arel_table[:project_id].in(ids).or(WorkPackage.arel_table[:id].in([])))
.select("COUNT(*)").to_sql
end
literal_wall = median.() { conn.select_value(literal_sql) }
literal_plan = explain.(literal_sql)
array_sql = "SELECT COUNT(*) FROM work_packages WHERE project_id = ANY('{#{ids.join(',')}}'::bigint[])"
array_plan = explain.(array_sql)
puts format("%6d memberships, %6d rows, %6d ids | pluck %6.2f ms, split %6.2f ms, to_sql %6.2f ms (%4d KB) | " \
"IN list: plan %6.2f ms, exec %6.2f ms, query wall %6.2f ms | one array literal: plan %6.2f ms, exec %6.2f ms",
memberships, rows.size, ids.size, pluck, split, build, literal_sql.bytesize / 1024,
literal_plan["Planning Time"], literal_plan["Execution Time"], literal_wall,
array_plan["Planning Time"], array_plan["Execution Time"])
end
end |
58d0d86 moved includes of BacklogBucket.for_project into Backlogs::BucketComponent. The spec kept passing only because the association proxy is lazy. Building the visible scope now plucks the visible project ids, which made the stale expectation fail.
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
|
Related #25207? |
ulferts
left a comment
ulferts
left a comment
There was a problem hiding this comment.
I think this is good. It is always hard to tell with these kind of improvements as there might be a combination of projects, members, permissions, .... out there where this performs worse than before. But at least on the data I have available it performed significantly better. So 🎉 . Well done, Oliver.
I have added one suggestion which might be worth comparing to the results in this PR.
| .pluck("member_projects.id", "member_projects.entity_id") | ||
| .partition { |_, entity_id| entity_id.nil? } | ||
|
|
||
| [project_rows.map(&:first).uniq, work_package_rows.map(&:last).uniq] |
There was a problem hiding this comment.
The pluck can be avoided:
def logged_in_non_admin_allowed_to(user, permissions)
member_projects = Arel::Table.new(:member_projects)
project_ids = member_projects.project(member_projects[:id]).where(member_projects[:entity_id].eq(nil))
work_package_ids = member_projects.project(member_projects[:entity_id])
with(member_projects: member_projects_relation(user, permissions))
.where(arel_table[:project_id].eq(any_of(project_ids)).or(arel_table[:id].eq(any_of(work_package_ids))))
end
def member_projects_relation(user, permissions)
union = Project
.unscoped
.allowed_to_member_union(user, permissions, entity_types: [nil, WorkPackage.name])
Project
.unscoped
.from(Arel::Nodes::TableAlias.new(Arel::Nodes::Grouping.new(union), "member_projects"))
.select(Arel.star)
end
def any_of(subquery)
Arel::Nodes::NamedFunction.new("ANY", [Arel::Nodes::NamedFunction.new("ARRAY", [subquery.ast])])
endAt least on my local machine, there was not that much difference. Sometimes, the pluck performed better, sometimes the ANY did. But a solution that avoids an extra round trip between application server and DB server could be faster on a production infrastructure.
There was a problem hiding this comment.
Still trying to take it all in, but my gut feeling also suggest that this kind of round trip should be avoided. It's at least worth a try to test against a realistic data set.
I added E to the benchmark script and ran that patch against community and the largest instance, Claude suggest to continue using D after all:
On community:
On the largest instance, for the broad user (sees 63,706):
community, ANY subquery (54,198 work packages, PostgreSQL 17.9, `work_mem` 4 MB)Factor wall A/D and A/E are the wall time of A divided by the wall time of D or E. Most memberships (sees 43,781, 33 unread notifications, 279 member union rows):
Most unread notifications (sees 29,467, 90,020 unread notifications, 37 member union rows):
One private project (sees 29,475, 38 member union rows) and, in parentheses, no memberships
Variant E as added to the benchmark script: "ANY subquery" => ->(user) {
member_projects = Arel::Table.new(:member_projects)
any_of = ->(subquery) { Arel::Nodes::NamedFunction.new("ANY", [Arel::Nodes::NamedFunction.new("ARRAY", [subquery.ast])]) }
relation = Project.unscoped
.from(Arel::Nodes::TableAlias.new(Arel::Nodes::Grouping.new(member_union.(user)), "member_projects"))
.select(Arel.star)
project_ids = member_projects.project(member_projects[:id]).where(member_projects[:entity_id].eq(nil))
work_package_ids = member_projects.project(member_projects[:entity_id])
WorkPackage
.with(member_projects: relation)
.where(WorkPackage.arel_table[:project_id].eq(any_of.(project_ids))
.or(WorkPackage.arel_table[:id].eq(any_of.(work_package_ids))))
}Largest SaaS instance, ANY subquery (2,470,642 work packages, PostgreSQL 17.9, `work_mem` 4 MB)Factor wall A/D and A/E are the wall time of A divided by the wall time of D or E. Broad user (sees 63,706, 47,857 unread notifications, 45 member union rows). This user had both the most
Narrow user (one private project, sees 432, no unread notifications, 1 member union row):
No memberships (sees 0, 0 member union rows):
|
|
Warning This pull request does not link an OpenProject work package. Please add a link to the work package in the description, or reference it in the |
|
Nevermind. This is already how it's done in your suggestion. It would be really great if the query planner of Postgres could be nudged into the right directions as one can do with MySQL/MariaDB. I hate that a round trip like here is even possible to be a faster option |
ulferts
left a comment
ulferts
left a comment
There was a problem hiding this comment.
Thanks for rechecking the alternative (ANY) approach @oliverguenther .
I validated your results against what I have locally and agree that pluck performs better. It also performs better than the other solutions I threw at it in frustration. So let's use pluck for the time being even when it is the least elegant solution of them all.
The only thing I would like added before merging would be a comment that using pluck is no mistake.
|
Thanks @ulferts , will add the comment. I find it hard to reason with as well. I would suggest we backport it to 17.9. so we can test it on community to notice issues before the big changes of 18.0 hit I merged the PR manually with the commit, so that the intermediate commit can stay on this PR. |
Following a demo extraction of pg_stat_statements on the community shard to make performance analysis easier to export later on, I noticed that
WITH "member_projects" AS ...made up a lot of the statements records. I let claude analyze the allowed_to CTE on the public anonymized community dump.On that dump, queries containing this CTE (visibility checks and unread notification counts) use about 50 % of the database time and write most of the temp files.
It suggests to use IN in both branches, so that each branch returns every work package at most once, and combine them with UNION ALL. Only work packages that are both in a member project and shared directly with the user can now appear twice, which the EXISTS check ignores. The CTE stays MATERIALIZED, so the LIMIT 1 planning issue it works around is not affected.
On the local dump, for a user with 263 member project rows, the CTE input drops from 22,617 rows into a HashAggregate to 10,387 rows with no aggregation. The result set is identical.
Median query times on the anonymized community dump over six runs:
Ticket
None
AI involvement
Directed – I specified the requirements and AI implemented most of it; I validated via testing rather than a full line-by-line review.
Merge checklist