Horton

Thread

Commits

  1. Fix subtly-incorrect matching of parent and child partitioned indexes.

  1. = TRUE vs IS TRUE confuses partition index creation

    Christophe Pettus <xof@thebuild.com> — 2022-08-16T21:33:51Z

    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

  2. Re: = TRUE vs IS TRUE confuses partition index creation

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-16T21:57:36Z

    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

  3. Re: = TRUE vs IS TRUE confuses partition index creation

    Richard Guo <guofenglinux@gmail.com> — 2022-08-17T04:27:30Z

    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

  4. Re: = TRUE vs IS TRUE confuses partition index creation

    Richard Guo <guofenglinux@gmail.com> — 2022-08-17T07:16:00Z

    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

  5. Re: = TRUE vs IS TRUE confuses partition index creation

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-17T21:50:52Z

    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

  6. Re: = TRUE vs IS TRUE confuses partition index creation

    Richard Guo <guofenglinux@gmail.com> — 2022-08-18T02:27:40Z

    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

  7. Re: = TRUE vs IS TRUE confuses partition index creation

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-18T16:14:29Z

    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