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

Stephen,

On Tue, May 30, 2017 at 8:41 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> 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.
>

​Excessive with respect to the problem at hand. A single comment in the
dump is unable to be restored. Because of that we are going to require
people to omit every comment in their database. The loss of all that
information is in excess of what is required to solve the stated problem
which is how I was thinking of excessive.

I agree that the general idea of allowing users to choose to include or
exclude comments is worth discussion along the same lines of large objects
and privileges.

> > > 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.
>

​I believe it would take a lot longer, possibly even until 12, to get the
linked comment feature committed compared ​to committing COMMENT IF NOT
EXISTS or some variation (or putting in a hack for that matter).

> > 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.

My characterization does actually describe the current system though.
While I won't doubt that people do hold your belief it is an underlying
mis-understanding as to how PostgreSQL works since comments aren't, as you
say below, actual attributes but rather objects in their own right. I
would love to have someone solve the systemic problem here. But the actual
complaint can probably be addressed without it.

> 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.

​pg_dump isn't in play during the restore which is where the error occurs.

During restore you have pg_restore+psql - and having cross-statement logic
in those applications is likely a non-starter. That is ultimately the
problem here, and which is indeed fixed by the outstanding proposal of
embedding COMMENT within the CREATE/ALTER object commands. But today,
comments are independent objects and solving the problem within that domain
will probably prove easier than changing how the system treats comments.

> > COMMENT ON object TRY 'text' -- i.e., replace the word IS with TRY
>
> Perhaps you could elaborate as to how this is different from IF NOT
> EXISTS?
>
>
​If the comment on plpgsql were removed for some reason the COMMENT ON IF
NOT EXISTS would fire and then it would fail due to permissions. With
"TRY" whether the comment (or object for that matter) exists or not the new
comment would be attempted and if the permission failure kicked in it
wouldn't care.​

> If the
> > affected users cannot make that work then maybe we should just remove the
> > comment from the extension.
>
> Removing the comment as a way to deal with our deficiency in this area
> strikes me as akin to adding planner hints.
>

​Maybe, but the proposal you are supporting has been around for years, with
people generally in favor of it, and hasn't happened yet. At some point
I'd rather hold my nose and fix the problem myself than wait for the
professional to arrive and do it right. Any, hey, we've had multiple
planner hints since 8.4 ;)

David J.