BUG #14247: COMMENT is restored on wrong database

On Wed, Jul 13, 2016 at 9:21 AM, <pgerber@tocco.ch> wrote:

The following bug has been logged on the website:

Bug reference: 14247
Logged by: Peter Gerber
Email address: pgerber@tocco.ch
PostgreSQL version: 9.5.3
Operating system: Linux
Description:

Comments on a databases are restored on the database named the same the
dump
was made from. For instance, if a dump is made from a database called
'source_db' and restored into 'target_db' the comment is restored on the
wrong database, 'source_db'.

How to reproduce:
1: CREATE DATABASE source_db;
2. COMMENT ON DATABASE source_db IS 'test';
3. pg_dump -Fc -d source_db -f dump;
4. COMMENT ON DATABASE source_db IS null;
5. CREATE DATABASE target_db;
6. pg_restore -d target_db dump
7. SELECT datname, description FROM pg_shdescription
JOIN pg_database ON objoid = pg_database.oid
WHERE datname in ('source_db', 'target_db');
datname | description
-----------+-------------
source_db | test
(1 row)

Also, if pg_dump is done without -Fc it contains this line:
COMMENT ON DATABASE dbrefactoring_new_210 IS 'test';

​I'd posit that attempting to issue the COMMENT command without the user
specifying "pg_restore --create" ​is the bug. pg_restore shouldn't go
about altering globals that it did not itself create. If --create is
specified you don't get to rename the database and the OP's problem cannot
happen.

pg_dump plain text should adhere to the same rule - the COMMENT command
should be omitted if pg_dump is not provided a "--create" flag.

Thus, if you want to rename the database it is your responsibility to
provide a valid comment for it. There is no way for pg_dump/pg_restore to
rename the database during the restoration procedure and adding logic to
them to detect when the user intended a rename and change the scripts
accordingly doesn't seem wise.

David J.

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.

​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

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

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

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

On Thu, Aug 4, 2016 at 4:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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