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

feat: support schema search path #286

Merged
merged 1 commit into from
Sep 19, 2023
Merged

feat: support schema search path #286

merged 1 commit into from
Sep 19, 2023

Conversation

BuonOmo
Copy link
Collaborator

@BuonOmo BuonOmo commented Sep 2, 2023

No description provided.

@BuonOmo BuonOmo linked an issue Sep 2, 2023 that may be closed by this pull request
3 tasks
@BuonOmo BuonOmo force-pushed the feat/support-seach-path branch 5 times, most recently from 412810e to 3d9a893 Compare September 6, 2023 18:05
Comment on lines +1 to +2
exclude :test_set_pk_sequence, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
exclude :test_pk_and_sequence_for_with_schema_specified, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rafiss these one are here because of the last bugfix you made in crdb (see #287)

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 8, 2023

One of the failing tests is new to this PR, but I'm not sure how to fix it.

We create a table using id integer, this is stored as INT8 in crdb. but when we use the the method column_definitions, it is shown as bigint. This bigint value is found in this query:

SELECT a.attname, format_type(a.atttypid, a.atttypmod),
        pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod,
        c.collname, NULL AS comment,
        attgenerated,
        NULL as is_hidden
  FROM pg_attribute a
  LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum
  LEFT JOIN pg_type t ON a.atttypid = t.oid
  LEFT JOIN pg_collation c ON a.attcollation = c.oid AND a.attcollation <> t.typcollation
  WHERE a.attrelid = 'thetable'::regclass
    AND a.attnum > 0 AND NOT a.attisdropped
  ORDER BY a.attnum

@rafiss do you have any idea if this is correct or expected ?

@rafiss
Copy link
Contributor

rafiss commented Sep 8, 2023

Yeah bigint is just an alias for INT8. Actually, it seems that Postgres also returns bigint for this case, so maybe something else is going wrong?

postgres=# create table thetable(a int8, b int4);
CREATE TABLE

postgres=# SELECT a.attname, format_type(a.atttypid, a.atttypmod),
        pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod,
        c.collname, NULL AS comment,
        attgenerated,
        NULL as is_hidden
  FROM pg_attribute a
  LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum
  LEFT JOIN pg_type t ON a.atttypid = t.oid
  LEFT JOIN pg_collation c ON a.attcollation = c.oid AND a.attcollation <> t.typcollation
  WHERE a.attrelid = 'thetable'::regclass
    AND a.attnum > 0 AND NOT a.attisdropped
  ORDER BY a.attnum;
 attname | format_type | pg_get_expr | attnotnull | atttypid | atttypmod | collname | comment | attgenerated | is_hidden
---------+-------------+-------------+------------+----------+-----------+----------+---------+--------------+-----------
 a       | bigint      | NuLL        | f          |       20 |        -1 | NuLL     | NuLL    |              | NuLL
 b       | integer     | NuLL        | f          |       23 |        -1 | NuLL     | NuLL    |              | NuLL
(2 rows)

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 9, 2023

Ah so the problem seems to be at creation then: we say create with integer and we get a bigint. Thank you! I'll look at this further during the next days

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 9, 2023

@rafiss so looking again at the issue, I think there is a CRDB issue.

Those statements show me that id is an int8, which

create table bar (id integer, num bigint);
show create table bar;

Shows both as the same bigint.

However doing the same with pg gives me integer and bigint.

@rafiss
Copy link
Contributor

rafiss commented Sep 9, 2023

Ah I see, yes there is a CRDB difference here. In CRDB, the default integer size is 8. It can be configured using the default_int_size session setting. (SET default_int_size = 4)

There are more docs here: https://www.cockroachlabs.com/docs/stable/int

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 9, 2023

@rafiss nice thank you ! I think I'll just set this in the database for tests to be ok and more in sync with the PG tests. I can also consider adding a dedicated test to show that this specific case of integer being the same as bigint is considered and not a bug.

@BuonOmo BuonOmo force-pushed the feat/support-seach-path branch 3 times, most recently from 519c542 to ff264c0 Compare September 17, 2023 22:55
@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 18, 2023

The default_int_size issue is more problematic than expected. I found a solution for the tests which is to also change the serial_normalization value. Without that, serial and int could be represented with different limits, which messes with rails internal (I've seen that on a many2many join table).

For this PR, I think we're fine as this is out of scope anyway. But shouldn't I open an issue to discuss that and what do we want to do ? For now I see two options:

  • tweak rails internal to use serial for its join tables, and maybe some other places, and play with the default_int_size setting in tests to be sure we have everything covered
  • find a way to warn users about setting default_int_size without changing serial_normalization.

WDYT @rafiss ?

@BuonOmo BuonOmo marked this pull request as ready for review September 18, 2023 16:13
@rafiss
Copy link
Contributor

rafiss commented Sep 18, 2023

shouldn't I open an issue to discuss that and what do we want to do ? For now I see two options:

  • tweak rails internal to use serial for its join tables, and maybe some other places, and play with the default_int_size setting in tests to be sure we have everything covered
  • find a way to warn users about setting default_int_size without changing serial_normalization.

I believe you encountered this problem: cockroachdb/cockroach#26925 (comment)

But it's possible we don't show this warning in all the cases we should. For now, I agree: let's start by opening an issue in this repo, with a full example of the table schema that causes the confusing behavior.

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Sep 18, 2023

Good news, the issue is only due to how tests are written, my attempt of a reproduction within the codebase luckily failed, and is covered by some other tests, so we're all set!

I'm not pushing the test change upstream this time as it would be meaningless to postgresql adapter (they don't give the option to change the default int size).

And btw, the warning is eclipsed because this happen when creating the database, which is extremely verbose. In the test we are removing all the logs at that moment to avoid being too verbose, and for any production app, I don't think someone would see this one line buried in logs!

And for completeness, here's the reproduction attempt
require "active_record"
require "activerecord-cockroachdb-adapter"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(
  adapter: "cockroachdb",
  host: "localhost",
  port: 26257,
  user: "root",
  database: "ar_crdb_console"
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Base.connection.execute("SET default_int_size = 4")

ActiveRecord::Schema.define do
  create_table :albums, force: true do |t|
    t.string :title
  end
  create_table :songs, force: true do |t|
    t.string :title
  end
  pp method(:create_join_table).super_method
  binding.irb
  create_join_table :albums, :songs, force: true
end

class Album < ActiveRecord::Base
	has_and_belongs_to_many :songs
end
class Song < ActiveRecord::Base
	has_and_belongs_to_many :albums
end


class BugTest < Minitest::Test
  def test_association_stuff
    song = Song.create(title: "Another Brick in the Wall")
    song.albums.create(title: "The Wall")
    conn = ActiveRecord::Base.connection
    pp conn.select_one("SHOW CREATE TABLE albums_songs")
    pp conn.select_one("SHOW CREATE TABLE albums")
    pp conn.select_one("SHOW CREATE TABLE songs")
  end
end

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

nice work on this!

@rafiss rafiss merged commit bbb320a into master Sep 19, 2023
1 check failed
@rafiss rafiss deleted the feat/support-seach-path branch September 19, 2023 15:13
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.

support custom schemas and search_path
2 participants