Content-Type: multipart/alternative;
 boundary="------------FtPRMuAm8ybwiksPndO209uL"
Message-ID: <3eaa2c33-2ed1-2996-c6b6-0e7552a64cae@gmail.com>
Date: Wed, 28 Jun 2023 09:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17997: Assert failed in validatePartitionedIndex() when
 attaching partition index to child of valid index
Content-Language: en-US
From: Alexander Lakhin <exclusion@gmail.com>
To: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
References: <17997-a044c27aef95daf8@postgresql.org>
 <509915.1687704058@sss.pgh.pa.us> <ZJk04dp0mz7s58Lx@paquier.xyz>
 <ZJlG0a/F03iPI/wj@paquier.xyz>
 <14987634-43c0-0cb3-e075-94d423607e08@gmail.com>
In-Reply-To: <14987634-43c0-0cb3-e075-94d423607e08@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/3eaa2c33-2ed1-2996-c6b6-0e7552a64cae%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------FtPRMuAm8ybwiksPndO209uL
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

27.06.2023 14:00, Alexander Lakhin wrote:
> 26.06.2023 11:05, Michael Paquier wrote:
>> A third solution that came into my mind just now would be to revisit
>> the choice done in AttachPartitionEnsureIndexes() where an invalid
>> index can be chosen as a match when creating the indexes on the
>> partitions, so as we are able to get to the bottom of a chain with
>> valid indexes for the whole tree.  I have been testing the attached
>> and it has been working here the way I'd expect when manipulating
>> partition trees with ATTACH PARTITION, though this breaks the scenario
>> of this bug report because we would now get a failure when attempting
>> to attach an index in the last command.
>
> Thanks for the fix!
>
> This solution seems sensible to me. The only downside I see is that an
> invalid index would be left orphaned after ATTACH PARTITION, but I couldn't
> find in doc/ or src/test/regress/ any promises that such index must be
> used. I also don't see a way to make a previously valid index inside the
> partition index tree invalid and available to attaching a child index to it
> in the same time.

There is also another scenario where the new behavior could be considered as more sensible:
create table t(a int, b int) partition by range (a);
create index on t((a / b));
create table tp1(a int, b int);
insert into tp1 values (1, 0);
create index concurrently on tp1((a/b)); --  division by zero occurs, but the index is created (as invalid)
alter table t attach partition tp1 for values from (1) to (10);

Without the fix you get partition tp1_1 attached and the following partition indexes:
Partitioned index "public.t_expr_idx"
btree, for table "public.t"
Partitions: tp1_expr_idx

Index "public.tp1_expr_idx"
btree, for table "public.tp1", invalid

But with the patch applied ATTACH PARTITION fails with ERROR: division by zero.

Though we still can get a partition index chain with invalid indexes as follows:
create table t(a int, b int) partition by range (a);
create table tp1(a int, b int) partition by range (a);
alter table t attach partition tp1 for values from (1) to (100);
create table tp1_1(a int, b int);
insert into tp1_1 values (1, 0);
create index concurrently on tp1_1((a/b)); --  division by zero occurs, but the index is created (as invalid)
alter table tp1 attach partition tp1_1 for values from (1) to (10);
create index on t((a / b));

here we get the following index chain:
Partitioned index "public.t_expr_idx"
btree, for table "public.t"
Partitions: tp1_expr_idx, PARTITIONED

Partitioned index "public.tp1_expr_idx"
btree, for table "public.tp1", invalid
Partitions: tp1_1_expr_idx

Index "public.tp1_1_expr_idx"
btree, for table "public.tp1_1", invalid

It's also interesting that REINDEX for the index tree validates only a leaf index:
reindex index t_expr_idx;
ERROR:  division by zero

update tp1_1 set b=1;
reindex index t_expr_idx; -- or even reindex index tp1_expr_idx;

Partitioned index "public.t_expr_idx"
btree, for table "public.t"
Partitions: tp1_expr_idx, PARTITIONED

Partitioned index "public.tp1_expr_idx"
btree, for table "public.tp1", invalid
Partitions: tp1_1_expr_idx

Index "public.tp1_1_expr_idx"
btree, for table "public.tp1_1"

Although it looks like the invalid mark for a non-leaf index doesn't prevent using an index below it:
set enable_seqscan = off;
explain select * from t where a / b = 1;
                                   QUERY PLAN
------------------------------------------------------------------------------
  Index Scan using tp1_1_expr_idx on tp1_1 t  (cost=0.12..8.14 rows=1 width=8)
    Index Cond: ((a / b) = 1)

So it's not clear (to me, at least), what exactly indisvalid means for indexes in a partition tree.

