Typed-tables patch broke pg_upgrade
I find that pg_upgrade fails in HEAD when asked to do a 9.1-to-9.1
upgrade of the regression database. It gets to this bit of the
restore script:
CREATE TABLE test_tbl2 OF public.test_type2;
-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = -1, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
AND attrelid = 'test_tbl2'::pg_catalog.regclass;
ALTER TABLE ONLY test_tbl2 DROP COLUMN "........pg.dropped.2........";
and fails with
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
Somebody has failed to think through something, because if this state of
affairs was allowed to be created during the regression tests, why
should we not be able to restore it?
(pg_upgrade's ENUM support is broken too, but at least that one is a
one-line fix.)
regards, tom lane
Tom Lane wrote:
I find that pg_upgrade fails in HEAD when asked to do a 9.1-to-9.1
upgrade of the regression database. It gets to this bit of the
restore script:CREATE TABLE test_tbl2 OF public.test_type2;
-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = -1, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
AND attrelid = 'test_tbl2'::pg_catalog.regclass;
ALTER TABLE ONLY test_tbl2 DROP COLUMN "........pg.dropped.2........";and fails with
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
Somebody has failed to think through something, because if this state of
affairs was allowed to be created during the regression tests, why
should we not be able to restore it?
I am not aware of this code changing in 9.1. Was this test in 9.0?
Does this problem happen for 9.0?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
On ons, 2011-02-09 at 18:43 -0500, Tom Lane wrote:
I find that pg_upgrade fails in HEAD when asked to do a 9.1-to-9.1
upgrade of the regression database. It gets to this bit of the
restore script:CREATE TABLE test_tbl2 OF public.test_type2;
-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = -1, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
AND attrelid = 'test_tbl2'::pg_catalog.regclass;
ALTER TABLE ONLY test_tbl2 DROP COLUMN "........pg.dropped.2........";and fails with
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
It should call
ALTER TYPE test_type2 DROP ATTRIBUTE xyz CASCADE;
instead. That will propagate to the table.
I'm not sure though, whether a composite type preserves the dropped
attribute for re-dropping in this case.
On ons, 2011-02-09 at 23:16 -0500, Bruce Momjian wrote:
I am not aware of this code changing in 9.1. Was this test in 9.0?
Does this problem happen for 9.0?
No, because you can't drop anything from a typed table in 9.0.
Is this still an open bug?
---------------------------------------------------------------------------
Tom Lane wrote:
I find that pg_upgrade fails in HEAD when asked to do a 9.1-to-9.1
upgrade of the regression database. It gets to this bit of the
restore script:CREATE TABLE test_tbl2 OF public.test_type2;
-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = -1, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
AND attrelid = 'test_tbl2'::pg_catalog.regclass;
ALTER TABLE ONLY test_tbl2 DROP COLUMN "........pg.dropped.2........";and fails with
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
Somebody has failed to think through something, because if this state of
affairs was allowed to be created during the regression tests, why
should we not be able to restore it?(pg_upgrade's ENUM support is broken too, but at least that one is a
one-line fix.)regards, tom lane
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
On Fri, Mar 11, 2011 at 8:28 AM, Bruce Momjian <bruce@momjian.us> wrote:
Is this still an open bug?
Is anyone working on fixing this?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On tor, 2011-02-10 at 06:31 +0200, Peter Eisentraut wrote:
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
It should call
ALTER TYPE test_type2 DROP ATTRIBUTE xyz CASCADE;
instead. That will propagate to the table.
Here is a patch that addresses this problem.
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.
Attachments:
type-table-pgupgrade.patchtext/x-patch; charset=UTF-8; name=type-table-pgupgrade.patchDownload
diff --git i/src/bin/pg_dump/pg_dump.c w/src/bin/pg_dump/pg_dump.c
index 5561295..4cea954 100644
--- i/src/bin/pg_dump/pg_dump.c
+++ w/src/bin/pg_dump/pg_dump.c
@@ -7889,6 +7889,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
int ntups;
int i_attname;
int i_atttypdefn;
+ int i_attisdropped;
int i_typrelid;
int i;
@@ -7900,11 +7901,11 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
appendPQExpBuffer(query, "SELECT a.attname, "
"pg_catalog.format_type(a.atttypid, a.atttypmod) AS atttypdefn, "
+ "a.attisdropped, "
"typrelid "
"FROM pg_catalog.pg_type t, pg_catalog.pg_attribute a "
"WHERE t.oid = '%u'::pg_catalog.oid "
"AND a.attrelid = t.typrelid "
- "AND NOT a.attisdropped "
"ORDER BY a.attnum ",
tyinfo->dobj.catId.oid);
@@ -7915,6 +7916,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
i_attname = PQfnumber(res, "attname");
i_atttypdefn = PQfnumber(res, "atttypdefn");
+ i_attisdropped = PQfnumber(res, "attisdropped");
i_typrelid = PQfnumber(res, "typrelid");
if (binary_upgrade)
@@ -7932,11 +7934,20 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
{
char *attname;
char *atttypdefn;
+ bool attisdropped;
attname = PQgetvalue(res, i, i_attname);
atttypdefn = PQgetvalue(res, i, i_atttypdefn);
+ attisdropped = (PQgetvalue(res, i, i_attisdropped)[0] == 't');
- appendPQExpBuffer(q, "\n\t%s %s", fmtId(attname), atttypdefn);
+ if (attisdropped)
+ {
+ if (binary_upgrade)
+ /* see under dumpTableSchema() */
+ appendPQExpBuffer(q, "\n\t%s INTEGER /* dummy */", fmtId(attname));
+ }
+ else
+ appendPQExpBuffer(q, "\n\t%s %s", fmtId(attname), atttypdefn);
if (i < ntups - 1)
appendPQExpBuffer(q, ",");
}
@@ -12105,14 +12116,26 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
tbinfo->attlen[j],
tbinfo->attalign[j]);
appendStringLiteralAH(q, tbinfo->attnames[j], fout);
- appendPQExpBuffer(q, "\n AND attrelid = ");
+ appendPQExpBuffer(q, "\n AND attrelid IN (");
appendStringLiteralAH(q, fmtId(tbinfo->dobj.name), fout);
- appendPQExpBuffer(q, "::pg_catalog.regclass;\n");
+ appendPQExpBuffer(q, "::pg_catalog.regclass");
+ if (tbinfo->reloftype)
+ appendPQExpBuffer(q, ", '%s'::pg_catalog.regclass", tbinfo->reloftype);
+ appendPQExpBuffer(q, ");\n");
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
- fmtId(tbinfo->dobj.name));
- appendPQExpBuffer(q, "DROP COLUMN %s;\n",
- fmtId(tbinfo->attnames[j]));
+ if (tbinfo->reloftype)
+ {
+ appendPQExpBuffer(q, "ALTER TYPE %s ",
+ tbinfo->reloftype);
+ appendPQExpBuffer(q, "DROP ATTRIBUTE %s CASCADE;\n",
+ fmtId(tbinfo->attnames[j]));
+ }
+ else {
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+ fmtId(tbinfo->dobj.name));
+ appendPQExpBuffer(q, "DROP COLUMN %s;\n",
+ fmtId(tbinfo->attnames[j]));
+ }
}
else if (!tbinfo->attislocal[j])
{
On Wed, Mar 30, 2011 at 07:50:12PM +0300, Peter Eisentraut wrote:
On tor, 2011-02-10 at 06:31 +0200, Peter Eisentraut wrote:
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
It should call
ALTER TYPE test_type2 DROP ATTRIBUTE xyz CASCADE;
instead. That will propagate to the table.
Here is a patch that addresses this problem.
This only works when exactly one typed table uses each composite type having
dropped columns. With zero users, the placeholder column never gets dropped.
Actually, it happens to work for >1 user, but only because ALTER TYPE mistakenly
only touches the first table-of-type:
create type t as (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter type t drop attribute y cascade, add attribute z int cascade;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
z | integer |
Typed table of type: t
\d is_a2
Table "public.is_a2"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: t
Might be a simple fix; looks like find_typed_table_dependencies() only grabs the
first match. Incidentally, this led me to notice that you can hang a typed
table off a table row type. ALTER TABLE never propagates to such typed tables,
allowing them to get out of sync:
create table t (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter table t drop y, add z int;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: t
Perhaps we should disallow the use of table row types in CREATE TABLE ... OF?
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.
Your bits in dumpCompositeType() are most of what's needed to fix that, I think.
Thanks,
nm
On Wed, Mar 30, 2011 at 9:32 PM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Mar 30, 2011 at 07:50:12PM +0300, Peter Eisentraut wrote:
On tor, 2011-02-10 at 06:31 +0200, Peter Eisentraut wrote:
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
It should call
ALTER TYPE test_type2 DROP ATTRIBUTE xyz CASCADE;
instead. That will propagate to the table.
Here is a patch that addresses this problem.
This only works when exactly one typed table uses each composite type having
dropped columns. With zero users, the placeholder column never gets dropped.
Actually, it happens to work for >1 user, but only because ALTER TYPE mistakenly
only touches the first table-of-type:create type t as (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter type t drop attribute y cascade, add attribute z int cascade;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
z | integer |
Typed table of type: t
\d is_a2
Table "public.is_a2"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: tMight be a simple fix; looks like find_typed_table_dependencies() only grabs the
first match. Incidentally, this led me to notice that you can hang a typed
table off a table row type. ALTER TABLE never propagates to such typed tables,
allowing them to get out of sync:create table t (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter table t drop y, add z int;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: tPerhaps we should disallow the use of table row types in CREATE TABLE ... OF?
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.Your bits in dumpCompositeType() are most of what's needed to fix that, I think.
Where are we on this?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Apr 05, 2011 at 09:44:44AM -0400, Robert Haas wrote:
On Wed, Mar 30, 2011 at 9:32 PM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Mar 30, 2011 at 07:50:12PM +0300, Peter Eisentraut wrote:
Here is a patch that addresses this problem.
This only works when exactly one typed table uses each composite type having
dropped columns. ?With zero users, the placeholder column never gets dropped.
Actually, it happens to work for >1 user, but only because ALTER TYPE mistakenly
only touches the first table-of-type:create type t as (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter type t drop attribute y cascade, add attribute z int cascade;
\d is_a
? ? Table "public.is_a"
?Column | ?Type ? | Modifiers
--------+---------+-----------
?x ? ? ?| integer |
?z ? ? ?| integer |
Typed table of type: t
\d is_a2
? ? Table "public.is_a2"
?Column | ?Type ? | Modifiers
--------+---------+-----------
?x ? ? ?| integer |
?y ? ? ?| integer |
Typed table of type: tMight be a simple fix; looks like find_typed_table_dependencies() only grabs the
first match. ?Incidentally, this led me to notice that you can hang a typed
table off a table row type. ?ALTER TABLE never propagates to such typed tables,
allowing them to get out of sync:create table t (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter table t drop y, add z int;
\d is_a
? ? Table "public.is_a"
?Column | ?Type ? | Modifiers
--------+---------+-----------
?x ? ? ?| integer |
?y ? ? ?| integer |
Typed table of type: tPerhaps we should disallow the use of table row types in CREATE TABLE ... OF?
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.Your bits in dumpCompositeType() are most of what's needed to fix that, I think.
Where are we on this?
Peter, were you planning to complete this? I can take a swing at it, if it
would be helpful.
On ons, 2011-04-06 at 11:49 -0400, Noah Misch wrote:
Peter, were you planning to complete this? I can take a swing at it, if it
would be helpful.
Help is always welcome.
On Wed, Mar 30, 2011 at 09:32:08PM -0400, Noah Misch wrote:
... ALTER TYPE mistakenly
only touches the first table-of-type:create type t as (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter type t drop attribute y cascade, add attribute z int cascade;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
z | integer |
Typed table of type: t
\d is_a2
Table "public.is_a2"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: tMight be a simple fix; looks like find_typed_table_dependencies() only grabs the
first match.
This is a fairly independent one-liner, so here's that patch. I didn't
incorporate the test case, because it seems distinctly unlikely to recur.
Attachments:
tt0v1-alter-multiple.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4a97819..bd18db3 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 4014,4020 **** find_typed_table_dependencies(Oid typeOid, const char *typeName, DropBehavior be
scan = heap_beginscan(classRel, SnapshotNow, 1, key);
! if (HeapTupleIsValid(tuple = heap_getnext(scan, ForwardScanDirection)))
{
if (behavior == DROP_RESTRICT)
ereport(ERROR,
--- 4014,4020 ----
scan = heap_beginscan(classRel, SnapshotNow, 1, key);
! while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
if (behavior == DROP_RESTRICT)
ereport(ERROR,
On Wed, Mar 30, 2011 at 9:32 PM, Noah Misch <noah@leadboat.com> wrote:
Incidentally, this led me to notice that you can hang a typed
table off a table row type. ALTER TABLE never propagates to such typed tables,
allowing them to get out of sync:create table t (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter table t drop y, add z int;
\d is_a
Table "public.is_a"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
y | integer |
Typed table of type: tPerhaps we should disallow the use of table row types in CREATE TABLE ... OF?
Yes, I think we need to do that.
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.Your bits in dumpCompositeType() are most of what's needed to fix that, I think.
Most?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Mar 30, 2011 at 12:50 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On tor, 2011-02-10 at 06:31 +0200, Peter Eisentraut wrote:
ERROR: cannot drop column from typed table
which probably is because test_type2 has a dropped column.
It should call
ALTER TYPE test_type2 DROP ATTRIBUTE xyz CASCADE;
instead. That will propagate to the table.
Here is a patch that addresses this problem.
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.
There's something wrong with this patch - it never arranges to
actually drop the phony column. Consider:
create type foo as (a int, b int);
alter table foo drop attribute b;
create table x (a int, b int);
alter table x drop column b;
Then pg_dump --binary-upgrade emits, in relevant part, the following for x:
CREATE TABLE x (
a integer,
"........pg.dropped.2........" INTEGER /* dummy */
);
-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = 4, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
AND attrelid IN ('x'::pg_catalog.regclass);
ALTER TABLE ONLY x DROP COLUMN "........pg.dropped.2........";
But for t we get only:
CREATE TYPE foo AS (
a integer,
"........pg.dropped.2........" INTEGER /* dummy */
);
...which is no good.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Apr 08, 2011 at 03:43:39PM -0400, Robert Haas wrote:
On Wed, Mar 30, 2011 at 9:32 PM, Noah Misch <noah@leadboat.com> wrote:
Incidentally, this led me to notice that you can hang a typed
table off a table row type. ?ALTER TABLE never propagates to such typed tables,
allowing them to get out of sync:create table t (x int, y int);
create table is_a of t;
create table is_a2 of t;
alter table t drop y, add z int;
\d is_a
? ? Table "public.is_a"
?Column | ?Type ? | Modifiers
--------+---------+-----------
?x ? ? ?| integer |
?y ? ? ?| integer |
Typed table of type: tPerhaps we should disallow the use of table row types in CREATE TABLE ... OF?
Yes, I think we need to do that.
Having thought about it some more, that would be unfortunate. We rarely
distinguish between table row types and CREATE TYPE AS types. Actually, I'm not
aware of any place we distinguish other than in ALTER TABLE/ALTER TYPE, to
instruct you to use the other.
But depending on how hard it is to fix, that might be a good stopgap.
It looks like Noah Misch might have found another problem in this area.
We'll have to investigate that.Your bits in dumpCompositeType() are most of what's needed to fix that, I think.
Most?
I think it will just fall out of the completed fix for the original reported
problem. Will keep you posted.
nm
Here is the patch to fix that, as discussed.
Attachments:
typed-table-pgupgrade.patchtext/x-patch; charset=UTF-8; name=typed-table-pgupgrade.patchDownload
diff --git i/src/bin/pg_dump/pg_dump.c w/src/bin/pg_dump/pg_dump.c
index c2f6180..afc7fd7 100644
--- i/src/bin/pg_dump/pg_dump.c
+++ w/src/bin/pg_dump/pg_dump.c
@@ -12004,7 +12004,11 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
"UNLOGGED " : "",
reltypename,
fmtId(tbinfo->dobj.name));
- if (tbinfo->reloftype)
+ /*
+ * In case of a binary upgrade, we dump the table normally and attach
+ * it to the type afterward.
+ */
+ if (tbinfo->reloftype && !binary_upgrade)
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
actual_atts = 0;
for (j = 0; j < tbinfo->numatts; j++)
@@ -12032,7 +12036,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
bool has_notnull = (tbinfo->notnull[j]
&& (!tbinfo->inhNotNull[j] || binary_upgrade));
- if (tbinfo->reloftype && !has_default && !has_notnull)
+ if (tbinfo->reloftype && !has_default && !has_notnull && !binary_upgrade)
continue;
/* Format properly if not first attr */
@@ -12060,7 +12064,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
}
/* Attribute type */
- if (tbinfo->reloftype)
+ if (tbinfo->reloftype && !binary_upgrade)
{
appendPQExpBuffer(q, "WITH OPTIONS");
}
@@ -12126,7 +12130,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (actual_atts)
appendPQExpBuffer(q, "\n)");
- else if (!tbinfo->reloftype)
+ else if (!(tbinfo->reloftype && !binary_upgrade))
{
/*
* We must have a parenthesized attribute list, even though empty,
@@ -12192,6 +12196,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* an INHERITS clause --- the latter would possibly mess up the column
* order. That also means we have to take care about setting
* attislocal correctly, plus fix up any inherited CHECK constraints.
+ * Analogously, we set up typed tables using ALTER TABLE / OF here.
*/
if (binary_upgrade && tbinfo->relkind == RELKIND_RELATION)
{
@@ -12268,6 +12273,14 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
}
}
+ if (tbinfo->reloftype)
+ {
+ appendPQExpBuffer(q, "\n-- For binary upgrade, set up typed tables this way.\n");
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s OF %s;\n",
+ fmtId(tbinfo->dobj.name),
+ tbinfo->reloftype);
+ }
+
appendPQExpBuffer(q, "\n-- For binary upgrade, set heap's relfrozenxid\n");
appendPQExpBuffer(q, "UPDATE pg_catalog.pg_class\n"
"SET relfrozenxid = '%u'\n"
Peter Eisentraut <peter_e@gmx.net> writes:
Here is the patch to fix that, as discussed.
Looks sane --- I assume you tested it against the originally
complained-of scenario?
http://archives.postgresql.org/message-id/201103111328.p2BDSFd10499@momjian.us
If so, please apply soon --- we need to wrap beta1 this evening.
regards, tom lane