From b5b0db19b895f033ada35bc7c337183be7356977 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 14 May 2017 12:22:16 -0400 Subject: [PATCH] Fix handling of extended statistics during ALTER COLUMN TYPE. ALTER COLUMN TYPE on a column used by a statistics object fails since commit 928c4de30, because the relevant switch in ATExecAlterColumnType is unprepared for columns to have dependencies from OCLASS_STATISTIC_EXT objects. Although the existing types of extended statistics don't actually need us to do any work for a column type change, it seems completely indefensible that that assumption is hidden behind the failure of an unrelated module to contain any code for the case. Hence, create and call an API function in statscmds.c where the assumption can be explained, and where we could add code to deal with the problem when it inevitably becomes real. Also, the reason this wasn't handled before, neither for extended stats nor for the last half-dozen new OCLASS kinds :-(, is that the default: in that switch suppresses compiler warnings, allowing people to miss the need to consider it when adding an OCLASS. We don't really need a default because surely getObjectClass should only return valid values of the enum; so remove it, and add the missed OCLASS entries where they should be. Discussion: https://postgr.es/m/20170512221010.nglatgt5azzdxjlj@alvherre.pgsql --- src/backend/commands/statscmds.c | 28 +++++++++++++++++++++++++ src/backend/commands/tablecmds.c | 24 ++++++++++++++++++--- src/include/commands/defrem.h | 3 +++ src/test/regress/expected/stats_ext.out | 25 ++++++++++++++++++++++ src/test/regress/sql/stats_ext.sql | 11 ++++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c index cab54c962c..94865b395b 100644 --- a/src/backend/commands/statscmds.c +++ b/src/backend/commands/statscmds.c @@ -378,3 +378,31 @@ RemoveStatisticsById(Oid statsOid) heap_close(relation, RowExclusiveLock); } + +/* + * Update a statistics object for ALTER COLUMN TYPE on a source column. + * + * This could throw an error if the type change can't be supported. + * If it can be supported, but the stats must be recomputed, a likely choice + * would be to set the relevant column(s) of the pg_statistic_ext tuple to + * null until the next ANALYZE. (Note that the type change hasn't actually + * happened yet, so one option that's *not* on the table is to recompute + * immediately.) + */ +void +UpdateStatisticsForTypeChange(Oid statsOid, Oid relationOid, int attnum, + Oid oldColumnType, Oid newColumnType) +{ + /* + * Currently, we don't actually need to do anything here. For both + * ndistinct and functional-dependencies stats, the on-disk representation + * is independent of the source column data types, and it is plausible to + * assume that the old statistic values will still be good for the new + * column contents. (Obviously, if the ALTER COLUMN TYPE has a USING + * expression that substantially alters the semantic meaning of the column + * values, this assumption could fail. But that seems like a corner case + * that doesn't justify zapping the stats in common cases.) + * + * Future types of extended stats will likely require us to work harder. + */ +} diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cdcb94929a..b94db89b25 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9236,6 +9236,17 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, Assert(defaultexpr); break; + case OCLASS_STATISTIC_EXT: + + /* + * Give the extended-stats machinery a chance to fix anything + * that this column type change would break. + */ + UpdateStatisticsForTypeChange(foundObject.objectId, + RelationGetRelid(rel), attnum, + attTup->atttypid, targettype); + break; + case OCLASS_PROC: case OCLASS_TYPE: case OCLASS_CAST: @@ -9246,6 +9257,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, case OCLASS_OPERATOR: case OCLASS_OPCLASS: case OCLASS_OPFAMILY: + case OCLASS_AM: case OCLASS_AMOP: case OCLASS_AMPROC: case OCLASS_SCHEMA: @@ -9261,6 +9273,11 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, case OCLASS_USER_MAPPING: case OCLASS_DEFACL: case OCLASS_EXTENSION: + case OCLASS_EVENT_TRIGGER: + case OCLASS_PUBLICATION: + case OCLASS_PUBLICATION_REL: + case OCLASS_SUBSCRIPTION: + case OCLASS_TRANSFORM: /* * We don't expect any of these sorts of objects to depend on @@ -9270,9 +9287,10 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, getObjectDescription(&foundObject)); break; - default: - elog(ERROR, "unrecognized object class: %u", - foundObject.classId); + /* + * There's intentionally no default: case here; we want the + * compiler to warn if a new OCLASS hasn't been handled above. + */ } } diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index c323e81e6c..79f3be36e4 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -80,6 +80,9 @@ extern ObjectAddress AlterOperator(AlterOperatorStmt *stmt); /* commands/statscmds.c */ extern ObjectAddress CreateStatistics(CreateStatsStmt *stmt); extern void RemoveStatisticsById(Oid statsOid); +extern void UpdateStatisticsForTypeChange(Oid statsOid, + Oid relationOid, int attnum, + Oid oldColumnType, Oid newColumnType); /* commands/aggregatecmds.c */ extern ObjectAddress DefineAggregate(ParseState *pstate, List *name, List *args, bool oldstyle, diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out index 564521957f..5fd1244bcb 100644 --- a/src/test/regress/expected/stats_ext.out +++ b/src/test/regress/expected/stats_ext.out @@ -479,4 +479,29 @@ EXPLAIN (COSTS OFF) Index Cond: ((a = 1) AND (b = '1'::text)) (5 rows) +-- check change of column type doesn't break it +ALTER TABLE functional_dependencies ALTER COLUMN c TYPE numeric; +EXPLAIN (COSTS OFF) + SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1' AND c = 1; + QUERY PLAN +--------------------------------------------------- + Bitmap Heap Scan on functional_dependencies + Recheck Cond: ((a = 1) AND (b = '1'::text)) + Filter: (c = '1'::numeric) + -> Bitmap Index Scan on fdeps_ab_idx + Index Cond: ((a = 1) AND (b = '1'::text)) +(5 rows) + +ANALYZE functional_dependencies; +EXPLAIN (COSTS OFF) + SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1' AND c = 1; + QUERY PLAN +--------------------------------------------------- + Bitmap Heap Scan on functional_dependencies + Recheck Cond: ((a = 1) AND (b = '1'::text)) + Filter: (c = '1'::numeric) + -> Bitmap Index Scan on fdeps_ab_idx + Index Cond: ((a = 1) AND (b = '1'::text)) +(5 rows) + RESET random_page_cost; diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql index a9dda051c2..4c2f514b93 100644 --- a/src/test/regress/sql/stats_ext.sql +++ b/src/test/regress/sql/stats_ext.sql @@ -266,6 +266,17 @@ ANALYZE functional_dependencies; EXPLAIN (COSTS OFF) SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1'; +EXPLAIN (COSTS OFF) + SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1' AND c = 1; + +-- check change of column type doesn't break it +ALTER TABLE functional_dependencies ALTER COLUMN c TYPE numeric; + +EXPLAIN (COSTS OFF) + SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1' AND c = 1; + +ANALYZE functional_dependencies; + EXPLAIN (COSTS OFF) SELECT * FROM functional_dependencies WHERE a = 1 AND b = '1' AND c = 1; -- 2.40.0