On Sat, Jan 18, 2025 at 7:45 PM Michael Paquier <mich...@paquier.xyz> wrote:
> On Sun, Jan 19, 2025 at 01:00:04AM +0800, jian he wrote: > > On Fri, Jan 17, 2025 at 10:20 PM jian he <jian.universal...@gmail.com> > wrote: > >> dump and execute the above query generated a warning > >> WARNING: missing lock for relation "tenk1_hundred" (OID 18431, > >> relkind i) @ TID (15,34) > > > > This seems to be an existing issue. > > For pg_restore_relation_stats, we don't have regress tests for index > relation. > > I am not sure the WARNING is ok. > > This is not OK based on the rules introduced by Noah at aac2c9b4fde8. > check_lock_if_inplace_updateable_rel() expects a lock to be taken. > Note that elog() are used when reports should never be user-facing, > for states that should not be reachable. > -- > Michael > Here's a patch that Attached is the patch, along with the regression test output prior to the change to stat_utils.c. Other pg_dump work has been left out of v39 to focus on this.
regression.diffs
Description: Binary data
From a86d16c6413e624d7785c8347fe4e9fbf9a8ce66 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 v39] 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. Confirmed that test case generates the reported error without the code fix. [1] https://www.postgresql.org/message-id/CACJufxGreTY7qsCV8%2BBkuv0p5SXGTScgh%3DD%2BDq6%3D%2B_%3DXTp7FWg%40mail.gmail.com --- src/backend/statistics/stat_utils.c | 29 ++++++++++++++++++++-- src/test/regress/expected/stats_import.out | 13 ++++++++++ src/test/regress/sql/stats_import.sql | 10 ++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c index 0d446f55b0..381eeb38e5 100644 --- a/src/backend/statistics/stat_utils.c +++ b/src/backend/statistics/stat_utils.c @@ -18,13 +18,16 @@ #include "access/relation.h" #include "catalog/pg_database.h" +#include "catalog/index.h" #include "funcapi.h" #include "miscadmin.h" #include "statistics/stat_utils.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,8 +129,30 @@ 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; + const char relkind = get_rel_relkind(reloid); + Relation rel; + Oid indrelid = InvalidOid; + + /* + * For indexes we must lock the table first to avoid deadlocking with + * analyze/vacuum. + */ + if (relkind == RELKIND_INDEX) + { + Relation parentrel; + + indrelid = IndexGetRelation(reloid, false); + + parentrel = relation_open(indrelid, ShareUpdateExclusiveLock); + + relation_close(parentrel, NoLock); + } + + rel = relation_open(reloid, ShareUpdateExclusiveLock); + + if (OidIsValid(indrelid) && + (indrelid != IndexGetRelation(rel->rd_rel->oid, false))) + elog(ERROR, "index parent oid recheck failed for index %u", reloid); /* All of the types that can be used with ANALYZE, plus indexes */ switch (relkind) diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out index fb50da1cd8..fd9a6d2c9b 100644 --- a/src/test/regress/expected/stats_import.out +++ b/src/test/regress/expected/stats_import.out @@ -1414,6 +1414,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..043708f529 100644 --- a/src/test/regress/sql/stats_import.sql +++ b/src/test/regress/sql/stats_import.sql @@ -1062,6 +1062,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
