Don't generate invalid SQL checking foreign keys
What does this MR do and why?
Describe in detail what your merge request does and why.
When migrating many versions of gitlab at once, it was possible to call foreign_key_exists? with a schema cache too old for rails to know about the constrained_columns column of the postgres_foreign_keys view.
This manifested with errors like
PG::InvalidTextRepresentation: ERROR: malformed array literal
Fallback to the previous behavior of the method in this case, to avoid the error.
Related to #388928 (closed)
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
The docker image registry.gitlab.com/gitlab-org/quality/performance-images/gitlab-ce-performance-test:15.8.0-ce.0-arm-broken
reproduces this error when run. We can verify the fix by patching this commit into it.
- Run the docker image
gitlab-ce-performance-test:15.8.0-ce.0-arm-broken
and wait for the migrations to fail with the expected error. - Create the following files:
Dockerfile
:
FROM registry.gitlab.com/gitlab-org/quality/performance-images/gitlab-ce-performance-test:15.8.0-ce.0-arm-broken
COPY migration_helpers.patch /opt/gitlab/embedded/service/gitlab-rails/lib/gitlab/database/migration_helpers.patch
RUN cd /opt/gitlab/embedded/service/gitlab-rails/lib/gitlab/database && patch < migration_helpers.patch
migration_helpers.patch
:
--- migration_helpers.rb 2023-01-20 05:07:24.000000000 -0600
+++ migration_helpers.rb 2023-03-02 13:57:58.000000000 -0600
@@ -371,14 +371,21 @@
# The behavior in the if block has a bug: it always returns false if the fk being checked has multiple columns.
# This can be removed after init_schema.rb passes 20221122210711_add_columns_to_postgres_foreign_keys.rb
# Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/386796
- if ActiveRecord::Migrator.current_version < 20221122210711
+ version_too_old = ActiveRecord::Migrator.current_version < 20221122210711
+
+ # We also fall back to previous behavior if the schema cache does not include the constrained_columns column.
+ # This happens if we've migrated up past the above migration and are still in the same migration execution on
+ # a subsequent migration. In that case, rails's default handling with where clauses and array columns leads
+ # to invalid SQL generating. So we fall back to the previous behavior instead.
+ schema_cache_stale = !Gitlab::Database::PostgresForeignKey.columns_hash.key?('constrained_columns')
+ if version_too_old || schema_cache_stale
return foreign_keys(source).any? do |foreign_key|
tables_match?(target.to_s, foreign_key.to_table.to_s) &&
options_match?(foreign_key.options, options)
end
end
- fks = Gitlab::Database::PostgresForeignKey.by_constrained_table_name(source)
+ fks = Gitlab::Database::PostgresForeignKey.by_constrained_table_name_or_identifier(source)
fks = fks.by_referenced_table_name(target) if target
fks = fks.by_name(options[:name]) if options[:name]
- Build and run the docker image:
docker build -t gitlab-fk-error:latest && docker run -it gitlab-fk-error:latest
- Watch the docker image proceed through migrations successfully and start gitlab.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.