Fix dependencies for extended statistics objects.

tglsfdc · tglsfdc · commit 928c4de30991 · 2017-05-12T16:26:31.000-04:00
A stats object ought to have a dependency on each individual column it reads, not the entire table. Doing this honestly lets us get rid of the hard-wired logic in RemoveStatisticsExt, which seems to have been misguidedly modeled on RemoveStatistics; and it will be far easier to extend to multiple tables later. Also, add overlooked dependency on owner, and make the dependency on schema be NORMAL like every other such dependency. There remains some unfinished work here, which is to allow statistics objects to be extension members. That takes more effort than just adding the dependency call, though, so I left it out for now. initdb forced because this changes the set of pg_depend records that should exist for a statistics object. Discussion: https://postgr.es/m/22676.1494557205@sss.pgh.pa.us
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
@@ -52,7 +52,6 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_partitioned_table.h"
 #include "catalog/pg_statistic.h"
-#include "catalog/pg_statistic_ext.h"
 #include "catalog/pg_subscription_rel.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_type.h"
@@ -1615,10 +1614,7 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	heap_close(attr_rel, RowExclusiveLock);
 
 	if (attnum > 0)
-	{
 		RemoveStatistics(relid, attnum);
-		RemoveStatisticsExt(relid, attnum);
-	}
 
 	relation_close(rel, NoLock);
 }
@@ -1873,7 +1869,6 @@ heap_drop_with_catalog(Oid relid)
 	 * delete statistics
 	 */
 	RemoveStatistics(relid, 0);
-	RemoveStatisticsExt(relid, 0);
 
 	/*
 	 * delete attribute tuples
@@ -2785,75 +2780,6 @@ RemoveStatistics(Oid relid, AttrNumber attnum)
 }
 
 
-/*
- * RemoveStatisticsExt --- remove entries in pg_statistic_ext for a relation
- *
- * If attnum is zero, remove all entries for rel; else remove only the
- * one(s) involving that column.
- */
-void
-RemoveStatisticsExt(Oid relid, AttrNumber attnum)
-{
-	Relation	pgstatisticext;
-	SysScanDesc scan;
-	ScanKeyData key;
-	HeapTuple	tuple;
-
-	/*
-	 * Scan pg_statistic_ext to delete relevant tuples
-	 */
-	pgstatisticext = heap_open(StatisticExtRelationId, RowExclusiveLock);
-
-	ScanKeyInit(&key,
-				Anum_pg_statistic_ext_stxrelid,
-				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(relid));
-
-	scan = systable_beginscan(pgstatisticext,
-							  StatisticExtRelidIndexId,
-							  true, NULL, 1, &key);
-
-	while (HeapTupleIsValid(tuple = systable_getnext(scan)))
-	{
-		bool		delete = false;
-
-		if (attnum == 0)
-			delete = true;
-		else if (attnum != 0)
-		{
-			Form_pg_statistic_ext	staForm;
-			int			i;
-
-			/*
-			 * Decode the stxkeys array and delete any stats that involve the
-			 * specified column.
-			 */
-			staForm = (Form_pg_statistic_ext) GETSTRUCT(tuple);
-			for (i = 0; i < staForm->stxkeys.dim1; i++)
-			{
-				if (staForm->stxkeys.values[i] == attnum)
-				{
-					delete = true;
-					break;
-				}
-			}
-		}
-
-		if (delete)
-		{
-			CatalogTupleDelete(pgstatisticext, &tuple->t_self);
-			deleteDependencyRecordsFor(StatisticExtRelationId,
-									   HeapTupleGetOid(tuple),
-									   false);
-		}
-	}
-
-	systable_endscan(scan);
-
-	heap_close(pgstatisticext, RowExclusiveLock);
-}
-
-
 /*
  * RelationTruncateIndexes - truncate all indexes associated
  * with the heap relation to zero tuples.
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
@@ -50,11 +50,11 @@ CreateStatistics(CreateStatsStmt *stmt)
 {
 	int16		attnums[STATS_MAX_DIMENSIONS];
 	int			numcols = 0;
-	ObjectAddress address = InvalidObjectAddress;
 	char	   *namestr;
 	NameData	stxname;
 	Oid			statoid;
 	Oid			namespaceId;
+	Oid			stxowner = GetUserId();
 	HeapTuple	htup;
 	Datum		values[Natts_pg_statistic_ext];
 	bool		nulls[Natts_pg_statistic_ext];
@@ -63,7 +63,7 @@ CreateStatistics(CreateStatsStmt *stmt)
 	Relation	rel = NULL;
 	Oid			relid;
 	ObjectAddress parentobject,
-				childobject;
+				myself;
 	Datum		types[2];		/* one for each possible type of statistics */
 	int			ntypes;
 	ArrayType  *stxkind;
@@ -140,7 +140,7 @@ CreateStatistics(CreateStatsStmt *stmt)
 							RelationGetRelationName(rel))));
 
 		/* You must own the relation to create stats on it */
-		if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
+		if (!pg_class_ownercheck(RelationGetRelid(rel), stxowner))
 			aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
 						   RelationGetRelationName(rel));
 	}
@@ -185,7 +185,7 @@ CreateStatistics(CreateStatsStmt *stmt)
 		attForm = (Form_pg_attribute) GETSTRUCT(atttuple);
 
 		/* Disallow use of system attributes in extended stats */
-		if (attForm->attnum < 0)
+		if (attForm->attnum <= 0)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 errmsg("statistics creation on system columns is not supported")));
@@ -205,7 +205,7 @@ CreateStatistics(CreateStatsStmt *stmt)
 					 errmsg("cannot have more than %d columns in statistics",
 							STATS_MAX_DIMENSIONS)));
 
-		attnums[numcols] = ((Form_pg_attribute) GETSTRUCT(atttuple))->attnum;
+		attnums[numcols] = attForm->attnum;
 		numcols++;
 		ReleaseSysCache(atttuple);
 	}
@@ -231,10 +231,12 @@ CreateStatistics(CreateStatsStmt *stmt)
 	 * just check consecutive elements.
 	 */
 	for (i = 1; i < numcols; i++)
+	{
 		if (attnums[i] == attnums[i - 1])
 			ereport(ERROR,
 					(errcode(ERRCODE_DUPLICATE_COLUMN),
 				  errmsg("duplicate column name in statistics definition")));
+	}
 
 	/* Form an int2vector representation of the sorted column list */
 	stxkeys = buildint2vector(attnums, numcols);
@@ -288,7 +290,7 @@ CreateStatistics(CreateStatsStmt *stmt)
 	values[Anum_pg_statistic_ext_stxrelid - 1] = ObjectIdGetDatum(relid);
 	values[Anum_pg_statistic_ext_stxname - 1] = NameGetDatum(&stxname);
 	values[Anum_pg_statistic_ext_stxnamespace - 1] = ObjectIdGetDatum(namespaceId);
-	values[Anum_pg_statistic_ext_stxowner - 1] = ObjectIdGetDatum(GetUserId());
+	values[Anum_pg_statistic_ext_stxowner - 1] = ObjectIdGetDatum(stxowner);
 	values[Anum_pg_statistic_ext_stxkeys - 1] = PointerGetDatum(stxkeys);
 	values[Anum_pg_statistic_ext_stxkind - 1] = PointerGetDatum(stxkind);
 
@@ -312,29 +314,35 @@ CreateStatistics(CreateStatsStmt *stmt)
 	relation_close(rel, NoLock);
 
 	/*
-	 * Add a dependency on the table, so that stats get dropped on DROP TABLE.
-	 *
-	 * XXX don't we need dependencies on the specific columns, instead?
+	 * Add an AUTO dependency on each column used in the stats, so that the
+	 * stats object goes away if any or all of them get dropped.
 	 */
-	ObjectAddressSet(parentobject, RelationRelationId, relid);
-	ObjectAddressSet(childobject, StatisticExtRelationId, statoid);
-	recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+	ObjectAddressSet(myself, StatisticExtRelationId, statoid);
+
+	for (i = 0; i < numcols; i++)
+	{
+		ObjectAddressSubSet(parentobject, RelationRelationId, relid, attnums[i]);
+		recordDependencyOn(&myself, &parentobject, DEPENDENCY_AUTO);
+	}
 
 	/*
-	 * Also add dependency on the schema.  This is required to ensure that we
-	 * drop the statistics on DROP SCHEMA.  This is not handled automatically
-	 * by DROP TABLE because the statistics might be in a different schema
-	 * from the table itself.  (This definition is a bit bizarre for the
-	 * single-table case, but it will make more sense if/when we support
-	 * extended stats across multiple tables.)
+	 * Also add dependencies on namespace and owner.  These are required
+	 * because the stats object might have a different namespace and/or owner
+	 * than the underlying table(s).
 	 */
 	ObjectAddressSet(parentobject, NamespaceRelationId, namespaceId);
-	recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+	recordDependencyOn(&myself, &parentobject, DEPENDENCY_NORMAL);
 
-	/* Return stats object's address */
-	ObjectAddressSet(address, StatisticExtRelationId, statoid);
+	recordDependencyOnOwner(StatisticExtRelationId, statoid, stxowner);
 
-	return address;
+	/*
+	 * XXX probably there should be a recordDependencyOnCurrentExtension call
+	 * here too, but we'd have to add support for ALTER EXTENSION ADD/DROP
+	 * STATISTICS, which is more work than it seems worth.
+	 */
+
+	/* Return stats object's address */
+	return myself;
 }
 
 /*
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	201705111
+#define CATALOG_VERSION_NO	201705121
 
 #endif
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
@@ -119,7 +119,6 @@ extern void RemoveAttrDefault(Oid relid, AttrNumber attnum,
 				  DropBehavior behavior, bool complain, bool internal);
 extern void RemoveAttrDefaultById(Oid attrdefId);
 extern void RemoveStatistics(Oid relid, AttrNumber attnum);
-extern void RemoveStatisticsExt(Oid relid, AttrNumber attnum);
 
 extern Form_pg_attribute SystemAttributeDefinition(AttrNumber attno,
 						  bool relhasoids);
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
@@ -47,7 +47,7 @@ DROP STATISTICS regress_schema_2.ab1_a_b_stats;
 -- Ensure statistics are dropped when columns are
 CREATE STATISTICS ab1_b_c_stats ON b, c FROM ab1;
 CREATE STATISTICS ab1_a_b_c_stats ON a, b, c FROM ab1;
-CREATE STATISTICS ab1_a_b_stats ON a, b FROM ab1;
+CREATE STATISTICS ab1_b_a_stats ON b, a FROM ab1;
 ALTER TABLE ab1 DROP COLUMN a;
 \d ab1
                 Table "public.ab1"
@@ -58,7 +58,19 @@ ALTER TABLE ab1 DROP COLUMN a;
 Statistics:
     "public"."ab1_b_c_stats" (ndistinct, dependencies) ON b, c FROM ab1
 
+-- Ensure statistics are dropped when table is
+SELECT stxname FROM pg_statistic_ext WHERE stxname LIKE 'ab1%';
+    stxname    
+---------------
+ ab1_b_c_stats
+(1 row)
+
 DROP TABLE ab1;
+SELECT stxname FROM pg_statistic_ext WHERE stxname LIKE 'ab1%';
+ stxname 
+---------
+(0 rows)
+
 -- Ensure things work sanely with SET STATISTICS 0
 CREATE TABLE ab1 (a INTEGER, b INTEGER);
 ALTER TABLE ab1 ALTER a SET STATISTICS 0;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
@@ -34,10 +34,13 @@ DROP STATISTICS regress_schema_2.ab1_a_b_stats;
 -- Ensure statistics are dropped when columns are
 CREATE STATISTICS ab1_b_c_stats ON b, c FROM ab1;
 CREATE STATISTICS ab1_a_b_c_stats ON a, b, c FROM ab1;
-CREATE STATISTICS ab1_a_b_stats ON a, b FROM ab1;
+CREATE STATISTICS ab1_b_a_stats ON b, a FROM ab1;
 ALTER TABLE ab1 DROP COLUMN a;
 \d ab1
+-- Ensure statistics are dropped when table is
+SELECT stxname FROM pg_statistic_ext WHERE stxname LIKE 'ab1%';
 DROP TABLE ab1;
+SELECT stxname FROM pg_statistic_ext WHERE stxname LIKE 'ab1%';
 
 -- Ensure things work sanely with SET STATISTICS 0
 CREATE TABLE ab1 (a INTEGER, b INTEGER);
