Tom Lane wrote:
> Although I've not done anything about it here, I'm not happy about the
> handling of dependencies for stats objects. I do not think that cloning
> RemoveStatistics as RemoveStatisticsExt is sane at all. The former is
> meant to deal with cleaning up pg_statistic rows that we know will be
> there, so there's no real need to expend pg_depend overhead to track them.
> For objects that are only loosely connected, the right thing is to use
> the dependency system; in particular, this design has no real chance of
> working well with cross-table stats. Also, it's really silly to have
> *both* this hard-wired mechanism and a pg_depend entry; the latter is
> surely redundant if you have the former. IMO we should revert
> RemoveStatisticsExt and instead handle things by making stats objects
> auto-dependent on the individual column(s) they reference (not the whole
> table).
>
> I'm also of the opinion that having an AUTO dependency, rather than
> a NORMAL dependency, on the stats object's schema is the wrong semantics.
> There isn't any other case where you can drop a non-empty schema without
> saying CASCADE, and I'm mystified why this case should act that way.
Here are two patches regarding handling of dependencies. The first one
implements your first suggestion: add a NORMAL dependency on each
column, and do away with RemoveStatisticsExt. This works well and
should uncontroversial.
If we only do this, then DROP TABLE needs to say CASCADE if there's a
statistics object in the table. This seems pointless to me, so the
second patch throws in an additional dependency on the table as a whole,
AUTO this time, so that if the table is dropped, the statistics goes
away without requiring cascade. There is no point in forcing CASCADE
for this case, ISTM. This works well too, but I admit there might be
resistance to doing it. OTOH this is how CREATE INDEX works.
(I considered what would happen with a stats object covering multiple
tables. I think it's reasonable to drop the stats too in that case,
under the rationale that the object is no longer useful. Not really
sure about this.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 06fad759888d5060f9b4cf4d4f4fdec777350027 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 12 May 2017 17:16:26 -0300
Subject: [PATCH 1/2] fix dependencies problem
---
src/backend/catalog/heap.c | 74 ---------------------------------
src/backend/catalog/objectaddress.c | 20 +++++++++
src/backend/commands/statscmds.c | 18 ++++----
src/test/regress/expected/stats_ext.out | 13 ++++--
src/test/regress/sql/stats_ext.sql | 9 ++--
5 files changed, 45 insertions(+), 89 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ab3d83f29b..ba63939022 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1615,10 +1615,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 +1870,6 @@ heap_drop_with_catalog(Oid relid)
* delete statistics
*/
RemoveStatistics(relid, 0);
- RemoveStatisticsExt(relid, 0);
/*
* delete attribute tuples
@@ -2784,76 +2780,6 @@ RemoveStatistics(Oid relid, AttrNumber attnum)
heap_close(pgstatistic, RowExclusiveLock);
}
-
-/*
- * 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/catalog/objectaddress.c
b/src/backend/catalog/objectaddress.c
index a9e529fba0..9443996dc9 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -3004,6 +3004,26 @@ getObjectDescription(const ObjectAddress *object)
break;
}
+ case OCLASS_STATISTIC_EXT:
+ {
+ HeapTuple stxTup;
+ Form_pg_statistic_ext stxForm;
+
+ stxTup = SearchSysCache1(STATEXTOID,
+
ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(stxTup))
+ elog(ERROR, "could not find tuple for
statistics object %u",
+ object->objectId);
+
+ stxForm = (Form_pg_statistic_ext)
GETSTRUCT(stxTup);
+
+ appendStringInfo(&buffer, _("statistics object
%s"),
+
NameStr(stxForm->stxname));
+
+ ReleaseSysCache(stxTup);
+ break;
+ }
+
case OCLASS_TRANSFORM:
{
HeapTuple trfTup;
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 662b4fa15d..d3b5430369 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -312,24 +312,24 @@ 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 a dependency on the columns of the table, so that stats get
dropped
+ * on DROP TABLE as well as ALTER TABLE .. DROP COLUMN.
*/
- ObjectAddressSet(parentobject, RelationRelationId, relid);
ObjectAddressSet(childobject, StatisticExtRelationId, statoid);
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+ for (i = 0; i < numcols; i++)
+ {
+ ObjectAddressSubSet(parentobject, RelationRelationId, relid,
attnums[i]);
+ recordDependencyOn(&childobject, &parentobject,
DEPENDENCY_NORMAL);
+ }
/*
* 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.)
+ * from the table itself.
*/
ObjectAddressSet(parentobject, NamespaceRelationId, namespaceId);
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+ recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
/* Return stats object's address */
ObjectAddressSet(address, StatisticExtRelationId, statoid);
diff --git a/src/test/regress/expected/stats_ext.out
b/src/test/regress/expected/stats_ext.out
index 4ccdf21a01..f2ae0ab80f 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -48,7 +48,11 @@ DROP STATISTICS regress_schema_2.ab1_a_b_stats;
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;
-ALTER TABLE ab1 DROP COLUMN a;
+\set VERBOSITY terse
+ALTER TABLE ab1 DROP COLUMN a; -- fail
+ERROR: cannot drop table ab1 column a because other objects depend on it
+ALTER TABLE ab1 DROP COLUMN a CASCADE;
+NOTICE: drop cascades to 2 other objects
\d ab1
Table "public.ab1"
Column | Type | Collation | Nullable | Default
@@ -58,7 +62,9 @@ ALTER TABLE ab1 DROP COLUMN a;
Statistics:
"public"."ab1_b_c_stats" (ndistinct, dependencies) ON b, c FROM ab1
-DROP TABLE ab1;
+DROP TABLE ab1 CASCADE;
+NOTICE: drop cascades to statistics object ab1_b_c_stats
+\set VERBOSITY default
-- Ensure things work sanely with SET STATISTICS 0
CREATE TABLE ab1 (a INTEGER, b INTEGER);
ALTER TABLE ab1 ALTER a SET STATISTICS 0;
@@ -71,7 +77,8 @@ ALTER TABLE ab1 ALTER a SET STATISTICS -1;
ANALYZE ab1 (a);
WARNING: extended statistics "public.ab1_a_b_stats" could not be collected
for relation public.ab1
ANALYZE ab1;
-DROP TABLE ab1;
+DROP TABLE ab1 CASCADE;
+NOTICE: drop cascades to statistics object ab1_a_b_stats
-- Verify supported object types for extended statistics
CREATE schema tststats;
CREATE TABLE tststats.t (a int, b int, c text);
diff --git a/src/test/regress/sql/stats_ext.sql
b/src/test/regress/sql/stats_ext.sql
index 4050f33c08..df9f47a3b6 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -35,9 +35,12 @@ DROP STATISTICS regress_schema_2.ab1_a_b_stats;
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;
-ALTER TABLE ab1 DROP COLUMN a;
+\set VERBOSITY terse
+ALTER TABLE ab1 DROP COLUMN a; -- fail
+ALTER TABLE ab1 DROP COLUMN a CASCADE;
\d ab1
-DROP TABLE ab1;
+DROP TABLE ab1 CASCADE;
+\set VERBOSITY default
-- Ensure things work sanely with SET STATISTICS 0
CREATE TABLE ab1 (a INTEGER, b INTEGER);
@@ -49,7 +52,7 @@ ALTER TABLE ab1 ALTER a SET STATISTICS -1;
-- partial analyze doesn't build stats either
ANALYZE ab1 (a);
ANALYZE ab1;
-DROP TABLE ab1;
+DROP TABLE ab1 CASCADE;
-- Verify supported object types for extended statistics
CREATE schema tststats;
--
2.11.0
>From 72b7c8402467b643785f7a3d3b4eeb7f575e3091 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 12 May 2017 17:35:49 -0300
Subject: [PATCH 2/2] add AUTO dependency on table
---
src/backend/commands/statscmds.c | 7 +++++++
src/test/regress/expected/stats_ext.out | 6 ++----
src/test/regress/sql/stats_ext.sql | 4 ++--
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index d3b5430369..46ccaeabac 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -323,6 +323,13 @@ CreateStatistics(CreateStatsStmt *stmt)
}
/*
+ * Also add an AUTO dependency on the table itself. If the table is
gone,
+ * we don't require a CASCADE in order to also drop the statistics
object.
+ */
+ ObjectAddressSet(parentobject, RelationRelationId, relid);
+ recordDependencyOn(&childobject, &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
diff --git a/src/test/regress/expected/stats_ext.out
b/src/test/regress/expected/stats_ext.out
index f2ae0ab80f..242a6bfff5 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -62,8 +62,7 @@ NOTICE: drop cascades to 2 other objects
Statistics:
"public"."ab1_b_c_stats" (ndistinct, dependencies) ON b, c FROM ab1
-DROP TABLE ab1 CASCADE;
-NOTICE: drop cascades to statistics object ab1_b_c_stats
+DROP TABLE ab1;
\set VERBOSITY default
-- Ensure things work sanely with SET STATISTICS 0
CREATE TABLE ab1 (a INTEGER, b INTEGER);
@@ -77,8 +76,7 @@ ALTER TABLE ab1 ALTER a SET STATISTICS -1;
ANALYZE ab1 (a);
WARNING: extended statistics "public.ab1_a_b_stats" could not be collected
for relation public.ab1
ANALYZE ab1;
-DROP TABLE ab1 CASCADE;
-NOTICE: drop cascades to statistics object ab1_a_b_stats
+DROP TABLE ab1;
-- Verify supported object types for extended statistics
CREATE schema tststats;
CREATE TABLE tststats.t (a int, b int, c text);
diff --git a/src/test/regress/sql/stats_ext.sql
b/src/test/regress/sql/stats_ext.sql
index df9f47a3b6..fd738dc9fe 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -39,7 +39,7 @@ CREATE STATISTICS ab1_a_b_stats ON a, b FROM ab1;
ALTER TABLE ab1 DROP COLUMN a; -- fail
ALTER TABLE ab1 DROP COLUMN a CASCADE;
\d ab1
-DROP TABLE ab1 CASCADE;
+DROP TABLE ab1;
\set VERBOSITY default
-- Ensure things work sanely with SET STATISTICS 0
@@ -52,7 +52,7 @@ ALTER TABLE ab1 ALTER a SET STATISTICS -1;
-- partial analyze doesn't build stats either
ANALYZE ab1 (a);
ANALYZE ab1;
-DROP TABLE ab1 CASCADE;
+DROP TABLE ab1;
-- Verify supported object types for extended statistics
CREATE schema tststats;
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
