Remove --no-comments from Postgres structure dump command

[ghiculescu]

Reverts #43216
Fixes #44498

Per the discussion at #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'].

[jaynetics]

@ghiculescu @rafaelfranca

this restores a state where structure:load is broken by default in some scenarios, to fix what IMO is more of an inconvenience in missing comments.

i'm not sure how common the scenario is, but i think all PaaS take the secure approach of avoiding superusers, and comments are not just "custom"-made but also automatically generated when using pg extensions, so it could be quite common.

if so, maybe the unclear error message (ActiveRecord::StatementInvalid: PG::InsufficientPrivilege: ERROR: must be owner of extension) should be augmented, i.e. matched and wrapped in a new error subclass that adds a hint about disabling comments?

[ghiculescu]

Anecdotally, we use RDS for both production and development, and we have never come across this issue. Could you share a bit more info on how to replicate and in what environment this is happening for you?

[jaynetics]

do you mean Amazon RDS? i suspect you're either not using extensions or using a superuser account then?

by PaaS i meant full platforms like Heroku.

i encountered the problem with Scalingo (Heroku clone), postgres 12, and the extension pgcrypto.

browsing through the available extensions here and looking at their *.control files, it seems like all extensions have such comments and apply them through this code.

so i guess it should be possible to reproduce the problem by creating a Heroku app, adding postgres, and adding any extension as described here. and of course setting up a rails app and integrating it with that. i still have the monkeypatch protecting me from this revert, so i'm not eager to do all of this at the moment 😁

[ghiculescu]

But when are you running structure:load? Is it just on the initial app creation?

[jaynetics]

yeah. it's kind of a whitelabel app with multiple deployments, that's why its a recurring problem for us.

[ghiculescu]

Ahhh, that makes more sense. I think the typical use case for most apps would not have them running structure:load against a Heroku Postgres / RDS production instance. And for those that need to they can set the extra structure dump flags.

[bglimepoint]

this restores a state where structure:load is broken by default in some scenarios, to fix what IMO is more of an inconvenience in missing comments.

As a note, the addition of no-comments was a hard-breakage for us with Que as Que uses comments to track which version of its migrations it has applied; and the --no-comments can't be undone using the structure_dump_flags option, whilst adding structure_dump_flags = ['--no-comments'] is achievable.

We also want to have comments in our DB as that's why we added them :-)

Just wanted to note that it's not just an inconvenience, and there is a config option to fix it; whereas the inverse was actually unresolvable for us :-) Just in case anyone considers reversing this in the future, our vote would be "please don't" :-D

[bvogel]

Any chance to get this released? I was surprised to see that it was omitted from the 7.0.2.3 release?

[skipkayhil]

7.0.2.3 was a security release, so it didn't include any bug fixes. I would expect to see this in 7.0.3 (you can point your Gemfile at 7-0-stable or the specific commit 53b9ecf to use it right now)