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

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

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Feb 9, 2012 at 6:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

I don't think it's too much work --- what I'm more worried about is
introducing new bugs. If I apply it only in HEAD then it will go
through a beta test cycle before anybody relies on it in production.
I *think* the patch is okay, but I've been wrong before.

regards, tom lane

On Fri, Feb 10, 2012 at 10:52 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Feb 9, 2012 at 6:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>
> I don't think it's too much work --- what I'm more worried about is
> introducing new bugs.  If I apply it only in HEAD then it will go
> through a beta test cycle before anybody relies on it in production.
> I *think* the patch is okay, but I've been wrong before.

Well, I'm not going to second-guess your judgement too much, but my
general feeling is that it's worth taking a bit of risk to get pg_dump
to DTRT. Dump and restore failures are extremely painful and
difficult to work around, so in my book they rank pretty highly in the
list of things that are important to get fixed. So if you're on the
fence, I'd lean toward back-patching.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> ... I'd lean toward back-patching.

Not hearing any contrary opinions, that's what I've done.

regards, tom lane