Message-ID: <611182a0-172e-cb80-965d-d6c69a1bde62@gmail.com>
Date: Thu, 29 Jun 2023 12: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
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, 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>
 <3eaa2c33-2ed1-2996-c6b6-0e7552a64cae@gmail.com>
 <ZJva9997u5H0RQe1@paquier.xyz>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <ZJva9997u5H0RQe1@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/611182a0-172e-cb80-965d-d6c69a1bde62%40gmail.com>
Precedence: bulk

Hi Michael,

28.06.2023 10:02, Michael Paquier wrote:
>> This looks like a second bug to me in DefineIndex() where we shouldn't
>> even try to link to the invalid index tp1_1_expr_idx, and we should
>> try to create a new index instead.  Repeating the last CREATE INDEX
>> command leads to a failure, which is what I'd expect, because we get
>> the computation failure.
>
>
> I have looked into this one, and decided that we should just rely on
> the existing business in DefineIndex() where we switch indisvalid to
> false on a parent if the index found for the match is itself invalid.
> The code had two bugs:
> - First, we did not re-check if an index freshly created was itself
> invalid, which could happen when recursing through more than one
> level for the creation of a partitioned index.
> - We need to have a CCI after switch the parent's indisvalid to false,
> so as the follow up recursions are able to see that, and do the update
> of indisvalid across the parents up to the top-most parent.
>
> Attached is a patch to fix all that, with regression tests that play
> with multiple partition levels to show the recursion and the problems
> we'd have without the patch.
>
> Alexander, what do you think?

I'm somewhat confused by two things.
First, if we extend your test case by adding DETACH parted_isvalid_tab_11, we get:
            indexrelid           | indisvalid |       indrelid        |           inhparent
--------------------------------+------------+-----------------------+-------------------------------
  parted_isvalid_idx             | f          | parted_isvalid_tab    |
  parted_isvalid_idx_11          | f          | parted_isvalid_tab_11 |
  parted_isvalid_tab_12_expr_idx | t          | parted_isvalid_tab_12 | parted_isvalid_tab_1_expr_idx
  parted_isvalid_tab_1_expr_idx  | f          | parted_isvalid_tab_1  | parted_isvalid_idx
  parted_isvalid_tab_2_expr_idx  | t          | parted_isvalid_tab_2  | parted_isvalid_idx
(5 rows)

That is, the partition tree is containing no invalid indexes now, but the
upper-level indexes in the tree are still invalid.
Moreover, I don't know how to make them valid:
reindex index parted_isvalid_idx; / reindex index parted_isvalid_tab_1_expr_idx;
doesn't affect their flags indisvalid.
(Though REINDEX for a top-level index can make leaf indexes valid.)
Reattaching parted_isvalid_tab_11 after "update parted_isvalid_tab_11 set b=1"
doesn't help either.

> It seems to me that validatePartitionedIndex() is what defines the
> use of the flag for partitioned indexes.  indisvalid = false for a
> partitioned index means that at least one of its partitions *may* not
> have a valid index mapping to it, and that it should be switched to
> true once we are sure that all its partitions have a valid index.
> indisvalid = true on a partitioned table has much a stronger meaning,
> where all the partitions *have* to have valid indexes.  Your first
> case would follow that, but not the second.

So the above test case makes me wonder, is it realistic in principle to
maintain this definition of indisvalid up-to-date for a partition tree?
And even if so, which visible behavior this definition leads (or should
lead) to?
As I noted previously, the leaf index is still used when it's parent has
indisvalid = false. (That's the second thing that confuses me.)
For example, with the partition index tree:
            indexrelid           | indisvalid |       indrelid        |           inhparent
--------------------------------+------------+-----------------------+-------------------------------
  parted_isvalid_idx             | f          | parted_isvalid_tab    |
  parted_isvalid_idx_11          | f          | parted_isvalid_tab_11 |
  parted_isvalid_tab_11_expr_idx | t          | parted_isvalid_tab_11 | parted_isvalid_tab_1_expr_idx
  parted_isvalid_tab_12_expr_idx | t          | parted_isvalid_tab_12 | parted_isvalid_tab_1_expr_idx
  parted_isvalid_tab_1_expr_idx  | f          | parted_isvalid_tab_1  | parted_isvalid_idx
  parted_isvalid_tab_2_expr_idx  | t          | parted_isvalid_tab_2  | parted_isvalid_idx
(6 rows)

explain select * from parted_isvalid_tab where a / b = 1;
  Append  (cost=0.12..23.14 rows=12 width=8)
    ->  Index Scan using parted_isvalid_tab_11_expr_idx on parted_isvalid_tab_11 parted_isvalid_tab_1  (cost=0.1
2..8.14 rows=1 width=8)
          Index Cond: ((a / b) = 1)
    ->  Bitmap Heap Scan on parted_isvalid_tab_12 parted_isvalid_tab_2  (cost=4.24..14.94 rows=11 width=8)
          Recheck Cond: ((a / b) = 1)
          ->  Bitmap Index Scan on parted_isvalid_tab_12_expr_idx (cost=0.00..4.24 rows=11 width=0)
                Index Cond: ((a / b) = 1)

So I can't resist asking a question: what would break if indisvalid flags
were always true (or always false) for the upper-level partition indexes?
I've made validatePartitionedIndex() a no-op just to check that and the
only differences I observe doing `make check` are indisvalid flags printed;
there are no behavioral changes.
Though `make check-world` fails on src/bin/pg_upgrade (only); I need more
time to investigate this — maybe more sophisticated query to select
appropriate indexes for processing could help there.

Best regards,
Alexander