From 2de404f8187466061469abfaf3a773290cce6af8 Mon Sep 17 00:00:00 2001 From: Corey Huinker Date: Tue, 21 Jan 2025 11:52:58 -0500 Subject: [PATCH v47 1/2] 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. There is no special case for partitioned indexes, so while we want to the ACL checks against the underlying relation, we need to take out the more restrictive ShareUpdateExclusiveLock on the partitioned index. [1] https://www.postgresql.org/message-id/CACJufxGreTY7qsCV8%2BBkuv0p5SXGTScgh%3DD%2BDq6%3D%2B_%3DXTp7FWg%40mail.gmail.com --- src/backend/statistics/stat_utils.c | 45 +++++++-- src/test/regress/expected/stats_import.out | 106 +++++++++++++++++++++ src/test/regress/sql/stats_import.sql | 72 ++++++++++++++ 3 files changed, 217 insertions(+), 6 deletions(-) diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c index 0d446f55b01..f87007e72c2 100644 --- a/src/backend/statistics/stat_utils.c +++ b/src/backend/statistics/stat_utils.c @@ -17,13 +17,16 @@ #include "postgres.h" #include "access/relation.h" +#include "catalog/index.h" #include "catalog/pg_database.h" #include "funcapi.h" #include "miscadmin.h" #include "statistics/stat_utils.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" /* @@ -126,18 +129,45 @@ 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; + LOCKMODE index_lockmode = AccessShareLock; - /* 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 then take an + * AccessShareLock on the index itself. See check_inplace_rel_lock() + * to see how this special case is implemented. + * + * Partitioned indexes do not have an exception in check_inplace_rel_lock(), + * so we want to take a ShareUpdateExclusive lock there instead. + */ + switch(get_rel_relkind(reloid)) { - case RELKIND_RELATION: case RELKIND_INDEX: + relation_oid = IndexGetRelation(reloid, false); + index_oid = reloid; + break; + case RELKIND_PARTITIONED_INDEX: + relation_oid = IndexGetRelation(reloid, false); + index_oid = reloid; + index_lockmode = ShareUpdateExclusiveLock; + break; + default: + break; + } + + rel = relation_open(relation_oid, ShareUpdateExclusiveLock); + + switch (rel->rd_rel->relkind) + { + /* All of the types that can be used with ANALYZE */ + case RELKIND_RELATION: case RELKIND_MATVIEW: case RELKIND_FOREIGN_TABLE: case RELKIND_PARTITIONED_TABLE: - case RELKIND_PARTITIONED_INDEX: break; default: ereport(ERROR, @@ -164,6 +194,9 @@ stats_lock_check_privileges(Oid reloid) NameStr(rel->rd_rel->relname)); } + if (OidIsValid(index_oid)) + LockRelationOid(index_oid, index_lockmode); + relation_close(rel, NoLock); } diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out index fb50da1cd83..a479300a68d 100644 --- a/src/test/regress/expected/stats_import.out +++ b/src/test/regress/expected/stats_import.out @@ -85,6 +85,46 @@ WHERE oid = 'stats_import.test'::regclass; 17 | 400 | 4 (1 row) +CREATE INDEX test_i ON stats_import.test(id); +BEGIN; +-- regular indexes have special case locking rules +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.test_i'::regclass, + relpages => 18::integer); + pg_set_relation_stats +----------------------- + +(1 row) + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.test'::regclass +AND granted; + mode +-------------------------- + ShareUpdateExclusiveLock +(1 row) + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.test_i'::regclass +AND granted; + mode +----------------- + AccessShareLock +(1 row) + +COMMIT; +SELECT + pg_catalog.pg_restore_relation_stats( + 'relation', 'stats_import.test_i'::regclass, + 'relpages', 19::integer ); + pg_restore_relation_stats +--------------------------- + t +(1 row) + -- positional arguments SELECT pg_catalog.pg_set_relation_stats( @@ -182,6 +222,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 @@ -193,6 +234,15 @@ WHERE oid = 'stats_import.part_parent'::regclass; -- although partitioned tables have no storage, setting relpages to a -- positive value is still allowed +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); + pg_set_relation_stats +----------------------- + +(1 row) + SELECT pg_catalog.pg_set_relation_stats( relation => 'stats_import.part_parent'::regclass, @@ -202,6 +252,49 @@ SELECT (1 row) +-- +-- Partitioned indexes aren't analyzed but it is possible to set stats. +-- Partitioned indexes also have less relaxed locking rules than regular +-- indexes. +-- +BEGIN; +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); + pg_set_relation_stats +----------------------- + +(1 row) + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.part_parent'::regclass +AND granted; + mode +-------------------------- + ShareUpdateExclusiveLock +(1 row) + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.part_parent_i'::regclass +AND granted; + mode +-------------------------- + ShareUpdateExclusiveLock +(1 row) + +COMMIT; +SELECT + pg_catalog.pg_restore_relation_stats( + 'relation', 'stats_import.part_parent_i'::regclass, + 'relpages', 2::integer); + pg_restore_relation_stats +--------------------------- + t +(1 row) + -- nothing stops us from setting it to -1 SELECT pg_catalog.pg_set_relation_stats( @@ -1414,6 +1507,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 d3058bf8f6b..f5b8bd5fc58 100644 --- a/src/test/regress/sql/stats_import.sql +++ b/src/test/regress/sql/stats_import.sql @@ -64,6 +64,32 @@ SELECT relpages, reltuples, relallvisible FROM pg_class WHERE oid = 'stats_import.test'::regclass; +CREATE INDEX test_i ON stats_import.test(id); + +BEGIN; +-- regular indexes have special case locking rules +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.test_i'::regclass, + relpages => 18::integer); + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.test'::regclass +AND granted; + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.test_i'::regclass +AND granted; + +COMMIT; + +SELECT + pg_catalog.pg_restore_relation_stats( + 'relation', 'stats_import.test_i'::regclass, + 'relpages', 19::integer ); + -- positional arguments SELECT pg_catalog.pg_set_relation_stats( @@ -127,6 +153,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 @@ -135,11 +163,45 @@ WHERE oid = 'stats_import.part_parent'::regclass; -- although partitioned tables have no storage, setting relpages to a -- positive value is still allowed +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); + SELECT pg_catalog.pg_set_relation_stats( relation => 'stats_import.part_parent'::regclass, relpages => 2::integer); +-- +-- Partitioned indexes aren't analyzed but it is possible to set stats. +-- Partitioned indexes also have less relaxed locking rules than regular +-- indexes. +-- +BEGIN; + +SELECT + pg_catalog.pg_set_relation_stats( + relation => 'stats_import.part_parent_i'::regclass, + relpages => 2::integer); + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.part_parent'::regclass +AND granted; + +SELECT mode +FROM pg_locks +WHERE relation = 'stats_import.part_parent_i'::regclass +AND granted; + +COMMIT; + +SELECT + pg_catalog.pg_restore_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 +1124,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; base-commit: 3d17d7d7fb7a4603b48acb275b5a416f110db464 -- 2.48.1