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 Tue, May 30, 2017 at 8:41 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * 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.

Fair enough. I tend to agree with that sentiment.

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

That would be most unfortunate, I agree.

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

Good, then we can at least consider this particular feature as being one
we're generally interested in and move forward with it, even if we also,
perhaps, come up with a better solution to the specific issue at hand.

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

Perhaps, but I'm not convinced that such speculation really helps move
us forward.

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

I'm not entirely sure what I was getting at above, to be honest, but I
believe we're generally on the same page here. I certainly agree that
users don't expect a pg_dump to not be able to be successfully restored.
What I may have been getting at is simply that it's not about a lack of
COMMENT IF NOT EXISTS, in an ideal world, but perhaps that's what we
need to solve this particular issue, for now.

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

Right, I tend to follow your line of thought here.

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

Ah, but pg_dump could potentially dump out more complicated logic than
it does today. We currently presume that there is never any need to
reason about the results of a prior command before executing the next in
pg_dump's output. In some ways, it's rather impressive that we've
gotten this far with that assumption, but ensuring that is the case
means that our users are also able to rely on that and write simple
scripts which can be rerun to reset the database to a specific state.

> During restore you have pg_restore+psql - and having cross-statement logic
> in those applications is likely a non-starter.

It would certainly be a very large shift from what we generate today. :)

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

I agree that adding COMMENT IF NOT EXISTS (or perhaps COMMENT ... TRY)
would be simpler than changing the syntax for every command to support a
COMMENT attribute being included. The issue here, however, is what I
mentioned previously- COMMENTs aren't the only attributes of an object
and if we really want to support "CREATE OBJECT + ATTRIBUTES IF NOT
EXISTS, otherwise do nothing" then that approach simply doesn't work,
without heavily modifying our syntax to allow much greater flexibility
than we've ever had before.

That's why I was suggesting that we have a mechanism for passing a set
of sub-commands to be performed on an object *if* we end up creating it,
on the basis of CREATE ... IF NOT EXISTS.

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

Ah, I see that distinction. I'm wondering how it might relate to other
attributes which an object might have and if we need to have similar
options for each (or perhaps we do? I've not looked, but I don't recall
anything similar offhand).

> > 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 ;)

That's a fair point, and might allow a quick-fix (I seriously doubt
anyone would really miss the comment we have on plpgsql today), but I'm
going to continue to push back a bit on this as I'd really like a better
solution, even if it's a bit more work. If such a solution doesn't
materialize before the last CF for PG 11, then I will support a motion
to simply remove the comment.

Thanks!

Stephen