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

The following bug has been logged on the website:

Bug reference: 9555
Logged by: Fazal Majid
Email address: majid(at)apsalar(dot)com
PostgreSQL version: 9.2.6
Operating system: OpenIndiana b151 Solaris x64
Description:

Reproduction case:

create table A(a int, b int, c int);
create table B(a int, c int);
alter table A inherit B;

pg_dump (whether in default or -Fc mode) generates:

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

and after pg_restore, the order of columns is inverted, as if the table
were:

CREATE TABLE a (
a integer,
c integer,
b intege,
)

This causes things like unqualified INSERT INTO statements to start failing
(bad practice, I know).

The solution would be to generate CREATE TABLE followed by ALTER TABLE
INHERIT rather than CREATE TABLE ... INHERITS.

majid(at)apsalar(dot)com writes:
> create table A(a int, b int, c int);
> create table B(a int, c int);
> alter table A inherit B;

> pg_dump (whether in default or -Fc mode) generates [ A with a,c,b ]

This is not a bug, but expected and desired behavior. The parent's column
order is considered canonical, and child tables are made to append their
columns to that. Perhaps in your use-case that's not how you'd like to
think about it, but if we were to change this we'd doubtless break things
for other people.

Possibly you could use pg_upgrade, which I believe does preserve physical
column order in such cases.

regards, tom lane

majid(at)apsalar(dot)com wrote:

> Reproduction case:
>
> create table A(a int, b int, c int);
> create table B(a int, c int);
> alter table A inherit B;

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.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> majid(at)apsalar(dot)com wrote:
>> Reproduction case:
>>
>> create table A(a int, b int, c int);
>> create table B(a int, c int);
>> alter table A inherit B;

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

regards, tom lane

Tom Lane-2 wrote
> Alvaro Herrera &lt;

> alvherre@

> &gt; writes:
>>

> majid@

> wrote:
>>> Reproduction case:
>>>
>>> create table A(a int, b int, c int);
>>> create table B(a int, c int);
>>> alter table A inherit B;
>
>> 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.

At least a warning is in order so that the user at least has chance to know
they are doing something inconsistent and can affect a manual refactoring of
their create/alter table commands to make them consistent.

Adding something like the following after the first paragraph in the inherit
clause of alter table would seem wise.

"The columns need not be in the same order, or in the leading position, in
the child as in the parent but if they are not then during a dump-restore
the resultant CREATE TABLE will physically re-order the columns. This will
affect any code that references the columns of this table using * (implied -
via insert - or otherwise)."

I imagine the rules are a little more complicated when more than one parent
is involved. The wording in CREATE TABLE is goes into great detail as to
how column names must be presented but do not comment of column ordering at
all. Passing around and using table references is not that uncommon that
the impact of inherent on column ordering should be ignored.

In the case of create table are there cases, without an intervening "alter
table ...inherits" that the initial create and the one run after
dump/restore could result in a different physical order? The main risk area
is adding/removing columns from the parent(s).

Without actually predicting the order would it at least be possible to run a
check of any inheritance trees affected by any kind of change to see if the
current column order would match the order resulting from a dump/restore and
emit a warning if a discrepancy is found?

Pg_upgrade may work but at minimum recovery is still broken as is creating
copies for development purposes.

David J.

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

On Mar 13, 2014, at 7:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> majid(at)apsalar(dot)com wrote:
>>> Reproduction case:
>>>
>>> create table A(a int, b int, c int);
>>> create table B(a int, c int);
>>> alter table A inherit B;
>
>> 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.

That makes sense, but then it would make sense to document this behavior under ALTER TABLE ... INHERIT, and possibly change its behavior so it reorders the columns on the source database’s data dictionary (I am not sure whether the logical column order has to match the physical order already embedded in the data files).

Thanks,

--
Fazal Majid
CTO, Apsalar Inc.

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > majid(at)apsalar(dot)com wrote:
> >> Reproduction case:
> >>
> >> create table A(a int, b int, c int);
> >> create table B(a int, c int);
> >> alter table A inherit B;
>
> > 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.

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

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Mar 14, 2014 at 12:33:04PM -0300, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > > majid(at)apsalar(dot)com wrote:
> > >> Reproduction case:
> > >>
> > >> create table A(a int, b int, c int);
> > >> create table B(a int, c int);
> > >> alter table A inherit B;
> >
> > > 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.

nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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(at)2ndquadrant(dot)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 definition
NOTICE: 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)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 &lt;

> bruce@

> &gt; 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(dot)g(dot)johnston(at)gmail(dot)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(dot)g(dot)johnston(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +