Обсуждение: Re: Fwd: [BUGS] BUG #14247: COMMENT is restored on wrong database
Moving to -hackers since this is getting into details
Bug Report # 14247
"David G. Johnston" <david.g.johnston@gmail.com> 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.
On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston <david.g.johnston@gmail.com> wrote: > Moving to -hackers since this is getting into details > > Bug Report # 14247 > > On Tue, Aug 2, 2016 at 4:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> "David G. Johnston" <david.g.johnston@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston
> <david.g.johnston@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 2, 2016 at 5:42 PM, David G. Johnston
> <david.g.johnston@gmail.com> 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.
On 5 August 2016 at 05:03, David G. Johnston <david.g.johnston@gmail.com> 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 DATABASE is 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.