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

The following bug has been logged on the website:

Bug reference:      18500
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 17beta1
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE TABLE t (a integer) PARTITION BY RANGE (a);
CREATE TABLE tp (a integer PRIMARY KEY);
ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1000);

CREATE UNIQUE INDEX t_a_idx ON t (a);
ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;

\d+ t*
ALTER TABLE t DETACH PARTITION tp;

triggers an assertion failure as below:
...
              Partitioned index "public.t_a_idx"
 Column |  Type   | Key? | Definition | Storage | Stats target 
--------+---------+------+------------+---------+--------------
 a      | integer | yes  | a          | plain   | 
unique, btree, for table "public.t"
Partitions: tp_pkey
Access method: btree
...
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
connection to server was lost

TRAP: failed Assert("constrForm->coninhcount == 0"), File:
"pg_constraint.c", Line: 873, PID: 3272276
...
#5  0x000055b26dc9b76f in ExceptionalCondition (
    conditionName=conditionName@entry=0x55b26dd2d254
"constrForm->coninhcount == 0", 
    fileName=fileName@entry=0x55b26dd2d230 "pg_constraint.c",
lineNumber=lineNumber@entry=873) at assert.c:66
#6  0x000055b26d8687d7 in ConstraintSetParentConstraint
(childConstrId=16392, parentConstrId=parentConstrId@entry=0, 
    childTableId=childTableId@entry=0) at pg_constraint.c:873
#7  0x000055b26d9361c7 in DetachPartitionFinalize
(rel=rel@entry=0x7f84a051d4b8, partRel=partRel@entry=0x7f84a0514428, 
    concurrent=concurrent@entry=false,
defaultPartOid=defaultPartOid@entry=0) at tablecmds.c:19306
#8  0x000055b26d94449a in ATExecDetachPartition
(wqueue=wqueue@entry=0x7fff8d338780, tab=tab@entry=0x55b26fcd6f90, 
    rel=rel@entry=0x7f84a051d4b8, name=<optimized out>, concurrent=false) at
tablecmds.c:19140
#9  0x000055b26d944fe0 in ATExecCmd (wqueue=wqueue@entry=0x7fff8d338780,
tab=tab@entry=0x55b26fcd6f90, 
    cmd=<optimized out>, lockmode=lockmode@entry=8,
cur_pass=cur_pass@entry=AT_PASS_MISC, 
    context=context@entry=0x7fff8d338890) at tablecmds.c:5515
...

Without asserts enabled, DETACH executed with no error.

Reproduced on REL_11_STABLE .. master.

On Sun, Jun 09, 2024 at 06:00:00AM +0000, PG Bug reporting form wrote:
> CREATE TABLE t (a integer) PARTITION BY RANGE (a);
> CREATE TABLE tp (a integer PRIMARY KEY);
> ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1000);
>
> CREATE UNIQUE INDEX t_a_idx ON t (a);
> ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;
>
> \d+ t*
> ALTER TABLE t DETACH PARTITION tp;
>
> TRAP: failed Assert("constrForm->coninhcount == 0"), File:
> "pg_constraint.c", Line: 873, PID: 3272276
> ...
> #5  0x000055b26dc9b76f in ExceptionalCondition (
>     conditionName=conditionName@entry=0x55b26dd2d254
> "constrForm->coninhcount == 0",
>     fileName=fileName@entry=0x55b26dd2d230 "pg_constraint.c",
> lineNumber=lineNumber@entry=873) at assert.c:66

This DDL sequence completely breaks partitioned tree indexes AFAIU.
As reported, the inheritance count is wrong.  Anyway, from what I can
see, I think that the ATTACH PARTITION is wrong and it should issue an
error because this is attempting to attach an index used by a
constraint to a partitioned index which is *not* related to a
constraint in the parent.  I am pretty sure that more chirurgy around
AttachPartitionEnsureIndexes() with lookups at pg_constraint are
required here.

It's a bit disappointing that this would not work to make sure that
the index used by the partitioned table is on a primary key, ensuring
a consistent partition tree for the whole, with all indexes used in
the pk:
=# alter table t add primary key using index t_a_idx ;
ERROR:  0A000: ALTER TABLE / ADD CONSTRAINT USING INDEX is not
supported on partitioned tables
LOCATION:  ATExecAddIndexConstraint, tablecmds.c:9259

This sequence, however, is fine:
CREATE TABLE t (a integer) PARTITION BY RANGE (a);
CREATE TABLE tp (a integer PRIMARY KEY);
ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1000);
alter table t add primary key (a);
ALTER TABLE t DETACH PARTITION tp;

Note that before the DETACH we have this index tree, with a consistent
coninhcount:
=# select * from pg_partition_tree('t_pkey');
  relid  | parentrelid | isleaf | level
---------+-------------+--------+-------
 t_pkey  | null        | f      |     0
 tp_pkey | t_pkey      | t      |     1
(2 rows)
--
Michael

