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.

5 files changed, 88 insertions, 2 deletions

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 6edd11fb986..8818b680c8b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1435,6 +1435,47 @@ DeleteAttributeTuples(Oid relid)
 }
 
 /*
+ *		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
  *
  * This is the guts of ALTER TABLE DROP COLUMN: actually mark the attribute
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 8efc9fc9d52..55b0fed5f79 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 1465456cc7d..a35829bb7b9 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 ad1591be598..a235571b3df 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 1de5b0b6853..458c2f026c0 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -860,6 +860,21 @@ 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
 --