feat: upgrade to Rails 8.1 #386

BuonOmo · 2025-10-09T11:18:56Z

TODOs:

publish the new rgeo-activerecord 8.1 compatible gem (see bump to 8.1 rgeo/rgeo-activerecord#84)
verify that the new implemetation of #foreign_keys works if t2 doesn't belong to the same namespace as t1

BuonOmo · 2025-10-30T10:46:04Z

lib/active_record/connection_adapters/cockroachdb_adapter.rb

-        #
-        # @see: https://github.com/rails/rails/blob/8695b028261bdd244e254993255c6641bdbc17a5/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L829
+        # OVERRIDE(v8.1.1):
+        #   - comment is retrieved differently than PG for performance


col_description is the method used to retrieve the comment in PG. @rafiss is still true that this is slower than using information_schema.columns ?

BuonOmo · 2025-10-30T10:50:08Z

lib/active_record/connection_adapters/cockroachdb/schema_statements.rb

+                            ARRAY(
+                              SELECT a.attname
+                              FROM pg_attribute a
+                              WHERE a.attrelid = d.indexrelid AND a.attishidden
+                            ) AS hidden_columns


@rafiss
I added this bit. I'm really unsure about performances, any better idea would be appreciated.

The goal is to remove hidden columns from the columns above. My main issue was that we are using pg_get_indexdef to retrieve the columns. I don't know edge cases so I was not eager to use another method, but if we were not to use it I feel like we could easily have a faster and simpler approach.

My second question is: do you have a simple routine to benchmark two slightly different columns in CRDB, and pitfalls I should avoid?

EDIT: one of the ideas I had:

ARRAY( SELECT a.attname FROM pg_attribute a JOIN unnest(d.indkey) AS k(attnum) ON a.attrelid = d.indrelid AND a.attnum = k.attnum AND NOT a.attishidden ) AS visible_columns

Using pg_get_indexdef should actually be fine. The CRDB implementation is a function with a SQL body (https://github.com/cockroachdb/cockroach/blob/08676709fc2c9ee4f9cdbfeb4601b71b273773d8/pkg/sql/sem/builtins/pg_builtins.go#L862), so what that means in practice is that all the SQL gets inlined into the query that's using the function.

do you have a simple routine to benchmark two slightly different columns in CRDB, and pitfalls I should avoid?

Hm, nothing is available externally, but we do have an internal benchmark suite in our codebase: https://github.com/cockroachdb/cockroach/blob/master/pkg/bench/rttanalysis/orm_queries_bench_test.go

I think that's overkill for what you're doing. If you are able to run locally, I think the best way to do a basic comparison is to create ~100 tables and then try executing the different queries to see what's faster.

Using pg_get_indexdef should actually be fine.

I actually wanted not to use it, to have better perf. Although from what I can see now, I doesn't seem trivial to replace it and improve perf

Maimer · 2025-11-13T20:02:15Z

Any timeline when this will get merged and released?

BuonOmo · 2025-11-13T22:28:28Z

@Maimer the PR should be ready during next week. @rafiss would you be able to make a release within two weeks from now?

BuonOmo · 2025-11-19T09:10:32Z

lib/active_record/connection_adapters/cockroachdb_adapter.rb

        false
      end

+      def supports_close_prepared?


This seemed true by testing in CRDB 25.2, but I could not see any documentation on the CRDB side.

Here's the pg backend function as a reference : https://github.com/postgres/postgres/blob/master/src/backend/commands/prepare.c#L519

And libpq interface is PqMsg_Close with the Statement type ('C', 'S'). In the cockroach codebase I could find ClientMsgClose that maps to PqMsg_Close, but I did not find handling of close_prepared, yet it works... Any clue on that feature @rafiss ?

See rails/rails@205cdd5

- update some outdated overrides. - update override comments to know when was the last override. - remove the `#column_names_from_column_numbers` method that was generating N+1 queries. - update related methods. See: - rails/rails@249c367 - rails/rails@4e0546c - rails/rails@c93d1b0

This would silently fail in the test `test_bulk_insert_with_a_multi_statement_query_raises_an_exception_when_any_insert_fails`. Removing some foreign keys and thus failing afterwards in the test suite. Moreover, the `#disable_referential_integrity` method is unfortunately public, so we need it to be robust.

BuonOmo · 2025-11-19T10:09:07Z

lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb

+        #   prefixes and suffixes. The good thing is: if this happens, tests will crash
+        #   hard, no way we miss it.


Well, the test retry logic was actually proving me wrong. But now that it is removed, we are fine (see later commit)

- remove `debugging?` utility - use variables for `autocommit_before_ddl`: > This is the default intended approach, as it will > quote value if needed. - remove spatial_column_info.rb and its calls as it is never used. See #391 to maybe include it back, properly. - fix geos-config file fetching - refactor for loop into any? - unexclude working tests - reset `test_create_fixtures` to the original, it works. And our version would corrupt foreign keys

If `#disable_referential_integrity` is ran within a transaction, we may be able to still run the user's code. Otherwise we warn the user why it failed.

Since CockroachDB does not support DDL transactions, we adapt the test.

This fixes a bug where our `TestRetryHelper` logic combined with `reset_fixtures` trying to reset a table without foreign keys from another table. It would first crash, removing the foreign key constraint (due to how we handle `disable_referential_integrity`). And then pass, since the foreign key constraint is gone. But we need that constraint in later tests.

This is WIP and there is likely more to do. Hence the reference #390. Fixes the flaky test `PostGISTest#test_spatial_factory_attrs_parsing`

Tests that should fail would instead corrupt the database state, causing subsequent tests to fail. This is of course related to the unfortunate way we disable referential integrity. If we see tests failing again with `InvalidForeignKey` errors, it will be a good time to reopen [cockroach#71496] [cockroach#71496]: cockroachdb/cockroach#71496

BuonOmo · 2025-11-19T10:48:41Z

@rafiss the PR is finally ready for review. It is packed with various intentions, all separated in various commits (except two small exceptions that I was too lazy to fixup). I strongly suggest review commit by commit rather than the whole PR at once. Have a good read :)

About flaky tests, I've ran many times the flaky.yml CI. It seems like the only flaky tests we have left are one rare connection leaked, I don't have a local reproduction, and I think we can easily merge without this.

This PR also fixes some past bugs I found along the way, we might want to cherry-pick some of the commits later on.

If that's fine with you, I'd rather rebase and merge, not squash nor merge.

In our adapter, we rely on the sql type string for spatial details to properly map the type to the correct ActiveRecord type. It was not shared.

Fixes #389

Minimal set of test reproduction: ```sh COCKROACH_SKIP_LOAD_SCHEMA=1 SEED=13092 bundle exec rake test TESTOPTS="--name='/\A("\ "test_concurrent_insert_with_processes"\ "|test_uniqueness_validations_work_when_using_old_encryption_schemes"\ "|test_fixtures_get_encrypted_automatically"\ "|test_ciphertext_for_returns_the_ciphertext_of_a_value_when_the_record_is_new)\z/'" ``` It is important NOT TO run `ActiveRecord::Base.reset_column_information`. Some tests are eagerly loading information to prevent side effects [^1]. We could run `Avenger.reset_column_information` but we actually do not need it here. [^1]: See https://github.com/rails/rails/blob/v8.1.1/activerecord/test/cases/encryption/helper.rb#L115-L120

BuonOmo force-pushed the feat/rails-8-1 branch 2 times, most recently from cb4869f to d8e3e07 Compare October 9, 2025 11:36

BuonOmo force-pushed the feat/rails-8-1 branch from d8e3e07 to f9b1c88 Compare October 16, 2025 16:16

BuonOmo force-pushed the feat/rails-8-1 branch 2 times, most recently from ab74ce6 to dc9a906 Compare October 30, 2025 10:20

BuonOmo commented Oct 30, 2025

View reviewed changes

BuonOmo force-pushed the feat/rails-8-1 branch 10 times, most recently from 4b74df4 to d477fd6 Compare November 4, 2025 16:25

BuonOmo force-pushed the feat/rails-8-1 branch 5 times, most recently from aebefbe to f6aa445 Compare November 18, 2025 19:22

BuonOmo commented Nov 19, 2025

View reviewed changes

BuonOmo force-pushed the feat/rails-8-1 branch 2 times, most recently from f5639cd to d3c0e76 Compare November 19, 2025 09:58

BuonOmo added 3 commits November 19, 2025 11:00

feat: bump to Rails 8.1

3ef4421

fix: add cast_type to Column.new

22f79b7

fix(test): don't alter backtrace from gems

0ed7b7c

BuonOmo added 8 commits November 19, 2025 11:00

fix: update run_command execution

bb34d48

fix: use class level native_database_type

1bf0a3f

fix(test): default is now serialized

a452092

See rails/rails@205cdd5

fix(ci): smaller summary without skips

8072f97

fix(test): us english names

41622c0

fix(test): coerced value for generate column

b7d0656

BuonOmo force-pushed the feat/rails-8-1 branch from d3c0e76 to 570a0af Compare November 19, 2025 10:01

BuonOmo commented Nov 19, 2025

View reviewed changes

BuonOmo added 12 commits November 19, 2025 11:25

feat(referential-integrity): handle autocommit_before_ddl

14f0180

If `#disable_referential_integrity` is ran within a transaction, we may be able to still run the user's code. Otherwise we warn the user why it failed.

feat(ci): more complete error output

2a03ac5

fix(test): test_remove_foreign_key_on_8_0

3fc2227

Since CockroachDB does not support DDL transactions, we adapt the test.

fix(test): type_map not ractor shareable

81699b3

fix: proper override for Column and Spatial

271e8de

This is WIP and there is likely more to do. Hence the reference #390. Fixes the flaky test `PostGISTest#test_spatial_factory_attrs_parsing`

fix(test): test_payload_name_on_eager_load

d8ae08e

feat(ci): timeout for flaky.yml

907fab1

feat: support close_prepared

9aef11e

fix: make sure we eagerly compute SerializeCastValue

1770c41

BuonOmo force-pushed the feat/rails-8-1 branch from 570a0af to 1770c41 Compare November 19, 2025 10:44

BuonOmo marked this pull request as ready for review November 19, 2025 10:45

fix: ensure type_map gets sql type details

ae9c973

In our adapter, we rely on the sql type string for spatial details to properly map the type to the correct ActiveRecord type. It was not shared.

BuonOmo force-pushed the feat/rails-8-1 branch from 96ecf5b to 4fc3e5c Compare November 19, 2025 18:26

feat: make type_map ractor shareable

df34946

Fixes #389

BuonOmo force-pushed the feat/rails-8-1 branch from 4fc3e5c to df34946 Compare November 19, 2025 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: upgrade to Rails 8.1 #386

feat: upgrade to Rails 8.1 #386

Uh oh!

BuonOmo commented Oct 9, 2025 •

edited

Loading

Uh oh!

BuonOmo Oct 30, 2025

Uh oh!

BuonOmo Oct 30, 2025 •

edited

Loading

Uh oh!

rafiss Nov 10, 2025

Uh oh!

BuonOmo Nov 17, 2025

Uh oh!

Maimer commented Nov 13, 2025

Uh oh!

BuonOmo commented Nov 13, 2025

Uh oh!

BuonOmo Nov 19, 2025

Uh oh!

BuonOmo Nov 19, 2025

Uh oh!

BuonOmo commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# prefixes and suffixes. The good thing is: if this happens, tests will crash
		# hard, no way we miss it.

feat: upgrade to Rails 8.1 #386

Are you sure you want to change the base?

feat: upgrade to Rails 8.1 #386

Uh oh!

Conversation

BuonOmo commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BuonOmo Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

BuonOmo Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

BuonOmo Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Maimer commented Nov 13, 2025

Uh oh!

BuonOmo commented Nov 13, 2025

Uh oh!

BuonOmo Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

BuonOmo Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

BuonOmo commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BuonOmo commented Oct 9, 2025 •

edited

Loading

BuonOmo Oct 30, 2025 •

edited

Loading