On Wed, Jun 12, 2024 at 01:49:32PM +0800, Tender Wang wrote:
> Hi, all
>
> I found another crash case as below:
> CREATE TABLE t (a integer) PARTITION BY RANGE (a);
> CREATE TABLE tp (a integer PRIMARY KEY);
> CREATE UNIQUE INDEX t_a_idx ON t (a);
> ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1000);
> ALTER TABLE t DETACH PARTITION tp;
>
> If index of parent is not created by adding constraint, it will trigger
> assert fail.

The 4th command is I think incorrect to attach "t_a_idx" to "tp"'s
primary key index because "t" does not have an equivalent entry in
pg_constraint.  So that does not address the actual issue.

> I look through the DetachPartitionFinalize(). The logic of detach indexes
> finds a pg_inherits tuple,
> then it goes to detach the constraint between parent and child. But in
> AttachPartitionEnsureIndexes(),
> it check whether constraint of parent is valid or not. If it is valid, then
> the code will build constraint
> connection between parent and child.
>
> I think we can do this check in DetachPartitionFinalize(). We skip to
> detach the constraint if parent actually
> does not have one. I try to fix theses issue in attached patch.

Are you sure that this is correct?  This still makes "tp_pkey" a
partition of "t_a_idx" while the parent table "t" has no constraint
equivalent to "tp"'s primary key.  It seems to me that the correct
answer in your command sequence is to *not* make "tp_pkey" a partition
of the partitioned index "t_a_idx", and leave it alone.  Attaching
"tp_pkey" as a partition of "t_a_idx" is only OK as long as this index
is used in a constraint equivalent to "tp_pkey".

Having a PK on a partition while the parent does not have one is
covered by a regression test in indexing.sql (see "Constraint-related
indexes" and "primary key on child is okay").
--
Michael

On Sun, 2024-06-09 at 06:00 +0000, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference:      18500
> Logged by:          Alexander Lakhin
> Email address:      exclusion@gmail.com
> PostgreSQL version: 17beta1
> Operating system:   Ubuntu 22.04
> Description:

FYI, I think that the same problem just happened to me with v17beta2:

test=> CREATE TABLE p (id integer) PARTITION BY RANGE (id);
CREATE TABLE
test=> CREATE TABLE p_1 PARTITION OF p FOR VALUES FROM (1) TO (2);
CREATE TABLE
test=> ALTER TABLE p_1 ADD PRIMARY KEY (id);
ALTER TABLE
test=> CREATE UNIQUE INDEX ON p (id);
CREATE INDEX
test=> ALTER TABLE p DETACH PARTITION p_1;
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.

Yours,
Laurenz Albe

On 2024-Jun-27, Daniel Westermann (DWE) wrote:

> Doesn't happen on FreeBSD:

Maybe this one uses Postgrs compiled with assertions disabled (would be
what I expect in a "production" build).

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No deja de ser humillante para una persona de ingenio saber
que no hay tonto que no le pueda enseñar algo." (Jean B. Say)

On 2024-Jun-12, Michael Paquier wrote:

> On Wed, Jun 12, 2024 at 01:49:32PM +0800, Tender Wang wrote:

> > I found another crash case as below:
> > CREATE TABLE t (a integer) PARTITION BY RANGE (a);
> > CREATE TABLE tp (a integer PRIMARY KEY);
> > CREATE UNIQUE INDEX t_a_idx ON t (a);
> > ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1000);
> > ALTER TABLE t DETACH PARTITION tp;
> > 
> > If index of parent is not created by adding constraint, it will trigger
> > assert fail.
> 
> The 4th command is I think incorrect to attach "t_a_idx" to "tp"'s
> primary key index because "t" does not have an equivalent entry in
> pg_constraint.  So that does not address the actual issue.

Actually, the partition index is attached to the parent index already at
CREATE UNIQUE INDEX, so it's the third line that's the problem (but what
you say is true too if the user says CREATE UNIQUE INDEX ON ONLY
parent).

I tend to agree that a good, consistent fix for this would be to forbid
a PK in a partition from becoming a child of a non-constraint index in
the parent ... A few ereport(ERROR)s sprinkled in various places might
be sufficient in the master branch, but perhaps we shouldn't disallow it
in stable branches because that might be problematic for existing apps;
and I suspect that we'd need to install protections in pg_upgrade to fix
databases from earlier branches being upgraded with the given hierarchy
(or just prevent the upgrade in --check and tell the users to fix it.)

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

On Thu, 2024-06-27 at 16:55 +0200, Alvaro Herrera wrote:
> On 2024-Jun-27, Daniel Westermann (DWE) wrote:
>
> > Doesn't happen on FreeBSD:
>
> Maybe this one uses Postgrs compiled with assertions disabled (would be
> what I expect in a "production" build).

Ah, right.  I used Devrim's RPM packages, and I see that they are built with
--enable-cassert.

Yours,
Laurenz Albe

On 2024-Jun-28, Laurenz Albe wrote:

> On Thu, 2024-06-27 at 16:55 +0200, Alvaro Herrera wrote:
> > On 2024-Jun-27, Daniel Westermann (DWE) wrote:
> > 
> > > Doesn't happen on FreeBSD:
> > 
> > Maybe this one uses Postgrs compiled with assertions disabled (would be
> > what I expect in a "production" build).
> 
> Ah, right.  I used Devrim's RPM packages, and I see that they are built with
> --enable-cassert.

