From d97c9b366273a49f0469184df9dfb3a312b2f3ff Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 12 May 2003 00:17:03 +0000 Subject: [PATCH] Apply fixes for problems with dropped columns whose types have also been dropped. The simplest fix for INSERT/UPDATE cases turns out to be for preptlist.c to insert NULLs of a known-good type (I used INT4) rather than making them match the deleted column's type. Since the representation of NULL is actually datatype-independent, this should work fine. I also re-reverted the patch to disable the use_physical_tlist optimization in the presence of dropped columns. It still doesn't look worth the trouble to be smarter, if there are no other bugs to fix. Added a regression test to catch future problems in this area. --- src/backend/catalog/heap.c | 49 +++++++++++++++---- src/backend/optimizer/plan/createplan.c | 8 +++- src/backend/optimizer/prep/preptlist.c | 58 +++++++++++++++++------ src/backend/optimizer/util/plancat.c | 20 ++++++-- src/test/regress/expected/alter_table.out | 41 ++++++++++++++++ src/test/regress/sql/alter_table.sql | 18 +++++++ 6 files changed, 167 insertions(+), 27 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ea70765d432..4c9c98e3856 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.243 2003/05/08 22:19:56 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.244 2003/05/12 00:17:02 tgl Exp $ * * * INTERFACE ROUTINES @@ -76,7 +76,7 @@ static void StoreAttrDefault(Relation rel, AttrNumber attnum, char *adbin); static void StoreRelCheck(Relation rel, char *ccname, char *ccbin); static void StoreConstraints(Relation rel, TupleDesc tupdesc); static void SetRelationNumChecks(Relation rel, int numchecks); -static void RemoveStatistics(Relation rel); +static void RemoveStatistics(Relation rel, AttrNumber attnum); /* ---------------------------------------------------------------- @@ -925,8 +925,9 @@ DeleteAttributeTuples(Oid relid) * RemoveAttributeById * * This is the guts of ALTER TABLE DROP COLUMN: actually mark the attribute - * deleted in pg_attribute. (Everything else needed, such as getting rid - * of any pg_attrdef entry, is handled by dependency.c.) + * deleted in pg_attribute. We also remove pg_statistic entries for it. + * (Everything else needed, such as getting rid of any pg_attrdef entry, + * is handled by dependency.c.) */ void RemoveAttributeById(Oid relid, AttrNumber attnum) @@ -960,6 +961,16 @@ RemoveAttributeById(Oid relid, AttrNumber attnum) /* Mark the attribute as dropped */ attStruct->attisdropped = true; + /* + * Set the type OID to invalid. A dropped attribute's type link cannot + * be relied on (once the attribute is dropped, the type might be too). + * Fortunately we do not need the type row --- the only really essential + * information is the type's typlen and typalign, which are preserved in + * the attribute's attlen and attalign. We set atttypid to zero here + * as a means of catching code that incorrectly expects it to be valid. + */ + attStruct->atttypid = InvalidOid; + /* Remove any NOT NULL constraint the column may have */ attStruct->attnotnull = false; @@ -984,6 +995,8 @@ RemoveAttributeById(Oid relid, AttrNumber attnum) heap_close(attr_rel, RowExclusiveLock); + RemoveStatistics(rel, attnum); + relation_close(rel, NoLock); } @@ -1158,7 +1171,7 @@ heap_drop_with_catalog(Oid rid) /* * delete statistics */ - RemoveStatistics(rel); + RemoveStatistics(rel, 0); /* * delete attribute tuples @@ -1819,27 +1832,45 @@ RemoveRelConstraints(Relation rel, const char *constrName, } +/* + * RemoveStatistics --- remove entries in pg_statistic for a rel or column + * + * If attnum is zero, remove all entries for rel; else remove only the one + * for that column. + */ static void -RemoveStatistics(Relation rel) +RemoveStatistics(Relation rel, AttrNumber attnum) { Relation pgstatistic; SysScanDesc scan; - ScanKeyData key; + ScanKeyData key[2]; + int nkeys; HeapTuple tuple; pgstatistic = heap_openr(StatisticRelationName, RowExclusiveLock); - ScanKeyEntryInitialize(&key, 0x0, + ScanKeyEntryInitialize(&key[0], 0x0, Anum_pg_statistic_starelid, F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(rel))); + if (attnum == 0) + nkeys = 1; + else + { + ScanKeyEntryInitialize(&key[1], 0x0, + Anum_pg_statistic_staattnum, F_INT2EQ, + Int16GetDatum(attnum)); + nkeys = 2; + } + scan = systable_beginscan(pgstatistic, StatisticRelidAttnumIndex, true, - SnapshotNow, 1, &key); + SnapshotNow, nkeys, key); while (HeapTupleIsValid(tuple = systable_getnext(scan))) simple_heap_delete(pgstatistic, &tuple->t_self); systable_endscan(scan); + heap_close(pgstatistic, RowExclusiveLock); } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index e9dd994cd21..f50eb792811 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -10,7 +10,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.141 2003/05/11 20:25:50 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.142 2003/05/12 00:17:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -295,6 +295,12 @@ use_physical_tlist(RelOptInfo *rel) */ if (rel->reloptkind != RELOPT_BASEREL) return false; + /* + * Can't do it if relation contains dropped columns. This is detected + * in plancat.c, see notes there. + */ + if (rel->varlist == NIL) + return false; /* * Can't do it if any system columns are requested, either. (This could * possibly be fixed but would take some fragile assumptions in setrefs.c, diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index dc8faa9d837..00132a53cde 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -15,7 +15,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/prep/preptlist.c,v 1.60 2003/02/03 21:15:44 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/prep/preptlist.c,v 1.61 2003/05/12 00:17:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -171,6 +171,15 @@ expand_targetlist(List *tlist, int command_type, * the attribute, so that it gets copied to the new tuple. But * generate a NULL for dropped columns (we want to drop any * old values). + * + * When generating a NULL constant for a dropped column, we label + * it INT4 (any other guaranteed-to-exist datatype would do as + * well). We can't label it with the dropped column's datatype + * since that might not exist anymore. It does not really + * matter what we claim the type is, since NULL is NULL --- its + * representation is datatype-independent. This could perhaps + * confuse code comparing the finished plan to the target + * relation, however. */ Oid atttype = att_tup->atttypid; int32 atttypmod = att_tup->atttypmod; @@ -179,31 +188,52 @@ expand_targetlist(List *tlist, int command_type, switch (command_type) { case CMD_INSERT: - new_expr = (Node *) makeConst(atttype, - att_tup->attlen, - (Datum) 0, - true, /* isnull */ - att_tup->attbyval); if (!att_tup->attisdropped) + { + new_expr = (Node *) makeConst(atttype, + att_tup->attlen, + (Datum) 0, + true, /* isnull */ + att_tup->attbyval); new_expr = coerce_to_domain(new_expr, InvalidOid, atttype, COERCE_IMPLICIT_CAST); + } + else + { + /* Insert NULL for dropped column */ + new_expr = (Node *) makeConst(INT4OID, + sizeof(int32), + (Datum) 0, + true, /* isnull */ + true /* byval */); + /* label resdom with INT4, too */ + atttype = INT4OID; + atttypmod = -1; + } break; case CMD_UPDATE: - /* Insert NULLs for dropped columns */ - if (att_tup->attisdropped) - new_expr = (Node *) makeConst(atttype, - att_tup->attlen, - (Datum) 0, - true, /* isnull */ - att_tup->attbyval); - else + if (!att_tup->attisdropped) + { new_expr = (Node *) makeVar(result_relation, attrno, atttype, atttypmod, 0); + } + else + { + /* Insert NULL for dropped column */ + new_expr = (Node *) makeConst(INT4OID, + sizeof(int32), + (Datum) 0, + true, /* isnull */ + true /* byval */); + /* label resdom with INT4, too */ + atttype = INT4OID; + atttypmod = -1; + } break; default: elog(ERROR, "expand_targetlist: unexpected command_type"); diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index c0c4775da85..9ec638a5a43 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/util/plancat.c,v 1.81 2003/05/11 20:25:50 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/util/plancat.c,v 1.82 2003/05/12 00:17:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -62,8 +62,15 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel) relation = heap_open(relationObjectId, AccessShareLock); /* - * Make list of physical Vars. Note we do NOT ignore dropped columns; - * the intent is to model the physical tuples of the relation. + * Make list of physical Vars. But if there are any dropped columns, + * punt and set varlist to NIL. (XXX Ideally we would like to include + * dropped columns so that the varlist models the physical tuples + * of the relation. However this creates problems for ExecTypeFromTL, + * which may be asked to build a tupdesc for a tlist that includes vars + * of no-longer-existent types. In theory we could dig out the required + * info from the pg_attribute entries of the relation, but that data is + * not readily available to ExecTypeFromTL. For now, punt and don't + * apply the physical-tlist optimization when there are dropped cols.) */ numattrs = RelationGetNumberOfAttributes(relation); @@ -71,6 +78,13 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel) { Form_pg_attribute att_tup = relation->rd_att->attrs[attrno - 1]; + if (att_tup->attisdropped) + { + /* found a dropped col, so punt */ + varlist = NIL; + break; + } + varlist = lappend(varlist, makeVar(varno, attrno, diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 26c8c2ed423..ed6a17e9d48 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1253,3 +1253,44 @@ select * from p1; drop table p1 cascade; NOTICE: Drop cascades to table c1 NOTICE: Drop cascades to constraint p1_a1 on table c1 +-- test that operations with a dropped column do not try to reference +-- its datatype +create domain mytype as text; +create temp table foo (f1 text, f2 mytype, f3 text); +insert into foo values('aa','bb','cc'); +select * from foo; + f1 | f2 | f3 +----+----+---- + aa | bb | cc +(1 row) + +drop domain mytype cascade; +NOTICE: Drop cascades to table foo column f2 +select * from foo; + f1 | f3 +----+---- + aa | cc +(1 row) + +insert into foo values('qq','rr'); +select * from foo; + f1 | f3 +----+---- + aa | cc + qq | rr +(2 rows) + +update foo set f3 = 'zz'; +select * from foo; + f1 | f3 +----+---- + aa | zz + qq | zz +(2 rows) + +select f3,max(f1) from foo group by f3; + f3 | max +----+----- + zz | qq +(1 row) + diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 9dec23499c3..c97b72cabef 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -895,3 +895,21 @@ update p1 set a1 = a1 + 1, f2 = upper(f2); select * from p1; drop table p1 cascade; + +-- test that operations with a dropped column do not try to reference +-- its datatype + +create domain mytype as text; +create temp table foo (f1 text, f2 mytype, f3 text); + +insert into foo values('aa','bb','cc'); +select * from foo; + +drop domain mytype cascade; + +select * from foo; +insert into foo values('qq','rr'); +select * from foo; +update foo set f3 = 'zz'; +select * from foo; +select f3,max(f1) from foo group by f3;