cockroachdb / activerecord-cockroachdb-adapter

CockroachDB adapter for ActiveRecord.
Apache License 2.0
103 stars 52 forks source link

Empty or incorrect unique_constraint generation #347

Open mmalek-sa opened 2 weeks ago

mmalek-sa commented 2 weeks ago

There seems to be a fundamental difference between PostgreSQL implementation of pg_constraint vs CockroachDB's implementation. When ActiveRecord queries this table here, Postgres returns an empty array while CockroachDB returns list of all indexes with a unique constraint.

This causes ActiveRecord to swap t.index definitions with t.unique_constraint definitions here. The problem with this is that Postgres Adapter doesn't seem to support conditions and functions for constraints properly. This results in these index definitions to change to a wrong definition when using CockroachDB:

t.index "lower(name::STRING) ASC", name: "index_redacted_table_name_on_name_unique", unique: true

becomes:

t.unique_constraint [], name: "index_redacted_table_name_on_name_unique"

and

t.index ["username"], name: "index_users_on_username_unique", , unique: true, where: "(deleted is false)"

becomes

t.unique_constraint ["username"], name: "index_users_on_username_unique"

I'm able to fix this by monkey patching SchemaStatements#unique_constraints method to return an empty array as PostgreSQL does:

def unique_constraints(_table_name)
  []
end

This is happening in Rails >= 7.1.

BuonOmo commented 2 weeks ago

Okay here's a reproduction of the bug:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "activerecord"

  gem "activerecord-cockroachdb-adapter"
end

require "activerecord-cockroachdb-adapter"
require "minitest/autorun"
require "logger"

# You might want to change the database name for another one.
ActiveRecord::Base.establish_connection("cockroachdb://root@localhost:26257/defaultdb")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :payments, force: true do |t|
    t.text "name"
    t.index "lower(name::STRING) ASC", name: "lower_name_index_on_payments", unique: true
  end
end

class Payment < ActiveRecord::Base
end

class BugTest < ActiveSupport::TestCase
  def test_dump_schema_should_give_an_index
     stream = StringIO.new
     ActiveRecord::SchemaDumper.dump(
       ActiveRecord::Base.connection_pool,
       stream
     )
     stream.rewind
     index_line = stream.each_line.find { _1[/lower_name_index_on_payments/] }
     assert_match /t.index/, index_line
     puts "\nResult:\n\n\t#{index_line}"
  end
end

However, adding the fix you proposed is not fixing it for me, the code somehow always end up using the default PG version. I'm curious to know where you monkeypatched ? I'll investigate further and look for a proper fix !

BuonOmo commented 2 weeks ago

@rafiss do you have any knowledge on why there is a difference between pg and cockroach on this query ?

BEGIN;
CREATE TABLE public.example(
    id serial,
    name text NULL
);
CREATE UNIQUE INDEX "lower_name_index_on_payments" ON "example" (lower(name) ASC);

SELECT c.conname, c.conrelid, c.conkey, c.condeferrable, c.condeferred
FROM pg_constraint c
JOIN pg_class t ON c.conrelid = t.oid
JOIN pg_namespace n ON n.oid = c.connamespace
WHERE c.contype = 'u'
  AND t.relname = 'example';

(pg returns nothing, and crdb returns the index lower_name_index_on_payments)

mmalek-sa commented 2 weeks ago

@BuonOmo This is how I'm patching it:

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  gem 'activerecord'

  gem 'activerecord-cockroachdb-adapter'
end

require 'activerecord-cockroachdb-adapter'
require 'minitest/autorun'
require 'logger'

# You might want to change the database name for another one.
ActiveRecord::Base.establish_connection('cockroachdb://root@localhost:26257/defaultdb')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::ConnectionAdapters::CockroachDB::SchemaStatements.class_eval do
  def unique_constraints(_table_name)
    []
  end
end

ActiveRecord::Schema.define do
  create_table :payments, force: true do |t|
    t.text 'name'
    t.index 'lower(name::STRING) ASC', name: 'lower_name_index_on_payments', unique: true
  end
end

class Payment < ActiveRecord::Base
end

class BugTest < ActiveSupport::TestCase
  def test_dump_schema_should_give_an_index
    stream = StringIO.new
    ActiveRecord::SchemaDumper.dump(
      ActiveRecord::Base.connection_pool,
      stream
    )
    stream.rewind
    index_line = stream.each_line.find { _1[/lower_name_index_on_payments/] }
    assert_match(/t.index/, index_line)
    puts "\nResult:\n\n\t#{index_line}"
  end
end