Oh yeah, I remember noticing that before and thinking that that was an
absolutely insane choice.  Performance really suffers.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

Hi,

On Fri, 2024-06-28 at 11:17 +0200, Alvaro Herrera wrote:
> > Ah, right.  I used Devrim's RPM packages, and I see that they are
> > built with
> > --enable-cassert.
>
> Oh yeah, I remember noticing that before and thinking that that was an
> absolutely insane choice.  Performance really suffers.

I think it is a very sane choice as we are talking about beta RPMs and
we need that for *testing*. I remove that flag in RC1.

Cheers,
--
Devrim Gündüz
Open Source Solution Architect, PostgreSQL Major Contributor
Twitter: @DevrimGunduz , @DevrimGunduzTR

On 2024-Jun-12, Tender Wang wrote:

> I think what you said above. I feel that we need that "tp_pkey" is a
> partition
> of the partitioned index "t_a_idx".  For example, below statement:
> 
> test=# alter table tp drop constraint tp_pkey;
> ERROR:  cannot drop index tp_pkey because index t_a_idx requires it
> HINT:  You can drop index t_a_idx instead.
> 
> If "tp_pkey" is not a partition of "t_a_idx", the primary key "tp_pkey" can
> be dropped.

I guess you could also fix this by adding a PK constraint to the parent
table, so that the situation is consistent in the other direction.

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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Fri, Jun 28, 2024 at 01:27:17PM +0200, Alvaro Herrera wrote:
> I guess you could also fix this by adding a PK constraint to the parent
> table, so that the situation is consistent in the other direction.

Yes, I've also considered this option when thinking about this thread,
but forcing a constraint creation up to the parent felt non-intuitive
to me as we force creation of catalog entries on the children based on
the state of the parent for basically everything else.  So introducing
a new path where we would do the reverse is a recipe for more complex
code in tablecmds.c, which I'd rather avoid.

> 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.

An error sounds saner here in the long term.

Tests for all of the code paths involved, perhaps?  ;)
--
Michael

On Sat, 2024-06-29 at 09:15 +0900, Michael Paquier 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.
>
> An error sounds saner here in the long term.
>
> Tests for all of the code paths involved, perhaps?  ;)

My example that triggered this assert runs just fine on v16.

So while an error is clearly better than a crash, that would constitute
a regression.  Is that really unavoidable?  It would be very unfortunate
if the only way to detach a partition would be to drop some indexes first...

Yours,
Laurenz Albe

On 2024-Jun-29, Laurenz Albe wrote:

> My example that triggered this assert runs just fine on v16.

Well, in a build without assertions enabled then yes it doesn't crash.
But if you do have asserts enabled in 16, it does crash.

> So while an error is clearly better than a crash, that would constitute
> a regression.  Is that really unavoidable?  It would be very unfortunate
> if the only way to detach a partition would be to drop some indexes first...

The error would not occur on detach, but on attach, and it'd be intended
to prevent an inconsistent situation.  I'm proposing that on older
branches we do what Tender proposed elsewhere, namely to cope with the
detach without crashing (and without leaving inconsistent catalog state,
such as bogus coninhcount values).

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pero la cosa no es muy grave ..."     (le petit Nicolas -- René Goscinny)

On Sat, 2024-06-29 at 19:56 +0200, Alvaro Herrera wrote:
> On 2024-Jun-29, Laurenz Albe wrote:
>
> > My example that triggered this assert runs just fine on v16.
>
> Well, in a build without assertions enabled then yes it doesn't crash.
> But if you do have asserts enabled in 16, it does crash.
>
> > So while an error is clearly better than a crash, that would constitute
> > a regression.  Is that really unavoidable?  It would be very unfortunate
> > if the only way to detach a partition would be to drop some indexes first...
>
> The error would not occur on detach, but on attach, and it'd be intended
> to prevent an inconsistent situation.  I'm proposing that on older
> branches we do what Tender proposed elsewhere, namely to cope with the
> detach without crashing (and without leaving inconsistent catalog state,
> such as bogus coninhcount values).


I should have read more closely, sorry.  That's great then; sorry for the noise.

Yours,
Laurenz Albe

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/

Doh, somehow I forgot the patches once again.  Here they are.  I intend
to push 0001 to all relevant branches soon, then park the rest until we
have ALTER TABLE ... ADD PRIMARY KEY USING INDEX.

On 2024-Jul-03, Alvaro Herrera wrote:

> 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).

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

On 2024-Jul-05, Tender Wang wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年7月4日周四 20:17写道：
> 
> > Doh, somehow I forgot the patches once again.  Here they are.  I intend
> > to push 0001 to all relevant branches soon, then park the rest until we
> > have ALTER TABLE ... ADD PRIMARY KEY USING INDEX.
> 
> The 0001 looks good to me.

Thank you, I have pushed it with some comment rewording and changes to
the tests.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/