Redmine

Sorry for syntax error in my code, I cannot edit it.

Just an info to execution time: if I change this line above back to the old version, database query needs 3 seconds. With the new security fix same query needs 59 seconds.

I found the problem: the created subquery with Issue.visible_condition(user) needs a relation to the main query, which is missing right know. We need main_query_issue.id = subquery_issue.id in subquery. This would fix the problem.
I am not sure, how this could be solved with Issue.visible_condition abstraction.

Hello,

holger mareck: thanks for fixing my broken syntax

More details to the problem: it appears, if a custom field is used as a filter.
We have a custom field with format "list" and used this filter with one selected value (operator => "="). If this filter is used, most of the time the database query needs forever (at least some minutes).

This is the created sql query:

SELECT COUNT(*)
FROM "issues" 
INNER JOIN "projects" ON "projects"."id" = "issues"."project_id" 
INNER JOIN "issue_statuses" ON "issue_statuses"."id" = "issues"."status_id" 
WHERE (PROJECTS.STATUS <> 9
                            AND EXISTS
                                (SELECT 1 AS ONE
                                    FROM ENABLED_MODULES EM
                                    WHERE EM.PROJECT_ID = PROJECTS.ID
                                        AND EM.NAME = 'issue_tracking'))
    AND ((ISSUES.STATUS_ID IN
                                (SELECT ID
                                    FROM ISSUE_STATUSES
                                    WHERE IS_CLOSED = FALSE))
                        AND ISSUES.ID IN
                            (SELECT ISSUES.ID
                                FROM ISSUES
                                LEFT OUTER JOIN CUSTOM_VALUES ON CUSTOM_VALUES.CUSTOMIZED_TYPE = 'Issue'
                                AND CUSTOM_VALUES.CUSTOMIZED_ID = ISSUES.ID
                                AND CUSTOM_VALUES.CUSTOM_FIELD_ID = 61
                                WHERE (CUSTOM_VALUES.VALUE IN ('Alignment'))
                                    AND (((1 = 1)
                                                            AND (ISSUES.TRACKER_ID IN
                                                                                    (SELECT TRACKER_ID
                                                                                        FROM CUSTOM_FIELDS_TRACKERS
                                                                                        WHERE CUSTOM_FIELD_ID = 61))
                                                            AND (EXISTS
                                                                                    (SELECT 1
                                                                                        FROM CUSTOM_FIELDS IFA
                                                                                        WHERE IFA.IS_FOR_ALL = TRUE
                                                                                            AND IFA.ID = 61)
                                                                                OR ISSUES.PROJECT_ID IN
                                                                                    (SELECT PROJECT_ID
                                                                                        FROM CUSTOM_FIELDS_PROJECTS
                                                                                        WHERE CUSTOM_FIELD_ID = 61))
                                                            AND (PROJECTS.STATUS <> 9
                                                                                AND EXISTS
                                                                                    (SELECT 1 AS ONE
                                                                                        FROM ENABLED_MODULES EM
                                                                                        WHERE EM.PROJECT_ID = PROJECTS.ID
                                                                                            AND EM.NAME = 'issue_tracking'))))))

and this is a test to fix it manually by sql (with "AND main_query.ID = ISSUES.ID"):

SELECT COUNT(*)
FROM "issues" AS main_query
INNER JOIN "projects" ON "projects"."id" = main_query."project_id" 
INNER JOIN "issue_statuses" ON "issue_statuses"."id" = main_query."status_id" 
WHERE (PROJECTS.STATUS <> 9
                            AND EXISTS
                                (SELECT 1 AS ONE
                                    FROM ENABLED_MODULES EM
                                    WHERE EM.PROJECT_ID = PROJECTS.ID
                                        AND EM.NAME = 'issue_tracking'))
    AND ((main_query.STATUS_ID IN
                                (SELECT ID
                                    FROM ISSUE_STATUSES
                                    WHERE IS_CLOSED = FALSE))
                        AND main_query.ID IN
                            (SELECT ISSUES.ID
                                FROM ISSUES
                                LEFT OUTER JOIN CUSTOM_VALUES ON CUSTOM_VALUES.CUSTOMIZED_TYPE = 'Issue'
                                AND main_query.ID = ISSUES.ID
                                AND CUSTOM_VALUES.CUSTOMIZED_ID = ISSUES.ID
                                AND CUSTOM_VALUES.CUSTOM_FIELD_ID = 61
                                WHERE (CUSTOM_VALUES.VALUE IN ('Alignment'))
                                    AND (((1 = 1)
                                                            AND (ISSUES.TRACKER_ID IN
                                                                                    (SELECT TRACKER_ID
                                                                                        FROM CUSTOM_FIELDS_TRACKERS
                                                                                        WHERE CUSTOM_FIELD_ID = 61))
                                                            AND (EXISTS
                                                                                    (SELECT 1
                                                                                        FROM CUSTOM_FIELDS IFA
                                                                                        WHERE IFA.IS_FOR_ALL = TRUE
                                                                                            AND IFA.ID = 61)
                                                                                OR ISSUES.PROJECT_ID IN
                                                                                    (SELECT PROJECT_ID
                                                                                        FROM CUSTOM_FIELDS_PROJECTS
                                                                                        WHERE CUSTOM_FIELD_ID = 61))
                                                            AND (PROJECTS.STATUS <> 9
                                                                                AND EXISTS
                                                                                    (SELECT 1 AS ONE
                                                                                        FROM ENABLED_MODULES EM
                                                                                        WHERE EM.PROJECT_ID = PROJECTS.ID
                                                                                            AND EM.NAME = 'issue_tracking'))))))

We also got a serious performance issue after upgrade from 4.2.6 to 4.2.7 as well.

The same change has been applied to 4.2.7.
https://github.com/redmine/redmine/blob/4.2.7/app/models/issue_custom_field.rb#L42

Thank you very much for your time for this issue.

Simon, which database type (and version) are you using?

The fix was security related and should not be reverted, lest we re-introduce the underlying security issue. However, we may want to fix the performance issue.

Marius: Could you please confirm which database you observed the issue on? Could you please also show the emitted SQL queries for both versions, in addition to an EXPLAIN of both queries?

Holger Just wrote:

The fix was security related and should not be reverted, lest we re-introduce the underlying security issue. However, we may want to fix the performance issue.

Marius: Could you please confirm which database you observed the issue on? Could you please also show the emitted SQL queries for both versions, in addition to an EXPLAIN of both queries?

I've tested with postgres:14-alpine, I'll be back with the explains and the scripts used to generate the necessary fixtures.

Holger, Felix, are you working on a fix for this? Or should I start doing it?

I have started working my way into the explains, unfortunately I think I will have to tinker around with it more as well as looking at the explains with the proposed patch to understand what difference it makes for the optimiser. I will only be back at my main computer with enough time to look into it by the weekend probably though.

Some of my issue queries takes thousands of seconds!

Any update?!

I am sorry for the delay, I was ill with COVID-19.

The patch proposed in #37268#note-8 sadly does not work in MySQL.

Query::StatementInvalid: Mysql2::Error: Unknown column 'issues.id' in 'on clause'

The following added line causes this error " AND #{queried_table_name}.id=cc_sub.id ". In the case of issues this line leads to issues.id=cc_sub.id where cc_sub is also issues. I think the result of this query is not correct.

From what I gathered the additional visible condition trips the query optimiser for PostgreSQL and leads to a much more costly merge mechanism.

I am not sure how to alleviate this issue for PostgreSQL. As MySQL does not have this problem I fear this is a PostgreSQL-specific problem that would require PostgreSQL knowledge to solve. I will however continue researching this issue and explore the other proposed options such as #37565.

Additionally joining on projects for Query#sql_for_custom_field will solve the performance issue. This is only required for IssueCustomField. The following patch implements this solution:

diff --git a/app/models/query.rb b/app/models/query.rb
index 1a614f1753..db977859f8 100644
--- a/app/models/query.rb
+++ b/app/models/query.rb
@@ -1159,11 +1159,15 @@ class Query < ActiveRecord::Base
     if /[<>]/.match?(operator)
       where = "(#{where}) AND #{db_table}.#{db_field} <> ''" 
     end
+
+    additional_joins = customized_class == Issue ? "LEFT OUTER JOIN #{Project.table_name} ON #{customized_class.table_name}.project_id = #{Project.table_name}.id" : "" 
+
     "#{queried_table_name}.#{customized_key} #{not_in} IN (" \
       "SELECT #{customized_class.table_name}.id FROM #{customized_class.table_name}" \
       " LEFT OUTER JOIN #{db_table} ON #{db_table}.customized_type='#{customized_class}'" \
       " AND #{db_table}.customized_id=#{customized_class.table_name}.id" \
       " AND #{db_table}.custom_field_id=#{custom_field_id}" \
+      " #{additional_joins}" \
       " WHERE (#{where}) AND (#{filter[:field].visibility_by_project_condition}))" 
   end

This should probably be added through a IssueCustomField#visibility_by_project_condition_joins method.

• Twitter post: https://twitter.com/nalsh/status/1561995447748947968
• GitHub commit: https://github.com/ruby/b.r-l.o/commit/91e830ef03d9290465d40eb2437308f9b33dd1cf
  

  diff --git a/app/models/query.rb b/app/models/query.rb
  index 1a614f1753..e098a7eb46 100644
  --- a/app/models/query.rb
  +++ b/app/models/query.rb
  @@ -1159,12 +1159,13 @@ def sql_for_custom_field(field, operator, value, custom_field_id)
       if /[<>]/.match?(operator)
         where = "(#{where}) AND #{db_table}.#{db_field} <> ''" 
       end
  -    "#{queried_table_name}.#{customized_key} #{not_in} IN (" \
  -      "SELECT #{customized_class.table_name}.id FROM #{customized_class.table_name}" \
  +    "#{not_in} EXISTS (" \
  +      "SELECT * FROM #{customized_class.table_name} i" \
         " LEFT OUTER JOIN #{db_table} ON #{db_table}.customized_type='#{customized_class}'" \
         " AND #{db_table}.customized_id=#{customized_class.table_name}.id" \
         " AND #{db_table}.custom_field_id=#{custom_field_id}" \
  -      " WHERE (#{where}) AND (#{filter[:field].visibility_by_project_condition}))" 
  +      " WHERE #{queried_table_name}.#{customized_key} = i.id AND " \
  +      "  (#{where}) AND (#{filter[:field].visibility_by_project_condition}))" 
     end
  
     def sql_for_chained_custom_field(field, operator, value, custom_field_id, chained_custom_field_id)