Best regards,
Alexander
--------------FtPRMuAm8ybwiksPndO209uL
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">27.06.2023 14:00, Alexander Lakhin
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:14987634-43c0-0cb3-e075-94d423607e08@gmail.com">26.06.2023
      11:05, Michael Paquier wrote:
      <br>
      <blockquote type="cite">A third solution that came into my mind
        just now would be to revisit
        <br>
        the choice done in AttachPartitionEnsureIndexes() where an
        invalid
        <br>
        index can be chosen as a match when creating the indexes on the
        <br>
        partitions, so as we are able to get to the bottom of a chain
        with
        <br>
        valid indexes for the whole tree.  I have been testing the
        attached
        <br>
        and it has been working here the way I'd expect when
        manipulating
        <br>
        partition trees with ATTACH PARTITION, though this breaks the
        scenario
        <br>
        of this bug report because we would now get a failure when
        attempting
        <br>
        to attach an index in the last command.
        <br>
      </blockquote>
      <br>
      Thanks for the fix!
      <br>
      <br>
      This solution seems sensible to me. The only downside I see is
      that an
      <br>
      invalid index would be left orphaned after ATTACH PARTITION, but I
      couldn't
      <br>
      find in doc/ or src/test/regress/ any promises that such index
      must be
      <br>
      used. I also don't see a way to make a previously valid index
      inside the
      <br>
      partition index tree invalid and available to attaching a child
      index to it
      <br>
      in the same time.
      <br>
    </blockquote>
    <br>
    There is also another scenario where the new behavior could be
    considered as more sensible:<br>
    create table t(a int, b int) partition by range (a);<br>
    create index on t((a / b));<br>
    create table tp1(a int, b int);<br>
    insert into tp1 values (1, 0);<br>
    create index concurrently on tp1((a/b)); --  division by zero
    occurs, but the index is created (as invalid)<br>
    alter table t attach partition tp1 for values from (1) to (10);<br>
    <br>
    Without the fix you get partition tp1_1 attached and the following
    partition indexes:<br>
    Partitioned index "public.t_expr_idx"<br>
    btree, for table "public.t"<br>
    Partitions: tp1_expr_idx<br>
    <br>
    Index "public.tp1_expr_idx"<br>
    btree, for table "public.tp1", invalid<br>
    <br>
    But with the patch applied ATTACH PARTITION fails with ERROR: 
    division by zero.<br>
    <br>
    Though we still can get a partition index chain with invalid indexes
    as follows:<br>
    create table t(a int, b int) partition by range (a);<br>
    create table tp1(a int, b int) partition by range (a);<br>
    alter table t attach partition tp1 for values from (1) to (100);<br>
    create table tp1_1(a int, b int);<br>
    insert into tp1_1 values (1, 0);<br>
    create index concurrently on tp1_1((a/b)); --  division by zero
    occurs, but the index is created (as invalid)<br>
    alter table tp1 attach partition tp1_1 for values from (1) to (10);<br>
    create index on t((a / b));<br>
    <br>
    here we get the following index chain:<br>
    Partitioned index "public.t_expr_idx"<br>
    btree, for table "public.t"<br>
    Partitions: tp1_expr_idx, PARTITIONED<br>
    <br>
    Partitioned index "public.tp1_expr_idx"<br>
    btree, for table "public.tp1", invalid<br>
    Partitions: tp1_1_expr_idx<br>
    <br>
    Index "public.tp1_1_expr_idx"<br>
    btree, for table "public.tp1_1", invalid<br>
    <br>
    It's also interesting that REINDEX for the index tree validates only
    a leaf index:<br>
    reindex index t_expr_idx;<br>
    ERROR:  division by zero<br>
    <br>
    update tp1_1 set b=1;<br>
    reindex index t_expr_idx; -- or even reindex index tp1_expr_idx;<br>
    <br>
    Partitioned index "public.t_expr_idx"<br>
    btree, for table "public.t"<br>
    Partitions: tp1_expr_idx, PARTITIONED<br>
    <br>
    Partitioned index "public.tp1_expr_idx"<br>
    btree, for table "public.tp1", invalid<br>
    Partitions: tp1_1_expr_idx<br>
    <br>
    Index "public.tp1_1_expr_idx"<br>
    btree, for table "public.tp1_1"<br>
    <br>
    Although it looks like the invalid mark for a non-leaf index doesn't
    prevent using an index below it:<br>
    set enable_seqscan = off;<br>
    explain select * from t where a / b = 1;<br>
                                      QUERY
    PLAN                                  <br>
------------------------------------------------------------------------------<br>
     Index Scan using tp1_1_expr_idx on tp1_1 t  (cost=0.12..8.14 rows=1
    width=8)<br>
       Index Cond: ((a / b) = 1)<br>
    <br>
    So it's not clear (to me, at least), what exactly indisvalid means
    for indexes in a partition tree.<br>
    <br>
    Best regards,<br>
    Alexander<span style="font-family:monospace"><span
        style="color:#000000;background-color:#ffffff;"><span
          style="font-family:monospace"><span
            style="font-family:monospace"><br>
          </span></span></span></span><span
      style="font-family:monospace"><span style="font-family:monospace"></span></span>
  </body>
</html>

--------------FtPRMuAm8ybwiksPndO209uL--