Re: BUG #18500: Detaching a partition with an index manually attached to the parent's index triggers Assert

Did anybody figure out a way to repair the problematic situation without
having to recreate the whole index hierarchy?  If so, please let me
know.  For full context, see below.

On 2024-Jun-28, Alvaro Herrera wrote:

> Here's a proposed patch for master only.  It turns all three situations
> being reported into ereport(ERROR); in one case I have an XXX comment,
> because we have an alternative when attaching a partition that already
> has a PK to a partitioned table that has a non-PK index: just create a
> separate index in the partition.  But that would cause slowness, which
> is probably undesirable.  I'm inclined to just remove the XXX comment,
> but if anyone has other thoughts, they are welcome.
> 
> I add an errhint() to one of these new errors, but I'm undecided about
> that idea --- should we add errhint() to all three messages, or none?
> 
> We can't do this in back branches, of course (incl. 17 at this point),
> because that might break existing applications, which means we need to
> adjust behavior in some other way, probably similar to what Tender Wang
> suggested, namely just don't crash on detach, or something like that.

So here are three patches.  0001 adds a check at DETACH time so that if
a constraint has the form that we don't want to see, it copes without
crashing in the way Tender suggested.  The code is different though,
mainly because we don't need his proposed has_superclass() function.  It
also adds the test cases, and leaves one of the tables from it so that
pg_upgrade is tested.

0002 is the code that throws an error in any of the three problem cases;
roughly the same as the patch I posted as 0001 upthread.  This is
incomplete in the sense that the regression tests would fail with it,
because I didn't modify the results.  (I think we'd also remove the
DETACH line from those tests, because it'd be useless).

0003 adds the pg_upgrade check I mentioned: it scans the databases in
the source cluster for any constraints of the bogus form and fails the
upgrade if any exist.  I tested this one very lightly (that is to say,
only with the database state left by the tests in 0001).


After staring at this third patch for a little bit, I'm not so sure
about rejecting the situation after all.  The problem is that it is
quite tricky to get rid of the problem objects: you may have to accept a
length of time during which your partitioned table has no unique index
and the partitions have no primary keys, and you need to have a lot of
time and effort to recreate essentially those very same things, just to
get the PK created at the partitioned-table level instead of at the
partitions level.  From a user's point of view this sounds rather
insane because of how inconvenient it is.

If we did have `ALTER TABLE partitioned ADD PRIMARY KEY USING INDEX`,
then it wouldn't be a problem, because you can repair the situation
cheaply and easily.  So maybe a better plan is to retain the lenient
behavior for now (that is, push patch 0001 only), then add ALTER TABLE
ADD PRIMARY KEY USING INDEX (to 18 only of course), and only _then_ get
patches 0002 and 0003 pushed (to 18 only).  If somebody does see a cheap
way to repair an existing database without having to recreate the
indexes, then my opinion would switch 180 degrees -- so please speak up
if you do.


BTW I didn't actually try the cases where the constraint is UNIQUE
rather than PRIMARY KEY (because that only occurred to me while I was
writing this email), but I suspect it should be more or less the same,
if not exactly identical.

Also, I should disclaim that I didn't try these scenarios on a server
compiled without assertions.  I'll give them a run tomorrow, in case
there are further problems hidden behind the assertion failures.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/