Fix subtly-incorrect matching of parent and child partitioned indexes.
authorTom Lane <[email protected]>
Thu, 18 Aug 2022 16:11:47 +0000 (12:11 -0400)
committerTom Lane <[email protected]>
Thu, 18 Aug 2022 16:12:03 +0000 (12:12 -0400)
When creating a partitioned index, DefineIndex tries to identify
any existing indexes on the partitions that match the partitioned
index, so that it can absorb those as child indexes instead of
building new ones.  Part of the matching is to compare IndexInfo
structs --- but that wasn't done quite right.  We're comparing
the IndexInfo built within DefineIndex itself to one made from
existing catalog contents by BuildIndexInfo.  Notably, while
BuildIndexInfo will run index expressions and predicates through
expression preprocessing, that has not happened to DefineIndex's
struct.  The result is failure to match and subsequent creation
of duplicate indexes.

The easiest and most bulletproof fix is to build a new IndexInfo
using BuildIndexInfo, thereby guaranteeing that the processing done
is identical.

While here, let's also extract the opfamily and collation data
from the new partitioned index, removing ad-hoc logic that
duplicated knowledge about how those are constructed.

Per report from Christophe Pettus.  Back-patch to v11 where
we invented partitioned indexes.

Richard Guo and Tom Lane

Discussion: https://postgr.es/m/8864BFAA-81FD-4BF9-8E06-7DEB8D4164ED@thebuild.com

src/backend/commands/indexcmds.c
src/test/regress/expected/indexing.out
src/test/regress/sql/indexing.sql

index 15a57ea9c3d0e709d8488766141eb2a67e6b2df5..667f2a4cd169d5bc7b52f98161869d413675ac79 100644 (file)
@@ -1213,18 +1213,27 @@ DefineIndex(Oid relationId,
            int         nparts = partdesc->nparts;
            Oid        *part_oids = palloc(sizeof(Oid) * nparts);
            bool        invalidate_parent = false;
+           Relation    parentIndex;
            TupleDesc   parentDesc;
-           Oid        *opfamOids;
 
            pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
                                         nparts);
 
+           /* Make a local copy of partdesc->oids[], just for safety */
            memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
 
+           /*
+            * We'll need an IndexInfo describing the parent index.  The one
+            * built above is almost good enough, but not quite, because (for
+            * example) its predicate expression if any hasn't been through
+            * expression preprocessing.  The most reliable way to get an
+            * IndexInfo that will match those for child indexes is to build
+            * it the same way, using BuildIndexInfo().
+            */
+           parentIndex = index_open(indexRelationId, lockmode);
+           indexInfo = BuildIndexInfo(parentIndex);
+
            parentDesc = RelationGetDescr(rel);
