Thread: Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns

On Mon, Mar 17, 2014 at 07:12:12PM -0400, Noah Misch wrote:
> On Fri, Mar 14, 2014 at 12:33:04PM -0300, Alvaro Herrera wrote:
> > Tom Lane wrote:
> > > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > > I wonder if the real fix here is to have ALTER / INHERIT error out of
> > > > the columns in B are not a prefix of those in A.
> > > 
> > > Years ago, we sweated quite a lot of blood to make these cases work.
> > > I'm not thrilled about throwing away all that effort because one person
> > > doesn't like the behavior.
> 
> Agreed.  That also makes the current pg_dump behavior a bug.  Column order
> matters; pg_dump is failing to recreate a semantically-equivalent database.
> 
> > Hm, well in that case it makes sense to consider the original
> > suggestion: if the columns in the parent are not a prefix of those of
> > the child, use ALTER INHERIT after creating both tables rather than
> > CREATE TABLE INHERITS.
> > 
> > It'd be a lot of new code in pg_dump though.  I am not volunteering ...
> 
> "pg_dump --binary-upgrade" already gets this right.  Perhaps it won't take too
> much code to make dumpTableSchema() reuse that one part of its binary-upgrade
> approach whenever the columns of B are not a prefix of those in A.

[thread moved to hackers]

I looked at this issue from March and I think we need to do something. 
In summary, the problem is that tables using inheritance can be dumped
and reloaded with columns in a different order from the original
cluster.  What is a basically happening is that these queries:
CREATE TABLE A(a int, b int, c int);CREATE TABLE B(a int, c int);ALTER TABLE A INHERIT B;

cause pg_dump to generate this:
CREATE TABLE b (    a integer,    c integer);CREATE TABLE a (    a integer,    b integer,    c integer)INHERITS (b);

which issues these warnings when run:
NOTICE:  merging column "a" with inherited definitionNOTICE:  merging column "c" with inherited definition

and produces this table "a":
test2=> \d a       Table "public.a" Column |  Type   | Modifiers--------+---------+----------- a      | integer |
-->     c      | integer | b      | integer |

Notice the column reordering.  The logic is that a CREATE TABLE INHERITS
should place the inherited parent columns _first_.  This can't be done
by ALTER TABLE INHERIT because the table might already contain data.

I think we have several options:

1.  document this behavior
2.  have ALTER TABLE INHERIT issue a warning about future reordering
3.  use the pg_dump binary-upgrade code when such cases happen

My crude approach for #3 would be for pg_dump to loop over the columns
and, where pg_attribute.attinhcount == 0, check to see if there is a
matching column name in any inherited table.  Will such tables load fine
because pg_dump binary-upgrade mode doesn't do any data loading?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> I looked at this issue from March and I think we need to do something. 
> In summary, the problem is that tables using inheritance can be dumped
> and reloaded with columns in a different order from the original
> cluster.

Yeah ... this has been a well-understood issue for a dozen years, and
pg_dump goes to considerable trouble to get it right.

> I think we have several options:

> 1.  document this behavior

That one.
        regards, tom lane

On Wed, Aug 27, 2014 at 11:24:53AM -0400, Tom Lane wrote:
> On Wed, Aug 27, 2014 at 10:40:53AM -0400, Bruce Momjian wrote:
> > I looked at this issue from March and I think we need to do something. 
> > In summary, the problem is that tables using inheritance can be dumped
> > and reloaded with columns in a different order from the original
> > cluster.
> 
> Yeah ... this has been a well-understood issue for a dozen years, and
> pg_dump goes to considerable trouble to get it right.

pg_dump goes to trouble to preserve attislocal but not to preserve inherited
column order.  Hence this thread about pg_dump getting column order wrong.

> > I think we have several options:
> > 
> > 1.  document this behavior
> 
> That one.

+1; certainly reasonable as a first step.

> > 2.  have ALTER TABLE INHERIT issue a warning about future reordering

That warning would summarize as "WARNING: this object is now subject to a
known bug".  -0; I'm not strongly opposed, but it's not our norm.

> > 3.  use the pg_dump binary-upgrade code when such cases happen

+1.  We have the convention that, while --binary-upgrade can inject catalog
hacks, regular pg_dump uses standard, documented DDL.  I like that convention
on general aesthetic grounds and for its benefit to non-superusers.  Let's
introduce the DDL needed to fix this bug while preserving that convention,
namely DDL to toggle attislocal.

> > My crude approach for #3 would be for pg_dump to loop over the columns
> > and, where pg_attribute.attinhcount == 0, check to see if there is a
> > matching column name in any inherited table.

That doesn't look right.  attinhcount is essentially a cache; it shall equal
the number of parents having a matching column.  The approach we use in binary
upgrade mode ought to suffice.

> > Will such tables load fine
> > because pg_dump binary-upgrade mode doesn't do any data loading?

We're now talking about changes to pg_dump's normal (non-binary-upgrade) mode,
right?  pg_dump always gives COPY a column list, so I don't expect trouble on
the data load side of things.

