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