From 0ee5a18512cc63128ba383e358e7c4f39d767e5a Mon Sep 17 00:00:00 2001 From: Adam Hegyi Date: Thu, 26 Oct 2023 07:31:49 +0200 Subject: [PATCH 1/7] Use SQL functions for primary key lookups This MR adds a feature flag to control whether to use SQL functions when looking up users, projects, namespaces via their primary key. Changelog: added --- ...se_sql_function_for_primary_key_lookups.rb | 39 ++ app/models/namespace.rb | 1 + app/models/project.rb | 1 + app/models/user.rb | 1 + ...red_statements_for_primary_key_lookups.yml | 8 + ...54_add_functions_for_primary_key_lookup.rb | 26 + db/schema_migrations/20231026050554 | 1 + db/structure.sql | 460 +++++++++--------- ...l_function_for_primary_key_lookups_spec.rb | 90 ++++ 9 files changed, 409 insertions(+), 218 deletions(-) create mode 100644 app/models/concerns/use_sql_function_for_primary_key_lookups.rb create mode 100644 config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml create mode 100644 db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb create mode 100644 db/schema_migrations/20231026050554 create mode 100644 spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb diff --git a/app/models/concerns/use_sql_function_for_primary_key_lookups.rb b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb new file mode 100644 index 00000000000000..8ab64133856c4b --- /dev/null +++ b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module UseSqlFunctionForPrimaryKeyLookups + extend ActiveSupport::Concern + + class_methods do + def find(*args) + return super unless Feature.enabled?(:use_prepared_statements_for_primary_key_lookups) + return super unless args.one? + return super if block_given? || primary_key.nil? || scope_attributes? + + return_array = false + id = args.first + + if id.is_a?(Array) + return super if id.many? + + return_array = true + + id = id.first + end + + return super if id.nil? || (id.is_a?(String) && !id.number?) + + from_clause = "find_#{table_name}_by_id(?) #{quoted_table_name}" + filter_empty_row = "#{quoted_table_name}.#{connection.quote_column_name(primary_key)} IS NOT NULL" + query = from(from_clause).where(filter_empty_row).limit(1).to_sql + # Using find_by_sql so we get query cache working + record = find_by_sql([query, id]).first + + unless record + message = "Couldn't find #{name} with '#{primary_key}'=#{id}" + raise(ActiveRecord::RecordNotFound.new(message, name, primary_key, id)) + end + + return_array ? [record] : record + end + end +end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 733b89fcaf254d..6414d6a0081c63 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -18,6 +18,7 @@ class Namespace < ApplicationRecord include Referable include CrossDatabaseIgnoredTables include IgnorableColumns + include UseSqlFunctionForPrimaryKeyLookups ignore_column :unlock_membership_to_ldap, remove_with: '16.7', remove_after: '2023-11-16' diff --git a/app/models/project.rb b/app/models/project.rb index f7e994d4beb5f9..72d66169798041 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -45,6 +45,7 @@ class Project < ApplicationRecord include UpdatedAtFilterable include IgnorableColumns include CrossDatabaseIgnoredTables + include UseSqlFunctionForPrimaryKeyLookups ignore_column :emails_disabled, remove_with: '16.3', remove_after: '2023-08-22' diff --git a/app/models/user.rb b/app/models/user.rb index fdc0b531521bc1..3b8588dae32906 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -32,6 +32,7 @@ class User < MainClusterwide::ApplicationRecord include EachBatch include CrossDatabaseIgnoredTables include IgnorableColumns + include UseSqlFunctionForPrimaryKeyLookups ignore_column %i[ email_opted_in diff --git a/config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml b/config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml new file mode 100644 index 00000000000000..e5c9d9f073abba --- /dev/null +++ b/config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml @@ -0,0 +1,8 @@ +--- +name: use_prepared_statements_for_primary_key_lookups +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/135196 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429479 +milestone: '16.6' +type: development +group: group::optimize +default_enabled: false diff --git a/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb b/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb new file mode 100644 index 00000000000000..396bca7c71b107 --- /dev/null +++ b/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class AddFunctionsForPrimaryKeyLookup < Gitlab::Database::Migration[2.2] + milestone '16.6' + + TABLES = %i[users namespaces projects].freeze + + def up + TABLES.each do |table| + execute <<~SQL + CREATE OR REPLACE FUNCTION find_#{table}_by_id(#{table}_id int) + RETURNS #{table} AS $$ + BEGIN + return (SELECT #{table} FROM #{table} WHERE id = #{table}_id LIMIT 1); + END; + $$ LANGUAGE plpgsql STABLE PARALLEL SAFE COST 1; + SQL + end + end + + def down + TABLES.each do |table| + execute "DROP FUNCTION IF EXISTS find_#{table}_by_id" + end + end +end diff --git a/db/schema_migrations/20231026050554 b/db/schema_migrations/20231026050554 new file mode 100644 index 00000000000000..d99dc675ab6c3b --- /dev/null +++ b/db/schema_migrations/20231026050554 @@ -0,0 +1 @@ +e71f80b77121722c75125e59ec2e9c3df323b34a107304447948bed05804224c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 043dd201d5a17c..652f2535f26e00 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -35,6 +35,248 @@ RETURN NULL; END $$; +CREATE TABLE namespaces ( + id integer NOT NULL, + name character varying NOT NULL, + path character varying NOT NULL, + owner_id integer, + created_at timestamp without time zone, + updated_at timestamp without time zone, + type character varying DEFAULT 'User'::character varying NOT NULL, + description character varying DEFAULT ''::character varying NOT NULL, + avatar character varying, + membership_lock boolean DEFAULT false, + share_with_group_lock boolean DEFAULT false, + visibility_level integer DEFAULT 20 NOT NULL, + request_access_enabled boolean DEFAULT true NOT NULL, + ldap_sync_status character varying DEFAULT 'ready'::character varying NOT NULL, + ldap_sync_error character varying, + ldap_sync_last_update_at timestamp without time zone, + ldap_sync_last_successful_update_at timestamp without time zone, + ldap_sync_last_sync_at timestamp without time zone, + description_html text, + lfs_enabled boolean, + parent_id integer, + shared_runners_minutes_limit integer, + repository_size_limit bigint, + require_two_factor_authentication boolean DEFAULT false NOT NULL, + two_factor_grace_period integer DEFAULT 48 NOT NULL, + cached_markdown_version integer, + project_creation_level integer, + runners_token character varying, + file_template_project_id integer, + saml_discovery_token character varying, + runners_token_encrypted character varying, + custom_project_templates_group_id integer, + auto_devops_enabled boolean, + extra_shared_runners_minutes_limit integer, + last_ci_minutes_notification_at timestamp with time zone, + last_ci_minutes_usage_notification_level integer, + subgroup_creation_level integer DEFAULT 1, + emails_disabled boolean, + max_pages_size integer, + max_artifacts_size integer, + mentions_disabled boolean, + default_branch_protection smallint, + unlock_membership_to_ldap boolean, + max_personal_access_token_lifetime integer, + push_rule_id bigint, + shared_runners_enabled boolean DEFAULT true NOT NULL, + allow_descendants_override_disabled_shared_runners boolean DEFAULT false NOT NULL, + traversal_ids integer[] DEFAULT '{}'::integer[] NOT NULL, + organization_id bigint DEFAULT 1 +); + +CREATE FUNCTION find_namespaces_by_id(namespaces_id integer) RETURNS namespaces + LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE + AS $$ +BEGIN + return (SELECT namespaces FROM namespaces WHERE id = namespaces_id LIMIT 1); +END; +$$; + +CREATE TABLE projects ( + id integer NOT NULL, + name character varying, + path character varying, + description text, + created_at timestamp without time zone, + updated_at timestamp without time zone, + creator_id integer, + namespace_id integer NOT NULL, + last_activity_at timestamp without time zone, + import_url character varying, + visibility_level integer DEFAULT 0 NOT NULL, + archived boolean DEFAULT false NOT NULL, + avatar character varying, + merge_requests_template text, + star_count integer DEFAULT 0 NOT NULL, + merge_requests_rebase_enabled boolean DEFAULT false, + import_type character varying, + import_source character varying, + approvals_before_merge integer DEFAULT 0 NOT NULL, + reset_approvals_on_push boolean DEFAULT true, + merge_requests_ff_only_enabled boolean DEFAULT false, + issues_template text, + mirror boolean DEFAULT false NOT NULL, + mirror_last_update_at timestamp without time zone, + mirror_last_successful_update_at timestamp without time zone, + mirror_user_id integer, + shared_runners_enabled boolean DEFAULT true NOT NULL, + runners_token character varying, + build_allow_git_fetch boolean DEFAULT true NOT NULL, + build_timeout integer DEFAULT 3600 NOT NULL, + mirror_trigger_builds boolean DEFAULT false NOT NULL, + pending_delete boolean DEFAULT false, + public_builds boolean DEFAULT true NOT NULL, + last_repository_check_failed boolean, + last_repository_check_at timestamp without time zone, + only_allow_merge_if_pipeline_succeeds boolean DEFAULT false NOT NULL, + has_external_issue_tracker boolean, + repository_storage character varying DEFAULT 'default'::character varying NOT NULL, + repository_read_only boolean, + request_access_enabled boolean DEFAULT true NOT NULL, + has_external_wiki boolean, + ci_config_path character varying, + lfs_enabled boolean, + description_html text, + only_allow_merge_if_all_discussions_are_resolved boolean, + repository_size_limit bigint, + printing_merge_request_link_enabled boolean DEFAULT true NOT NULL, + auto_cancel_pending_pipelines integer DEFAULT 1 NOT NULL, + service_desk_enabled boolean DEFAULT true, + cached_markdown_version integer, + delete_error text, + last_repository_updated_at timestamp without time zone, + disable_overriding_approvers_per_merge_request boolean, + storage_version smallint, + resolve_outdated_diff_discussions boolean, + remote_mirror_available_overridden boolean, + only_mirror_protected_branches boolean, + pull_mirror_available_overridden boolean, + jobs_cache_index integer, + external_authorization_classification_label character varying, + mirror_overwrites_diverged_branches boolean, + pages_https_only boolean DEFAULT true, + external_webhook_token character varying, + packages_enabled boolean, + merge_requests_author_approval boolean DEFAULT false, + pool_repository_id bigint, + runners_token_encrypted character varying, + bfg_object_map character varying, + detected_repository_languages boolean, + merge_requests_disable_committers_approval boolean, + require_password_to_approve boolean, + emails_disabled boolean, + max_pages_size integer, + max_artifacts_size integer, + pull_mirror_branch_prefix character varying(50), + remove_source_branch_after_merge boolean, + marked_for_deletion_at date, + marked_for_deletion_by_user_id integer, + autoclose_referenced_issues boolean, + suggestion_commit_message character varying(255), + project_namespace_id bigint, + hidden boolean DEFAULT false NOT NULL, + organization_id bigint DEFAULT 1 +); + +CREATE FUNCTION find_projects_by_id(projects_id integer) RETURNS projects + LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE + AS $$ +BEGIN + return (SELECT projects FROM projects WHERE id = projects_id LIMIT 1); +END; +$$; + +CREATE TABLE users ( + id integer NOT NULL, + email character varying DEFAULT ''::character varying NOT NULL, + encrypted_password character varying DEFAULT ''::character varying NOT NULL, + reset_password_token character varying, + reset_password_sent_at timestamp without time zone, + remember_created_at timestamp without time zone, + sign_in_count integer DEFAULT 0, + current_sign_in_at timestamp without time zone, + last_sign_in_at timestamp without time zone, + current_sign_in_ip character varying, + last_sign_in_ip character varying, + created_at timestamp without time zone, + updated_at timestamp without time zone, + name character varying, + admin boolean DEFAULT false NOT NULL, + projects_limit integer NOT NULL, + failed_attempts integer DEFAULT 0, + locked_at timestamp without time zone, + username character varying, + can_create_group boolean DEFAULT true NOT NULL, + can_create_team boolean DEFAULT true NOT NULL, + state character varying, + color_scheme_id integer DEFAULT 1 NOT NULL, + password_expires_at timestamp without time zone, + created_by_id integer, + last_credential_check_at timestamp without time zone, + avatar character varying, + confirmation_token character varying, + confirmed_at timestamp without time zone, + confirmation_sent_at timestamp without time zone, + unconfirmed_email character varying, + hide_no_ssh_key boolean DEFAULT false, + admin_email_unsubscribed_at timestamp without time zone, + notification_email character varying, + hide_no_password boolean DEFAULT false, + password_automatically_set boolean DEFAULT false, + encrypted_otp_secret character varying, + encrypted_otp_secret_iv character varying, + encrypted_otp_secret_salt character varying, + otp_required_for_login boolean DEFAULT false NOT NULL, + otp_backup_codes text, + public_email character varying, + dashboard integer DEFAULT 0, + project_view integer DEFAULT 2, + consumed_timestep integer, + layout integer DEFAULT 0, + hide_project_limit boolean DEFAULT false, + note text, + unlock_token character varying, + otp_grace_period_started_at timestamp without time zone, + external boolean DEFAULT false, + incoming_email_token character varying, + auditor boolean DEFAULT false NOT NULL, + require_two_factor_authentication_from_group boolean DEFAULT false NOT NULL, + two_factor_grace_period integer DEFAULT 48 NOT NULL, + last_activity_on date, + notified_of_own_activity boolean DEFAULT false, + preferred_language character varying, + theme_id smallint, + accepted_term_id integer, + feed_token character varying, + private_profile boolean DEFAULT false NOT NULL, + roadmap_layout smallint, + include_private_contributions boolean, + commit_email character varying, + group_view integer, + managing_group_id integer, + first_name character varying(255), + last_name character varying(255), + static_object_token character varying(255), + role smallint, + user_type smallint DEFAULT 0, + static_object_token_encrypted text, + otp_secret_expires_at timestamp with time zone, + onboarding_in_progress boolean DEFAULT false NOT NULL, + CONSTRAINT check_0dd5948e38 CHECK ((user_type IS NOT NULL)), + CONSTRAINT check_7bde697e8e CHECK ((char_length(static_object_token_encrypted) <= 255)) +); + +CREATE FUNCTION find_users_by_id(users_id integer) RETURNS users + LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE + AS $$ +BEGIN + return (SELECT users FROM users WHERE id = users_id LIMIT 1); +END; +$$; + CREATE FUNCTION gitlab_schema_prevent_write() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -19192,58 +19434,6 @@ CREATE SEQUENCE namespace_statistics_id_seq ALTER SEQUENCE namespace_statistics_id_seq OWNED BY namespace_statistics.id; -CREATE TABLE namespaces ( - id integer NOT NULL, - name character varying NOT NULL, - path character varying NOT NULL, - owner_id integer, - created_at timestamp without time zone, - updated_at timestamp without time zone, - type character varying DEFAULT 'User'::character varying NOT NULL, - description character varying DEFAULT ''::character varying NOT NULL, - avatar character varying, - membership_lock boolean DEFAULT false, - share_with_group_lock boolean DEFAULT false, - visibility_level integer DEFAULT 20 NOT NULL, - request_access_enabled boolean DEFAULT true NOT NULL, - ldap_sync_status character varying DEFAULT 'ready'::character varying NOT NULL, - ldap_sync_error character varying, - ldap_sync_last_update_at timestamp without time zone, - ldap_sync_last_successful_update_at timestamp without time zone, - ldap_sync_last_sync_at timestamp without time zone, - description_html text, - lfs_enabled boolean, - parent_id integer, - shared_runners_minutes_limit integer, - repository_size_limit bigint, - require_two_factor_authentication boolean DEFAULT false NOT NULL, - two_factor_grace_period integer DEFAULT 48 NOT NULL, - cached_markdown_version integer, - project_creation_level integer, - runners_token character varying, - file_template_project_id integer, - saml_discovery_token character varying, - runners_token_encrypted character varying, - custom_project_templates_group_id integer, - auto_devops_enabled boolean, - extra_shared_runners_minutes_limit integer, - last_ci_minutes_notification_at timestamp with time zone, - last_ci_minutes_usage_notification_level integer, - subgroup_creation_level integer DEFAULT 1, - emails_disabled boolean, - max_pages_size integer, - max_artifacts_size integer, - mentions_disabled boolean, - default_branch_protection smallint, - unlock_membership_to_ldap boolean, - max_personal_access_token_lifetime integer, - push_rule_id bigint, - shared_runners_enabled boolean DEFAULT true NOT NULL, - allow_descendants_override_disabled_shared_runners boolean DEFAULT false NOT NULL, - traversal_ids integer[] DEFAULT '{}'::integer[] NOT NULL, - organization_id bigint DEFAULT 1 -); - CREATE SEQUENCE namespaces_id_seq START WITH 1 INCREMENT BY 1 @@ -21900,92 +22090,6 @@ CREATE SEQUENCE project_wiki_repositories_id_seq ALTER SEQUENCE project_wiki_repositories_id_seq OWNED BY project_wiki_repositories.id; -CREATE TABLE projects ( - id integer NOT NULL, - name character varying, - path character varying, - description text, - created_at timestamp without time zone, - updated_at timestamp without time zone, - creator_id integer, - namespace_id integer NOT NULL, - last_activity_at timestamp without time zone, - import_url character varying, - visibility_level integer DEFAULT 0 NOT NULL, - archived boolean DEFAULT false NOT NULL, - avatar character varying, - merge_requests_template text, - star_count integer DEFAULT 0 NOT NULL, - merge_requests_rebase_enabled boolean DEFAULT false, - import_type character varying, - import_source character varying, - approvals_before_merge integer DEFAULT 0 NOT NULL, - reset_approvals_on_push boolean DEFAULT true, - merge_requests_ff_only_enabled boolean DEFAULT false, - issues_template text, - mirror boolean DEFAULT false NOT NULL, - mirror_last_update_at timestamp without time zone, - mirror_last_successful_update_at timestamp without time zone, - mirror_user_id integer, - shared_runners_enabled boolean DEFAULT true NOT NULL, - runners_token character varying, - build_allow_git_fetch boolean DEFAULT true NOT NULL, - build_timeout integer DEFAULT 3600 NOT NULL, - mirror_trigger_builds boolean DEFAULT false NOT NULL, - pending_delete boolean DEFAULT false, - public_builds boolean DEFAULT true NOT NULL, - last_repository_check_failed boolean, - last_repository_check_at timestamp without time zone, - only_allow_merge_if_pipeline_succeeds boolean DEFAULT false NOT NULL, - has_external_issue_tracker boolean, - repository_storage character varying DEFAULT 'default'::character varying NOT NULL, - repository_read_only boolean, - request_access_enabled boolean DEFAULT true NOT NULL, - has_external_wiki boolean, - ci_config_path character varying, - lfs_enabled boolean, - description_html text, - only_allow_merge_if_all_discussions_are_resolved boolean, - repository_size_limit bigint, - printing_merge_request_link_enabled boolean DEFAULT true NOT NULL, - auto_cancel_pending_pipelines integer DEFAULT 1 NOT NULL, - service_desk_enabled boolean DEFAULT true, - cached_markdown_version integer, - delete_error text, - last_repository_updated_at timestamp without time zone, - disable_overriding_approvers_per_merge_request boolean, - storage_version smallint, - resolve_outdated_diff_discussions boolean, - remote_mirror_available_overridden boolean, - only_mirror_protected_branches boolean, - pull_mirror_available_overridden boolean, - jobs_cache_index integer, - external_authorization_classification_label character varying, - mirror_overwrites_diverged_branches boolean, - pages_https_only boolean DEFAULT true, - external_webhook_token character varying, - packages_enabled boolean, - merge_requests_author_approval boolean DEFAULT false, - pool_repository_id bigint, - runners_token_encrypted character varying, - bfg_object_map character varying, - detected_repository_languages boolean, - merge_requests_disable_committers_approval boolean, - require_password_to_approve boolean, - emails_disabled boolean, - max_pages_size integer, - max_artifacts_size integer, - pull_mirror_branch_prefix character varying(50), - remove_source_branch_after_merge boolean, - marked_for_deletion_at date, - marked_for_deletion_by_user_id integer, - autoclose_referenced_issues boolean, - suggestion_commit_message character varying(255), - project_namespace_id bigint, - hidden boolean DEFAULT false NOT NULL, - organization_id bigint DEFAULT 1 -); - CREATE SEQUENCE projects_id_seq START WITH 1 INCREMENT BY 1 @@ -24396,86 +24500,6 @@ CREATE SEQUENCE user_synced_attributes_metadata_id_seq ALTER SEQUENCE user_synced_attributes_metadata_id_seq OWNED BY user_synced_attributes_metadata.id; -CREATE TABLE users ( - id integer NOT NULL, - email character varying DEFAULT ''::character varying NOT NULL, - encrypted_password character varying DEFAULT ''::character varying NOT NULL, - reset_password_token character varying, - reset_password_sent_at timestamp without time zone, - remember_created_at timestamp without time zone, - sign_in_count integer DEFAULT 0, - current_sign_in_at timestamp without time zone, - last_sign_in_at timestamp without time zone, - current_sign_in_ip character varying, - last_sign_in_ip character varying, - created_at timestamp without time zone, - updated_at timestamp without time zone, - name character varying, - admin boolean DEFAULT false NOT NULL, - projects_limit integer NOT NULL, - failed_attempts integer DEFAULT 0, - locked_at timestamp without time zone, - username character varying, - can_create_group boolean DEFAULT true NOT NULL, - can_create_team boolean DEFAULT true NOT NULL, - state character varying, - color_scheme_id integer DEFAULT 1 NOT NULL, - password_expires_at timestamp without time zone, - created_by_id integer, - last_credential_check_at timestamp without time zone, - avatar character varying, - confirmation_token character varying, - confirmed_at timestamp without time zone, - confirmation_sent_at timestamp without time zone, - unconfirmed_email character varying, - hide_no_ssh_key boolean DEFAULT false, - admin_email_unsubscribed_at timestamp without time zone, - notification_email character varying, - hide_no_password boolean DEFAULT false, - password_automatically_set boolean DEFAULT false, - encrypted_otp_secret character varying, - encrypted_otp_secret_iv character varying, - encrypted_otp_secret_salt character varying, - otp_required_for_login boolean DEFAULT false NOT NULL, - otp_backup_codes text, - public_email character varying, - dashboard integer DEFAULT 0, - project_view integer DEFAULT 2, - consumed_timestep integer, - layout integer DEFAULT 0, - hide_project_limit boolean DEFAULT false, - note text, - unlock_token character varying, - otp_grace_period_started_at timestamp without time zone, - external boolean DEFAULT false, - incoming_email_token character varying, - auditor boolean DEFAULT false NOT NULL, - require_two_factor_authentication_from_group boolean DEFAULT false NOT NULL, - two_factor_grace_period integer DEFAULT 48 NOT NULL, - last_activity_on date, - notified_of_own_activity boolean DEFAULT false, - preferred_language character varying, - theme_id smallint, - accepted_term_id integer, - feed_token character varying, - private_profile boolean DEFAULT false NOT NULL, - roadmap_layout smallint, - include_private_contributions boolean, - commit_email character varying, - group_view integer, - managing_group_id integer, - first_name character varying(255), - last_name character varying(255), - static_object_token character varying(255), - role smallint, - user_type smallint DEFAULT 0, - static_object_token_encrypted text, - otp_secret_expires_at timestamp with time zone, - onboarding_in_progress boolean DEFAULT false NOT NULL, - CONSTRAINT check_0dd5948e38 CHECK ((user_type IS NOT NULL)), - CONSTRAINT check_7bde697e8e CHECK ((char_length(static_object_token_encrypted) <= 255)) -); - CREATE SEQUENCE users_id_seq START WITH 1 INCREMENT BY 1 diff --git a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb new file mode 100644 index 00000000000000..c6c85f19019449 --- /dev/null +++ b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe UseSqlFunctionForPrimaryKeyLookups, feature_category: :groups_and_projects do + let_it_be(:project) { create(:project) } + + let(:model) do + Class.new(ApplicationRecord) do + self.table_name = :projects + + include UseSqlFunctionForPrimaryKeyLookups + end + end + + subject(:query_log) { ActiveRecord::QueryRecorder.new { model.find(project.id) }.log } + + context 'when the use_prepared_statements_for_primary_key_lookups FF is on' do + before do + stub_feature_flags(use_prepared_statements_for_primary_key_lookups: true) + end + + it 'loads the correct record' do + expect(model.find(project.id).id).to eq(project.id) + end + + it 'uses the fuction-based finder query' do + query = <<~SQL + SELECT "projects".* FROM find_projects_by_id(#{project.id})#{' '} + "projects" WHERE ("projects"."id" IS NOT NULL) LIMIT 1 + SQL + + expect(query_log).to match_array(include(query.tr("\n", ''))) + end + + it 'uses query cache', :use_sql_query_cache do + query = <<~SQL + SELECT "projects".* FROM find_projects_by_id(#{project.id})#{' '} + "projects" WHERE ("projects"."id" IS NOT NULL) LIMIT 1 + SQL + + recorder = ActiveRecord::QueryRecorder.new do + model.find(project.id) + model.find(project.id) + model.find(project.id) + end + + expect(recorder.data.each_value.first[:count]).to eq(1) + expect(recorder.cached).to include(query.tr("\n", '')) + end + + context 'when the model has ignored columns' do + around do |example| + model.ignored_columns = %i[path] + example.run + model.ignored_columns = [] + end + + it 'enumerates the column names' do + column_list = model.columns.map do |column| + %("projects"."#{column.name}") + end.join(', ') + + expect(column_list).not_to include(%("projects"."path")) + + query = <<~SQL + SELECT #{column_list} FROM find_projects_by_id(#{project.id})#{' '} + "projects" WHERE ("projects"."id" IS NOT NULL) LIMIT 1 + SQL + + expect(query_log).to match_array(include(query.tr("\n", ''))) + end + end + end + + context 'when the use_prepared_statements_for_primary_key_lookups FF is off' do + before do + stub_feature_flags(use_prepared_statements_for_primary_key_lookups: false) + end + + it 'loads the correct record' do + expect(model.find(project.id).id).to eq(project.id) + end + + it 'uses the SQL-based finder query' do + expected_query = %(SELECT "projects".* FROM \"projects\" WHERE "projects"."id" = #{project.id} LIMIT 1) + expect(query_log).to match_array(include(expected_query)) + end + end +end -- GitLab From 4c792f7c4ff085b8e0db17b2b71bd9e9b9e0d535 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Mon, 30 Oct 2023 16:53:45 +1300 Subject: [PATCH 2/7] Update PK lookup function arg to bigint Use `bigint` as it will work for tables with both `integer` and `bigint` PKs. --- ...ookups.yml => use_sql_functions_for_primary_key_lookups.yml} | 0 .../20231026050554_add_functions_for_primary_key_lookup.rb | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename config/feature_flags/development/{use_prepared_statements_for_primary_key_lookups.yml => use_sql_functions_for_primary_key_lookups.yml} (100%) diff --git a/config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml b/config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml similarity index 100% rename from config/feature_flags/development/use_prepared_statements_for_primary_key_lookups.yml rename to config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml diff --git a/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb b/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb index 396bca7c71b107..ecf32f74e4b0eb 100644 --- a/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb +++ b/db/migrate/20231026050554_add_functions_for_primary_key_lookup.rb @@ -8,7 +8,7 @@ class AddFunctionsForPrimaryKeyLookup < Gitlab::Database::Migration[2.2] def up TABLES.each do |table| execute <<~SQL - CREATE OR REPLACE FUNCTION find_#{table}_by_id(#{table}_id int) + CREATE OR REPLACE FUNCTION find_#{table}_by_id(#{table}_id bigint) RETURNS #{table} AS $$ BEGIN return (SELECT #{table} FROM #{table} WHERE id = #{table}_id LIMIT 1); -- GitLab From 2edb0d6878d686b6be1d7044822daa275d69df80 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Mon, 30 Oct 2023 16:55:00 +1300 Subject: [PATCH 3/7] Rename feature flag From `use_prepared_statements_for_primary_key_lookups` to `use_sql_functions_for_primary_key_lookups`. --- .../concerns/use_sql_function_for_primary_key_lookups.rb | 2 +- .../use_sql_functions_for_primary_key_lookups.yml | 2 +- .../use_sql_function_for_primary_key_lookups_spec.rb | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/use_sql_function_for_primary_key_lookups.rb b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb index 8ab64133856c4b..19842d1c6e59cc 100644 --- a/app/models/concerns/use_sql_function_for_primary_key_lookups.rb +++ b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb @@ -5,7 +5,7 @@ module UseSqlFunctionForPrimaryKeyLookups class_methods do def find(*args) - return super unless Feature.enabled?(:use_prepared_statements_for_primary_key_lookups) + return super unless Feature.enabled?(:use_sql_functions_for_primary_key_lookups) return super unless args.one? return super if block_given? || primary_key.nil? || scope_attributes? diff --git a/config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml b/config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml index e5c9d9f073abba..c8ee2894aef7c8 100644 --- a/config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml +++ b/config/feature_flags/development/use_sql_functions_for_primary_key_lookups.yml @@ -1,5 +1,5 @@ --- -name: use_prepared_statements_for_primary_key_lookups +name: use_sql_functions_for_primary_key_lookups introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/135196 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429479 milestone: '16.6' diff --git a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb index c6c85f19019449..aca7f8546f27a0 100644 --- a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb +++ b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb @@ -15,9 +15,9 @@ subject(:query_log) { ActiveRecord::QueryRecorder.new { model.find(project.id) }.log } - context 'when the use_prepared_statements_for_primary_key_lookups FF is on' do + context 'when the use_sql_functions_for_primary_key_lookups FF is on' do before do - stub_feature_flags(use_prepared_statements_for_primary_key_lookups: true) + stub_feature_flags(use_sql_functions_for_primary_key_lookups: true) end it 'loads the correct record' do @@ -73,9 +73,9 @@ end end - context 'when the use_prepared_statements_for_primary_key_lookups FF is off' do + context 'when the use_sql_functions_for_primary_key_lookups FF is off' do before do - stub_feature_flags(use_prepared_statements_for_primary_key_lookups: false) + stub_feature_flags(use_sql_functions_for_primary_key_lookups: false) end it 'loads the correct record' do -- GitLab From cc2b827081e6e1bf6e16ab7bb47f456fcab17d9d Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Tue, 31 Oct 2023 10:53:31 +1300 Subject: [PATCH 4/7] Update functions definition in structure.sql --- db/structure.sql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index 652f2535f26e00..3e050fd0853b9e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -87,7 +87,7 @@ CREATE TABLE namespaces ( organization_id bigint DEFAULT 1 ); -CREATE FUNCTION find_namespaces_by_id(namespaces_id integer) RETURNS namespaces +CREATE FUNCTION find_namespaces_by_id(namespaces_id bigint) RETURNS namespaces LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE AS $$ BEGIN @@ -181,7 +181,7 @@ CREATE TABLE projects ( organization_id bigint DEFAULT 1 ); -CREATE FUNCTION find_projects_by_id(projects_id integer) RETURNS projects +CREATE FUNCTION find_projects_by_id(projects_id bigint) RETURNS projects LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE AS $$ BEGIN @@ -269,7 +269,7 @@ CREATE TABLE users ( CONSTRAINT check_7bde697e8e CHECK ((char_length(static_object_token_encrypted) <= 255)) ); -CREATE FUNCTION find_users_by_id(users_id integer) RETURNS users +CREATE FUNCTION find_users_by_id(users_id bigint) RETURNS users LANGUAGE plpgsql STABLE COST 1 PARALLEL SAFE AS $$ BEGIN -- GitLab From e9ff8273d96bd08ed4219c98713d9cd5d046476a Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Thu, 2 Nov 2023 14:28:26 +1300 Subject: [PATCH 5/7] Use current request as FF actor as this is the recommended way - https://docs.gitlab.com/ee/development/feature_flags/#current-request-actor. --- app/models/concerns/use_sql_function_for_primary_key_lookups.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/use_sql_function_for_primary_key_lookups.rb b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb index 19842d1c6e59cc..c3ca3cfc038496 100644 --- a/app/models/concerns/use_sql_function_for_primary_key_lookups.rb +++ b/app/models/concerns/use_sql_function_for_primary_key_lookups.rb @@ -5,7 +5,7 @@ module UseSqlFunctionForPrimaryKeyLookups class_methods do def find(*args) - return super unless Feature.enabled?(:use_sql_functions_for_primary_key_lookups) + return super unless Feature.enabled?(:use_sql_functions_for_primary_key_lookups, Feature.current_request) return super unless args.one? return super if block_given? || primary_key.nil? || scope_attributes? -- GitLab From 35f4a7b77c1df1a7a73622a1777babfd5838cb21 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Fri, 3 Nov 2023 11:29:13 +1300 Subject: [PATCH 6/7] WIP --- ...l_function_for_primary_key_lookups_spec.rb | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb index aca7f8546f27a0..60c1cc7c50df70 100644 --- a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb +++ b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb @@ -4,6 +4,7 @@ RSpec.describe UseSqlFunctionForPrimaryKeyLookups, feature_category: :groups_and_projects do let_it_be(:project) { create(:project) } + let_it_be(:another_project) { create(:project) } let(:model) do Class.new(ApplicationRecord) do @@ -13,8 +14,6 @@ end end - subject(:query_log) { ActiveRecord::QueryRecorder.new { model.find(project.id) }.log } - context 'when the use_sql_functions_for_primary_key_lookups FF is on' do before do stub_feature_flags(use_sql_functions_for_primary_key_lookups: true) @@ -29,6 +28,7 @@ SELECT "projects".* FROM find_projects_by_id(#{project.id})#{' '} "projects" WHERE ("projects"."id" IS NOT NULL) LIMIT 1 SQL + query_log = ActiveRecord::QueryRecorder.new { model.find(project.id) }.log expect(query_log).to match_array(include(query.tr("\n", ''))) end @@ -67,10 +67,44 @@ SELECT #{column_list} FROM find_projects_by_id(#{project.id})#{' '} "projects" WHERE ("projects"."id" IS NOT NULL) LIMIT 1 SQL + query_log = ActiveRecord::QueryRecorder.new { model.find(project.id) }.log expect(query_log).to match_array(include(query.tr("\n", ''))) end end + + context 'when there are scope attributes' do + let(:scoped_model) do + Class.new(model) do + default_scope { where.not(path: nil) } # rubocop: disable Cop/DefaultScope + end + end + + it 'loads the correct record' do + expect(scoped_model.find(project.id).id).to eq(project.id) + end + + it 'does not use the function-based finder query' do + query_log = ActiveRecord::QueryRecorder.new { scoped_model.find(project.id) }.log + + expect(query_log).not_to include(match(/find_projects_by_id/)) + end + end + + context 'when there are multiple arguments' do + it 'loads the correct records' do + expect(model.find(project.id, another_project.id).map(&:id)).to match_array([project.id, another_project.id]) + end + + it 'does not use the function-based finder query' do + query_log = ActiveRecord::QueryRecorder.new { model.find(project.id, another_project.id) }.log + + expect(query_log).not_to include(match(/find_projects_by_id/)) + end + end + + context 'when there is block given' + context 'when there is no primary key defined' end context 'when the use_sql_functions_for_primary_key_lookups FF is off' do @@ -84,6 +118,8 @@ it 'uses the SQL-based finder query' do expected_query = %(SELECT "projects".* FROM \"projects\" WHERE "projects"."id" = #{project.id} LIMIT 1) + query_log = ActiveRecord::QueryRecorder.new { model.find(project.id) }.log + expect(query_log).to match_array(include(expected_query)) end end -- GitLab From ecfe1d6b49bd0ac080086770074e76ace3916616 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Fri, 3 Nov 2023 16:02:17 +1300 Subject: [PATCH 7/7] Update tests to cover early returns --- ...l_function_for_primary_key_lookups_spec.rb | 61 ++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb index 60c1cc7c50df70..f6f53c9aad50b6 100644 --- a/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb +++ b/spec/models/concerns/use_sql_function_for_primary_key_lookups_spec.rb @@ -76,7 +76,7 @@ context 'when there are scope attributes' do let(:scoped_model) do Class.new(model) do - default_scope { where.not(path: nil) } # rubocop: disable Cop/DefaultScope + default_scope { where.not(path: nil) } # rubocop: disable Cop/DefaultScope -- Needed for testing a specific case end end @@ -103,8 +103,63 @@ end end - context 'when there is block given' - context 'when there is no primary key defined' + context 'when there is block given' do + it 'loads the correct records' do + expect(model.find(0) { |p| p.path == project.path }.id).to eq(project.id) + end + + it 'does not use the function-based finder query' do + query_log = ActiveRecord::QueryRecorder.new { model.find(0) { |p| p.path == project.path } }.log + + expect(query_log).not_to include(match(/find_projects_by_id/)) + end + end + + context 'when there is no primary key defined' do + let(:model_without_pk) do + Class.new(model) do + def self.primary_key + nil + end + end + end + + it 'raises ActiveRecord::UnknownPrimaryKey' do + expect { model_without_pk.find(0) }.to raise_error ActiveRecord::UnknownPrimaryKey + end + end + + context 'when id is provided as an array' do + it 'returns the correct record as an array' do + expect(model.find([project.id]).map(&:id)).to eq([project.id]) + end + + it 'does use the function-based finder query' do + query_log = ActiveRecord::QueryRecorder.new { model.find([project.id]) }.log + + expect(query_log).to include(match(/find_projects_by_id/)) + end + + context 'when array has multiple elements' do + it 'does not use the function-based finder query' do + query_log = ActiveRecord::QueryRecorder.new { model.find([project.id, another_project.id]) }.log + + expect(query_log).not_to include(match(/find_projects_by_id/)) + end + end + end + + context 'when the provided id is null' do + it 'raises ActiveRecord::RecordNotFound' do + expect { model.find(nil) }.to raise_error ActiveRecord::RecordNotFound, "Couldn't find without an ID" + end + end + + context 'when the provided id is not a string that can cast to numeric' do + it 'raises ActiveRecord::RecordNotFound' do + expect { model.find('foo') }.to raise_error ActiveRecord::RecordNotFound, "Couldn't find with 'id'=foo" + end + end end context 'when the use_sql_functions_for_primary_key_lookups FF is off' do -- GitLab