Thanks,
nm

On Wed, Aug 27, 2014 at 09:40:30PM -0400, Noah Misch wrote:
> > > 3.  use the pg_dump binary-upgrade code when such cases happen
>
> +1.  We have the convention that, while --binary-upgrade can inject catalog
> hacks, regular pg_dump uses standard, documented DDL.  I like that convention
> on general aesthetic grounds and for its benefit to non-superusers.  Let's
> introduce the DDL needed to fix this bug while preserving that convention,
> namely DDL to toggle attislocal.

I have spend some time researching this, and I am not sure what to
recommend.  The basic issue is that CREATE TABLE INHERITS always puts
the inherited columns first, so to preserve column ordering, you have to
use CREATE TABLE and then ALTER TABLE INHERIT.  The problem there is
that ALTER TABLE INHERIT doesn't preserve attislocal, and it also has
problems with constraints not being marked local.  I am just not sure we
want to add SQL-level code to do that.  Would it be documented?

I decided to step back and consider some issues.  Basically, in
non-binary-upgrade mode, pg_dump is take:

    CREATE TABLE A(a int, b int, c int);
    CREATE TABLE B(a int, c int);
    ALTER TABLE a INHERIT B;

and dumping it as:

    CREATE TABLE a (
        a integer,
        b integer,
        c integer
    )
    INHERITS (b);

This looks perfect, but, of course, it isn't.  Columns c is going to be
placed before 'b'.  You do get a notice about merged columns, but no
mention of the column reordering:

    NOTICE:  merging column "a" with inherited definition
    NOTICE:  merging column "c" with inherited definition

I think there are two common cases for CREATE TABLE INHERITS, and then
there is this odd one.  The common cases are cases where all columns
inherited are mentioned explicitly in CREATE TABLE INHERITS, in order,
and the other case where none of the inherited columns are mentioned.
The case above is the odd case where some are mentioned, but in a
different order.

I have developed the attached patch to warn about column reordering in
this odd case.  The patch mentions the reordering of c:

    NOTICE:  merging column "a" with inherited definition
    NOTICE:  merging column "c" with inherited definition;  column moved earlier to match inherited column location

This, of course, will be emitted when the pg_dump output is restored.
This is probably the minimum we should do.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> I have developed the attached patch to warn about column reordering in
> this odd case.  The patch mentions the reordering of c:

>     NOTICE:  merging column "a" with inherited definition
>     NOTICE:  merging column "c" with inherited definition;  column moved earlier to match inherited column location

This does not comport with our error message style guidelines.
You could put the additional information as an errdetail sentence,
perhaps.
        regards, tom lane

Tom Lane-2 wrote
> Bruce Momjian <

> bruce@

> > writes:
>> I have developed the attached patch to warn about column reordering in
>> this odd case.  The patch mentions the reordering of c:
> 
>>     NOTICE:  merging column "a" with inherited definition
>>     NOTICE:  merging column "c" with inherited definition;  column moved
>> earlier to match inherited column location
> 
> This does not comport with our error message style guidelines.
> You could put the additional information as an errdetail sentence,
> perhaps.

Would it be proper to issue an additional top-level warning with the column
moved notification?  Thus there would be NOTICE, NOTICE, WARNING in the
above example?  Or, more generically, "columns reordered to match inherited
column order" to avoid multiple warnings if more than one column is moved.

David J.



--
View this message in context:
http://postgresql.1045698.n5.nabble.com/Re-BUGS-Re-BUG-9555-pg-dump-for-tables-with-inheritance-recreates-the-table-with-the-wrong-order-of-s-tp5816566p5817073.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston <david.g.johnston@gmail.com> writes:
> Would it be proper to issue an additional top-level warning with the column
> moved notification?  Thus there would be NOTICE, NOTICE, WARNING in the
> above example?  Or, more generically, "columns reordered to match inherited
> column order" to avoid multiple warnings if more than one column is moved.

That's a good point: if this message fires at all, it will probably fire
more than once; do we want that?  If we do it as you suggest here, we'll
lose the information as to exactly which columns got relocated, which
perhaps is bad, or maybe it doesn't matter.  Also, I don't remember the
exact code structure in that area, but it might be a bit painful to
arrange that we get only one such warning even when inheriting from
multiple parents.

If we do want the specific moved columns to be identified, I'd still go
with errdetail on the NOTICE rather than two separate messages.  I think
calling it a WARNING is a bit extreme anyway.
        regards, tom lane

On Sun, Aug 31, 2014 at 02:10:33PM -0400, Tom Lane wrote:
> David G Johnston <david.g.johnston@gmail.com> writes:
> > Would it be proper to issue an additional top-level warning with the column
> > moved notification?  Thus there would be NOTICE, NOTICE, WARNING in the
> > above example?  Or, more generically, "columns reordered to match inherited
> > column order" to avoid multiple warnings if more than one column is moved.
>
> That's a good point: if this message fires at all, it will probably fire
> more than once; do we want that?  If we do it as you suggest here, we'll
> lose the information as to exactly which columns got relocated, which
> perhaps is bad, or maybe it doesn't matter.  Also, I don't remember the
> exact code structure in that area, but it might be a bit painful to
> arrange that we get only one such warning even when inheriting from
> multiple parents.
>
> If we do want the specific moved columns to be identified, I'd still go
> with errdetail on the NOTICE rather than two separate messages.  I think
> calling it a WARNING is a bit extreme anyway.

