v28-0001-minor-refactor-based-on-28j.no-cfbot
application/octet-stream
Filename: v28-0001-minor-refactor-based-on-28j.no-cfbot
Type: application/octet-stream
Part: 0
Message:
Re: Statistics Import and Export
From ad16b462761eb56f47840237bfb21f9fa5606afa Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
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