On Mon, Jan 20, 2025 at 10:59 PM Corey Huinker <corey.huin...@gmail.com> wrote:
> Are you saying that there is a path for a partitioned index to have >> stats today? If so, we can just follow that locking protocol. If not, >> I'm concerned about trying to define the locking protocol for doing so >> as a side-effect of this work. >> > > A quick test on v17 indicates that, no, there is no path for a partitioned > index to have stats of its own, so I'm fine with removing > RELKIND_PARTITIONED_INDEX from the allowable relkinds. The partitions of > the partitioned index have stats but the umbrella index does not. > > >> Wouldn't it be easy enough to issue a WARNING and skip it? >> > > Sure. > > So this is the next iteration - it rejects setting stats on partitioned indexes entirely, and it does all the ACL checks against the underlying relation instead of the indexrel. The only oddity is that if a user calls a statistics setting function on some_index of some_table, and they do not have permission to modify stats on some_table, the error message they get reflects the lack of permission on some_table, not the some_index that was invoked. That may be initially surprising, but it actually is more helpful because the permissions that need to change are on the table not the index.
From 2d42f2e87a35eb51615df9f7617b143437b1a356 Mon Sep 17 00:00:00 2001 From: Corey Huinker <corey.huin...@gmail.com> Date: Mon, 20 Jan 2025 16:18:51 -0500 Subject: [PATCH v41] Lock table first when setting index relation statistics. Jian He reported [1] a missing lock relation bug in pg_restore_relation_stats when restoring stats to an index. This fix follows the steps for proper locking prior to an inplace update of a relation as specified in aac2c9b4fde8, and mimics the locking behavior of the analyze command, as well as correctly doing the ACL checks against the underlying relation of an index rather than the index itself. [1] https://www.postgresql.org/message-id/CACJufxGreTY7qsCV8%2BBkuv0p5SXGTScgh%3DD%2BDq6%3D%2B_%3DXTp7FWg%40mail.gmail.com --- src/backend/statistics/attribute_stats.c | 3 +- src/backend/statistics/stat_utils.c | 37 ++++++++++++++++++---- src/test/regress/expected/stats_import.out | 21 ++++++++++++ src/test/regress/sql/stats_import.sql | 18 +++++++++++ 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.c index 94f7dd63a0..ddb62921c1 100644 --- a/src/backend/statistics/attribute_stats.c +++ b/src/backend/statistics/attribute_stats.c @@ -480,8 +480,7 @@ attribute_statistics_update(FunctionCallInfo fcinfo, int elevel) static Node * get_attr_expr(Relation rel, int attnum) { - if ((rel->rd_rel->relkind == RELKIND_INDEX - || (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)) + if ((rel->rd_rel->relkind == RELKIND_INDEX) && (rel->rd_indexprs != NIL) && (rel->rd_index->indkey.values[attnum - 1] == 0)) { diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c index 0d446f55b0..061d722908 100644 --- a/src/backend/statistics/stat_utils.c +++ b/src/backend/statistics/stat_utils.c @@ -17,14 +17,21 @@ #include "postgres.h" #include "access/relation.h" +#include "c.h" +#include "catalog/pg_class_d.h" #include "catalog/pg_database.h" +#include "catalog/index.h" #include "funcapi.h" #include "miscadmin.h" #include "statistics/stat_utils.h" +#include "storage/lockdefs.h" +#include "storage/lmgr.h" #include "utils/acl.h" #include "utils/array.h" #include "utils/builtins.h" +#include "utils/lsyscache.h" #include "utils/rel.h" +#include "utils/syscache.h" /* * Ensure that a given argument is not null. @@ -126,18 +133,32 @@ stats_check_arg_pair(FunctionCallInfo fcinfo, void stats_lock_check_privileges(Oid reloid) { - Relation rel = relation_open(reloid, ShareUpdateExclusiveLock); - const char relkind = rel->rd_rel->relkind; + Relation rel; + Oid relation_oid = reloid; + Oid index_oid = InvalidOid; - /* All of the types that can be used with ANALYZE, plus indexes */ - switch (relkind) + + /* + * For indexes, we follow what do_analyze_rel() does so as to avoid any + * deadlocks with analyze/vacuum, which is to take out a + * ShareUpdateExclusive on table/matview first and only after that take + * a a AccessShareLock on the index itself. + */ + if (get_rel_relkind(reloid) == RELKIND_INDEX) + { + relation_oid = IndexGetRelation(reloid, false); + index_oid = reloid; + } + + rel = relation_open(relation_oid, ShareUpdateExclusiveLock); + + /* All of the types that can be used with ANALYZE */ + switch (rel->rd_rel->relkind) { case RELKIND_RELATION: - case RELKIND_INDEX: case RELKIND_MATVIEW: case RELKIND_FOREIGN_TABLE: case RELKIND_PARTITIONED_TABLE: - case RELKIND_PARTITIONED_INDEX: break; default: ereport(ERROR, @@ -164,7 +185,11 @@ stats_lock_check_privileges(Oid reloid) NameStr(rel->rd_rel->relname)); } + if (OidIsValid(index_oid)) + LockRelationOid(index_oid, AccessShareLock); + relation_close(rel, NoLock); + } /* diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out index fb50da1cd8..0b706f0981 100644 --- a/src/test/regress/expected/stats_import.out +++ b/src/test/regress/expected/stats_import.out @@ -182,6 +182,7 @@ CREATE TABLE stats_import.part_child_1 PARTITION OF stats_import.part_parent FOR VALUES FROM (0) TO (10) WITH (autovacuum_enabled = false); +CREATE INDEX part_parent_i ON stats_import.part_parent(i); ANALYZE stats_import.part_parent; SELECT relpages FROM pg_class @@ -202,6 +203,13 @@ SELECT (1 row) +-- Cannot set stats on a partitioned index +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); +ERROR: cannot modify statistics for relation "part_parent_i" +DETAIL: This operation is not supported for partitioned indexes. -- nothing stops us from setting it to -1 SELECT pg_catalog.pg_set_relation_stats( @@ -1414,6 +1422,19 @@ SELECT 3, 'tre', (3, 3.3, 'TRE', '2003-03-03', NULL)::stats_import.complex_type, UNION ALL SELECT 4, 'four', NULL, int4range(0,100), NULL; CREATE INDEX is_odd ON stats_import.test(((comp).a % 2 = 1)); +/* Test for proper locking */ +SELECT * FROM pg_catalog.pg_restore_relation_stats( + 'relation', 'stats_import.is_odd'::regclass, + 'version', '180000'::integer, + 'relpages', '11'::integer, + 'reltuples', '10000'::real, + 'relallvisible', '0'::integer +); + pg_restore_relation_stats +--------------------------- + t +(1 row) + -- Generate statistics on table with data ANALYZE stats_import.test; CREATE TABLE stats_import.test_clone ( LIKE stats_import.test ) diff --git a/src/test/regress/sql/stats_import.sql b/src/test/regress/sql/stats_import.sql index d3058bf8f6..efe17ff456 100644 --- a/src/test/regress/sql/stats_import.sql +++ b/src/test/regress/sql/stats_import.sql @@ -127,6 +127,8 @@ CREATE TABLE stats_import.part_child_1 FOR VALUES FROM (0) TO (10) WITH (autovacuum_enabled = false); +CREATE INDEX part_parent_i ON stats_import.part_parent(i); + ANALYZE stats_import.part_parent; SELECT relpages @@ -140,6 +142,12 @@ SELECT relation => 'stats_import.part_parent'::regclass, relpages => 2::integer); +-- Cannot set stats on a partitioned index +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); + -- nothing stops us from setting it to -1 SELECT pg_catalog.pg_set_relation_stats( @@ -1062,6 +1070,16 @@ SELECT 4, 'four', NULL, int4range(0,100), NULL; CREATE INDEX is_odd ON stats_import.test(((comp).a % 2 = 1)); + +/* Test for proper locking */ +SELECT * FROM pg_catalog.pg_restore_relation_stats( + 'relation', 'stats_import.is_odd'::regclass, + 'version', '180000'::integer, + 'relpages', '11'::integer, + 'reltuples', '10000'::real, + 'relallvisible', '0'::integer +); + -- Generate statistics on table with data ANALYZE stats_import.test; -- 2.48.1
