From ad16b462761eb56f47840237bfb21f9fa5606afa Mon Sep 17 00:00:00 2001 From: jian he Date: Sun, 8 Sep 2024 00:23:06 +0800 Subject: [PATCH v28 1/1] minor refactor based on 28j 1. make sure these three functions: 'pg_set_relation_stats', 'pg_restore_relation_stats','pg_clear_relation_stats' proisstrict to true. because in pg_class catalog, these three attributes (relpages, reltuples, relallvisible) is marked as not null. updating it to null will violate these constraints. 2. refactor relation_statistics_update. first sanity check first argument ("relation"). not all kinds of relation can pass on relation_statistics_update, so do the sanity check. also do sanity check for the remaining 3 arguments. if not ok, bail it, return false immediately. 3. add some tests for partitioned table, view, and materialized view. 4. minor sanity check output of "attnum = get_attnum(reloid, NameStr(*attname));" 5. create table t(a int, b int); alter table t drop column b; SELECT pg_catalog.pg_set_attribute_stats( relation => 't'::regclass, attname => 'b'::name, inherited => false::boolean, null_frac => 0.1::real, avg_width => 2::integer, n_distinct => 0.3::real); output ERROR: attribute 0 of relation with OID 34316 does not exist The error message is not good, i think. Also, in this case, I think we may need soft errors. instead of returning ERROR, make it return FALSE would be more ok. --- src/backend/statistics/import_stats.c | 215 ++++++++++++++------- src/include/catalog/pg_proc.dat | 6 +- src/test/regress/expected/stats_import.out | 81 +++++++- src/test/regress/sql/stats_import.sql | 43 ++++- 4 files changed, 258 insertions(+), 87 deletions(-) diff --git a/src/backend/statistics/import_stats.c b/src/backend/statistics/import_stats.c index ea87a8e849..9fa3bdda6f 100644 --- a/src/backend/statistics/import_stats.c +++ b/src/backend/statistics/import_stats.c @@ -148,17 +148,108 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel) Relation crel; HeapTuple ctup; Form_pg_class pgcform; + int32 relpages; + float reltuples; + int32 relallvisible; + Relation rel; int replaces[3] = {0}; Datum values[3] = {0}; bool nulls[3] = {false, false, false}; - int ncols = 0; TupleDesc tupdesc; HeapTuple newtup; - - check_required_arg(fcinfo, relarginfo, RELATION_ARG); reloid = PG_GETARG_OID(RELATION_ARG); + relpages = PG_GETARG_INT32(RELPAGES_ARG); + reltuples = PG_GETARG_FLOAT4(RELTUPLES_ARG); + relallvisible = PG_GETARG_INT32(RELALLVISIBLE_ARG); + /* first argument (input relation) sanity check */ + rel = relation_open(reloid, AccessShareLock); + + /* + * Silently ignore tables that are temp tables of other backends trying to + * set these is rather pointless, since their contents are probably not + * up-to-date on disk. + */ + if (RELATION_IS_OTHER_TEMP(rel)) + { + relation_close(rel, AccessShareLock); + return false; + } + + /* we don't manually set table statistics for system tables */ + if (IsCatalogNamespace(rel->rd_rel->relnamespace) || + IsToastNamespace(rel->rd_rel->relnamespace)) + { + relation_close(rel, AccessShareLock); + return false; + } + + if (rel->rd_rel->relkind == RELKIND_RELATION || + rel->rd_rel->relkind == RELKIND_MATVIEW) + { + /* this hould be fine.*/ + } + else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + { + /* TODO is foreign table OK? */ + ereport(WARNING, + (errmsg("skipping \"%s\" --- cannot update statistic on foreign table", + RelationGetRelationName(rel)))); + relation_close(rel, AccessShareLock); + return false; + } + else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + ereport(WARNING, + (errmsg("skipping \"%s\" --- cannot update statistic on partitioned table", + RelationGetRelationName(rel)))); + relation_close(rel, AccessShareLock); + return false; + } + else + { + ereport(WARNING, + (errmsg("skipping \"%s\" --- cannot update statistic non-tables or special system tables", + RelationGetRelationName(rel)))); + relation_close(rel, AccessShareLock); + return false; + } + relation_close(rel, AccessShareLock); + + /* input argument relpages, reltuples, relallvisible sanity check. */ + if (relpages < 0) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("relpages cannot be < 0"))); + return false; + } + + if (reltuples < -1.0) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("reltuples cannot be < -1.0"))); + return false; + } + + if (relallvisible < 0) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("relallvisible cannot be < 0"))); + return false; + } + + /* ensure relpages strictly larger than relallvisible */ + if (relallvisible > relpages) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("relallvisible cannot larger than relpages"))); + return false; + } lock_check_privileges(reloid); /* @@ -180,78 +271,24 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel) pgcform = (Form_pg_class) GETSTRUCT(ctup); - /* relpages */ - if (!PG_ARGISNULL(RELPAGES_ARG)) + if (relpages != pgcform->relpages || + reltuples != pgcform->reltuples || + relallvisible != pgcform->relallvisible) { - int32 relpages = PG_GETARG_INT32(RELPAGES_ARG); - - if (relpages < 0) - { - ereport(elevel, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("relpages cannot be < 0"))); - table_close(crel, RowExclusiveLock); - return false; - } - - if (relpages != pgcform->relpages) - { - replaces[ncols] = Anum_pg_class_relpages; - values[ncols] = Int32GetDatum(relpages); - ncols++; - } + replaces[0] = Anum_pg_class_relpages; + replaces[1] = Anum_pg_class_reltuples; + replaces[2] = Anum_pg_class_relallvisible; + values[0] = Int32GetDatum(relpages); + values[1] = Float4GetDatum(reltuples); + values[2] = Int32GetDatum(relallvisible); } - - if (!PG_ARGISNULL(RELTUPLES_ARG)) - { - float reltuples = PG_GETARG_FLOAT4(RELTUPLES_ARG); - - if (reltuples < -1.0) - { - ereport(elevel, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("reltuples cannot be < -1.0"))); - table_close(crel, RowExclusiveLock); - return false; - } - - if (reltuples != pgcform->reltuples) - { - replaces[ncols] = Anum_pg_class_reltuples; - values[ncols] = Float4GetDatum(reltuples); - ncols++; - } - } - - if (!PG_ARGISNULL(RELALLVISIBLE_ARG)) - { - int32 relallvisible = PG_GETARG_INT32(RELALLVISIBLE_ARG); - - if (relallvisible < 0) - { - ereport(elevel, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("relallvisible cannot be < 0"))); - table_close(crel, RowExclusiveLock); - return false; - } - - if (relallvisible != pgcform->relallvisible) - { - replaces[ncols] = Anum_pg_class_relallvisible; - values[ncols] = Int32GetDatum(relallvisible); - ncols++; - } - } - - /* only update pg_class if there is a meaningful change */ - if (ncols == 0) + else { table_close(crel, RowExclusiveLock); return false; } - newtup = heap_modify_tuple_by_cols(ctup, tupdesc, ncols, replaces, values, + newtup = heap_modify_tuple_by_cols(ctup, tupdesc, 3, replaces, values, nulls); CatalogTupleUpdate(crel, &newtup->t_self, newtup); @@ -312,6 +349,23 @@ attribute_statistics_update(FunctionCallInfo fcinfo, int elevel) attname = PG_GETARG_NAME(ATTNAME_ARG); attnum = get_attnum(reloid, NameStr(*attname)); + if (attnum < 0) + { + ereport(elevel, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("statistics update on system columns is not supported"))); + return false; + } + + if (attnum == 0) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("attribute \"%s\" of relation OID %d does not exist", + NameStr(*attname), reloid))); + return false; + } + if (PG_ARGISNULL(INHERITED_ARG)) { ereport(elevel, @@ -440,7 +494,7 @@ attribute_statistics_update(FunctionCallInfo fcinfo, int elevel) /* * STATISTIC_KIND_MCV - * + * * Convert most_common_vals from text to anyarray, where the element type * is the attribute type, and store in stavalues. Store most_common_freqs * in stanumbers. @@ -689,8 +743,8 @@ get_attr_stat_type(Oid reloid, AttrNumber attnum, int elevel, if (!HeapTupleIsValid(atup)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), - errmsg("attribute %d of relation with OID %u does not exist", - attnum, reloid))); + errmsg("attribute %d of relation \"%s\" does not exist", + attnum, RelationGetRelationName(rel)))); attr = (Form_pg_attribute) GETSTRUCT(atup); @@ -736,7 +790,7 @@ get_attr_stat_type(Oid reloid, AttrNumber attnum, int elevel, *eq_opr = typcache->eq_opr; *lt_opr = typcache->lt_opr; - /* TODO: explain special case for tsvector */ + /* TODO: explain special case for tsvector */ if (*atttypid == TSVECTOROID) *atttypcoll = DEFAULT_COLLATION_OID; @@ -1242,6 +1296,23 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS) attname = PG_GETARG_NAME(ATTNAME_ARG); attnum = get_attnum(reloid, NameStr(*attname)); + if (attnum < 0) + { + ereport(WARNING, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("statistics update on system columns is not supported"))); + PG_RETURN_BOOL(false); + } + + if (attnum == 0) + { + ereport(WARNING, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("attribute \"%s\" of relation OID %d does not exist", + NameStr(*attname), reloid))); + PG_RETURN_BOOL(false); + } + check_required_arg(fcinfo, attarginfo, INHERITED_ARG); inherited = PG_GETARG_BOOL(INHERITED_ARG); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 80e224beb1..d1cf035f18 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -12258,21 +12258,21 @@ # Statistics Import { oid => '8048', descr => 'set statistics on relation', - proname => 'pg_set_relation_stats', provolatile => 'v', proisstrict => 'f', + proname => 'pg_set_relation_stats', provolatile => 'v', proisstrict => 't', proparallel => 'u', prorettype => 'bool', proargtypes => 'regclass int4 float4 int4', proargnames => '{relation,relpages,reltuples,relallvisible}', prosrc => 'pg_set_relation_stats' }, { oid => '8049', descr => 'clear statistics on relation', - proname => 'pg_clear_relation_stats', provolatile => 'v', proisstrict => 'f', + proname => 'pg_clear_relation_stats', provolatile => 'v', proisstrict => 't', proparallel => 'u', prorettype => 'bool', proargtypes => 'regclass', proargnames => '{relation}', prosrc => 'pg_clear_relation_stats' }, { oid => '8050', descr => 'restore statistics on relation', - proname => 'pg_restore_relation_stats', provolatile => 'v', proisstrict => 'f', + proname => 'pg_restore_relation_stats', provolatile => 'v', proisstrict => 't', provariadic => 'any', proparallel => 'u', prorettype => 'bool', proargtypes => 'any', diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out index 5edfb7451b..2af1b064c0 100644 --- a/src/test/regress/expected/stats_import.out +++ b/src/test/regress/expected/stats_import.out @@ -12,14 +12,59 @@ CREATE TABLE stats_import.test( arange int4range, tags text[] ); +CREATE TABLE stats_import.part( + id INTEGER generated by default as identity , + name text, + CONSTRAINT part_pkey PRIMARY KEY (id) +) partition by range (id); +CREATE TABLE stats_import.part1 PARTITION OF stats_import.part FOR VALUES FROM (1) TO (10); +CREATE view stats_import.partv as select id, name from stats_import.part; +CREATE materialized view stats_import.partmv as select id, name from stats_import.part; +--false for partitioned table +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); +WARNING: skipping "part" --- cannot update statistic on partitioned table + pg_set_relation_stats +----------------------- + f +(1 row) + +--false for partitioned index +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_pkey'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); +WARNING: skipping "part_pkey" --- cannot update statistic non-tables or special system tables + pg_set_relation_stats +----------------------- + f +(1 row) + +--false for view +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.partv'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); +WARNING: skipping "partv" --- cannot update statistic non-tables or special system tables + pg_set_relation_stats +----------------------- + f +(1 row) + -- starting stats SELECT relpages, reltuples, relallvisible FROM pg_class -WHERE oid = 'stats_import.test'::regclass; +WHERE oid in ('stats_import.test'::regclass, 'stats_import.partmv'::regclass); relpages | reltuples | relallvisible ----------+-----------+--------------- 0 | -1 | 0 -(1 row) + 0 | -1 | 0 +(2 rows) -- error: regclass not found SELECT @@ -38,7 +83,7 @@ SELECT relallvisible => 4::integer); pg_set_relation_stats ----------------------- - t + (1 row) -- reltuples default @@ -50,7 +95,7 @@ SELECT relallvisible => 4::integer); pg_set_relation_stats ----------------------- - t + (1 row) -- relallvisible default @@ -62,7 +107,7 @@ SELECT relallvisible => NULL::integer); pg_set_relation_stats ----------------------- - f + (1 row) -- named arguments @@ -74,16 +119,29 @@ SELECT relallvisible => 4::integer); pg_set_relation_stats ----------------------- - f + t +(1 row) + +-- named arguments for materialized view +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.partmv'::regclass, + relpages => 17::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); + pg_set_relation_stats +----------------------- + t (1 row) SELECT relpages, reltuples, relallvisible FROM pg_class -WHERE oid = 'stats_import.test'::regclass; +WHERE oid in ('stats_import.test'::regclass, 'stats_import.partmv'::regclass); relpages | reltuples | relallvisible ----------+-----------+--------------- 17 | 400 | 4 -(1 row) + 17 | 400 | 4 +(2 rows) -- positional arguments SELECT @@ -113,7 +171,7 @@ SELECT pg_catalog.pg_set_attribute_stats( null_frac => 0.1::real, avg_width => 2::integer, n_distinct => 0.3::real); -ERROR: could not open relation with OID 0 +ERROR: attribute "id" of relation OID 0 does not exist -- error: relation null SELECT pg_catalog.pg_set_attribute_stats( relation => NULL::oid, @@ -760,7 +818,10 @@ WHERE s.starelid = 'stats_import.is_odd'::regclass; (0 rows) DROP SCHEMA stats_import CASCADE; -NOTICE: drop cascades to 3 other objects +NOTICE: drop cascades to 6 other objects DETAIL: drop cascades to type stats_import.complex_type drop cascades to table stats_import.test +drop cascades to table stats_import.part +drop cascades to view stats_import.partv +drop cascades to materialized view stats_import.partmv drop cascades to table stats_import.test_clone diff --git a/src/test/regress/sql/stats_import.sql b/src/test/regress/sql/stats_import.sql index 0582c27146..79e6a9c6b4 100644 --- a/src/test/regress/sql/stats_import.sql +++ b/src/test/regress/sql/stats_import.sql @@ -15,10 +15,41 @@ CREATE TABLE stats_import.test( tags text[] ); +CREATE TABLE stats_import.part( + id INTEGER generated by default as identity , + name text, + CONSTRAINT part_pkey PRIMARY KEY (id) +) partition by range (id); + +CREATE TABLE stats_import.part1 PARTITION OF stats_import.part FOR VALUES FROM (1) TO (10); +CREATE view stats_import.partv as select id, name from stats_import.part; +CREATE materialized view stats_import.partmv as select id, name from stats_import.part; + +--false for partitioned table +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); + +--false for partitioned index +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_pkey'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); + +--false for view +SELECT pg_catalog.pg_set_relation_stats( + relation => 'stats_import.partv'::regclass, + relpages => 11::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); + -- starting stats SELECT relpages, reltuples, relallvisible FROM pg_class -WHERE oid = 'stats_import.test'::regclass; +WHERE oid in ('stats_import.test'::regclass, 'stats_import.partmv'::regclass); -- error: regclass not found SELECT @@ -60,9 +91,17 @@ SELECT reltuples => 400.0::real, relallvisible => 4::integer); +-- named arguments for materialized view +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.partmv'::regclass, + relpages => 17::integer, + reltuples => 400.0::real, + relallvisible => 4::integer); + SELECT relpages, reltuples, relallvisible FROM pg_class -WHERE oid = 'stats_import.test'::regclass; +WHERE oid in ('stats_import.test'::regclass, 'stats_import.partmv'::regclass); -- positional arguments SELECT -- 2.34.1