Re: Patch: fix pg_dump for inherited defaults & not-null flags

On Thu, Feb 9, 2012 at 6:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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?

I guess I'd be in favor of back-patching it, if that doesn't look like
too much of a job. We shouldn't assume that because only one person
reports a problem, no one else has been or will be affected.

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