Lock table in ShareUpdateExclusive when importing index stats.

Follow locking behavior of ANALYZE when importing statistics. In
particular, when importing index statistics, the table must be locked
in ShareUpdateExclusive mode. Fixes bug reportd by Jian He.

ANALYZE doesn't update statistics on partitioned indexes, and the
locking requirements are slightly different for in-place updates on
partitioned indexes versus normal indexes. To be conservative, lock
both the partitioned table and the partitioned index in
ShareUpdateExclusive mode when importing stats for a partitioned
index.

Author: Corey Huinker
Reported-by: Jian He
Reviewed-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/CACJufxGreTY7qsCV8%2BBkuv0p5SXGTScgh%3DD%2BDq6%3D%2B_%3DXTp7FWg%40mail.gmail.com

diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index 0d446f55b012f7934d0ba141c7301aefd9dbb2e7..e70ea1ce73857e47c0740357793538a9c33232b7 100644 (file)
--- 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,45 +129,86 @@ 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    table;
+   Oid         table_oid = reloid;
+   Oid         index_oid = InvalidOid;
+   LOCKMODE    index_lockmode = NoLock;
 
-   /* All of the types that can be used with ANALYZE, plus indexes */
-   switch (relkind)
+   /*
+    * For indexes, we follow the locking behavior in do_analyze_rel() and
+    * check_inplace_rel_lock(), which is to lock the table first in
+    * ShareUpdateExclusive mode and then the index in AccessShare mode.
+    *
+    * Partitioned indexes are treated differently than normal indexes in
+    * check_inplace_rel_lock(), so we take a ShareUpdateExclusive lock on
+    * both the partitioned table and the partitioned index.
+    */
+   switch (get_rel_relkind(reloid))
    {
-       case RELKIND_RELATION:
        case RELKIND_INDEX:
+           index_oid = reloid;
+           table_oid = IndexGetRelation(index_oid, false);
+           index_lockmode = AccessShareLock;
+           break;
+       case RELKIND_PARTITIONED_INDEX:
+           index_oid = reloid;
+           table_oid = IndexGetRelation(index_oid, false);
+           index_lockmode = ShareUpdateExclusiveLock;
+           break;
+       default:
+           break;
+   }
+
+   table = relation_open(table_oid, ShareUpdateExclusiveLock);
+
+   /* the relkinds that can be used with ANALYZE */
+   switch (table->rd_rel->relkind)
+   {
+       case RELKIND_RELATION:
        case RELKIND_MATVIEW:
        case RELKIND_FOREIGN_TABLE:
        case RELKIND_PARTITIONED_TABLE:
-       case RELKIND_PARTITIONED_INDEX:
            break;
        default:
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot modify statistics for relation \"%s\"",
-                           RelationGetRelationName(rel)),
-                    errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+                           RelationGetRelationName(table)),
+                    errdetail_relkind_not_supported(table->rd_rel->relkind)));
+   }
+
+   if (OidIsValid(index_oid))
+   {
+       Relation    index;
+
+       Assert(index_lockmode != NoLock);
+       index = relation_open(index_oid, index_lockmode);
+
+       Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+
+       /* retain lock on index */
+       relation_close(index, NoLock);
    }
 
-   if (rel->rd_rel->relisshared)
+   if (table->rd_rel->relisshared)
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("cannot modify statistics for shared relation")));
 
    if (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()))
    {
-       AclResult   aclresult = pg_class_aclcheck(RelationGetRelid(rel),
+       AclResult   aclresult = pg_class_aclcheck(RelationGetRelid(table),
                                                  GetUserId(),
                                                  ACL_MAINTAIN);
 
        if (aclresult != ACLCHECK_OK)
            aclcheck_error(aclresult,
-                          get_relkind_objtype(rel->rd_rel->relkind),
-                          NameStr(rel->rd_rel->relname));
+                          get_relkind_objtype(table->rd_rel->relkind),
+                          NameStr(table->rd_rel->relname));
    }
 
-   relation_close(rel, NoLock);
+   /* retain lock on table */
+   relation_close(table, NoLock);
 }
 
 /*

diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out
index fb50da1cd839688f1f5fceb871c0930d3c987da3..0e8491131e3a048c6b0746848bf5cc803f92c5dd 100644 (file)
--- a/src/test/regress/expected/stats_import.out
+++ b/src/test/regress/expected/stats_import.out
@@ -85,6 +85,44 @@ 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
+      pid = pg_backend_pid() AND granted;
+           mode           
+--------------------------
+ ShareUpdateExclusiveLock
+(1 row)
+
+SELECT mode FROM pg_locks
+WHERE relation = 'stats_import.test_i'::regclass AND
+      pid = pg_backend_pid() 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 +220,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 +232,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 +250,48 @@ SELECT
  
 (1 row)
 
+--
+-- Partitioned indexes aren't analyzed but it is possible to set
+-- stats. The locking rules are different from normal indexes due to
+-- the rules for in-place updates: both the partitioned table and the
+-- partitioned index are locked in ShareUpdateExclusive mode.
+--
+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
+      pid = pg_backend_pid() AND granted;
+           mode           
+--------------------------
+ ShareUpdateExclusiveLock
+(1 row)
+
+SELECT mode FROM pg_locks
+WHERE relation = 'stats_import.part_parent_i'::regclass AND
+      pid = pg_backend_pid() 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 +1504,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));
+-- restoring stats on index
+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 d3058bf8f6b40ba0d88779aa2e8f131ad95ed1a0..5e24c779d80071c1f4efc2ae2d0b18e0f16500f5 100644 (file)
--- a/src/test/regress/sql/stats_import.sql
+++ b/src/test/regress/sql/stats_import.sql
@@ -64,6 +64,30 @@ 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
+      pid = pg_backend_pid() AND granted;
+
+SELECT mode FROM pg_locks
+WHERE relation = 'stats_import.test_i'::regclass AND
+      pid = pg_backend_pid() 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 +151,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 +161,44 @@ 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. The locking rules are different from normal indexes due to
+-- the rules for in-place updates: both the partitioned table and the
+-- partitioned index are locked in ShareUpdateExclusive mode.
+--
+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
+      pid = pg_backend_pid() AND granted;
+
+SELECT mode FROM pg_locks
+WHERE relation = 'stats_import.part_parent_i'::regclass AND
+      pid = pg_backend_pid() 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 +1121,15 @@ SELECT 4, 'four', NULL, int4range(0,100), NULL;
 
 CREATE INDEX is_odd ON stats_import.test(((comp).a % 2 = 1));
 
+-- restoring stats on index
+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;