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

Schema search path should not be included in schema.rb when a migration is executed. #325

Open
rapito opened this issue Mar 23, 2021 · 10 comments · May be fixed by #327
Open

Schema search path should not be included in schema.rb when a migration is executed. #325

rapito opened this issue Mar 23, 2021 · 10 comments · May be fixed by #327
Milestone

Comments

@rapito
Copy link
Contributor

rapito commented Mar 23, 2021

When executing a migration the schema search path should not be included on the schema.rb since this is Environment dependant and will break on a different configuration:

Example of schema.rb generated because of scenic:

  create_view "my_schema_search_path.foo", materialized: true, sql_definition: <<-SQL
      SELECT * from foo;
  SQL
  add_index "my_schema_search_path.foo", ["bar"], name: "index_on_foo_bar"

Expected schema.rb appendix is:

  create_view "foo", materialized: true, sql_definition: <<-SQL
      SELECT * from foo;
  SQL
  add_index "foo", ["bar"], name: "index_on_foo_bar"

To be more specific, this breaks the following rake task for any env it runs with a different configuration than the one used to generate the schema.rb file.

rake db:schema:load
@derekprior
Copy link
Contributor

derekprior commented Mar 23, 2021

Is it not reasonable to expect schema names to be consistent across environments?

@rapito
Copy link
Contributor Author

rapito commented Mar 23, 2021

There are several use cases where the schema_search_path is what changes across the environments and the db stays the same, thats why activerecord does not include the search_path on the schema.rb.

Is it not reasonable to expect schema names to be consistent across environments?

So in reality, adding the schema name makes it inconsistent because it changes every time it runs /is generated ​on a different schema_search path.

@rapito rapito linked a pull request Apr 8, 2021 that will close this issue
@jdejong
Copy link

jdejong commented Apr 8, 2021

I agree with this approach since it directly aligns with ActiveRecord's approach.

Additionally, many DBaaS providers use Schema or Database names as a way to uniquely identify databases and companies in their platform so you might not have control over either across your environments.

@derekprior
Copy link
Contributor

This seems like it would be a breaking change given that the reason this exists in the first place was due to users taking the opposite approach. However, given that Rails now has native multiple database support, it may be worth revisiting. I haven't done anything personally with multi db support though.

@calebthompson what do you think?

@calebhearth
Copy link
Contributor

calebhearth commented Apr 15, 2021 via email

@jdejong
Copy link

jdejong commented Apr 15, 2021

Hi @calebthompson, yup Heroku uses database names, I think it was ClearDB that used unique schemas.

Regardless this breaks from normal rails conventions, and as @derekprior mentioned, will also break multi-database support in Rails 6/6.1.

Perhaps, making this an opt-out/in behavior with the default either enabled or disabled depending on whether it is opt-in or out?

@rapito
Copy link
Contributor Author

rapito commented Apr 15, 2021

@derekprior @calebthompson
This is an example on how the schema_search_path can be used on database yml configurations:

default_pg: &default_pg
  adapter: postgresql
  username: <%= ENV['PG_DB_USR'] %>
  password: <%= ENV['PG_DB_PW'] %>
  host: <%= ENV['PG_DB_HOST'] %>
  database: <%= ENV['PG_DB_DATABASE'] %>
  schema_search_path: <%= ENV['PG_DB_SCHEMA'] %>
  port: <%= ENV['PG_DB_PORT'] %>

Besides that, (as mentioned before) the way scenic is working right now is a breaking behavior since it forces the search path on the schema.rb and rails doesn't do this so it's also a breaking convention.

If you open up any schema.rb after running a scenic view migration you can verify that only the scenic entries are doing this:

create_table "foo", id: :serial, force: :cascade do |t|
  t.integer "bar"
  t.index ["bar"], name: "random_index_name_0"
end

create_view "this_is_development.my_view", materialized: true, sql_definition: <<-SQL
	SELECT foo.bar
 FROM foo foo;
SQL
add_index "this_is_development.my_view", ["bar"], name: "random_index_name_1"

As @jdejong proposed, at the very least I would like to push for an opt-out mechanism. 🙏

@derekprior
Copy link
Contributor

I agree with this approach since it directly aligns with ActiveRecord's approach.

This from @jdejong is the most compelling argument that our current implementation is wrong. Can this be substantiated by anyone? It's not that I don't trust @jdejong but since we're going back on an earlier change, I'd love some verification.

@kopchickm
Copy link

@derekprior We'd love to have this functionality, too. Since Rails conventions don't include the schema when creating tables, the current implementation here is breaking for us since our different environments have different database (and schema) names. I'd love to see #327 merged. Thanks!

@rapito
Copy link
Contributor Author

rapito commented Aug 22, 2022

I agree with this approach since it directly aligns with ActiveRecord's approach.

This from @jdejong is the most compelling argument that our current implementation is wrong. Can this be substantiated by anyone? It's not that I don't trust @jdejong but since we're going back on an earlier change, I'd love some verification.

Hopefully the above comment serve as verification? @derekprior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants