pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed= TRUE vs IS TRUE confuses partition index creation
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-08-18 2cf16cd749 116f20f926 9f0073ef7d 3bfea5cbba e6dbb48487 2a42c1c51d Fix subtly-incorrect matching of parent and child partitioned indexes.
* = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-16 21:33 Christophe Pettus <xof@thebuild.com>
0 siblings, 1 reply; 7+ messages in thread
From: Christophe Pettus @ 2022-08-16 21:33 UTC (permalink / raw)
To: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
This has been tested on 14.5 and 13.7.
When an index is created on the root of a (declarative) partitioned table, that index is also created on the children, unless there is an existing index on that child that matches the definition of the new index. It seems that using `= TRUE` confuses it, compared to `IS TRUE`.
Test case:
BEGIN;
CREATE TABLE public.t (
id bigint NOT NULL,
t timestamp without time zone NOT NULL,
b boolean NOT NULL
) PARTITION BY RANGE (t);
CREATE TABLE public.t_older (
id bigint NOT NULL,
t timestamp without time zone NOT NULL,
b boolean NOT NULL
);
CREATE INDEX ON public.t_older USING btree (id) WHERE b IS TRUE;
CREATE INDEX ON public.t_older USING btree (id) WHERE b = TRUE;
ALTER TABLE t ATTACH PARTITION t_older
FOR VALUES FROM ('2010-01-01') TO ('2022-01-01');
CREATE INDEX ON public.t USING btree (id) WHERE b IS TRUE;
CREATE INDEX ON public.t USING btree (id) WHERE b = TRUE;
COMMIT;
The result is:
xof=# \d t
Partitioned table "public.t"
Column | Type | Collation | Nullable | Default
--------+-----------------------------+-----------+----------+---------
id | bigint | | not null |
t | timestamp without time zone | | not null |
b | boolean | | not null |
Partition key: RANGE (t)
Indexes:
"t_id_idx" btree (id) WHERE b IS TRUE
"t_id_idx1" btree (id) WHERE b = true
Number of partitions: 1 (Use \d+ to list them.)
fin_test=# \d t_older
Table "public.t_older"
Column | Type | Collation | Nullable | Default
--------+-----------------------------+-----------+----------+---------
id | bigint | | not null |
t | timestamp without time zone | | not null |
b | boolean | | not null |
Partition of: t FOR VALUES FROM ('2010-01-01 00:00:00') TO ('2022-01-01 00:00:00')
Indexes:
"t_older_id_idx" btree (id) WHERE b IS TRUE -- Correctly does not create a new index
"t_older_id_idx1" btree (id) WHERE b = true
"t_older_id_idx2" btree (id) WHERE b = true -- Unexpected duplicated index
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-16 21:57 Tom Lane <tgl@sss.pgh.pa.us>
parent: Christophe Pettus <xof@thebuild.com>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2022-08-16 21:57 UTC (permalink / raw)
To: Christophe Pettus <xof@thebuild.com>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Christophe Pettus <xof@thebuild.com> writes:
> When an index is created on the root of a (declarative) partitioned table, that index is also created on the children, unless there is an existing index on that child that matches the definition of the new index. It seems that using `= TRUE` confuses it, compared to `IS TRUE`.
IIRC, "b = true" will be simplified to just "b" somewhere in expression
preprocessing. I'm betting that something in the partitioned index
matching code is applying that preprocessing to one index predicate and
not the other, whereupon they look different. If that's the explanation,
there are likely other cases that should match and fail to.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-17 04:27 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Richard Guo @ 2022-08-17 04:27 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Christophe Pettus <xof@thebuild.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Wed, Aug 17, 2022 at 5:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> IIRC, "b = true" will be simplified to just "b" somewhere in expression
> preprocessing. I'm betting that something in the partitioned index
> matching code is applying that preprocessing to one index predicate and
> not the other, whereupon they look different. If that's the explanation,
> there are likely other cases that should match and fail to.
Yeah, you're right. The matching work happens in indexcmds.c, using
CompareIndexInfo to compare 'cldIdxInfo' and 'indexInfo'. The
'cldIdxInfo' is constructed with BuildIndexInfo, which would run index
expressions and index predicates through const-simplification before
creating the IndexInfo node. While 'indexInfo' is created without any
const-simplification.
This can be verified with the attached changes, which would make it work
for this case.
Thanks
Richard
Attachments:
v1-0001-fix-partitioned-index-matching.patch (application/octet-stream, 1.4K) download
From 5ea8ac9c4aadb69a3d4dc26999e76bf6e614c19b Mon Sep 17 00:00:00 2001
From: pgsql-guo <richard.guo@openpie.com>
Date: Wed, 17 Aug 2022 04:16:46 +0000
Subject: [PATCH v1] fix partitioned index matching
---
src/backend/commands/indexcmds.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 15a57ea9c3..f189ab0db7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1286,6 +1286,7 @@ DefineIndex(Oid relationId,
Oid cldidxid = lfirst_oid(cell);
Relation cldidx;
IndexInfo *cldIdxInfo;
+ Relation index;
/* this index is already partition of another one */
if (has_superclass(cldidxid))
@@ -1293,6 +1294,9 @@ DefineIndex(Oid relationId,
cldidx = index_open(cldidxid, lockmode);
cldIdxInfo = BuildIndexInfo(cldidx);
+
+ index = index_open(indexRelationId, lockmode);
+ indexInfo = BuildIndexInfo(index);
if (CompareIndexInfo(cldIdxInfo, indexInfo,
cldidx->rd_indcollation,
collationObjectId,
@@ -1336,10 +1340,12 @@ DefineIndex(Oid relationId,
found = true;
/* keep lock till commit */
index_close(cldidx, NoLock);
+ index_close(index, NoLock);
break;
}
index_close(cldidx, lockmode);
+ index_close(index, lockmode);
}
list_free(childidxs);
--
2.25.1
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-17 07:16 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Richard Guo @ 2022-08-17 07:16 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Christophe Pettus <xof@thebuild.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Wed, Aug 17, 2022 at 12:27 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Wed, Aug 17, 2022 at 5:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> IIRC, "b = true" will be simplified to just "b" somewhere in expression
>> preprocessing. I'm betting that something in the partitioned index
>> matching code is applying that preprocessing to one index predicate and
>> not the other, whereupon they look different. If that's the explanation,
>> there are likely other cases that should match and fail to.
>
>
> Yeah, you're right. The matching work happens in indexcmds.c, using
> CompareIndexInfo to compare 'cldIdxInfo' and 'indexInfo'. The
> 'cldIdxInfo' is constructed with BuildIndexInfo, which would run index
> expressions and index predicates through const-simplification before
> creating the IndexInfo node. While 'indexInfo' is created without any
> const-simplification.
>
> This can be verified with the attached changes, which would make it work
> for this case.
>
BTW, I searched other callers of CompareIndexInfo and they all have both
IndexInfo nodes to be compared constructed from BuildIndexInfo, which
means both nodes have applied const-simplification to their index
expressions and index predicates. So those callers are fine.
Thanks
Richard
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-17 21:50 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2022-08-17 21:50 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Christophe Pettus <xof@thebuild.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
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) download
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 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.out
index 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.sql
index 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)
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-18 02:27 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Richard Guo @ 2022-08-18 02:27 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Christophe Pettus <xof@thebuild.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Thu, Aug 18, 2022 at 5:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
Thanks. The v2 patch is a good improvement on the two aspects. It's in
good shape to me.
Thanks
Richard
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: = TRUE vs IS TRUE confuses partition index creation
@ 2022-08-18 16:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Tom Lane @ 2022-08-18 16:14 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Christophe Pettus <xof@thebuild.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Richard Guo <guofenglinux@gmail.com> writes:
> Thanks. The v2 patch is a good improvement on the two aspects. It's in
> good shape to me.
Pushed that version.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2022-08-18 16:14 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 21:33 = TRUE vs IS TRUE confuses partition index creation Christophe Pettus <xof@thebuild.com>
2022-08-16 21:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-17 04:27 ` Richard Guo <guofenglinux@gmail.com>
2022-08-17 07:16 ` Richard Guo <guofenglinux@gmail.com>
2022-08-17 21:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-18 02:27 ` Richard Guo <guofenglinux@gmail.com>
2022-08-18 16:14 ` Tom Lane <tgl@sss.pgh.pa.us>
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