From 928c4de30991ca24a46a92f006892c039af30833 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 12 May 2017 16:26:31 -0400 Subject: [PATCH] Fix dependencies for extended statistics objects. 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 --- src/backend/catalog/heap.c | 74 ------------------------- src/backend/commands/statscmds.c | 52 +++++++++-------- src/include/catalog/catversion.h | 2 +- src/include/catalog/heap.h | 1 - src/test/regress/expected/stats_ext.out | 14 ++++- src/test/regress/sql/stats_ext.sql | 5 +- 6 files changed, 48 insertions(+), 100 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ab3d83f29b..0f1547b567 100644 --- 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 index 662b4fa15d..db5f4278d5 100644 --- 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); + + recordDependencyOnOwner(StatisticExtRelationId, statoid, stxowner); + + /* + * 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 */ - ObjectAddressSet(address, StatisticExtRelationId, statoid); - - return address; + return myself; } /* diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index e91479e089..85ee2f6bde 100644 --- 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 index 473fe177ba..1187797fd9 100644 --- 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 index 4ccdf21a01..13780897c5 100644 --- 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 index 4050f33c08..09b7d8e102 100644 --- 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);