From b247ca04b6dc108e75c43512abc03cc2782d9570 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 5 Oct 2024 14:33:08 +0200 Subject: [PATCH 01/17] chore: Backport #345 to 7-0-stable --- .github/workflows/ci.yml | 13 +-- Gemfile | 8 ++ activerecord-cockroachdb-adapter.gemspec | 2 + .../cockroachdb/schema_statements.rb | 109 ++++++++++++++++++ test/cases/migration/hidden_column_test.rb | 11 ++ test/cases/primary_keys_test.rb | 34 ++++++ 6 files changed, 170 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c7e7a61..b50d00b8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,16 +18,16 @@ on: # This allows a subsequently queued workflow run to interrupt previous runs. concurrency: - group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' + group: "${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}" cancel-in-progress: true jobs: test: - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 strategy: matrix: - crdb: [v23.1.5] - ruby: [ruby-head] + crdb: [v24.1.10] + ruby: ["3.4"] name: Test (crdb=${{ matrix.crdb }} ruby=${{ matrix.ruby }}) steps: - name: Set Up Actions @@ -37,8 +37,8 @@ jobs: - name: Set Up Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: ${{ matrix.ruby }} - bundler-cache: true + ruby-version: ${{ matrix.ruby }} + bundler-cache: true - name: Install and Start Cockroachdb run: | # Download CockroachDB @@ -75,7 +75,6 @@ jobs: ALTER RANGE liveness CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms'; - SET CLUSTER SETTING kv.raft_log.disable_synchronization_unsafe = 'true'; SET CLUSTER SETTING jobs.registry.interval.cancel = '180s'; SET CLUSTER SETTING jobs.registry.interval.gc = '30s'; SET CLUSTER SETTING kv.range_split.by_load_merge_delay = '5s'; diff --git a/Gemfile b/Gemfile index 8c4d1671..59681eb9 100644 --- a/Gemfile +++ b/Gemfile @@ -47,6 +47,14 @@ group :development, :test do gem "byebug" gem "minitest-excludes", "~> 2.0.1" + # Needed for the test suite + gem "msgpack", ">= 1.7.0" + gem "mutex_m", "~> 0.2.0" + gem "drb" + gem "bigdecimal" + gem "benchmark" + gem "logger" + # Gems used by the ActiveRecord test suite gem "bcrypt", "~> 3.1.18" gem "mocha", "~> 1.14.0" diff --git a/activerecord-cockroachdb-adapter.gemspec b/activerecord-cockroachdb-adapter.gemspec index 2fb2c459..049254a7 100644 --- a/activerecord-cockroachdb-adapter.gemspec +++ b/activerecord-cockroachdb-adapter.gemspec @@ -17,6 +17,8 @@ Gem::Specification.new do |spec| spec.add_dependency "activerecord", "~> 7.0.3" spec.add_dependency "pg", "~> 1.2" spec.add_dependency "rgeo-activerecord", "~> 7.0.0" + # See https://github.com/rails/rails/issues/54263 + spec.add_dependency 'concurrent-ruby', '1.3.4' spec.add_development_dependency "benchmark-ips", "~> 2.9.1" diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index 637f3331..14c0848e 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -32,6 +32,115 @@ def primary_key(table_name) end end + def primary_keys(table_name) + return super unless database_version >= 24_02_02 + + query_values(<<~SQL, "SCHEMA") + SELECT a.attname + FROM ( + SELECT indrelid, indkey, generate_subscripts(indkey, 1) idx + FROM pg_index + WHERE indrelid = #{quote(quote_table_name(table_name))}::regclass + AND indisprimary + ) i + JOIN pg_attribute a + ON a.attrelid = i.indrelid + AND a.attnum = i.indkey[i.idx] + AND NOT a.attishidden + ORDER BY i.idx + SQL + end + + def column_names_from_column_numbers(table_oid, column_numbers) + return super unless database_version >= 24_02_02 + + Hash[query(<<~SQL, "SCHEMA")].values_at(*column_numbers).compact + SELECT a.attnum, a.attname + FROM pg_attribute a + WHERE a.attrelid = #{table_oid} + AND a.attnum IN (#{column_numbers.join(", ")}) + AND NOT a.attishidden + SQL + end + + # OVERRIDE: CockroachDB does not support deferrable constraints. + # See: https://go.crdb.dev/issue-v/31632/v23.1 + def foreign_key_options(from_table, to_table, options) + options = super + options.delete(:deferrable) unless supports_deferrable_constraints? + options + end + + # OVERRIDE: Added `unique_rowid` to the last line of the second query. + # This is a CockroachDB-specific function used for primary keys. + # Also make sure we don't consider `NOT VISIBLE` columns. + # + # Returns a table's primary key and belonging sequence. + def pk_and_sequence_for(table) # :nodoc: + # First try looking for a sequence with a dependency on the + # given table's primary key. + result = query(<<~SQL, "SCHEMA")[0] + SELECT attr.attname, nsp.nspname, seq.relname + FROM pg_class seq, + pg_attribute attr, + pg_depend dep, + pg_constraint cons, + pg_namespace nsp, + -- TODO: use the pg_catalog.pg_attribute(attishidden) column when + -- it is added instead of joining on crdb_internal. + -- See https://github.com/cockroachdb/cockroach/pull/126397 + crdb_internal.table_columns tc + WHERE seq.oid = dep.objid + AND seq.relkind = 'S' + AND attr.attrelid = dep.refobjid + AND attr.attnum = dep.refobjsubid + AND attr.attrelid = cons.conrelid + AND attr.attnum = cons.conkey[1] + AND seq.relnamespace = nsp.oid + AND attr.attrelid = tc.descriptor_id + AND attr.attname = tc.column_name + AND tc.hidden = false + AND cons.contype = 'p' + AND dep.classid = 'pg_class'::regclass + AND dep.refobjid = #{quote(quote_table_name(table))}::regclass + SQL + + if result.nil? || result.empty? + result = query(<<~SQL, "SCHEMA")[0] + SELECT attr.attname, nsp.nspname, + CASE + WHEN pg_get_expr(def.adbin, def.adrelid) !~* 'nextval' THEN NULL + WHEN split_part(pg_get_expr(def.adbin, def.adrelid), '''', 2) ~ '.' THEN + substr(split_part(pg_get_expr(def.adbin, def.adrelid), '''', 2), + strpos(split_part(pg_get_expr(def.adbin, def.adrelid), '''', 2), '.')+1) + ELSE split_part(pg_get_expr(def.adbin, def.adrelid), '''', 2) + END + FROM pg_class t + JOIN pg_attribute attr ON (t.oid = attrelid) + JOIN pg_attrdef def ON (adrelid = attrelid AND adnum = attnum) + JOIN pg_constraint cons ON (conrelid = adrelid AND adnum = conkey[1]) + JOIN pg_namespace nsp ON (t.relnamespace = nsp.oid) + -- TODO: use the pg_catalog.pg_attribute(attishidden) column when + -- it is added instead of joining on crdb_internal. + -- See https://github.com/cockroachdb/cockroach/pull/126397 + JOIN crdb_internal.table_columns tc ON (attr.attrelid = tc.descriptor_id AND attr.attname = tc.column_name) + WHERE t.oid = #{quote(quote_table_name(table))}::regclass + AND tc.hidden = false + AND cons.contype = 'p' + AND pg_get_expr(def.adbin, def.adrelid) ~* 'nextval|uuid_generate|gen_random_uuid|unique_rowid' + SQL + end + + pk = result.shift + if result.last + [pk, PostgreSQL::Name.new(*result)] + else + [pk, nil] + end + rescue + nil + end + # override # Modified version of the postgresql foreign_keys method. # Replaces t2.oid::regclass::text with t2.relname since this is diff --git a/test/cases/migration/hidden_column_test.rb b/test/cases/migration/hidden_column_test.rb index 7a1ddc91..956b8e1e 100644 --- a/test/cases/migration/hidden_column_test.rb +++ b/test/cases/migration/hidden_column_test.rb @@ -53,6 +53,17 @@ def test_add_hidden_column output = dump_table_schema "rockets" assert_match %r{t.uuid "new_col", hidden: true$}, output end + + # Since 24.2.2, hash sharded indexes add a hidden column to the table. + # This tests ensure that the user can still drop the index even if they + # call `#remove_index` with the column name rather than the index name. + def test_remove_index_with_a_hidden_column + @connection.execute <<-SQL + CREATE INDEX hash_idx ON rockets (name) USING HASH WITH (bucket_count=8); + SQL + @connection.remove_index :rockets, :name + # assert :ok + end end end end diff --git a/test/cases/primary_keys_test.rb b/test/cases/primary_keys_test.rb index a614d23c..e99fdfce 100644 --- a/test/cases/primary_keys_test.rb +++ b/test/cases/primary_keys_test.rb @@ -97,4 +97,38 @@ def test_schema_dump_primary_key_integer_with_default_nil assert_match %r{create_table "int_defaults", id: :bigint, default: nil}, schema end end + + class PrimaryKeyHiddenColumnTest < ActiveRecord::TestCase + class Rocket < ActiveRecord::Base + end + + def setup + connection = ActiveRecord::Base.lease_connection + connection.execute <<-SQL + CREATE TABLE rockets( + id SERIAL PRIMARY KEY USING HASH WITH (bucket_count=4) + ) + SQL + end + + def teardown + ActiveRecord::Base.connection.drop_table :rockets + end + + def test_to_key_with_hidden_primary_key_part + rocket = Rocket.new + assert_nil rocket.to_key + rocket.save + assert_equal rocket.to_key, [rocket.id] + end + + def test_read_attribute_with_hidden_primary_key_part + rocket = Rocket.create! + id = assert_not_deprecated(ActiveRecord.deprecator) do + rocket.read_attribute(:id) + end + + assert_equal rocket.id, id + end + end end From 1f91c616241087e1952fb20991f1845d4384f3de Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 23 Sep 2023 09:50:34 -0500 Subject: [PATCH 02/17] fix(test): Ensure `$stdout` is always restored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the `IOError` we could encounter in tests. Found using: ```ruby def bugmsg(header, stdout) bugged = begin; print; rescue IOError; "💥 "; end STDOUT.puts "🤞#{header.rjust(17)}: stdout=#{stdout.inspect} " \ "#{bugged}(at #{caller_locations(2, 1).first})" end trace_var :$stdout, (proc { bugmsg("$stdout=", _1) }) TracePoint.trace(:a_return) do |tp| next unless tp.method_id == :reopen && tp.self == $stdout bugmsg("$stdout#reopen", $stdout) end ``` --- test/cases/schema_dumper_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/cases/schema_dumper_test.rb b/test/cases/schema_dumper_test.rb index ca687639..ccecc463 100644 --- a/test/cases/schema_dumper_test.rb +++ b/test/cases/schema_dumper_test.rb @@ -83,7 +83,7 @@ def down end ensure migration.migrate(:down) - # $stdout = original + $stdout = original end def test_timestamps_schema_dump_before_rails_7_with_timestamptz_setting @@ -119,7 +119,7 @@ def down end ensure migration.migrate(:down) - # $stdout = original + $stdout = original end def test_schema_dump_with_correct_timestamp_types_via_add_column_before_rails_7_with_timestamptz_setting @@ -153,7 +153,7 @@ def down end ensure migration.migrate(:down) - # $stdout = original + $stdout = original end def test_schema_dump_when_changing_datetime_type_for_an_existing_app From 98cd7d2a1a74adb686b370bd365b70179e405ca6 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 22 Sep 2023 17:16:26 -0500 Subject: [PATCH 03/17] fix(test): fk always valid This fixes the test `#test_all_foreign_keys_valid_having_foreign_keys_in_multiple_schemas`. --- .../cockroachdb/referential_integrity.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 548f772f..09604bdf 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -11,6 +11,14 @@ module ActiveRecord module ConnectionAdapters module CockroachDB module ReferentialIntegrity + # CockroachDB will raise a `PG::ForeignKeyViolation` when re-enabling + # referential integrity (e.g: adding a foreign key with invalid data + # raises). + # So foreign keys should always be valid for that matter. + def all_foreign_keys_valid? + true + end + def disable_referential_integrity foreign_keys = tables.map { |table| foreign_keys(table) }.flatten From b4bf6cbab7c32846d50fd46ebc450bce2091d8d9 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sat, 23 Sep 2023 10:23:42 -0500 Subject: [PATCH 04/17] chore(devex): Ease development - Allow using various schemas for the console, and always start with a clean slate. - If no internet, select latest used Rails version. - Allow to easily show schema loading logs. - Closer CRDB configuration between CI and local dev. - Add editor config file. --- .editorconfig | 7 +++++ Gemfile | 5 ++++ bin/console | 50 ++++++++++++---------------------- bin/console_schemas/default.rb | 9 ++++++ bin/console_schemas/schemas.rb | 23 ++++++++++++++++ bin/start-cockroachdb | 4 +++ test/cases/helper.rb | 12 ++++---- 7 files changed, 72 insertions(+), 38 deletions(-) create mode 100644 .editorconfig create mode 100644 bin/console_schemas/default.rb create mode 100644 bin/console_schemas/schemas.rb diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..770c0a21 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,7 @@ +root = true + +[*.rb] +indent_style = space +indent_size = 2 +trim_trailing_whitespace = true +insert_final_newline = true diff --git a/Gemfile b/Gemfile index 59681eb9..154f8d3d 100644 --- a/Gemfile +++ b/Gemfile @@ -10,6 +10,11 @@ module RailsTag def call req = gemspec_requirement "v" + all_activerecord_versions.find { req.satisfied_by?(_1) }.version + rescue => e + warn "Unable to determine Rails version. Using last used. Error: #{e.message}" + lockfile = File.expand_path("Gemfile.lock", __dir__) + File.foreach(lockfile, chomp: true).find { _1[/tag: (.*)$/] } + Regexp.last_match(1) end def gemspec_requirement diff --git a/bin/console b/bin/console index a88aae76..d81c2886 100755 --- a/bin/console +++ b/bin/console @@ -12,39 +12,23 @@ require "active_record" # structure_load(Post.connection_db_config, "awesome-file.sql") require "active_record/connection_adapters/cockroachdb/database_tasks" -begin - retried = false - ActiveRecord::Base.establish_connection( - #Alternative version: "cockroachdb://root@localhost:26257/ar_crdb_console" - adapter: "cockroachdb", - host: "localhost", - port: 26257, - user: "root", - database: "ar_crdb_console" - ) - ActiveRecord::Base.connection -rescue ActiveRecord::NoDatabaseError - raise if retried - system("cockroach sql --insecure --host=localhost:26257 --execute='create database ar_crdb_console'", - exception: true) - retried = true - retry -end - -class Post < ActiveRecord::Base -end - -unless Post.table_exists? - migration = Class.new(ActiveRecord::Migration::Current) do - def up - create_table("posts") do |t| - t.string :title - t.text :body - end - end - end - migration.migrate(:up) -end +schema_kind = ENV.fetch("SCHEMA_KIND", "default") + +system("cockroach sql --insecure --host=localhost:26257 --execute='drop database if exists ar_crdb_console'", + exception: true) +system("cockroach sql --insecure --host=localhost:26257 --execute='create database ar_crdb_console'", + exception: true) + +ActiveRecord::Base.establish_connection( + #Alternative version: "cockroachdb://root@localhost:26257/ar_crdb_console" + adapter: "cockroachdb", + host: "localhost", + port: 26257, + user: "root", + database: "ar_crdb_console" +) + +load "#{__dir__}/console_schemas/#{schema_kind}.rb" require "irb" IRB.start(__FILE__) diff --git a/bin/console_schemas/default.rb b/bin/console_schemas/default.rb new file mode 100644 index 00000000..ee3e8209 --- /dev/null +++ b/bin/console_schemas/default.rb @@ -0,0 +1,9 @@ +class Post < ActiveRecord::Base +end + +ActiveRecord::Schema.define do + create_table("posts") do |t| + t.string :title + t.text :body + end +end diff --git a/bin/console_schemas/schemas.rb b/bin/console_schemas/schemas.rb new file mode 100644 index 00000000..36a50c2e --- /dev/null +++ b/bin/console_schemas/schemas.rb @@ -0,0 +1,23 @@ +class Post < ActiveRecord::Base + self.table_name = "bar.posts" +end + +class Comment < ActiveRecord::Base + self.table_name = "foo.comments" +end + +ActiveRecord::Schema.define do + create_schema("foo") + create_schema("bar") + create_table("bar.posts") do |t| + t.string :title + t.text :body + end + + create_table("foo.comments") do |t| + t.integer :post_id + t.text :body + end + + add_foreign_key "foo.comments", "bar.posts", column: "post_id" +end diff --git a/bin/start-cockroachdb b/bin/start-cockroachdb index dd23e338..798418a2 100755 --- a/bin/start-cockroachdb +++ b/bin/start-cockroachdb @@ -9,6 +9,7 @@ pid_file="$root_dir/tmp/cockroach.pid" log_file="$root_dir/tmp/cockroachdb.log" mkdir -p "$root_dir/tmp" +[[ -f "$pid_file" ]] && kill -9 $(cat "$pid_file") rm -f "$pid_file" if ! (( ${+commands[cockroach]} )); then @@ -41,6 +42,9 @@ ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600; CREATE DATABASE activerecord_unittest; CREATE DATABASE activerecord_unittest2; + +SET CLUSTER SETTING sql.defaults.experimental_alter_column_type.enabled = 'true'; +SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true'; SQL tail -f "$log_file" diff --git a/test/cases/helper.rb b/test/cases/helper.rb index 64ace3b0..923e2d2c 100644 --- a/test/cases/helper.rb +++ b/test/cases/helper.rb @@ -194,10 +194,12 @@ def clean_up_connection_handler end end -def load_schema - # silence verbose schema loading - original_stdout = $stdout - $stdout = StringIO.new +def load_schema(shush = true) + if shush + # silence verbose schema loading + original_stdout = $stdout + $stdout = StringIO.new + end adapter_name = ActiveRecord::Base.connection.adapter_name.downcase adapter_specific_schema_file = SCHEMA_ROOT + "/#{adapter_name}_specific_schema.rb" @@ -210,7 +212,7 @@ def load_schema ActiveRecord::FixtureSet.reset_cache ensure - $stdout = original_stdout + $stdout = original_stdout if shush end if ENV['COCKROACH_LOAD_FROM_TEMPLATE'].nil? && ENV['COCKROACH_SKIP_LOAD_SCHEMA'].nil? From 121a89179c8849bbae1d7e6739863ec850a45656 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 3 Oct 2023 12:25:16 -0400 Subject: [PATCH 05/17] workflows: only trigger on pull_request Previously the action would be executed twice, once by a push event and once by a pull_request event. --- .github/workflows/ci.yml | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b50d00b8..77afa3bc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,12 +4,7 @@ name: Test on: - # Triggers the workflow on push or pull request events. - push: - # This should disable running the workflow on tags, according to the - # on.. GitHub Actions docs. - branches: - - "*" + # Triggers the workflow on pull request events. pull_request: types: [opened, reopened, synchronize] @@ -18,7 +13,7 @@ on: # This allows a subsequently queued workflow run to interrupt previous runs. concurrency: - group: "${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}" + group: "${{ github.workflow }} @ ${{ github.ref }}" cancel-in-progress: true jobs: From 01ab1281f8ecd6b25cddc48aba73a909fa7fbaee Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 3 Oct 2023 12:32:07 -0400 Subject: [PATCH 06/17] workflows: add a wrapper job to report test results This wrapper job will have a stable name, so it's easy to configure a required check on it. --- .github/workflows/ci.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77afa3bc..862b8365 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,21 @@ concurrency: cancel-in-progress: true jobs: + # Since the name of the matrix job depends on the version, we define another job with a more stable name. + test_results: + if: ${{ always() }} + runs-on: ubuntu-latest + name: Test Results + needs: [test] + steps: + - run: | + result="${{ needs.test.result }}" + if [[ $result == "success" || $result == "skipped" ]]; then + exit 0 + else + exit 1 + fi + test: runs-on: ubuntu-22.04 strategy: From 7beedf995c0ca8dcedead47b41830eefc36f2880 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 11 Jan 2024 08:21:35 -0300 Subject: [PATCH 07/17] workflow: trigger tests on master --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 862b8365..ee0957c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,8 @@ name: Test on: + push: + branches: [master] # Triggers the workflow on pull request events. pull_request: types: [opened, reopened, synchronize] From c8baf51f562b97b2192b19427a70fa474f045315 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sun, 18 Feb 2024 14:58:44 -0300 Subject: [PATCH 08/17] chore: matrix test for every crdb supported version --- .github/workflows/ci.yml | 43 +++++++++++++++------------------------- bin/start-cockroachdb | 25 +++++------------------ setup.sql | 18 +++++++++++++++++ 3 files changed, 39 insertions(+), 47 deletions(-) create mode 100644 setup.sql diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee0957c5..91e415aa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,15 +37,17 @@ jobs: test: runs-on: ubuntu-22.04 strategy: + fail-fast: false matrix: - crdb: [v24.1.10] - ruby: ["3.4"] + # https://www.cockroachlabs.com/docs/releases/release-support-policy + crdb: [v23.2, v24.1] + ruby: [head] name: Test (crdb=${{ matrix.crdb }} ruby=${{ matrix.ruby }}) steps: - name: Set Up Actions uses: actions/checkout@v3 - name: Install GEOS - run: sudo apt-get install libgeos-dev + run: sudo apt-get install -yqq libgeos-dev - name: Set Up Ruby uses: ruby/setup-ruby@v1 with: @@ -54,16 +56,23 @@ jobs: - name: Install and Start Cockroachdb run: | # Download CockroachDB - wget -qO- https://binaries.cockroachdb.com/cockroach-${{ matrix.crdb }}.linux-amd64.tgz | tar xvz + readonly patch=$( + ruby -rnet/http -ruri -e 'puts Net::HTTP.get( + URI("https://www.cockroachlabs.com/docs/releases/${{ matrix.crdb }}") + )[%r(https://binaries.cockroachdb.com/cockroach-${{ matrix.crdb }}.(\d+).linux-amd64.tgz), 1]' + ) - export PATH=./cockroach-${{ matrix.crdb }}.linux-amd64/:$PATH + echo "Downloading ${{ matrix.crdb }}.$patch" + wget -qO- "https://binaries.cockroachdb.com/cockroach-${{ matrix.crdb }}.$patch.linux-amd64.tgz" | tar xvz + + export PATH=./cockroach-${{ matrix.crdb }}.$patch.linux-amd64/:$PATH readonly urlfile=cockroach-url # Start a CockroachDB server and wait for it to become ready. rm -f "$urlfile" rm -rf cockroach-data # Start CockroachDB. - cockroach start-single-node --max-sql-memory=25% --cache=25% --insecure --host=localhost --spatial-libs=./cockroach-${{ matrix.crdb }}.linux-amd64/lib --listening-url-file="$urlfile" >/dev/null 2>&1 & + cockroach start-single-node --max-sql-memory=25% --cache=25% --insecure --host=localhost --spatial-libs=./cockroach-${{ matrix.crdb }}.$patch.linux-amd64/lib --listening-url-file="$urlfile" >/dev/null 2>&1 & # Ensure CockroachDB is stopped on script exit. # Wait until CockroachDB has started. for i in {0..3}; do @@ -72,26 +81,6 @@ jobs: echo "server not yet available; sleeping for $backoff seconds" sleep $backoff done - cockroach sql --insecure -e " - CREATE DATABASE activerecord_unittest; - CREATE DATABASE activerecord_unittest2; - SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false; - SET CLUSTER SETTING sql.stats.histogram_collection.enabled = false; - SET CLUSTER SETTING jobs.retention_time = '180s'; - SET CLUSTER SETTING sql.defaults.experimental_alter_column_type.enabled = 'true'; - - ALTER RANGE default CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; - ALTER TABLE system.public.jobs CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; - ALTER RANGE meta CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; - ALTER RANGE system CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; - ALTER RANGE liveness CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 30; - - SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms'; - SET CLUSTER SETTING jobs.registry.interval.cancel = '180s'; - SET CLUSTER SETTING jobs.registry.interval.gc = '30s'; - SET CLUSTER SETTING kv.range_split.by_load_merge_delay = '5s'; - - SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true'; - " + cat ${{ github.workspace }}/setup.sql | cockroach sql --insecure - name: Test run: bundle exec rake test diff --git a/bin/start-cockroachdb b/bin/start-cockroachdb index 798418a2..0f59cd49 100755 --- a/bin/start-cockroachdb +++ b/bin/start-cockroachdb @@ -9,7 +9,7 @@ pid_file="$root_dir/tmp/cockroach.pid" log_file="$root_dir/tmp/cockroachdb.log" mkdir -p "$root_dir/tmp" -[[ -f "$pid_file" ]] && kill -9 $(cat "$pid_file") +[[ -f "$pid_file" ]] && kill -9 $(cat "$pid_file") || true rm -f "$pid_file" if ! (( ${+commands[cockroach]} )); then @@ -18,7 +18,9 @@ See https://www.cockroachlabs.com/docs/stable/install-cockroachdb.html' fi cockroach start-single-node \ - --insecure --store=type=mem,size=0.25 --advertise-addr=localhost --pid-file "$pid_file" \ + --insecure --store=type=mem,size=0.25 --advertise-addr=localhost \ + --spatial-libs="$(geos-config --includes)" \ + --pid-file "$pid_file" \ &> "$log_file" & cockroach_pid=$! @@ -28,24 +30,7 @@ until [[ -f "$pid_file" ]]; do done -cat <<-SQL | cockroach sql --insecure --host=localhost:26257 > /dev/null --- https://www.cockroachlabs.com/docs/stable/local-testing.html -SET CLUSTER SETTING kv.raft_log.disable_synchronization_unsafe = true; -SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms'; -SET CLUSTER SETTING jobs.registry.interval.gc = '30s'; -SET CLUSTER SETTING jobs.registry.interval.cancel = '180s'; -SET CLUSTER SETTING jobs.retention_time = '15s'; -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false; -SET CLUSTER SETTING kv.range_split.by_load_merge_delay = '5s'; -ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600; -ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600; - -CREATE DATABASE activerecord_unittest; -CREATE DATABASE activerecord_unittest2; - -SET CLUSTER SETTING sql.defaults.experimental_alter_column_type.enabled = 'true'; -SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true'; -SQL +cat "$root_dir/setup.sql" | cockroach sql --insecure --host=localhost:26257 > /dev/null tail -f "$log_file" diff --git a/setup.sql b/setup.sql new file mode 100644 index 00000000..62c5086c --- /dev/null +++ b/setup.sql @@ -0,0 +1,18 @@ +-- https://www.cockroachlabs.com/docs/stable/local-testing.html +SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms'; +SET CLUSTER SETTING jobs.registry.interval.gc = '30s'; +SET CLUSTER SETTING jobs.registry.interval.cancel = '180s'; +SET CLUSTER SETTING jobs.retention_time = '15s'; +SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false; +SET CLUSTER SETTING kv.range_split.by_load_merge_delay = '5s'; +ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600; +ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600; + +CREATE DATABASE activerecord_unittest; +CREATE DATABASE activerecord_unittest2; + +SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false; +SET CLUSTER SETTING sql.stats.histogram_collection.enabled = false; + +SET CLUSTER SETTING sql.defaults.experimental_alter_column_type.enabled = 'true'; +SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true'; From 0d952e8d9861b4b01bb620e5fc5170efc383903f Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 21 Feb 2024 19:21:40 -0300 Subject: [PATCH 09/17] refactor: scrap yaml rather than html --- .github/workflows/ci.yml | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91e415aa..2664c72f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,23 +56,29 @@ jobs: - name: Install and Start Cockroachdb run: | # Download CockroachDB - readonly patch=$( - ruby -rnet/http -ruri -e 'puts Net::HTTP.get( - URI("https://www.cockroachlabs.com/docs/releases/${{ matrix.crdb }}") - )[%r(https://binaries.cockroachdb.com/cockroach-${{ matrix.crdb }}.(\d+).linux-amd64.tgz), 1]' - ) + readonly full_version=$(ruby -rnet/http -ruri -ryaml -e ' + link = "https://raw.githubusercontent.com/cockroachdb/docs/main/src/current/_data/releases.yml" + puts YAML.safe_load(Net::HTTP.get(URI(link))).reverse.find { + _1["major_version"] == "${{ matrix.crdb }}" && + _1["release_type"] == "Production" && + _1["linux"] && _1["linux"]["linux_intel_fips"] && + !_1["cloud_only"] && + !_1["withdrawn"] && + !_1["release_name"].include?("-") # Pre-release + }["release_name"] + ') - echo "Downloading ${{ matrix.crdb }}.$patch" - wget -qO- "https://binaries.cockroachdb.com/cockroach-${{ matrix.crdb }}.$patch.linux-amd64.tgz" | tar xvz + echo "Downloading $full_version..." + wget -qO- "https://binaries.cockroachdb.com/cockroach-$full_version.linux-amd64.tgz" | tar xvz - export PATH=./cockroach-${{ matrix.crdb }}.$patch.linux-amd64/:$PATH + export PATH=./cockroach-$full_version.linux-amd64/:$PATH readonly urlfile=cockroach-url # Start a CockroachDB server and wait for it to become ready. rm -f "$urlfile" rm -rf cockroach-data # Start CockroachDB. - cockroach start-single-node --max-sql-memory=25% --cache=25% --insecure --host=localhost --spatial-libs=./cockroach-${{ matrix.crdb }}.$patch.linux-amd64/lib --listening-url-file="$urlfile" >/dev/null 2>&1 & + cockroach start-single-node --max-sql-memory=25% --cache=25% --insecure --host=localhost --spatial-libs=./cockroach-$full_version.linux-amd64/lib --listening-url-file="$urlfile" >/dev/null 2>&1 & # Ensure CockroachDB is stopped on script exit. # Wait until CockroachDB has started. for i in {0..3}; do From 21965bafd43789141bd6a9ff7c496b754ed7116b Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 21 Feb 2024 23:44:19 -0300 Subject: [PATCH 10/17] fix: remove linux clause from version scanning --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2664c72f..ef66dfe2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,7 +61,6 @@ jobs: puts YAML.safe_load(Net::HTTP.get(URI(link))).reverse.find { _1["major_version"] == "${{ matrix.crdb }}" && _1["release_type"] == "Production" && - _1["linux"] && _1["linux"]["linux_intel_fips"] && !_1["cloud_only"] && !_1["withdrawn"] && !_1["release_name"].include?("-") # Pre-release From 64deeb3aecd884f43f2a0f7c6941f8e0dfde8379 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Sun, 5 Jan 2025 20:41:47 +0100 Subject: [PATCH 11/17] chore: faster referential integrity for tests --- CONTRIBUTING.md | 39 +++++--- .../cockroachdb/referential_integrity.rb | 90 ++++++++++++++++--- .../cockroachdb/schema_statements.rb | 25 ------ test/support/sql_logger.rb | 55 ++++++++++++ 4 files changed, 159 insertions(+), 50 deletions(-) create mode 100644 test/support/sql_logger.rb diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 22311f00..0a448545 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,6 +1,5 @@ # Getting started - ## ActiveRecord adapters and you There are two repositories for the ActiveRecord adapter. The one you're in @@ -121,7 +120,6 @@ master branch, with an alpha build of CockroachDB. it would be even better to be able to test multiple versions of the adapter, and do so against different versions of CockroachDB. - ## Adding feature support As CockroachDB improves, so do the features that can be supported in @@ -131,15 +129,34 @@ gates should be toggled. Something that would help this process would be linking those issues back to this adapter so that part of the feature completing includes updating the adapter. - ## Execute only tests that run with a connection -I have not investigated if this is already possible, but I would assume -no. - -A possible way to approach this would be to add a shim to cause any -tests that use it to fail, and grep the tests that pass and then skip -them. +- Check for TODO or NOTE tags that are referencing the old or new version of + rails. + ```bash + rg 'TODO|NOTE' --after-context=2 + ``` +- Check postgresql_specific_schema.rb changelog in rails, and apply the changes + you want. Ex: + ```bash + git diff v7.1.4..v7.2.1 -- $(fd postgresql_specific_schema) + ``` +- Verify the written text at the beginning of the test suite, there are likely + some changes in excluded tests. +- Check for some important methods, some will change for sure: + - [ ] `def new_column_from_field(` + - [ ] `def column_definitions(` + - [ ] `def pk_and_sequence_for(` + - [ ] `def foreign_keys(` and `def all_foreign_keys(` + - [ ] ... +- Check for setups containing `drop_table` in the test suite. + Especially if you have tons of failure, this is likely the cause. +- In the same way, run `test/cases/fixtures_test.rb` first, and check + if this corrupted the test database for other tests. +- For both of the above, the diff of `schema.rb` can be useful: + ```bash + git diff v7.1.2..v7.2.1 -- activerecord/test/schema/schema.rb + ``` ## Publishing to Rubygems @@ -151,7 +168,6 @@ gem build ... gem publish ``` - # Notes When executing the test suite, each test file will reload fixtures. This @@ -182,10 +198,8 @@ cleaned up, or skipped until passing. The purpose of these was to make the tests grep-able while going through all the failures. - [cockroachdb/cockroach#20753]: https://github.com/cockroachdb/cockroach/issues/20753#issuecomment-352810425 - ## Tracked test failures Some of the skipped failures are: @@ -224,7 +238,6 @@ most of the touchpoints including test failures and temporary monkey patches. Some monkey patches were made directly to Rails, which will need to be cleaned up. - # Notes for the non-Rubyer rbenv is an environment manager that lets you manage and swap between diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 09604bdf..f8f78813 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -20,10 +20,16 @@ def all_foreign_keys_valid? end def disable_referential_integrity - foreign_keys = tables.map { |table| foreign_keys(table) }.flatten + foreign_keys = all_foreign_keys foreign_keys.each do |foreign_key| - remove_foreign_key(foreign_key.from_table, name: foreign_key.options[:name]) + # We do not use the `#remove_foreign_key` method here because it + # checks for foreign keys existance in the schema cache. This method + # is performance critical and we know the foreign key exist. + at = create_alter_table foreign_key.from_table + at.drop_foreign_key foreign_key.name + + execute schema_creation.accept(at) end yield @@ -38,23 +44,83 @@ def disable_referential_integrity ActiveRecord::Base.table_name_suffix = "" begin + # 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) + # + # We avoid using `foreign_key_exists?` here because it checks the schema cache + # for every key. This method is performance critical for the test suite, hence + # we use the `#all_foreign_keys` method that only make one query to the database. + already_inserted_foreign_keys = all_foreign_keys foreign_keys.each do |foreign_key| - begin - add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options) - rescue ActiveRecord::StatementInvalid => error - if error.cause.class == PG::DuplicateObject - # This error is safe to ignore because the yielded caller - # already re-added the foreign key constraint. - else - raise error - end - end + next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[: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 + + private + + # Copy/paste of the `#foreign_keys(table)` method adapted to return every single + # foreign key in the database. + def all_foreign_keys + fk_info = internal_exec_query(<<~SQL, "SCHEMA") + SELECT CASE + WHEN n1.nspname = current_schema() + THEN '' + ELSE n1.nspname || '.' + END || t1.relname AS from_table, + CASE + WHEN n2.nspname = current_schema() + THEN '' + ELSE n2.nspname || '.' + END || t2.relname AS to_table, + a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, + c.conkey, c.confkey, c.conrelid, c.confrelid + FROM pg_constraint c + JOIN pg_class t1 ON c.conrelid = t1.oid + JOIN pg_class t2 ON c.confrelid = t2.oid + JOIN pg_attribute a1 ON a1.attnum = c.conkey[1] AND a1.attrelid = t1.oid + JOIN pg_attribute a2 ON a2.attnum = c.confkey[1] AND a2.attrelid = t2.oid + JOIN pg_namespace t3 ON c.connamespace = t3.oid + JOIN pg_namespace n1 ON t1.relnamespace = n1.oid + JOIN pg_namespace n2 ON t2.relnamespace = n2.oid + WHERE c.contype = 'f' + ORDER BY c.conname + SQL + + fk_info.map do |row| + from_table = PostgreSQL::Utils.unquote_identifier(row["from_table"]) + to_table = PostgreSQL::Utils.unquote_identifier(row["to_table"]) + conkey = row["conkey"].scan(/\d+/).map(&:to_i) + confkey = row["confkey"].scan(/\d+/).map(&:to_i) + + if conkey.size > 1 + column = column_names_from_column_numbers(row["conrelid"], conkey) + primary_key = column_names_from_column_numbers(row["confrelid"], confkey) + else + column = PostgreSQL::Utils.unquote_identifier(row["column"]) + primary_key = row["primary_key"] + end + + options = { + column: column, + name: row["name"], + primary_key: primary_key + } + options[:on_delete] = extract_foreign_key_action(row["on_delete"]) + options[:on_update] = extract_foreign_key_action(row["on_update"]) + options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"]) + + options[:validate] = row["valid"] + + ForeignKeyDefinition.new(from_table, to_table, options) + end + end end end end diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index 14c0848e..a5aa8266 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -245,31 +245,6 @@ def type_to_sql(type, limit: nil, precision: nil, scale: nil, array: nil, **) # sql end - # This overrides the method from PostegreSQL adapter - # Resets the sequence of a table's primary key to the maximum value. - def reset_pk_sequence!(table, pk = nil, sequence = nil) - unless pk && sequence - default_pk, default_sequence = pk_and_sequence_for(table) - - pk ||= default_pk - sequence ||= default_sequence - end - - if @logger && pk && !sequence - @logger.warn "#{table} has primary key #{pk} with no default sequence." - end - - if pk && sequence - quoted_sequence = quote_table_name(sequence) - max_pk = query_value("SELECT MAX(#{quote_column_name pk}) FROM #{quote_table_name(table)}", "SCHEMA") - if max_pk.nil? - minvalue = query_value("SELECT seqmin FROM pg_sequence WHERE seqrelid = #{quote(quoted_sequence)}::regclass", "SCHEMA") - end - - query_value("SELECT setval(#{quote(quoted_sequence)}, #{max_pk ? max_pk : minvalue}, #{max_pk ? true : false})", "SCHEMA") - end - end - # override def native_database_types # Add spatial types diff --git a/test/support/sql_logger.rb b/test/support/sql_logger.rb new file mode 100644 index 00000000..e548e05e --- /dev/null +++ b/test/support/sql_logger.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module SQLLogger + module_function + + def stdout_log + ActiveRecord::Base.logger = Logger.new(STDOUT) + ActiveRecord::Base.logger.level = Logger::DEBUG + ActiveRecord::LogSubscriber::IGNORE_PAYLOAD_NAMES.clear + ActiveRecord::Base.logger.formatter = proc { |severity, time, progname, msg| + th = Thread.current[:name] + th = "THREAD=#{th}" if th + Logger::Formatter.new.call(severity, time, progname || th, msg) + } + end + + def summary_log + ActiveRecord::TotalTimeSubscriber.attach_to :active_record + Minitest.after_run { + detail = ActiveRecord::TotalTimeSubscriber.hash.map { |k,v| [k, [v.sum, v.sum / v.size, v.size]]}.sort_by { |_, (_total, avg, _)| -avg }.to_h + time = detail.values.sum { |(total, _, _)| total } / 1_000 + count = detail.values.sum { |(_, _, count)| count } + puts "Total time spent in SQL: #{time}s (#{count} queries)" + puts "Detail per query kind available in tmp/query_time.json (total time in ms, avg time in ms, query count). Sorted by avg time." + File.write( + "tmp/query_time.json", + JSON.pretty_generate(detail) + ) + } + end + + # Remove content between single quotes and double quotes from keys + # to have a clear idea of which queries are being executed. + def clean_sql(sql) + sql.gsub(/".*?"/m, "\"...\"").gsub("''", "").gsub(/'.*?'/m, "'...'") + end +end + +class ActiveRecord::TotalTimeSubscriber < ActiveRecord::LogSubscriber + def self.hash + @@hash + end + + def sql(event) + # NOTE: If you want to debug a specific query, you can use a 'binding.irb' here with + # a specific condition on 'event.payload[:sql]' content. + # + # binding.irb if event.payload[:sql].include?("attr.attname, nsp.nspname") + # + @@hash ||= {} + key = SQLLogger.clean_sql(event.payload[:sql]) + @@hash[key] ||= [] + @@hash[key].push event.duration + end +end From 2874f9f7f859d979852d5d0d0368aeeecda91353 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 21 Jan 2025 15:25:12 +0100 Subject: [PATCH 12/17] refactor(test): only dump necessary table in schema dumper tests --- test/cases/migration/hidden_column_test.rb | 2 +- test/cases/schema_dumper_test.rb | 353 +++++++++------------ test/excludes/UnloggedTablesTest.rb | 4 +- 3 files changed, 160 insertions(+), 199 deletions(-) diff --git a/test/cases/migration/hidden_column_test.rb b/test/cases/migration/hidden_column_test.rb index 956b8e1e..785d0beb 100644 --- a/test/cases/migration/hidden_column_test.rb +++ b/test/cases/migration/hidden_column_test.rb @@ -62,7 +62,7 @@ def test_remove_index_with_a_hidden_column CREATE INDEX hash_idx ON rockets (name) USING HASH WITH (bucket_count=8); SQL @connection.remove_index :rockets, :name - # assert :ok + assert :ok end end end diff --git a/test/cases/schema_dumper_test.rb b/test/cases/schema_dumper_test.rb index ccecc463..7a09e6db 100644 --- a/test/cases/schema_dumper_test.rb +++ b/test/cases/schema_dumper_test.rb @@ -11,158 +11,42 @@ class SchemaDumperTest < ActiveRecord::TestCase self.use_transactional_tests = false setup do - ActiveRecord::SchemaMigration.create_table + @schema_migration = ActiveRecord::Base.connection_pool.schema_migration + @schema_migration.create_table end - def standard_dump - @@standard_dump ||= perform_schema_dump - end - - def perform_schema_dump - dump_all_table_schema [] - end - - if current_adapter?(:PostgreSQLAdapter) - def test_schema_dump_with_timestamptz_datetime_format - migration, original, $stdout = nil, $stdout, StringIO.new - - with_cockroachdb_datetime_type(:timestamptz) do - migration = Class.new(ActiveRecord::Migration::Current) do - def up - create_table("timestamps") do |t| - t.datetime :this_should_remain_datetime - t.timestamptz :this_is_an_alias_of_datetime - t.column :without_time_zone, :timestamp - t.column :with_time_zone, :timestamptz - end - end - def down - drop_table("timestamps") - end - end - migration.migrate(:up) - - output = perform_schema_dump - assert output.include?('t.datetime "this_should_remain_datetime"') - assert output.include?('t.datetime "this_is_an_alias_of_datetime"') - assert output.include?('t.timestamp "without_time_zone"') - assert output.include?('t.datetime "with_time_zone"') + # See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/347 + def test_dump_index_rather_than_unique_constraints + ActiveRecord::Base.with_connection do |conn| + conn.create_table :payments, force: true do |t| + t.text "name" + t.integer "value" + t.unique_constraint ["name", "value"], name: "as_unique_constraint" # Will be ignored + t.index "lower(name::STRING) ASC", name: "simple_unique", unique: true + t.index "name", name: "unique_with_where", where: "name IS NOT NULL", unique: true end - ensure - migration.migrate(:down) - $stdout = original end - def test_schema_dump_with_correct_timestamp_types_via_add_column_with_type_as_string - migration, original, $stdout = nil, $stdout, StringIO.new + output = dump_table_schema("payments") - with_cockroachdb_datetime_type(:timestamptz) do - migration = Class.new(ActiveRecord::Migration[6.1]) do - def up - create_table("timestamps") - - add_column :timestamps, :this_should_change_to_timestamp, "datetime" - add_column :timestamps, :this_should_stay_as_timestamp, "timestamp" - end - def down - drop_table("timestamps") - end - end - migration.migrate(:up) - - output = perform_schema_dump - # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` - # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns - # are still created as a `:timestamp` we need to change what is written to the schema dump. - # - # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) - # but that doesn't work here because the schema dumper is not aware of which migration - # a column was added in. - assert output.include?('t.timestamp "this_should_change_to_timestamp"') - assert output.include?('t.timestamp "this_should_stay_as_timestamp"') - end - ensure - migration.migrate(:down) - $stdout = original - end - - def test_timestamps_schema_dump_before_rails_7_with_timestamptz_setting - migration, original, $stdout = nil, $stdout, StringIO.new - - with_cockroachdb_datetime_type(:timestamptz) do - migration = Class.new(ActiveRecord::Migration[6.1]) do - def up - create_table("timestamps") do |t| - t.datetime :this_should_change_to_timestamp - t.timestamp :this_should_stay_as_timestamp - t.column :this_should_also_stay_as_timestamp, :timestamp - end - end - def down - drop_table("timestamps") - end - end - migration.migrate(:up) - - output = perform_schema_dump - # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` - # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns - # are still created as a `:timestamp` we need to change what is written to the schema dump. - # - # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) - # but that doesn't work here because the schema dumper is not aware of which migration - # a column was added in. - - assert output.include?('t.timestamp "this_should_change_to_timestamp"') - assert output.include?('t.timestamp "this_should_stay_as_timestamp"') - assert output.include?('t.timestamp "this_should_also_stay_as_timestamp"') - end - ensure - migration.migrate(:down) - $stdout = original - end - - def test_schema_dump_with_correct_timestamp_types_via_add_column_before_rails_7_with_timestamptz_setting - migration, original, $stdout = nil, $stdout, StringIO.new - - with_cockroachdb_datetime_type(:timestamptz) do - migration = Class.new(ActiveRecord::Migration[6.1]) do - def up - create_table("timestamps") - - add_column :timestamps, :this_should_change_to_timestamp, :datetime - add_column :timestamps, :this_should_stay_as_timestamp, :timestamp - end - def down - drop_table("timestamps") - end - end - migration.migrate(:up) - - output = perform_schema_dump - # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` - # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns - # are still created as a `:timestamp` we need to change what is written to the schema dump. - # - # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) - # but that doesn't work here because the schema dumper is not aware of which migration - # a column was added in. - - assert output.include?('t.timestamp "this_should_change_to_timestamp"') - assert output.include?('t.timestamp "this_should_stay_as_timestamp"') - end - ensure - migration.migrate(:down) - $stdout = original + index_lines = output.each_line.select { _1[/simple_unique|unique_with_where|as_unique_constraint/] } + assert_equal 2, index_lines.size + index_lines.each do |line| + assert_match(/t.index/, line) end + ensure + ActiveRecord::Base.with_connection { _1.drop_table :payments, if_exists: true } + end - def test_schema_dump_when_changing_datetime_type_for_an_existing_app - original, $stdout = $stdout, StringIO.new + def test_schema_dump_with_timestamptz_datetime_format + migration, original, $stdout = nil, $stdout, StringIO.new + with_cockroachdb_datetime_type(:timestamptz) do migration = Class.new(ActiveRecord::Migration::Current) do def up create_table("timestamps") do |t| - t.datetime :default_format + t.datetime :this_should_remain_datetime + t.timestamptz :this_is_an_alias_of_datetime t.column :without_time_zone, :timestamp t.column :with_time_zone, :timestamptz end @@ -173,88 +57,163 @@ def down end migration.migrate(:up) - output = perform_schema_dump - assert output.include?('t.datetime "default_format"') - assert output.include?('t.datetime "without_time_zone"') - assert output.include?('t.timestamptz "with_time_zone"') + output = dump_table_schema "timestamps" - datetime_type_was = ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type - ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type = :timestamptz - - output = perform_schema_dump - assert output.include?('t.timestamp "default_format"') + assert output.include?('t.datetime "this_should_remain_datetime"') + assert output.include?('t.datetime "this_is_an_alias_of_datetime"') assert output.include?('t.timestamp "without_time_zone"') assert output.include?('t.datetime "with_time_zone"') - ensure - ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type = datetime_type_was - migration.migrate(:down) - $stdout = original end + ensure + migration.migrate(:down) + $stdout = original + end + + def test_schema_dump_with_correct_timestamp_types_via_add_column_with_type_as_string + migration, original, $stdout = nil, $stdout, StringIO.new + + with_cockroachdb_datetime_type(:timestamptz) do + migration = Class.new(ActiveRecord::Migration[6.1]) do + def up + create_table("timestamps") - if ActiveRecord::Base.connection.supports_check_constraints? - def test_schema_dumps_check_constraints - constraint_definition = dump_table_schema("products").split(/\n/).grep(/t.check_constraint.*products_price_check/).first.strip - if current_adapter?(:Mysql2Adapter) - assert_equal 't.check_constraint "`price` > `discounted_price`", name: "products_price_check"', constraint_definition - else - assert_equal 't.check_constraint "(price > discounted_price)", name: "products_price_check"', constraint_definition + add_column :timestamps, :this_should_change_to_timestamp, "datetime" + add_column :timestamps, :this_should_stay_as_timestamp, "timestamp" + end + def down + drop_table("timestamps") end end + migration.migrate(:up) + + output = dump_table_schema "timestamps" + # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` + # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns + # are still created as a `:timestamp` we need to change what is written to the schema dump. + # + # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) + # but that doesn't work here because the schema dumper is not aware of which migration + # a column was added in. + assert output.include?('t.timestamp "this_should_change_to_timestamp"') + assert output.include?('t.timestamp "this_should_stay_as_timestamp"') end + ensure + migration.migrate(:down) + $stdout = original + end - def test_schema_dump_defaults_with_universally_supported_types - migration = Class.new(ActiveRecord::Migration::Current) do + def test_timestamps_schema_dump_before_rails_7_with_timestamptz_setting + migration, original, $stdout = nil, $stdout, StringIO.new + + with_cockroachdb_datetime_type(:timestamptz) do + migration = Class.new(ActiveRecord::Migration[6.1]) do def up - create_table("defaults_with_universally_supported_types") do |t| - t.string :string_with_default, default: 'Hello!' - t.date :date_with_default, default: '2014-06-05' - t.datetime :datetime_with_default, default: '2014-06-05 07:17:04' - t.time :time_with_default, default: '2000-01-01 07:17:04' - t.decimal :decimal_with_default, precision: 20, scale: 10, default: '1234567890.0123456789' + create_table("timestamps") do |t| + t.datetime :this_should_change_to_timestamp + t.timestamp :this_should_stay_as_timestamp + t.column :this_should_also_stay_as_timestamp, :timestamp end end def down - drop_table("defaults_with_universally_supported_types") + drop_table("timestamps") end end migration.migrate(:up) - output = perform_schema_dump + output = dump_table_schema "timestamps" + # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` + # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns + # are still created as a `:timestamp` we need to change what is written to the schema dump. + # + # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) + # but that doesn't work here because the schema dumper is not aware of which migration + # a column was added in. + + assert output.include?('t.timestamp "this_should_change_to_timestamp"') + assert output.include?('t.timestamp "this_should_stay_as_timestamp"') + assert output.include?('t.timestamp "this_should_also_stay_as_timestamp"') + end + ensure + migration.migrate(:down) + $stdout = original + end - assert output.include?('t.string "string_with_default", default: "Hello!"') - assert output.include?('t.date "date_with_default", default: "2014-06-05"') + def test_schema_dump_with_correct_timestamp_types_via_add_column_before_rails_7_with_timestamptz_setting + migration, original, $stdout = nil, $stdout, StringIO.new - if supports_datetime_with_precision? - assert output.include?('t.datetime "datetime_with_default", default: "2014-06-05 07:17:04"') - else - assert output.include?('t.datetime "datetime_with_default", precision: nil, default: "2014-06-05 07:17:04"') + with_cockroachdb_datetime_type(:timestamptz) do + migration = Class.new(ActiveRecord::Migration[6.1]) do + def up + create_table("timestamps") + + add_column :timestamps, :this_should_change_to_timestamp, :datetime + add_column :timestamps, :this_should_stay_as_timestamp, :timestamp + end + def down + drop_table("timestamps") + end end + migration.migrate(:up) - assert output.include?('t.time "time_with_default", default: "2000-01-01 07:17:04"') - assert output.include?('t.decimal "decimal_with_default", precision: 20, scale: 10, default: "1234567890.0123456789"') - ensure - migration.migrate(:down) + output = dump_table_schema "timestamps" + # Normally we'd write `t.datetime` here. But because you've changed the `datetime_type` + # to something else, `t.datetime` now means `:timestamptz`. To ensure that old columns + # are still created as a `:timestamp` we need to change what is written to the schema dump. + # + # Typically in Rails we handle this through Migration versioning (`ActiveRecord::Migration::Compatibility`) + # but that doesn't work here because the schema dumper is not aware of which migration + # a column was added in. + + assert output.include?('t.timestamp "this_should_change_to_timestamp"') + assert output.include?('t.timestamp "this_should_stay_as_timestamp"') end + ensure + migration.migrate(:down) + $stdout = original + end - if supports_text_column_with_default? - def test_schema_dump_with_text_column - migration = Class.new(ActiveRecord::Migration::Current) do - def up - create_table("text_column_with_default") do |t| - t.text :text_with_default, default: "John' Doe" - end - end - def down - drop_table("text_column_with_default") - end - end - migration.migrate(:up) + def test_schema_dump_when_changing_datetime_type_for_an_existing_app + original, $stdout = $stdout, StringIO.new - output = perform_schema_dump + migration = Class.new(ActiveRecord::Migration::Current) do + def up + create_table("timestamps") do |t| + t.datetime :default_format + t.column :without_time_zone, :timestamp + t.column :with_time_zone, :timestamptz + end + end + def down + drop_table("timestamps") + end + end + migration.migrate(:up) + + output = dump_table_schema "timestamps" + assert output.include?('t.datetime "default_format"') + assert output.include?('t.datetime "without_time_zone"') + assert output.include?('t.timestamptz "with_time_zone"') + + datetime_type_was = ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type + ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type = :timestamptz + + output = dump_table_schema "timestamps" + assert output.include?('t.timestamp "default_format"') + assert output.include?('t.timestamp "without_time_zone"') + assert output.include?('t.datetime "with_time_zone"') + ensure + ActiveRecord::ConnectionAdapters::CockroachDBAdapter.datetime_type = datetime_type_was + migration.migrate(:down) + $stdout = original + end - assert output.include?('t.text "text_with_default", default: "John\' Doe"') - ensure - migration.migrate(:down) + if ActiveRecord::Base.lease_connection.supports_check_constraints? + def test_schema_dumps_check_constraints + constraint_definition = dump_table_schema("products").split(/\n/).grep(/t.check_constraint.*products_price_check/).first.strip + if current_adapter?(:Mysql2Adapter) + assert_equal 't.check_constraint "`price` > `discounted_price`", name: "products_price_check"', constraint_definition + else + assert_equal 't.check_constraint "(price > discounted_price)", name: "products_price_check"', constraint_definition end end end diff --git a/test/excludes/UnloggedTablesTest.rb b/test/excludes/UnloggedTablesTest.rb index 6b0f7b13..dc25c6ba 100644 --- a/test/excludes/UnloggedTablesTest.rb +++ b/test/excludes/UnloggedTablesTest.rb @@ -1 +1,3 @@ -exclude :test_unlogged_in_test_environment_when_unlogged_setting_enabled, "Override because UNLOGGED cannot be specified in CockroachDB. Related https://github.com/cockroachdb/cockroach/issues/56827" +instance_methods.grep(/\Atest_\w+\z/).each do |method_name| + exclude method_name, "UNLOGGED has no effect in CockroachDB." +end From 26f7e439457402cd1c8985f04cb141692d042ea2 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 22 Jan 2025 16:29:12 +0100 Subject: [PATCH 13/17] refactor(fixtures): Avoid disabling referential integrity Avoid 87% of the `#disable_referential_integrity` calls made in fixture setup by just trying first without disabling and just disabling in case of failure. Also batch foreign keys removals and addition (for clarity only). --- .../cockroachdb/database_statements.rb | 14 +++++++++++--- .../cockroachdb/referential_integrity.rb | 14 ++++++++++---- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/database_statements.rb b/lib/active_record/connection_adapters/cockroachdb/database_statements.rb index 84295bb8..3170fb47 100644 --- a/lib/active_record/connection_adapters/cockroachdb/database_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/database_statements.rb @@ -23,9 +23,17 @@ def insert_fixtures_set(fixture_set, tables_to_delete = []) table_deletes = tables_to_delete.map { |table| "DELETE FROM #{quote_table_name(table)}" } statements = table_deletes + fixture_inserts - with_multi_statements do - disable_referential_integrity do - execute_batch(statements, "Fixtures Load") + begin # much faster without disabling referential integrity, worth trying. + with_multi_statements do + transaction(requires_new: true) do + execute_batch(statements, "Fixtures Load") + end + end + rescue + with_multi_statements do + disable_referential_integrity do + execute_batch(statements, "Fixtures Load") + end end end end diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index f8f78813..6e05f0ff 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -22,15 +22,16 @@ def all_foreign_keys_valid? def disable_referential_integrity foreign_keys = all_foreign_keys - foreign_keys.each do |foreign_key| + statements = foreign_keys.map do |foreign_key| # We do not use the `#remove_foreign_key` method here because it # checks for foreign keys existance in the schema cache. This method # is performance critical and we know the foreign key exist. at = create_alter_table foreign_key.from_table at.drop_foreign_key foreign_key.name - execute schema_creation.accept(at) + schema_creation.accept(at) end + execute_batch(statements, "Disable referential integrity -> remove foreign keys") yield @@ -52,11 +53,16 @@ def disable_referential_integrity # for every key. This method is performance critical for the test suite, hence # we use the `#all_foreign_keys` method that only make one query to the database. already_inserted_foreign_keys = all_foreign_keys - foreign_keys.each do |foreign_key| + statements = foreign_keys.map do |foreign_key| next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[:name] == foreign_key.options[:name] } - add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options) + options = foreign_key_options(foreign_key.from_table, foreign_key.to_table, foreign_key.options) + at = create_alter_table foreign_key.from_table + at.add_foreign_key foreign_key.to_table, options + + schema_creation.accept(at) end + execute_batch(statements.compact, "Disable referential integrity -> add foreign keys") ensure ActiveRecord::Base.table_name_prefix = old_prefix ActiveRecord::Base.table_name_suffix = old_suffix From 10fdc7e4a7d07b6a4553a1fde2339282ebd24b19 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 23 Jan 2025 11:28:09 +0100 Subject: [PATCH 14/17] workflows: set ruby to 3.4 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef66dfe2..b5db62df 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,7 +41,7 @@ jobs: matrix: # https://www.cockroachlabs.com/docs/releases/release-support-policy crdb: [v23.2, v24.1] - ruby: [head] + ruby: [3.4] name: Test (crdb=${{ matrix.crdb }} ruby=${{ matrix.ruby }}) steps: - name: Set Up Actions From c901774d07f9df5407302c4abcf9979da852d9bd Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 23 Jan 2025 11:30:10 +0100 Subject: [PATCH 15/17] fix: internal_exec_query not yet in rails --- .../connection_adapters/cockroachdb/referential_integrity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 6e05f0ff..64193434 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -74,7 +74,7 @@ def disable_referential_integrity # Copy/paste of the `#foreign_keys(table)` method adapted to return every single # foreign key in the database. def all_foreign_keys - fk_info = internal_exec_query(<<~SQL, "SCHEMA") + fk_info = exec_query(<<~SQL, "SCHEMA") SELECT CASE WHEN n1.nspname = current_schema() THEN '' From 9854845af6b0417482daafe17614b348dd30c8d4 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 23 Jan 2025 11:34:58 +0100 Subject: [PATCH 16/17] fix: old naming for constraint_deferrable --- .../connection_adapters/cockroachdb/referential_integrity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 64193434..24f9830f 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -120,7 +120,7 @@ def all_foreign_keys } options[:on_delete] = extract_foreign_key_action(row["on_delete"]) options[:on_update] = extract_foreign_key_action(row["on_update"]) - options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"]) + options[:deferrable] = extract_foreign_key_deferrable(row["deferrable"], row["deferred"]) options[:validate] = row["valid"] From e3279a070e5f1ea3d66226ad3c1ca5ccdae68988 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 23 Jan 2025 11:37:41 +0100 Subject: [PATCH 17/17] fix: lease_connection -> connection --- test/cases/primary_keys_test.rb | 2 +- test/cases/schema_dumper_test.rb | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/test/cases/primary_keys_test.rb b/test/cases/primary_keys_test.rb index e99fdfce..6b8276f0 100644 --- a/test/cases/primary_keys_test.rb +++ b/test/cases/primary_keys_test.rb @@ -103,7 +103,7 @@ class Rocket < ActiveRecord::Base end def setup - connection = ActiveRecord::Base.lease_connection + connection = ActiveRecord::Base.connection connection.execute <<-SQL CREATE TABLE rockets( id SERIAL PRIMARY KEY USING HASH WITH (bucket_count=4) diff --git a/test/cases/schema_dumper_test.rb b/test/cases/schema_dumper_test.rb index 7a09e6db..9147c72b 100644 --- a/test/cases/schema_dumper_test.rb +++ b/test/cases/schema_dumper_test.rb @@ -10,11 +10,6 @@ class SchemaDumperTest < ActiveRecord::TestCase include SchemaDumpingHelper self.use_transactional_tests = false - setup do - @schema_migration = ActiveRecord::Base.connection_pool.schema_migration - @schema_migration.create_table - end - # See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/347 def test_dump_index_rather_than_unique_constraints ActiveRecord::Base.with_connection do |conn| @@ -207,7 +202,7 @@ def down $stdout = original end - if ActiveRecord::Base.lease_connection.supports_check_constraints? + if ActiveRecord::Base.connection.supports_check_constraints? def test_schema_dumps_check_constraints constraint_definition = dump_table_schema("products").split(/\n/).grep(/t.check_constraint.*products_price_check/).first.strip if current_adapter?(:Mysql2Adapter)