Patch: fix pg_dump for inherited defaults & not-null flags
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@postgreSQL.org
Date: 2012-02-09T23:21:58Z
Lists: pgsql-hackers
Attachments
- dump-inherited-columns-properly.patch (text/x-patch) patch
Attached is a proposed patch to deal with the issue described here: http://archives.postgresql.org/pgsql-bugs/2012-02/msg00000.php Even though we'd previously realized that comparing the text of inherited CHECK expressions is an entirely unsafe way to detect expression equivalence (cf comments for guessConstraintInheritance), pg_dump is still doing that for inherited DEFAULT expressions, with the predictable result that it does the wrong thing in this sort of example. Furthermore, as I looked more closely at the code, I realized that there is another pretty fundamental issue: if an inherited column has a default expression or NOT NULL bit that it did not inherit from its parent, flagInhAttrs forces the column to be treated as non-inherited, so that it will be emitted as part of the child table's CREATE TABLE command. This is *wrong* if the column is not attislocal; it will result in the column incorrectly having the attislocal property after restore. (Note: such a situation could only arise if the user had altered the column's default or NOT NULL property with ALTER TABLE after creation.) All of this logic predates the invention of attislocal, and really is attempting to make up for the lack of that bit, so it's not all that surprising that it falls down. So the attached patch makes the emit-column-or-not behavior depend only on attislocal (except for binary upgrade which has its own kluge solution for the problem; though note that the whether-to-dump tests now exactly match the special cases for binary upgrade, which they did not before). Also, I've dropped the former attempts to exploit inheritance of defaults, and so the code will now emit a default explicitly for each child in an inheritance hierarchy, even if it didn't really need to. Since the backend doesn't track whether defaults are inherited, this doesn't cause any failure to restore the catalog state properly. Although this is a bug fix, it's a nontrivial change in the logic and so I'm hesitant to back-patch into stable branches. Given the lack of prior complaints, maybe it would be best to leave it unfixed in existing branches? Not sure. Thoughts? regards, tom lane