From a4e8680a6c337955c021177457147f4b4d9a5df5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 24 Oct 2012 13:39:37 -0400 Subject: [PATCH] When converting a table to a view, remove its system columns. Views should not have any pg_attribute entries for system columns. However, we forgot to remove such entries when converting a table to a view. This could lead to crashes later on, if someone attempted to reference such a column, as reported by Kohei KaiGai. Patch in HEAD only. This bug has been there forever, but in the back branches we will have to defend against existing mis-converted views, so it doesn't seem worthwhile to change the conversion code too. --- src/backend/catalog/heap.c | 41 +++++++++++++++++++++++++++++ src/backend/rewrite/rewriteDefine.c | 11 ++++++-- src/include/catalog/heap.h | 1 + src/test/regress/expected/rules.out | 22 ++++++++++++++++ src/test/regress/sql/rules.sql | 15 +++++++++++ 5 files changed, 88 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 6edd11fb98..8818b680c8 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -1434,6 +1434,47 @@ DeleteAttributeTuples(Oid relid) heap_close(attrel, RowExclusiveLock); } +/* + * DeleteSystemAttributeTuples + * + * Remove pg_attribute rows for system columns of the given relid. + * + * Note: this is only used when converting a table to a view. Views don't + * have system columns, so we should remove them from pg_attribute. + */ +void +DeleteSystemAttributeTuples(Oid relid) +{ + Relation attrel; + SysScanDesc scan; + ScanKeyData key[2]; + HeapTuple atttup; + + /* Grab an appropriate lock on the pg_attribute relation */ + attrel = heap_open(AttributeRelationId, RowExclusiveLock); + + /* Use the index to scan only system attributes of the target relation */ + ScanKeyInit(&key[0], + Anum_pg_attribute_attrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(relid)); + ScanKeyInit(&key[1], + Anum_pg_attribute_attnum, + BTLessEqualStrategyNumber, F_INT2LE, + Int16GetDatum(0)); + + scan = systable_beginscan(attrel, AttributeRelidNumIndexId, true, + SnapshotNow, 2, key); + + /* Delete all the matching tuples */ + while ((atttup = systable_getnext(scan)) != NULL) + simple_heap_delete(attrel, &atttup->t_self); + + /* Clean up after the scan */ + systable_endscan(scan); + heap_close(attrel, RowExclusiveLock); +} + /* * RemoveAttributeById * diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c index 8efc9fc9d5..55b0fed5f7 100644 --- a/src/backend/rewrite/rewriteDefine.c +++ b/src/backend/rewrite/rewriteDefine.c @@ -18,6 +18,7 @@ #include "access/htup_details.h" #include "catalog/catalog.h" #include "catalog/dependency.h" +#include "catalog/heap.h" #include "catalog/indexing.h" #include "catalog/namespace.h" #include "catalog/objectaccess.h" @@ -510,13 +511,19 @@ DefineQueryRewrite(char *rulename, } /* - * IF the relation is becoming a view, delete the storage files associated - * with it. NB: we had better have AccessExclusiveLock to do this ... + * If the relation is becoming a view, delete the storage files associated + * with it. Also, get rid of any system attribute entries in pg_attribute, + * because a view shouldn't have any of those. + * + * NB: we had better have AccessExclusiveLock to do this ... * * XXX what about getting rid of its TOAST table? For now, we don't. */ if (RelisBecomingView) + { RelationDropStorage(event_relation); + DeleteSystemAttributeTuples(event_relid); + } /* Close rel, but keep lock till commit... */ heap_close(event_relation, NoLock); diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index 1465456cc7..a35829bb7b 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -107,6 +107,7 @@ extern Node *cookDefault(ParseState *pstate, extern void DeleteRelationTuple(Oid relid); extern void DeleteAttributeTuples(Oid relid); +extern void DeleteSystemAttributeTuples(Oid relid); extern void RemoveAttributeById(Oid relid, AttrNumber attnum); extern void RemoveAttrDefault(Oid relid, AttrNumber attnum, DropBehavior behavior, bool complain, bool internal); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index ad1591be59..a235571b3d 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1457,6 +1457,28 @@ ERROR: cannot drop rule _RETURN on view fooview because view fooview requires i HINT: You can drop view fooview instead. drop view fooview; -- +-- test conversion of table to view (needed to load some pg_dump files) +-- +create table fooview (x int, y text); +select xmin, * from fooview; + xmin | x | y +------+---+--- +(0 rows) + +create rule "_RETURN" as on select to fooview do instead + select 1 as x, 'aaa'::text as y; +select * from fooview; + x | y +---+----- + 1 | aaa +(1 row) + +select xmin, * from fooview; -- fail, views don't have such a column +ERROR: column "xmin" does not exist +LINE 1: select xmin, * from fooview; + ^ +drop view fooview; +-- -- check for planner problems with complex inherited UPDATES -- create table id (id serial primary key, name text); diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 1de5b0b685..458c2f026c 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -859,6 +859,21 @@ create view fooview as select 'foo'::text; drop rule "_RETURN" on fooview; drop view fooview; +-- +-- test conversion of table to view (needed to load some pg_dump files) +-- + +create table fooview (x int, y text); +select xmin, * from fooview; + +create rule "_RETURN" as on select to fooview do instead + select 1 as x, 'aaa'::text as y; + +select * from fooview; +select xmin, * from fooview; -- fail, views don't have such a column + +drop view fooview; + -- -- check for planner problems with complex inherited UPDATES --