Re: BUG #17997: Assert failed in validatePartitionedIndex() when attaching partition index to child of valid index

On Fri, Jun 30, 2023 at 07:00:00AM +0300, Alexander Lakhin wrote:
> Yeah, I meant DETACH PARTITION for a table. And when I tried the sequence
> you have described, I couldn't get a completely valid tree:
> alter table parted_isvalid_tab_1 detach partition parted_isvalid_tab_11;
> drop index parted_isvalid_idx_11;
> update parted_isvalid_tab_11 set b = 1;
> alter table parted_isvalid_tab_1 attach partition parted_isvalid_tab_11 for values from (1) to (5);
>
>            indexrelid           | indisvalid | indrelid        |           inhparent
> --------------------------------+------------+-----------------------+-------------------------------
>  parted_isvalid_idx             | f          | parted_isvalid_tab    |
>  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
> (5 rows)
>
> The index  parted_isvalid_tab_11_expr_idx is created anew and it's valid,
> but the higher-level indexes are still invalid.

Indeed.  Hmm.  There is room for improvement to make some of the
partitioned indexes valid when doing such manipulations and be more
opportunistic when switching the flag on.  At least, based on what
validatePartitionedIndex() defines, this is not wrongly shaped as far
as I get it, indisvalid is a bit lossy as well when disabled.  Perhaps
this could justify having an ALTER INDEX .. VALID or something like
that as grammar?  Some folks I know of wanted an INVALID flavor to
influence the planner in the index choices, similarly to a hint.

One thing that can be done is to drop the top-parent and recreate it,
though it is not the best option performance-wise, of course.

>> Anyway, DefineIndex() and its handling of indisvalid is clearly wrong
>> when it comes to multiple partition layers as it forgets that we may
>> have to update the parents recursively or we could once again trigger
>> the assertion from validatePartitionedIndex(), which would not be a
>> good idea..  So my previous patch fixes that, at least.
>
> Yeah, in regard to that, your patch looks good to me.
> Thank you!

Cool, I have fixed this issue, then.  The buildfarm looks OK with it.
--
Michael