Thread: 7.3 and HEAD broken for dropped columns of dropped types

I first thought that Andy Lewis' recent complaint might be a GIST
problem, but it's not.  Observe the following:

regression=# create domain mytype as int;
CREATE DOMAIN
regression=# create table foo (f1 int, f2 mytype);
CREATE TABLE
regression=# drop type mytype cascade;
NOTICE:  Drop cascades to table foo column f2
DROP TYPE
-- so far so good, but:
regression=# insert into foo values(1);
ERROR:  Unable to look up type id 703560
regression=# update foo set f1=1;
ERROR:  Unable to look up type id 703560

The failure occurs in ExecTypeFromTL(), which is required to build a
tuple descriptor for the output tuple of each plan node.  In these
cases, there is an output column (which will be NULL) for the dropped
column foo.f2 ... so the code goes off to get the type properties.
Oops.

Fortunately, we are not up the proverbial creek with no paddle, because
the only things we really need to know about the dropped column are its
typlen and typalign --- which just happen to still be recorded in
pg_attribute.attlen and attalign.  (Let's hear it for denormalization.)
It will take a little bit of code rearrangement to make that information
available to ExecTypeFromTL(), but I see no alternative.

I am thinking that it might be a good idea for ALTER TABLE DROP COLUMN
to reset atttypid to zero in the dropped column's pg_attribute row.
This would help catch any other places that are depending on a dropped
column's atttypid to still be valid.  On the other hand, it would
possibly confuse applications that are looking at pg_attribute.
Comments anyone?
        regards, tom lane

On Sun, May 11, 2003 at 04:20:59PM -0400, Tom Lane wrote:

> I am thinking that it might be a good idea for ALTER TABLE DROP COLUMN
> to reset atttypid to zero in the dropped column's pg_attribute row.
> This would help catch any other places that are depending on a dropped
> column's atttypid to still be valid.  On the other hand, it would
> possibly confuse applications that are looking at pg_attribute.
> Comments anyone?

Applications looking at pg_attribute and ignoring attisdropped are
broken anyway, so just exposing their brokenness by means of setting an
invalid atttypid does not make them any more (nor any less) broken.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

I said:
> Fortunately, we are not up the proverbial creek with no paddle, because
> the only things we really need to know about the dropped column are its
> typlen and typalign --- which just happen to still be recorded in
> pg_attribute.attlen and attalign.  (Let's hear it for denormalization.)
> It will take a little bit of code rearrangement to make that information
> available to ExecTypeFromTL(), but I see no alternative.

Actually, that idea doesn't work at all for the INSERT and UPDATE cases.
I was thinking that ExecTypeFromTL would see a Var that it could use to
look up the pg_attribute entry, but it won't --- what it will see in the
targetlist is a NULL constant.  There is actually no way to determine
the required information from just looking at the targetlist entry; you
need to know the result-relation tupledesc more or less a-priori.

After chewing on that for awhile, I have decided that our current
approach to doing INSERT/UPDATE in tables with deleted columns is all
wrong.  We should not try to generate a Plan tree in which there are
already nulls for deleted columns; instead, let the junkfilter code
insert the nulls.  The junkfilter is only used at the top level of a
plan, and it can easily be handed the result-relation tupledesc that
it needs to match.  This solution is free in the case of UPDATE,
since it will always need a junkfilter phase. It's not free for
INSERT, but the runtime cost seems fairly minimal.  Given that INSERT
most commonly inserts only one row, runtime cost is probably not the
thing to worry about anyway --- setup costs are, and I think this'll be
more or less a wash, since the planner end of things is simplified.

Any comments?
        regards, tom lane

> I am thinking that it might be a good idea for ALTER TABLE DROP COLUMN
> to reset atttypid to zero in the dropped column's pg_attribute row.
> This would help catch any other places that are depending on a dropped
> column's atttypid to still be valid.  On the other hand, it would
> possibly confuse applications that are looking at pg_attribute.
> Comments anyone?

I think you should zero it - we keep finding these little places where we've
missed thinking about dropped columns and that should flush them out.  Any
application looking at the type of a dropped column is confused anyway...
(Or they're doing a join...which could result in the dropped column not
appearing in their result set but hey...)

Chris