From a09a87d66c0862bd6af4649a7d9faff07816295e Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 22 Feb 2024 14:26:52 -0300 Subject: [PATCH 1/5] test --- .github/workflows/ci.yml | 12 ++++++++++-- .ruby-version | 2 +- Gemfile | 1 + test/cases/helper_cockroachdb.rb | 11 +++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5612649d..09d8a127 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - crdb: [v23.1.11] + crdb: [v23.1.15] ruby: [head] name: Test (crdb=${{ matrix.crdb }} ruby=${{ matrix.ruby }}) steps: @@ -95,4 +95,12 @@ jobs: SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true'; " - name: Test - run: bundle exec rake test TESTOPTS='--profile=3' + run: bundle exec rake test TESTOPTS='--profile=3 --stackprof --profile-setup' + - name: Save timing artifacts + if: ${{ always() }} + uses: actions/upload-artifact@v4 + with: + name: performance + path: | + sql.ljson + stackprof-minitest-*.dump diff --git a/.ruby-version b/.ruby-version index e4604e3a..b347b11e 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -3.2.1 +3.2.3 diff --git a/Gemfile b/Gemfile index a7c41bb5..2ad1aa60 100644 --- a/Gemfile +++ b/Gemfile @@ -4,6 +4,7 @@ source "https://rubygems.org" gemspec +gem "minitest-stackprof" module RailsTag class << self diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index eb9b5f21..8c61813a 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -176,3 +176,14 @@ def header(stream) end ActiveRecord::SchemaDumper.prepend(NoHeaderExt) + + +ActiveSupport::Notifications.subscribe 'sql.active_record' do |*args| + event = ActiveSupport::Notifications::Event.new(*args) + sql = event.payload[:sql] + duration = event.duration # ms + + File.open("sql.ljson", "a") do |f| + f.puts JSON.dump({sql: sql, duration: duration}) + end +end From d4b5482cd20d1e2948fe10bc69bd9a6aa4532680 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 23 Feb 2024 11:30:19 -0300 Subject: [PATCH 2/5] bypass disable_referential_integrity --- .../cockroachdb/referential_integrity.rb | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 476c21b1..32dfd3b1 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -20,36 +20,36 @@ def check_all_foreign_keys_valid! end def disable_referential_integrity - foreign_keys = tables.map { |table| foreign_keys(table) }.flatten + # foreign_keys = tables.map { |table| foreign_keys(table) }.flatten - foreign_keys.each do |foreign_key| - remove_foreign_key(foreign_key.from_table, name: foreign_key.options[:name]) - end + # foreign_keys.each do |foreign_key| + # remove_foreign_key(foreign_key.from_table, name: foreign_key.options[:name]) + # end yield # Prefixes and suffixes are added in add_foreign_key # in AR7+ so we need to temporarily disable them here, # otherwise prefixes/suffixes will be erroneously added. - old_prefix = ActiveRecord::Base.table_name_prefix - old_suffix = ActiveRecord::Base.table_name_suffix - - ActiveRecord::Base.table_name_prefix = "" - ActiveRecord::Base.table_name_suffix = "" - - begin - foreign_keys.each do |foreign_key| - # Avoid having PG:DuplicateObject error if a test is ran in transaction. - # TODO: verify that there is no cache issue related to running this (e.g: fk - # still in cache but not in db) - next if foreign_key_exists?(foreign_key.from_table, name: foreign_key.options[:name]) - - add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options) - end - ensure - ActiveRecord::Base.table_name_prefix = old_prefix - ActiveRecord::Base.table_name_suffix = old_suffix - end + # old_prefix = ActiveRecord::Base.table_name_prefix + # old_suffix = ActiveRecord::Base.table_name_suffix + + # ActiveRecord::Base.table_name_prefix = "" + # ActiveRecord::Base.table_name_suffix = "" + + # begin + # foreign_keys.each do |foreign_key| + # # Avoid having PG:DuplicateObject error if a test is ran in transaction. + # # TODO: verify that there is no cache issue related to running this (e.g: fk + # # still in cache but not in db) + # next if foreign_key_exists?(foreign_key.from_table, name: foreign_key.options[:name]) + + # add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options) + # end + # ensure + # ActiveRecord::Base.table_name_prefix = old_prefix + # ActiveRecord::Base.table_name_suffix = old_suffix + # end end end end From bd15a40512b0b39054af7c82d5fd5961bf069f06 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 24 Feb 2024 11:54:30 -0300 Subject: [PATCH 3/5] without fks --- test/cases/helper_cockroachdb.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index 8c61813a..a36eabed 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -27,6 +27,18 @@ def load_schema end LoadSchemaHelper.prepend(LoadSchemaHelperExt) +require "active_record" +require "active_record/connection_adapters" +require "active_record/connection_adapters/postgresql/schema_statements" +require "active_record/connection_adapters/cockroachdb/schema_statements" + +# Disable foreign_keys altogether. +ActiveRecord::ConnectionAdapters::CockroachDB::SchemaStatements.prepend(Module.new do + def add_foreign_key(*) + end +end) + + # Load ActiveRecord test helper require "cases/helper" From bb8ecdf0987af5a67409ef1d6c07f9bad6aad581 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 24 Feb 2024 17:54:35 -0300 Subject: [PATCH 4/5] prefix fix --- test/cases/migration/foreign_key_test.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/cases/migration/foreign_key_test.rb b/test/cases/migration/foreign_key_test.rb index e9274e5d..9a54da9f 100644 --- a/test/cases/migration/foreign_key_test.rb +++ b/test/cases/migration/foreign_key_test.rb @@ -281,8 +281,11 @@ def test_add_foreign_key_with_prefix silence_stream($stdout) { migration.migrate(:up) } assert_equal 1, @connection.foreign_keys("p_classes").size ensure - silence_stream($stdout) { migration.migrate(:down) } - ActiveRecord::Base.table_name_prefix = nil + begin + silence_stream($stdout) { migration.migrate(:down) } + ensure + ActiveRecord::Base.table_name_prefix = nil + end end def test_add_foreign_key_with_suffix @@ -291,8 +294,11 @@ def test_add_foreign_key_with_suffix silence_stream($stdout) { migration.migrate(:up) } assert_equal 1, @connection.foreign_keys("classes_s").size ensure - silence_stream($stdout) { migration.migrate(:down) } - ActiveRecord::Base.table_name_suffix = nil + begin + silence_stream($stdout) { migration.migrate(:down) } + ensure + ActiveRecord::Base.table_name_suffix = nil + end end def test_remove_foreign_key_with_if_exists_not_set From 25dff4b2305e7a761f1bd36484336c251a2b9ca3 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 24 Feb 2024 18:06:45 -0300 Subject: [PATCH 5/5] remove not needed --- test/cases/helper_cockroachdb.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index a36eabed..611945ff 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -34,8 +34,7 @@ def load_schema # Disable foreign_keys altogether. ActiveRecord::ConnectionAdapters::CockroachDB::SchemaStatements.prepend(Module.new do - def add_foreign_key(*) - end + def use_foreign_keys?; false; end end)