Re: [HACKERS] Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database

[Craig Ringer]

On 5 August 2016 at 05:03, David G. Johnston 
wrote:


> ​
> I'm all for an elegant solution here though at some point having a working
> solution now beats waiting for someone to willingly dive more deeply into
> pg_dump.  I too seem to recall previous proposals for COMMON ON CURRENT
> DATABASE yet here we are...
>
>
SECURITY LABEL ... ON CURRENT DATABASEis needed too, and has caused
real-world pain.

I tend to agree that adding and using ... ON CURRENT DATABASE is worthwhile
now. It's guaranteed to be at least as correct as what pg_dump emits now.

We do have a horrible mess with how pg_dump handles database level
properties, but I'd rather not try to deal with that at the same time, get
into a long discussion and land up fixing nothing.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database

[David G. Johnston]

On Thu, Aug 4, 2016 at 4:50 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston
> >  wrote:
> > The fact that pg_dump is emitting COMMENT ON DATABASE at all is
> > fundamentally wrong given the existing division-of-labor decisions,
> > namely that pg_dump is responsible for objects within a database
> > not for database-level properties.
>
> > I think a while back somebody had the idea of making COMMENT ON
> > CURRENT_DATABASE or COMMENT ON CURRENT DATABASE work, which seems like
> > an elegant solution to me.  Of course, I just work here.
>
> I'm fairly annoyed at David for having selectively quoted from private
> email in a public forum, but that was one of the points I touched on
> in material that he cut.



> The point I tried to make to him is that
> possibly COMMENT ON CURRENT DATABASE is a portion of a holistic solution,
> but it's only a portion.


I should have asked first and ​I'll take the heat for choosing to re-post
publicly but I kept this aspect of your recommendation precisely because
that was indeed your position.

TL>> It's entirely possible that some feature like COMMENT ON CURRENT
DATABASE
TL>> would be a necessary component of a holistic solution,​ [ various
other ON CURRENT commands elidded ]​
​
I'm all for an elegant solution here though at some point having a working
solution now beats waiting for someone to willingly dive more deeply into
pg_dump.  I too seem to recall previous proposals for COMMON ON CURRENT
DATABASE yet here we are...

​David J.​

Re: [HACKERS] Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database

[Tom Lane]

Robert Haas  writes:
> On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston
>  wrote:
> The fact that pg_dump is emitting COMMENT ON DATABASE at all is
> fundamentally wrong given the existing division-of-labor decisions,
> namely that pg_dump is responsible for objects within a database
> not for database-level properties.

> I think a while back somebody had the idea of making COMMENT ON
> CURRENT_DATABASE or COMMENT ON CURRENT DATABASE work, which seems like
> an elegant solution to me.  Of course, I just work here.

I'm fairly annoyed at David for having selectively quoted from private
email in a public forum, but that was one of the points I touched on
in material that he cut.  The point I tried to make to him is that
possibly COMMENT ON CURRENT DATABASE is a portion of a holistic solution,
but it's only a portion.  We need to rethink exactly what pg_dump is
supposed to do with database-level properties.  And if it does need
COMMENT ON CURRENT DATABASE, it likely also needs GRANT ... ON CURRENT
DATABASE, ALTER CURRENT DATABASE OWNER TO, ALTER CURRENT DATABASE SET,
and maybe some other things.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database

[Robert Haas]

On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston
 wrote:
> Moving to -hackers since this is getting into details
>
> Bug Report # 14247
>
> On Tue, Aug 2, 2016 at 4:58 PM, Tom Lane  wrote:
>>
>> "David G. Johnston"  writes:
>> > Do you have an opinion on this following?
>>
>> I think the real problem in this area is that the division of labor
>> we have between pg_dump and pg_dumpall isn't very good for real-world
>> use cases.
>
>
> I won't disagree here.
>
>>
>>   I'm not at all sure what a better answer would look like.
>> But I'd rather see us take two steps back and consider the issue
>> holistically instead of trying to band-aid individual misbehaviors.
>>
>> The fact that pg_dump is emitting COMMENT ON DATABASE at all is
>> fundamentally wrong given the existing division-of-labor decisions,
>> namely that pg_dump is responsible for objects within a database
>> not for database-level properties.

I think a while back somebody had the idea of making COMMENT ON
CURRENT_DATABASE or COMMENT ON CURRENT DATABASE work, which seems like
an elegant solution to me.  Of course, I just work here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database

[David G. Johnston]

Moving to -hackers since this is getting into details

Bug Report # 14247

On Tue, Aug 2, 2016 at 4:58 PM, Tom Lane  wrote:

> "David G. Johnston"  writes:
> > Do you have an opinion on this following?
>
> I think the real problem in this area is that the division of labor
> we have between pg_dump and pg_dumpall isn't very good for real-world
> use cases.


​I won't disagree here​.


>   I'm not at all sure what a better answer would look like.
> But I'd rather see us take two steps back and consider the issue
> holistically instead of trying to band-aid individual misbehaviors.
>
> The fact that pg_dump is emitting COMMENT ON DATABASE at all is
> fundamentally wrong given the existing division-of-labor decisions,
> namely that pg_dump is responsible for objects within a database
> not for database-level properties.


​But I have to disagree with this in the presence of the --create flag.


> It's entirely possible that some feature like COMMENT ON CURRENT DATABASE
> would be a necessary component of a holistic solution,
> ​ [ various other ON CURRENT commands elidded ]​
>

In reading the code for the ​public schema bug I never fully got my head
around how dump/restore interacts with multiple databases.  Specifically,
in RestoreArchive, why we basically continue instead of breaking once we've
encountered the "DATABASE" entry and decide that we need to drop the DB due
to (createDB && dropSchema).

​OTOH, seeing the code for the public schema exception I'm inclined to
think ​that adding something like the follow just after the public schema
block just patched:

/* If the user didn't request that a new database be created skip emitting
 * commands targeting the current database as they may not have the same
 * name as the database being restored into.
 *
 * XXX This too is ugly but the treatment of globals is not presently
amenable to pretty solutions
 */
if (!ropt->createDB))
{
if (strcmp(te->desc, "DATABASE") != 0  //this is quite possibly too broad a
check...I'm always concerned about false positives in cases like these.
 return;
}

​Though reading it now that seems a bit like throwing out the baby with the
bath water; but then again if they are not requesting a database be created
and they happen to have a database of the same name already in place it
would be unexpected that it would not have been properly configured.

Assuming I'm not missing something here it seems like an easy and
internally consistent hack/fix for a rare but real concern.  However, since
the existing code doesn't provoke an error it doesn't seem like something
this should back-patched (I'm not convinced either way though).

David J.