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

Incorrect db/schema.rb file for Postgres when using Cockroach and Postgres together. #251

Closed
jviney opened this issue May 5, 2022 · 1 comment · Fixed by #283
Closed

Comments

@jviney
Copy link

jviney commented May 5, 2022

In our Rails app we use both Postgres and Cockroach at the same time with Rails 6 multi-database support.

This adapter monkey patches ActiveRecord::ConnectionAdapters::PostgreSQL::Column#serial? in a way that causes the Postgres db/schema.rb file to be incorrectly generated, with primary key values like:

create_table "access_logs", id: :bigint, default: -> { "nextval('admin_access_logs_id_seq'::regclass)" }, force: :cascade do |t|

Instead of:

create_table "access_logs", force: :cascade do |t|

We've applied this patch below to fix the issue so that the @serial value is still respected when it's been supplied for Postgres columns.

module ActiveRecord
  # activerecord-cockroachdb-adapter monkey patches ActiveRecord::ConnectionAdapters::PostgreSQL::Column and
  # overwrites the #serial? method in a way that works for Cockroach, but always returns false for Postgres.
  #
  # This affects the generation of db/schema.rb for Postgres because the primary key column is not identified.
  #
  # Replace the method with one that works for both Postgres and Cockroach.
  #
  # I'm not sure why activerecord-cockroachdb-adapter monkey patches the PostgreSQL::Column class instead of extending
  # it, like it does for other classes.
  module FixDbSchemaDump
    # This combines two methods:
    #  - ActiveRecord::ConnectionAdapters::PostgreSQL::Column#serial?
    #  - ActiveRecord::ConnectionAdapters::CockroachDB::PostgreSQLColumnMonkeyPatch#serial?
    #
    # @serial will always be nil for Cockroach, but may be set for Postgres.
    def serial?
      @serial || default_function == "unique_rowid()"
    end
  end

  ConnectionAdapters::PostgreSQL::Column.class_eval do
    prepend FixDbSchemaDump
  end
end

What's the reason for the CockroachDB adapter monkey patching the PostgresSQL::Column class instead of creating a subclass?

@BuonOmo
Copy link
Collaborator

BuonOmo commented Jun 20, 2023

@jviney thanks for reporting the bug. I could indeed reproduce it (repo here) with this config/database.yml file:

default: &default
  primary:
    adapter: postgresql
    database: foo
  other:
    adapter: cockroachdb
    database: bar
    port: 26257
    host: localhost
    user: root

development:
  <<: *default
test:
  <<: *default
production:
  <<: *default

Here is the culprit PR: #44. I think we are monkeypatching because there is no specific place where we are using this class in the codebase (rg PostgreSQLColumn yields only the monkey-patching). So we'd need for a distinct subclass to bubble up. I don't know yet how this class is referenced, but I'll look and find a better way.

Worst case scenario, we can use the content of your hack since it seems to work both for pg and cockroach.

BuonOmo added a commit to BuonOmo/activerecord-cockroachdb-adapter that referenced this issue Jun 23, 2023
BuonOmo added a commit that referenced this issue Aug 13, 2023
@BuonOmo BuonOmo linked a pull request Aug 13, 2023 that will close this issue
BuonOmo added a commit that referenced this issue Aug 13, 2023
BuonOmo added a commit that referenced this issue Aug 13, 2023
We used to monkeypatch freely some classes. However, Rails is now
supporting multiple databases, and these patches are in the way
of this usage.

Fixes #251
BuonOmo added a commit that referenced this issue Aug 13, 2023
We used to monkeypatch freely some classes. However, Rails is now
supporting multiple databases, and these patches are in the way
of this usage.

Fixes #251
rafiss pushed a commit that referenced this issue Aug 14, 2023
We used to monkeypatch freely some classes. However, Rails is now
supporting multiple databases, and these patches are in the way
of this usage.

Fixes #251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants