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

On Wed, Aug 02, 2023 at 02:01:40PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Cool, I have fixed this issue, then.  The buildfarm looks OK with it.

(I was out for the last couple of days, apologies for the reply
delay.)

> While going through the commit log to prepare release notes, I was
> struck by the fact that this commit (fc55c7ff8 et al) takes the
> approach of ignoring invalid child indexes during ATTACH PARTITION:
>
>     I have studied a few options here (like the possibility to switch
>     indisvalid to false for the parent), but came down to the conclusion
>     that we'd better rely on a simple rule: invalid indexes had better never
>     be chosen, so as the partition attached uses and creates indexes that
>     the parent expects.
>
> However, your later fix for handling invalid child indexes during
> CREATE PARTITIONED INDEX (cfc43aeb3 et al) does the opposite:
>
>     This patch makes sure that indisvalid is set to false on a partitioned
>     index if at least one of its partition is invalid.  The flag is set to
>     true if *all* its partitions are valid.
>
> This seems inconsistent: why do we behave one way during CREATE and
> another during ATTACH?  Do we even need fc55c7ff8's logic change
> anymore?  That is, is it possible that the fixes added by cfc43aeb3
> would be enough to resolve the problem (though with a different
> outcome that the partitioned index remains invalid after ATTACH
> PARTITION)?

Removing fc55c7f is not enough as outlined by the test added in this
commit.  There is a lot of logic in tablecmds.c to flip indisvalid
from false to true in a partitioned index after attaching a new
partition table once all its index partitions are valid, but this
would require us to support the opposite switch.

In short, there would be cases where a partitioned index is switched
from true to false, while we work hard in making sure that only the
opposite is possible, say:
- A partitioned table is created to be a top-most parent "p", with an
index "i".  indisvalid is true for "i" since its creation as it has no
partitions.
- Now assume that there's a second partitioned table "p1" attached
nowhere, that has an existing set of partition.  It will be attached
to the previous partitioned "p".  "p1" has an index "i1" that has been
created with ONLY, some of the partitions may have, or not, a valid
index matching it.  Hence, "i1" has indisvalid=false.

When attaching "p1" to "p", should we force "i" to become invalid as
an effect of "i1" being invalid, meaning that for some reason a
portion of the partitioned index tree is not complete yet (either
index not created, or concurrent index creation failure)?  Note that
we don't move to the creation of indexes in the partitions if finding
an invalid index.  Or it is better to ignore "i1" and go through the
index creation, finding out that suddenly a part of the tree is
invalid?

CREATE INDEX .. ON ONLY gives a lot of ways to manipulate index
partitions (one cannot detach indexes with ALTER INDEX), and I don't
really see it being in many useful ways, TBH, but we support the
grammar.  Anyway, I think that the point is in usability here: by
ensuring that the creation of valid indexes are enforced when
attaching a partition, we have less bad surprises with portions of a
partitioned index tree becoming suddenly invalid when it was
previously valid.

Perhaps Alvaro could weigh in here, as the former author of 8b08f7d
and CREATE INDEX ON ONLY, so I am attaching him in CC.
--
Michael