From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Christophe Pettus <xof@thebuild.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: = TRUE vs IS TRUE confuses partition index creation
Date: Wed, 17 Aug 2022 17:50:52 -0400
Message-ID: <2155685.1660773052@sss.pgh.pa.us> (raw)
In-Reply-To: <CAMbWs4--MS+KzwbyR+4hM1tP_gzBA-aCr-4k1-4v=+ONLMWwTQ@mail.gmail.com>
References: <8864BFAA-81FD-4BF9-8E06-7DEB8D4164ED@thebuild.com>
<1551758.1660687056@sss.pgh.pa.us>
<CAMbWs49SVPtFGy4+55eTLR++XPAhhH2VPcUdqvC08Dr4-amRQg@mail.gmail.com>
<CAMbWs4--MS+KzwbyR+4hM1tP_gzBA-aCr-4k1-4v=+ONLMWwTQ@mail.gmail.com>
Richard Guo <guofenglinux@gmail.com> writes:
>> This can be verified with the attached changes, which would make it work
>> for this case.
I don't like this patch too much, because it will result in opening
the new index, building an IndexInfo, and closing the index again
for each index of each partition. We only need to do that once.
Another thing that struck me as poor practice was not getting the
other arguments of CompareIndexInfo (opfamilies and collation)
from the new index. At best this is making the code know more
than it needs to.
Hence, v2 patch attached, now with a test case.
regards, tom lane
Attachments:v2-0001-fix-partitioned-index-matching.patch (text/x-diff, 7.8K) downloaddiff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex 15a57ea9c3..667f2a4cd1 100644--- a/src/backend/commands/indexcmds.c+++ b/src/backend/commands/indexcmds.c@@ -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
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.outindex 193f780191..1bdd430f06 100644--- a/src/test/regress/expected/indexing.out+++ b/src/test/regress/expected/indexing.out@@ -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
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sqlindex 42f398b67c..429120e710 100644--- a/src/test/regress/sql/indexing.sql+++ b/src/test/regress/sql/indexing.sql@@ -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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, guofenglinux@gmail.com, xof@thebuild.com, pgsql-bugs@lists.postgresql.org
Subject: Re: = TRUE vs IS TRUE confuses partition index creation
In-Reply-To: <2155685.1660773052@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox