Re: 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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)gmx(dot)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(at)leadboat(dot)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

Here is the patch to fix that, as discussed.

Peter Eisentraut <peter_e(at)gmx(dot)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.