Re: Patch: Add --no-comments to skip COMMENTs with pg_dump

David,

* David G. Johnston (david(dot)g(dot)johnston(at)gmail(dot)com) wrote:
> On Fri, May 26, 2017 at 7:47 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robins Tharakan (tharakan(at)gmail(dot)com) wrote:
> > > Attached is a patch adds a --no-comments argument to pg_dump to skip
> > > generation of COMMENT statements when generating a backup. This is
> > crucial
> > > for non-superusers to restore a database backup in a Single Transaction.
> > > Currently, this requires one to remove COMMENTs via scripts, which is
> > > inelegant at best.
> >
> > Having --no-comments seems generally useful to me, in any case.
>
> I​t smacks of being excessive to me.
> ​

I have a hard time having an issue with an option that exists to exclude
a particular type of object from being in the dump. A user who never
uses large objects/blobs might feel the same way about "--no-blobs", or
a user who never uses ACLs wondering why we have a "--no-privileges".
In the end, these are also possible components that users may wish to
have for their own reasons.

What I, certainly, agree isn't ideal is requiring users to use such an
option to generate a database-level dump file (assuming they have access
to more-or-less all database objects) which can be restored as a
non-superuser, that's why I was a bit hesitant about this particular
solution to the overall problem.

I do agree that if there is simply no use-case, ever, for wishing to
strip comments from a database then it might be excessive to provide
such an option, but I tend to feel that there is a reasonable, general,
use-case for the option.

> > CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;
>
> ​A less verbose way to add comments to objects would be nice but we have an
> immediate problem that we either need to solve or document a best practice
> for.

The above would be a solution to the immediate problem in as much as
adding COMMENT IF NOT EXISTS would be.

> COMMENT IF NOT EXISTS has been brought up but it doesn't actually map to
> what seems to me is the underlying problem...that people don't want a
> non-functional (usually...) aspect preventing successful restoration.

I tend to disagree with this characterization- I'm of the opinion that
people are, rightly, confused as to why we bother to try and add a
COMMENT to an object which we didn't actually end up creating (as it
already existed), and then throw an error on it to boot. Were pg_dump a
bit more intelligent of an application, it would realize that once the
CREATE ... IF NOT EXISTS returned a notice saying "this thing already
existed" that it would realize that it shouldn't try to adjust the
attributes of that object, as it was already existing. That, however,
would preclude the ability of pg_dump to produce a text file used for
restore, unless we started to write into that text file DO blocks, which
I doubt would go over very well.

> COMMENT ON object TRY 'text' -- i.e., replace the word IS with TRY

I'm not sure that I see why we should invent wholly new syntax for
something which we already have in IF NOT EXISTS, or why this should
really be viewed or considered any differently from the IF NOT EXISTS
syntax.

Perhaps you could elaborate as to how this is different from IF NOT
EXISTS?

> If the attempt to comment fails for any reason log a warning (maybe) but
> otherwise ignore the result and continue on without invoking an error.

In the IF NOT EXISTS case we log a NOTICE, which seems like it's what
would be appropriate here also, again begging the question of it this is
really different from IF NOT EXISTS in a meaningful way.

> One suggestion I've seen is to simply "COMMENT ON EXTENSION plpgsql IS
> NULL;" If that works in the scenarios people are currently dealing with
> then I'd say we should advise that such an action be taken for those whom
> wish to generate dumps that can be loaded by non-super-users. If the
> affected users cannot make that work then maybe we should just remove the
> comment from the extension. People can lookup "plpgsql" in the docs easily
> enough and "PL/pgSQL procedural language" doesn't do anything more than
> expand the acronym.

Removing the comment as a way to deal with our deficiency in this area
strikes me as akin to adding planner hints.

Thanks!

Stephen