Обсуждение: 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.
Вложения
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: 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. 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: 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. > > 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: t > > Might 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.
Вложения
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: t > > Perhaps 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: t > > > > Perhaps 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
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
On Wed, Apr 27, 2011 at 09:30:41PM +0300, Peter Eisentraut wrote: > Here is the patch to fix that, as discussed. Looks correct. Thanks.