Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: bump activerecord #337

Merged
merged 1 commit into from
Sep 24, 2024
Merged

chore: bump activerecord #337

merged 1 commit into from
Sep 24, 2024

Conversation

BuonOmo
Copy link
Collaborator

@BuonOmo BuonOmo commented Aug 26, 2024

Closes #336
Closes #305

@BuonOmo BuonOmo force-pushed the bump-7.2 branch 6 times, most recently from 08dddfe to b29a9f8 Compare August 29, 2024 19:59
@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 2, 2024

TODOs:

  • fix the tests below
Test is missing assertions: `test_yaml_file_with_symbol_columns` /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/test/cases/fixtures_test.rb:112
Test is missing assertions: `test_set_session_variable_nil` /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/test/cases/adapters/postgresql/connection_test.rb:27
Test is missing assertions: `test_set_session_variable_default` /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/test/cases/adapters/postgresql/connection_test.rb:34

@BuonOmo BuonOmo force-pushed the bump-7.2 branch 5 times, most recently from 1ee8d62 to 405e962 Compare September 3, 2024 21:28
@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 4, 2024

There's one error left, and it is due to a different behaviour in PG and CRDB. I'm keeping the demo code here for history:

def failure(&conn_block)
  conn = conn_block.call

  res = conn.exec_params(<<-SQL, [])
  SELECT t.oid, t.typname, t.typelem, t.typdelim, t.typinput, r.rngsubtype, t.typtype, t.typbasetype
  FROM pg_type as t
  LEFT JOIN pg_range as r ON oid = rngtypid
  WHERE
    t.typelem IN (16, 17, 18, 19, 20, 21, 23, 25, 26, 114, 142, 600, 601, 602, 603, 604, 628, 700, 701, 718, 790, 829, 869, 650, 1042, 1043, 1082, 1083, 1114, 1184, 1186, 1560, 1562, 1700, 2950, 3614, 3802, 13682, 13685, 13687, 13693, 13695, 3904, 3906, 3908, 3910, 3912, 3926)
  SQL
rescue PG::Error => e
  puts e.full_message(highlight: true)
end


puts "CockroachDB"
puts "==========="
puts
failure { PG.connect(dbname: "non_existent_db", port: 26257, user: "root", host: "localhost") }
puts

puts "PostgreSQL"
puts "=========="
puts
failure { PG.connect(dbname: "non_existent_db", port: 5432) }
puts

We can see that failure occur at connection for pg and at query execution for cockroachdb

Company.connection.exec_query("DROP SEQUENCE IF EXISTS companies_nonstd_seq")
Company.connection.create_table :companies, force: true do |t|
Company.lease_connection.drop_table :companies, if_exists: true
Company.lease_connection.exec_query("DROP SEQUENCE IF EXISTS companies_nonstd_seq CASCADE")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This started failing with CRDB v24.1 with message:

cannot drop sequence companies_nonstd_seq because other objects depend on it

It would be nice to see what are the depending objects. In PG there's this command:

CREATE OR REPLACE VIEW dependencies AS 
SELECT DISTINCT srcobj.oid AS src_oid
  , srcnsp.nspname AS src_schemaname
  , srcobj.relname AS src_objectname
  , tgtobj.oid AS dependent_viewoid
  , tgtnsp.nspname AS dependant_schemaname
  , tgtobj.relname AS dependant_objectname
FROM pg_class srcobj
  JOIN pg_depend srcdep ON srcobj.oid = srcdep.refobjid
  JOIN pg_depend tgtdep ON srcdep.objid = tgtdep.objid
  JOIN pg_class tgtobj ON tgtdep.refobjid = tgtobj.oid AND srcobj.oid <> tgtobj.oid
  LEFT JOIN pg_namespace srcnsp ON srcobj.relnamespace = srcnsp.oid
  LEFT JOIN pg_namespace tgtnsp ON tgtobj.relnamespace = tgtnsp.oid
WHERE tgtdep.deptype = 'i'::"char" AND tgtobj.relkind = 'v'::"char";

https://stackoverflow.com/a/48770535/6320039

But it doesn't work in CRDB. Any idea @rafiss ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crdb_internal.forward_dependencies is what you are looking for. add a WHERE descriptor_name = 'companies_nonstd_seq'; the object that is depended on by this sequence will be in the dependedonby_id

you could probably join this with backward_dependencies to get a more illustrative picture:

demo@127.0.0.1:26257/movr> SELECT                                                                                          
                        ->     b.descriptor_id AS dependent_id,                                                            
                        ->     f.descriptor_name AS depended_on_by,                                                        
                        ->     b.descriptor_name AS depedent_name                                                          
                        -> FROM                                                                                            
                        ->     crdb_internal.backward_dependencies AS b                                                    
                        -> JOIN                                                                                            
                        ->     crdb_internal.forward_dependencies AS f                                                     
                        -> ON                                                                                              
                        ->     f.dependedonby_id = b.descriptor_id                                                         
                        -> WHERE f.descriptor_name = 'companies_nonstd_seq';                                               
  dependent_id |    depended_on_by    | depedent_name
---------------+----------------------+----------------
           113 | companies_nonstd_seq | companies
(1 row)

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 4, 2024

Still one todo as of now: understanding where the notice messages come from:

NOTICE:  datestyle_enabled no longer has any effect
NOTICE:  enable_drop_enum_value no longer has any effect
NOTICE:  intervalstyle_enabled no longer has any effect

crdb: [v22.2, v23.1, v23.2]
ruby: [head]
crdb: [v23.2, v24.1, v24.2]
ruby: ["3.3"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby 3.3 is the LTS branch, and the current ruby-head on github is a beta version...

@BuonOmo BuonOmo marked this pull request as ready for review September 5, 2024 08:21
Copy link
Contributor

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had one minor comment for you on this

def check_version # :nodoc:
# https://www.cockroachlabs.com/docs/releases/release-support-policy
if database_version < 23_01_12 # < 23.1.12
raise "Your version of CockroachDB (#{database_version}) is too old. Active Record supports CockroachDB >= 23.1.12."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add the minimum version in the readme?

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 24, 2024

@fqazi done. I also took the liberty to bump version and squash the commit so it is ready to be pushed to rubygem (I do not have the rights to do it myself). If you do so, please also add the git tag v7.2.0!

- Update supported versions of Cockroach and ActiveRecord
- Fix some tests, updgrade codebase to adapt to rails 7.2

Closes #305. Closes #336
@fqazi fqazi merged commit 11c2767 into master Sep 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to rails 7.2 Fully support MultiDbMigrator
4 participants