OK, here is the updated output based on the comments:

    CREATE TABLE B(a int, c int);
    CREATE TABLE a5 (
        a integer,
        b integer,
        c integer
    )
    INHERITS (b);
    NOTICE:  merging column "a" with inherited definition
    NOTICE:  moving and merging column "c" with inherited definition
    DETAIL:  user-specified column moved to the location of the inherited
    column

I think we have to mention "move" in the error message because
mentioning "move" only in the detail means that the detail actually has
new information, not more detailed information.

Patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
>     NOTICE:  moving and merging column "c" with inherited definition
>     DETAIL:  user-specified column moved to the location of the inherited
>     column

Dept of nitpicking: errdetail messages are supposed to be complete
sentences, properly capitalized and punctuated.  Please re-read the
style guidelines if you have forgotten them.
        regards, tom lane

On Mon, Sep  1, 2014 at 04:06:58PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> >     NOTICE:  moving and merging column "c" with inherited definition
> >     DETAIL:  user-specified column moved to the location of the inherited
> >     column
>
> Dept of nitpicking: errdetail messages are supposed to be complete
> sentences, properly capitalized and punctuated.  Please re-read the
> style guidelines if you have forgotten them.

Oh, yeah;  updated patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Sat, Aug 30, 2014 at 07:32:26PM -0400, Bruce Momjian wrote:
> On Wed, Aug 27, 2014 at 09:40:30PM -0400, Noah Misch wrote:
> > > > 3.  use the pg_dump binary-upgrade code when such cases happen
> > 
> > +1.  We have the convention that, while --binary-upgrade can inject catalog
> > hacks, regular pg_dump uses standard, documented DDL.  I like that convention
> > on general aesthetic grounds and for its benefit to non-superusers.  Let's
> > introduce the DDL needed to fix this bug while preserving that convention,
> > namely DDL to toggle attislocal.
> 
> I have spend some time researching this, and I am not sure what to
> recommend.  The basic issue is that CREATE TABLE INHERITS always puts
> the inherited columns first, so to preserve column ordering, you have to
> use CREATE TABLE and then ALTER TABLE INHERIT.  The problem there is
> that ALTER TABLE INHERIT doesn't preserve attislocal, and it also has
> problems with constraints not being marked local.  I am just not sure we
> want to add SQL-level code to do that.  Would it be documented?

Yes; I value the fact that ordinary pg_dump emits only documented SQL.  In a
similar vein, we added ALTER TABLE OF for the benefit of pg_dump.

> I have developed the attached patch to warn about column reordering in
> this odd case.  The patch mentions the reordering of c:

This, as amended downthread, seems useful.

On Mon, Sep  1, 2014 at 04:40:11PM -0400, Bruce Momjian wrote:
> On Mon, Sep  1, 2014 at 04:06:58PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > >     NOTICE:  moving and merging column "c" with inherited definition
> > >     DETAIL:  user-specified column moved to the location of the inherited
> > >     column
> > 
> > Dept of nitpicking: errdetail messages are supposed to be complete
> > sentences, properly capitalized and punctuated.  Please re-read the
> > style guidelines if you have forgotten them.
> 
> Oh, yeah;  updated patch attached.

OK, patch applied.  This will warn about reordering that happens via
SQL, and via pg_dump restore.  Do we want to go farther and preserve
column ordering by adding ALTER TABLE [constraint] ISLOCAL and have
pg_dump reuse binary-upgrade mode?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Wed, Sep  3, 2014 at 12:07:55PM -0400, Bruce Momjian wrote:
> On Mon, Sep  1, 2014 at 04:40:11PM -0400, Bruce Momjian wrote:
> > On Mon, Sep  1, 2014 at 04:06:58PM -0400, Tom Lane wrote:
> > > Bruce Momjian <bruce@momjian.us> writes:
> > > >     NOTICE:  moving and merging column "c" with inherited definition
> > > >     DETAIL:  user-specified column moved to the location of the inherited
> > > >     column
> > > 
> > > Dept of nitpicking: errdetail messages are supposed to be complete
> > > sentences, properly capitalized and punctuated.  Please re-read the
> > > style guidelines if you have forgotten them.
> > 
> > Oh, yeah;  updated patch attached.
> 
> OK, patch applied.  This will warn about reordering that happens via
> SQL, and via pg_dump restore.  Do we want to go farther and preserve
> column ordering by adding ALTER TABLE [constraint] ISLOCAL and have
> pg_dump reuse binary-upgrade mode?

OK, hearing nothing, I will consider the improved notice message as
sufficient and this item closed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +