Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improve performance of relations visible scope #17755

Open
wants to merge 1 commit into
base: release/15.3
Choose a base branch
from
Open

improve performance of relations visible scope #17755

wants to merge 1 commit into from
+79 −10

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented Jan 28, 2025 •
edited
Loading

Ticket

https://community.openproject.org/wp/61058

What are you trying to accomplish?

When calling some_work_package.relations.visible or its alias some_work_package.visible_relations, the following SQL is executed for a non admin user.

SELECT
	"relations".*
FROM
	"relations"
WHERE
	(
		"relations"."from_id" = 1030549
		OR "relations"."to_id" = 1030549
	)
	AND "relations"."from_id" IN (
		WITH
			"allowed_work_packages" AS (
				SELECT
					"work_packages"."id"
				FROM
					"members"
					INNER JOIN "work_packages" ON "members"."entity_id" = "work_packages"."id"
					INNER JOIN "projects" ON "projects"."active" = TRUE
					AND "projects"."id" = "work_packages"."project_id"
					INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
					AND "enabled_modules"."name" IN ('work_package_tracking')
					AND "projects"."active" = TRUE
					INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
					INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
					INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
					AND (
						FALSE
						OR "role_permissions"."permission" = 'view_work_packages'
						AND "enabled_modules"."name" = 'work_package_tracking'
					)
				WHERE
					"members"."user_id" = 33612
					AND "members"."entity_type" = 'WorkPackage'
			),
			"allowed_projects" AS (
				SELECT
					"projects"."id"
				FROM
					"projects"
				WHERE
					"projects"."id" IN (
						(
							(
								SELECT
									"projects"."id"
								FROM
									"members"
									INNER JOIN "projects" ON "projects"."active" = TRUE
									AND "members"."project_id" = "projects"."id"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
									INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
								WHERE
									"members"."user_id" = 33612
									AND "members"."entity_id" IS NULL
									AND "members"."entity_type" IS NULL
							)
							UNION ALL
							(
								SELECT
									"projects"."id"
								FROM
									"projects"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "roles" ON "roles"."builtin" = 1
									AND "projects"."active"
									AND "projects"."public"
									AND NOT (
										EXISTS (
											SELECT
												1
											FROM
												"members"
											WHERE
												"members"."user_id" = 33612
												AND "members"."entity_id" IS NULL
												AND "members"."entity_type" IS NULL
												AND "members"."project_id" = "projects"."id"
										)
									)
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
							)
						)
					)
			)
		SELECT
			"work_packages"."id"
		FROM
			"work_packages"
		WHERE
			(
				WORK_PACKAGES.PROJECT_ID IN (
					SELECT
						ID
					FROM
						ALLOWED_PROJECTS
				)
				OR WORK_PACKAGES.ID IN (
					SELECT
						ID
					FROM
						ALLOWED_WORK_PACKAGES
				)
			)
	)
	AND "relations"."to_id" IN (
		WITH
			"allowed_work_packages" AS (
				SELECT
					"work_packages"."id"
				FROM
					"members"
					INNER JOIN "work_packages" ON "members"."entity_id" = "work_packages"."id"
					INNER JOIN "projects" ON "projects"."active" = TRUE
					AND "projects"."id" = "work_packages"."project_id"
					INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
					AND "enabled_modules"."name" IN ('work_package_tracking')
					AND "projects"."active" = TRUE
					INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
					INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
					INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
					AND (
						FALSE
						OR "role_permissions"."permission" = 'view_work_packages'
						AND "enabled_modules"."name" = 'work_package_tracking'
					)
				WHERE
					"members"."user_id" = 33612
					AND "members"."entity_type" = 'WorkPackage'
			),
			"allowed_projects" AS (
				SELECT
					"projects"."id"
				FROM
					"projects"
				WHERE
					"projects"."id" IN (
						(
							(
								SELECT
									"projects"."id"
								FROM
									"members"
									INNER JOIN "projects" ON "projects"."active" = TRUE
									AND "members"."project_id" = "projects"."id"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
									INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
								WHERE
									"members"."user_id" = 33612
									AND "members"."entity_id" IS NULL
									AND "members"."entity_type" IS NULL
							)
							UNION ALL
							(
								SELECT
									"projects"."id"
								FROM
									"projects"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "roles" ON "roles"."builtin" = 1
									AND "projects"."active"
									AND "projects"."public"
									AND NOT (
										EXISTS (
											SELECT
												1
											FROM
												"members"
											WHERE
												"members"."user_id" = 33612
												AND "members"."entity_id" IS NULL
												AND "members"."entity_type" IS NULL
												AND "members"."project_id" = "projects"."id"
										)
									)
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
							)
						)
					)
			)
		SELECT
			"work_packages"."id"
		FROM
			"work_packages"
		WHERE
			(
				WORK_PACKAGES.PROJECT_ID IN (
					SELECT
						ID
					FROM
						ALLOWED_PROJECTS
				)
				OR WORK_PACKAGES.ID IN (
					SELECT
						ID
					FROM
						ALLOWED_WORK_PACKAGES
				)
			)
	)

