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

Avoid comment calls in pg:dump #43216

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Avoid comment calls in pg:dump #43216

merged 1 commit into from
Sep 20, 2021

Conversation

jaynetics
Copy link
Contributor

Summary

This causes lines like COMMENT ON EXTENSION plpgsql IS 'PL/pgSQL procedural language'; to no longer appear in the db:structure:dump when using PostgreSQL.

Such comment calls don't seem to add a lot of value.

They are bad, however, because with them, db:structure:load only works for db superusers, as described in #36816, which is hard to debug due to the generic error ActiveRecord::StatementInvalid: PG::InsufficientPrivilege: ERROR: must be owner of extension.

@ghiculescu
Copy link
Member

@jaynetics thanks for the PR. There was a recent discussion about this in #43107 where @eileencodes pointed out that this is pretty easy to fix without framework changes.

I'll leave this open for now, but just wanted to share some context in case it doesn't get much traction.

@jaynetics
Copy link
Contributor Author

@ghiculescu thanks for the input.

i agree that it is easy to fix once you know what the problem is. just finding out the problem is a bit hard based on the given error message.

in my opinion, it would be best if the structure stuff just works in most normal scenarios, and does not rely on some of the users googling error messages.

if you agree, i'd do some more work on this to make it apply only to PostgreSQL >= 11 as described in #43107.

otherwise, at least the error message should be improved in my opinion.

@ghiculescu
Copy link
Member

@jaynetics I'm not on the core team so don't take this as an endorsement either way, but I personally would like to see this in Rails. Just don't want to get your hopes up too much if it doesn't happen!

Making it work with Postgres 11 is definitely needed, it'd look something like this:

This also needs a CHANGELOG entry.

@jaynetics
Copy link
Contributor Author

@ghiculescu i've added the version check and changelog entry.

@ghiculescu

This comment was marked as outdated.

ghiculescu added a commit to ghiculescu/rails that referenced this pull request Mar 4, 2022
Introduces `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.comments_in_structure_dump` to allow users to determine if `structure.sql` will include custom comments on tables or columns.

The default is true, meaning comments will be included. This is because prior to rails#43216, comments were included, and I think now we have a configuration option that is the more correct default.

Fixes rails#44498
ghiculescu added a commit to ghiculescu/rails that referenced this pull request Mar 7, 2022
Reverts rails#43216
Fixes rails#44498

Per the discussion at rails#44603 (comment), reverting the feature as it's not clear it should be the default. Users who don't want comments in structure dumps can use `ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = ['--no-comment']`.
@ghiculescu
Copy link
Member

heads up - this was reverted in #44633

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

Successfully merging this pull request may close these issues.

3 participants