-           opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
-           for (i = 0; i < numberOfKeyAttributes; i++)
-               opfamOids[i] = get_opclass_family(classObjectId[i]);
 
            /*
             * For each partition, scan all existing indexes; if one matches
@@ -1295,9 +1304,9 @@ DefineIndex(Oid relationId,
                    cldIdxInfo = BuildIndexInfo(cldidx);
                    if (CompareIndexInfo(cldIdxInfo, indexInfo,
                                         cldidx->rd_indcollation,
-                                        collationObjectId,
+                                        parentIndex->rd_indcollation,
                                         cldidx->rd_opfamily,
-                                        opfamOids,
+                                        parentIndex->rd_opfamily,
                                         attmap))
                    {
                        Oid         cldConstrOid = InvalidOid;
@@ -1424,6 +1433,8 @@ DefineIndex(Oid relationId,
                free_attrmap(attmap);
            }
 
+           index_close(parentIndex, lockmode);
+
            /*
             * The pg_index row we inserted for this index was marked
             * indisvalid=true.  But if we attached an existing index that is
index 193f7801912dfddb122b013c43e69beb00e6718a..1bdd430f063c04f653e2fef2a858ef0b9431d6dc 100644 (file)
@@ -378,7 +378,7 @@ drop table idxpart;
 -- When a table is attached a partition and it already has an index, a
 -- duplicate index should not get created, but rather the index becomes
 -- attached to the parent's index.
-create table idxpart (a int, b int, c text) partition by range (a);
+create table idxpart (a int, b int, c text, d bool) partition by range (a);
 create index idxparti on idxpart (a);
 create index idxparti2 on idxpart (b, c);
 create table idxpart1 (like idxpart including indexes);
@@ -389,6 +389,7 @@ create table idxpart1 (like idxpart including indexes);
  a      | integer |           |          | 
  b      | integer |           |          | 
  c      | text    |           |          | 
+ d      | boolean |           |          | 
 Indexes:
     "idxpart1_a_idx" btree (a)
     "idxpart1_b_c_idx" btree (b, c)
@@ -415,6 +416,7 @@ alter table idxpart attach partition idxpart1 for values from (0) to (10);
  a      | integer |           |          | 
  b      | integer |           |          | 
  c      | text    |           |          | 
+ d      | boolean |           |          | 
 Partition of: idxpart FOR VALUES FROM (0) TO (10)
 Indexes:
     "idxpart1_a_idx" btree (a)
@@ -434,6 +436,68 @@ select relname, relkind, inhparent::regclass
  idxparti2        | I       | 
 (6 rows)
 
+-- While here, also check matching when creating an index after the fact.
+create index on idxpart1 ((a+b)) where d = true;
+\d idxpart1
+              Table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+ d      | boolean |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_b_c_idx" btree (b, c)
+    "idxpart1_expr_idx" btree ((a + b)) WHERE d = true
+
+select relname, relkind, inhparent::regclass
+    from pg_class left join pg_index ix on (indexrelid = oid)
+   left join pg_inherits on (ix.indexrelid = inhrelid)
+   where relname like 'idxpart%' order by relname;
+      relname      | relkind | inhparent 
+-------------------+---------+-----------
+ idxpart           | p       | 
+ idxpart1          | r       | 
+ idxpart1_a_idx    | i       | idxparti
+ idxpart1_b_c_idx  | i       | idxparti2
+ idxpart1_expr_idx | i       | 
+ idxparti          | I       | 
+ idxparti2         | I       | 
+(7 rows)
+
+create index idxparti3 on idxpart ((a+b)) where d = true;
+\d idxpart1
+              Table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+ d      | boolean |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_b_c_idx" btree (b, c)
+    "idxpart1_expr_idx" btree ((a + b)) WHERE d = true
+
+select relname, relkind, inhparent::regclass
+    from pg_class left join pg_index ix on (indexrelid = oid)
+   left join pg_inherits on (ix.indexrelid = inhrelid)
+   where relname like 'idxpart%' order by relname;
+      relname      | relkind | inhparent 
+-------------------+---------+-----------
+ idxpart           | p       | 
+ idxpart1          | r       | 
+ idxpart1_a_idx    | i       | idxparti
+ idxpart1_b_c_idx  | i       | idxparti2
+ idxpart1_expr_idx | i       | idxparti3
+ idxparti          | I       | 
+ idxparti2         | I       | 
+ idxparti3         | I       | 
+(8 rows)
+
 drop table idxpart;
 -- Verify that attaching an invalid index does not mark the parent index valid.
 -- On the other hand, attaching a valid index marks not only its direct
index 42f398b67c2c4dc5fdeba06625c6014543f38a00..429120e7104347a8eec7c22def7a762ef24f6615 100644 (file)
@@ -192,7 +192,7 @@ drop table idxpart;
 -- When a table is attached a partition and it already has an index, a
 -- duplicate index should not get created, but rather the index becomes
 -- attached to the parent's index.
-create table idxpart (a int, b int, c text) partition by range (a);
+create table idxpart (a int, b int, c text, d bool) partition by range (a);
 create index idxparti on idxpart (a);
 create index idxparti2 on idxpart (b, c);
 create table idxpart1 (like idxpart including indexes);
@@ -203,6 +203,19 @@ select relname, relkind, inhparent::regclass
    where relname like 'idxpart%' order by relname;
 alter table idxpart attach partition idxpart1 for values from (0) to (10);
 \d idxpart1
+select relname, relkind, inhparent::regclass
+    from pg_class left join pg_index ix on (indexrelid = oid)
+   left join pg_inherits on (ix.indexrelid = inhrelid)
+   where relname like 'idxpart%' order by relname;
+-- While here, also check matching when creating an index after the fact.
+create index on idxpart1 ((a+b)) where d = true;
+\d idxpart1
+select relname, relkind, inhparent::regclass
+    from pg_class left join pg_index ix on (indexrelid = oid)
+   left join pg_inherits on (ix.indexrelid = inhrelid)
+   where relname like 'idxpart%' order by relname;
+create index idxparti3 on idxpart ((a+b)) where d = true;
+\d idxpart1
 select relname, relkind, inhparent::regclass
     from pg_class left join pg_index ix on (indexrelid = oid)
    left join pg_inherits on (ix.indexrelid = inhrelid)