This sometimes performs poorly in cases where there are a lot of work packages. The exact trigger for when Postgres starts to perform poorly is unknown. But EXPLAIN shows why the performance is poor:

image

The planner is way off estimating the number of work packages that would be returned as visible ones and chooses a full table scan. And it does so twice within the same query as the visible scope is applied two times.

Some API responses like GET api/v3/work_packages/:id and PATCH api/v3/work_packages/:id the visible relations are queried twice when rendering the response as the relations are embedded in the work package resource. This can lead to poor performance on those requests.

Without checking for confirmation, the code indicates that the reworked relations tab fetches the visible scope multiple times as well.

What approach did you choose and why?

The problem arrises when some_work_package.relations.visible is concatenated. While it leads to the desired results, it does not make use of the fact that the number of relations directly connected to a work package will likely be small. For most work packages the number will be below 10 and even more connected work packages will not have thousands of relations. Using this, the already existing visible_relations can be employed to combine the visible scope with a subselect so that it focuses on work packages that are related to the work package the relations are searched for. Additionally, instead of providing the visible scope as subselects twice, the visible scope is provided once as a CTE which the from_id/to_id condition both reference.

This results in the following SQL:

WITH
	"visible_work_packages" AS (
		WITH
			"allowed_work_packages" AS (
				SELECT
					"work_packages"."id"
				FROM
					"members"
					INNER JOIN "work_packages" ON "members"."entity_id" = "work_packages"."id"
					INNER JOIN "projects" ON "projects"."active" = TRUE
					AND "projects"."id" = "work_packages"."project_id"
					INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
					AND "enabled_modules"."name" IN ('work_package_tracking')
					AND "projects"."active" = TRUE
					INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
					INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
					INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
					AND (
						FALSE
						OR "role_permissions"."permission" = 'view_work_packages'
						AND "enabled_modules"."name" = 'work_package_tracking'
					)
				WHERE
					"members"."user_id" = 33612
					AND "members"."entity_type" = 'WorkPackage'
			),
			"allowed_projects" AS (
				SELECT
					"projects"."id"
				FROM
					"projects"
				WHERE
					"projects"."id" IN (
						(
							(
								SELECT
									"projects"."id"
								FROM
									"members"
									INNER JOIN "projects" ON "projects"."active" = TRUE
									AND "members"."project_id" = "projects"."id"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "member_roles" ON "member_roles"."member_id" = "members"."id"
									INNER JOIN "roles" ON "roles"."id" = "member_roles"."role_id"
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
								WHERE
									"members"."user_id" = 33612
									AND "members"."entity_id" IS NULL
									AND "members"."entity_type" IS NULL
							)
							UNION ALL
							(
								SELECT
									"projects"."id"
								FROM
									"projects"
									INNER JOIN "enabled_modules" ON "projects"."id" = "enabled_modules"."project_id"
									AND "enabled_modules"."name" IN ('work_package_tracking')
									AND "projects"."active" = TRUE
									INNER JOIN "roles" ON "roles"."builtin" = 1
									AND "projects"."active"
									AND "projects"."public"
									AND NOT (
										EXISTS (
											SELECT
												1
											FROM
												"members"
											WHERE
												"members"."user_id" = 33612
												AND "members"."entity_id" IS NULL
												AND "members"."entity_type" IS NULL
												AND "members"."project_id" = "projects"."id"
										)
									)
									INNER JOIN "role_permissions" ON "roles"."id" = "role_permissions"."role_id"
									AND (
										FALSE
										OR "role_permissions"."permission" = 'view_work_packages'
										AND "enabled_modules"."name" = 'work_package_tracking'
									)
							)
						)
					)
			)
		SELECT
			"work_packages".*
		FROM
			"work_packages"
		WHERE
			(
				WORK_PACKAGES.PROJECT_ID IN (
					SELECT
						ID
					FROM
						ALLOWED_PROJECTS
				)
				OR WORK_PACKAGES.ID IN (
					SELECT
						ID
					FROM
						ALLOWED_WORK_PACKAGES
				)
			)
			AND "work_packages"."id" IN (
				(
					(
						SELECT
							CASE
								WHEN FROM_ID = 1030549 THEN TO_ID
								ELSE FROM_ID
							END ID
						FROM
							"relations"
						WHERE
							(
								RELATIONS.FROM_ID = 1030549
								OR RELATIONS.TO_ID = 1030549
							)
					)
					UNION ALL
					SELECT
						1030549 AS ID
				)
			)
	)
