Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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 br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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 br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
On Sun, Aug 31, 2014 at 02:10:33PM -0400, Tom Lane wrote:
David G Johnston david.g.johns...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 3720a0f..b88b664
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** MergeAttributes(List *schema, List *supe
*** 1756,1767
--- 1756,1771
*/
if (inhSchema != NIL)
{
+ int schema_attno = 0;
+
foreach(entry, schema)
{
ColumnDef *newdef = lfirst(entry);
char *attributeName = newdef-colname;
int exist_attno;
+ schema_attno++;
+
/*
* Does it conflict with some previously inherited column?
*/
*** MergeAttributes(List *schema, List *supe
*** 1780,1788
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--- 1784,1797
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! if (exist_attno == schema_attno)
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
! else
! ereport(NOTICE,
! (errmsg(moving and merging column \%s\ with inherited definition, attributeName),
! errdetail(user-specified column moved to the location of the inherited column)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
On Mon, Sep 1, 2014 at 04:06:58PM -0400, Tom Lane wrote:
Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 3720a0f..ece6b0f
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** MergeAttributes(List *schema, List *supe
*** 1756,1767
--- 1756,1771
*/
if (inhSchema != NIL)
{
+ int schema_attno = 0;
+
foreach(entry, schema)
{
ColumnDef *newdef = lfirst(entry);
char *attributeName = newdef-colname;
int exist_attno;
+ schema_attno++;
+
/*
* Does it conflict with some previously inherited column?
*/
*** MergeAttributes(List *schema, List *supe
*** 1780,1788
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--- 1784,1797
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! if (exist_attno == schema_attno)
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
! else
! ereport(NOTICE,
! (errmsg(moving and merging column \%s\ with inherited definition, attributeName),
! errdetail(User-specified column moved to the location of the inherited column.)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
David G Johnston david.g.johns...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 3720a0f..4846d25
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** MergeAttributes(List *schema, List *supe
*** 1756,1767
--- 1756,1771
*/
if (inhSchema != NIL)
{
+ int schema_attno = 0;
+
foreach(entry, schema)
{
ColumnDef *newdef = lfirst(entry);
char *attributeName = newdef-colname;
int exist_attno;
+ schema_attno++;
+
/*
* Does it conflict with some previously inherited column?
*/
*** MergeAttributes(List *schema, List *supe
*** 1780,1788
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--- 1784,1797
* Yes, try to merge the two column definitions. They must
* have the same type, typmod, and collation.
*/
! if (exist_attno == schema_attno)
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition,
! attributeName)));
! else
! ereport(NOTICE,
! (errmsg(merging column \%s\ with inherited definition; column moved earlier to match inherited column location,
! attributeName)));
def = (ColumnDef *) list_nth(inhSchema, exist_attno - 1);
typenameTypeIdAndMod(NULL, def-typeName, defTypeId, deftypmod);
typenameTypeIdAndMod(NULL, newdef-typeName, newTypeId, newtypmod);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 alvhe...@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 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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] Re: BUG #9555: pg_dump for tables with inheritance recreates the table with the wrong order of columns
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