SELECT
	"relations".*
FROM
	"relations"
WHERE
	(
		"relations"."from_id" = 1030549
		OR "relations"."to_id" = 1030549
	)
	AND "relations"."from_id" IN (
		SELECT
			"work_packages"."id"
		FROM
			VISIBLE_WORK_PACKAGES WORK_PACKAGES
	)
	AND "relations"."to_id" IN (
		SELECT
			"work_packages"."id"
		FROM
			VISIBLE_WORK_PACKAGES WORK_PACKAGES
	)

The EXPLAIN outputs show that the planned rows are closer to the actual ones and that indices are used now:

image

It is unfortunate, that this requires to always use visible_relations when doing relations.visible is the code most developers would be using naturally. I did not find a better way here.

The performance is also stable when querying like it is done in the relations tab by now e.g.:

work_package.relations.includes(:to, :from).where.not(id: visible_relations.select(:id))

@ulferts ulferts force-pushed the fix/improve_performance_of_relations_visible_scope branch 4 times, most recently from 7dcb115 to 3adac9b Compare February 5, 2025 16:39
@ulferts ulferts force-pushed the fix/improve_performance_of_relations_visible_scope branch from 3adac9b to 47565f2 Compare February 6, 2025 16:47
@ulferts ulferts changed the base branch from release/15.2 to release/15.3 February 6, 2025 16:47
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2025

1 Warning
⚠️ This PR has migration-related changes on a release branch. Ping @opf/operations

Generated by 🚫 Danger

@ulferts
Copy link
Contributor Author

ulferts commented Feb 6, 2025

The ping was sent out before I could change the branch and is a false positive. Sorry for this.

@ulferts ulferts marked this pull request as ready for review February 7, 2025 10:27
toy
toy reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@toy toy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think association extensions can be used to resolve the problem of requiring to use visible_relations instead of relations.visible. proxy_association.owner and proxy_association.target should give access to the work_package or scoped relation query

app/models/work_packages/relations.rb
END id
SQL
)
.where("relations.from_id = :id OR relations.to_id = :id", id: id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relation. … .where("relations.from_id = :id OR relations.to_id = :id", id: id) can be replaced with just relations

app/models/work_packages/relations.rb
Comment on lines +97 to +112
relation_from_or_to = Relation
.select(
<<~SQL.squish
CASE
WHEN from_id = #{id}
THEN to_id
ELSE from_id
END id
SQL
)
.where("relations.from_id = :id OR relations.to_id = :id", id: id)
.arel

self_id = Arel.sql("SELECT #{id} AS id")

wp_focus_scope = Arel::Nodes::UnionAll.new(relation_from_or_to, self_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

wp_focus_scope = Arel::Nodes::Union.new(relations.select("from_id as id").arel, relations.select("to_id as id").arel)

app/models/relations/scopes/visible.rb
Comment on lines +48 to +51
wp_arel = work_package_focus_scope.respond_to?(:arel) ? work_package_focus_scope.arel : work_package_focus_scope
visible_work_packages = WorkPackage.visible(user)

visible_work_packages = visible_work_packages.where(WorkPackage.arel_table[:id].in(wp_arel)) if wp_arel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wp_arel = work_package_focus_scope.respond_to?(:arel) ? work_package_focus_scope.arel : work_package_focus_scope
visible_work_packages = WorkPackage.visible(user)
visible_work_packages = visible_work_packages.where(WorkPackage.arel_table[:id].in(wp_arel)) if wp_arel
visible_work_packages = WorkPackage.visible(user)
if work_package_focus_scope
wp_arel = work_package_focus_scope.respond_to?(:arel) ? work_package_focus_scope.arel : work_package_focus_scope
visible_work_packages = visible_work_packages.where(WorkPackage.arel_table[:id].in(wp_arel))
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toy toy toy left review comments

Assignees
No one assigned
Labels
needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ulferts @toy