Self FK oddity when attaching a partition

Hi all,

I've been able to work on this issue and isolate where in the code the oddity
is laying.

During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for existing
required index on the partition to attach. It creates missing index, or sets the
parent's index when a matching one exists on the partition. Good.

When a matching index is found, if the parent index enforce a constraint, the
function look for the similar constraint in the partition-to-be, and set the
constraint parent as well:

constraintOid = get_relation_idx_constraint_oid(RelationGetRelid(rel),
idx);

[...]

/*
* If this index is being created in the parent because of a
* constraint, then the child needs to have a constraint also,
* so look for one. If there is no such constraint, this
* index is no good, so keep looking.
*/
if (OidIsValid(constraintOid))
{
cldConstrOid = get_relation_idx_constraint_oid(
RelationGetRelid(attachrel),
cldIdxId);
/* no dice */
if (!OidIsValid(cldConstrOid))
continue;
}
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrOid, constraintOid,
RelationGetRelid(attachrel));

However, it seems get_relation_idx_constraint_oid(), introduced in eb7ed3f3063,
assume there could be only ONE constraint depending to an index. But in fact,
multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending on an
index for the given relation, either the FK or a PK can appears first depending
on various conditions. It is then possible to trick it make a FK constraint a
parent of a PK...

In the following little scenario, when looking at the constraint linked to
the PK unique index using the same index than get_relation_idx_constraint_oid
use, this is the self-FK that is actually returned first by
get_relation_idx_constraint_oid(), NOT the PK:

postgres=# DROP TABLE IF EXISTS parent, child1;

CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);

CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);

-- force an indexscan as get_relation_idx_constraint_oid() use the unique
-- index on (conrelid, contypid, conname) to scan pg_cosntraint
set enable_seqscan TO off;
set enable_bitmapscan TO off;

SELECT conname
FROM pg_constraint
WHERE conrelid = 'parent'::regclass <=== parent
AND conindid = 'parent_pkey'::regclass; <=== PK index

DROP TABLE
CREATE TABLE
CREATE TABLE
SET
SET
conname
----------------------------
parent_id_abc_no_part_fkey <==== WOOPS!
parent_pkey
(2 rows)

In consequence, when attaching the partition, the PK of child1 is not marked as
partition of the parent's PK, which is wrong. WORST, the PK of child1 is
actually unexpectedly marked as a partition of the parent's **self-FK**:

postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
FOR VALUES IN ('1');

SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;

ALTER TABLE
oid | conname | conparentid | conrelid | confrelid
-------+-----------------------------+-------------+----------+-----------
16700 | parent_pkey | 0 | 16695 | 0
16701 | parent_id_abc_no_part_fkey | 0 | 16695 | 16695
16706 | child1 | 0 | 16702 | 0
16708 | **child1_pkey** | **16701** | 16702 | 0
16709 | parent_id_abc_no_part_fkey1 | 16701 | 16695 | 16702
16712 | parent_id_abc_no_part_fkey | 16701 | 16702 | 16695
(6 rows)

The expected result should probably be something like:

oid | conname | conparentid | conrelid | confrelid
-------+-----------------------------+-------------+----------+-----------
16700 | parent_pkey | 0 | 16695 | 0
...
16708 | child1_pkey | 16700 | 16702 | 0

I suppose this bug might exists in ATExecAttachPartitionIdx(),
DetachPartitionFinalize() and DefineIndex() where there's similar code and logic
using get_relation_idx_constraint_oid(). I didn't check for potential bugs there
though.

I'm not sure yet of how this bug should be fixed. Any comment?

Regards,

On Tue, Aug 23, 2022 at 8:07 AM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
wrote:

Hi all,

I've been able to work on this issue and isolate where in the code the
oddity
is laying.

During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for
existing
required index on the partition to attach. It creates missing index, or
sets the
parent's index when a matching one exists on the partition. Good.

When a matching index is found, if the parent index enforce a constraint,
the
function look for the similar constraint in the partition-to-be, and set
the
constraint parent as well:

constraintOid =
get_relation_idx_constraint_oid(RelationGetRelid(rel),
idx);

[...]

/*
* If this index is being created in the parent because of a
* constraint, then the child needs to have a constraint also,
* so look for one. If there is no such constraint, this
* index is no good, so keep looking.
*/
if (OidIsValid(constraintOid))
{
cldConstrOid = get_relation_idx_constraint_oid(
RelationGetRelid(attachrel),
cldIdxId);
/* no dice */
if (!OidIsValid(cldConstrOid))
continue;
}
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrOid, constraintOid,
RelationGetRelid(attachrel));

However, it seems get_relation_idx_constraint_oid(), introduced in
eb7ed3f3063,
assume there could be only ONE constraint depending to an index. But in
fact,
multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending on
an
index for the given relation, either the FK or a PK can appears first
depending
on various conditions. It is then possible to trick it make a FK
constraint a
parent of a PK...

In the following little scenario, when looking at the constraint linked to
the PK unique index using the same index than
get_relation_idx_constraint_oid
use, this is the self-FK that is actually returned first by
get_relation_idx_constraint_oid(), NOT the PK:

postgres=# DROP TABLE IF EXISTS parent, child1;

CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);

CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);

-- force an indexscan as get_relation_idx_constraint_oid() use the
unique
-- index on (conrelid, contypid, conname) to scan pg_cosntraint
set enable_seqscan TO off;
set enable_bitmapscan TO off;

SELECT conname
FROM pg_constraint
WHERE conrelid = 'parent'::regclass <=== parent
AND conindid = 'parent_pkey'::regclass; <=== PK index

DROP TABLE
CREATE TABLE
CREATE TABLE
SET
SET
conname
----------------------------
parent_id_abc_no_part_fkey <==== WOOPS!
parent_pkey
(2 rows)

In consequence, when attaching the partition, the PK of child1 is not
marked as
partition of the parent's PK, which is wrong. WORST, the PK of child1 is
actually unexpectedly marked as a partition of the parent's **self-FK**:

postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
FOR VALUES IN ('1');

SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;

ALTER TABLE
oid | conname | conparentid | conrelid |
confrelid

-------+-----------------------------+-------------+----------+-----------
16700 | parent_pkey | 0 | 16695 | 0
16701 | parent_id_abc_no_part_fkey | 0 | 16695 | 16695
16706 | child1 | 0 | 16702 | 0
16708 | **child1_pkey** | **16701** | 16702 | 0
16709 | parent_id_abc_no_part_fkey1 | 16701 | 16695 | 16702
16712 | parent_id_abc_no_part_fkey | 16701 | 16702 | 16695
(6 rows)

The expected result should probably be something like:

oid | conname | conparentid | conrelid |
confrelid

-------+-----------------------------+-------------+----------+-----------
16700 | parent_pkey | 0 | 16695 | 0
...
16708 | child1_pkey | 16700 | 16702 | 0

I suppose this bug might exists in ATExecAttachPartitionIdx(),
DetachPartitionFinalize() and DefineIndex() where there's similar code and
logic
using get_relation_idx_constraint_oid(). I didn't check for potential bugs
there
though.

I'm not sure yet of how this bug should be fixed. Any comment?

Regards,

Hi,

In this case the confrelid field of FormData_pg_constraint for the first
constraint would carry a valid Oid.
Can we use this information and continue searching in
get_relation_idx_constraint_oid() until an entry with 0 confrelid is found ?
If there is no such (secondary) entry, we return the first entry.

Cheers

On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:

Hi,

[...]

However, it seems get_relation_idx_constraint_oid(), introduced in eb7ed3f3063,
assume there could be only ONE constraint depending to an index. But in fact,
multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending on an
index for the given relation, either the FK or a PK can appears first depending
on various conditions. It is then possible to trick it make a FK constraint a
parent of a PK...

Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
be to have get_relation_idx_constraint_oid ignore any constraints that
are not unique or primary keys. I tried your scenario with the attached
and it seems to work correctly. Can you confirm? (I only ran the
pg_regress tests, not anything else for now.)

If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.

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

On Tue, Aug 23, 2022 at 9:30 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:

Hi,

[...]

However, it seems get_relation_idx_constraint_oid(), introduced in

eb7ed3f3063,

assume there could be only ONE constraint depending to an index. But in

fact,

multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending

on an

index for the given relation, either the FK or a PK can appears first

depending

on various conditions. It is then possible to trick it make a FK

constraint a

parent of a PK...

Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
be to have get_relation_idx_constraint_oid ignore any constraints that
are not unique or primary keys. I tried your scenario with the attached
and it seems to work correctly. Can you confirm? (I only ran the
pg_regress tests, not anything else for now.)

If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.

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

I was thinking of the following patch.
Basically, if there is only one matching constraint. we still return it.

diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
b/src/postgres/src/backend/catalog/pg_constraint.c
index f0726e9aa0..ddade138b4 100644
--- a/src/postgres/src/backend/catalog/pg_constraint.c
+++ b/src/postgres/src/backend/catalog/pg_constraint.c
@@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
indexId)
  constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
  if (constrForm->conindid == indexId)
  {
- constraintId = HeapTupleGetOid(tuple);
+ if (constraintId == InvalidOid || constrForm->confrelid == 0)
+ constraintId = HeapTupleGetOid(tuple);
  break;
  }
  }

On 2022-Aug-23, Zhihong Yu wrote:

I was thinking of the following patch.
Basically, if there is only one matching constraint. we still return it.

diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
b/src/postgres/src/backend/catalog/pg_constraint.c
index f0726e9aa0..ddade138b4 100644
--- a/src/postgres/src/backend/catalog/pg_constraint.c
+++ b/src/postgres/src/backend/catalog/pg_constraint.c
@@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
indexId)
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
if (constrForm->conindid == indexId)
{
- constraintId = HeapTupleGetOid(tuple);
+ if (constraintId == InvalidOid || constrForm->confrelid == 0)
+ constraintId = HeapTupleGetOid(tuple);
break;
}
}

We could do this, but what do we gain by doing so? It seems to me that
my proposed formulation achieves the same and is less fuzzy about what
the returned constraint is. Please try to write a code comment that
explains what this does and see if it makes sense.

For my proposal, it would be "return the OID of a primary key or unique
constraint associated with the given index in the given relation, or OID
if no such index is catalogued". This definition is clearly useful for
partitioned tables, on which the unique and primary key constraints are
useful elements. There's nothing that cares about foreign keys.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

On Tue, Aug 23, 2022 at 9:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2022-Aug-23, Zhihong Yu wrote:

I was thinking of the following patch.
Basically, if there is only one matching constraint. we still return it.

diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
b/src/postgres/src/backend/catalog/pg_constraint.c
index f0726e9aa0..ddade138b4 100644
--- a/src/postgres/src/backend/catalog/pg_constraint.c
+++ b/src/postgres/src/backend/catalog/pg_constraint.c
@@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
indexId)
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
if (constrForm->conindid == indexId)
{
- constraintId = HeapTupleGetOid(tuple);
+ if (constraintId == InvalidOid || constrForm->confrelid == 0)
+ constraintId = HeapTupleGetOid(tuple);
break;
}
}

We could do this, but what do we gain by doing so? It seems to me that
my proposed formulation achieves the same and is less fuzzy about what
the returned constraint is. Please try to write a code comment that
explains what this does and see if it makes sense.

For my proposal, it would be "return the OID of a primary key or unique
constraint associated with the given index in the given relation, or OID
if no such index is catalogued". This definition is clearly useful for
partitioned tables, on which the unique and primary key constraints are
useful elements. There's nothing that cares about foreign keys.

--
Álvaro Herrera PostgreSQL Developer —
https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

A bigger question I have, even with the additional filtering, is what if
there are multiple constraints ?
How do we decide which unique / primary key constraint to return ?

Looks like there is no known SQL statements leading to such state, but
should we consider such possibility ?

Cheers

On 2022-Aug-23, Zhihong Yu wrote:

A bigger question I have, even with the additional filtering, is what if
there are multiple constraints ?
How do we decide which unique / primary key constraint to return ?

Looks like there is no known SQL statements leading to such state, but
should we consider such possibility ?

I don't think we care, but feel free to experiment and report any
problems. You should be able to have multiple UNIQUE constraints on the
same column, for example.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.

Yeah. See lsyscache.c's get_constraint_index(), as well as commit
641f3dffc which fixed a mighty similar-seeming bug. One question
that precedent raises is whether to also include CONSTRAINT_EXCLUSION.
But in any case a positive test for the constraint types to allow
seems best.

regards, tom lane

On Tue, 23 Aug 2022 18:30:06 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:

Hi,

[...]

However, it seems get_relation_idx_constraint_oid(), introduced in
eb7ed3f3063, assume there could be only ONE constraint depending to an
index. But in fact, multiple constraints can rely on the same index, eg.:
the PK and a self referencing FK. In consequence, when looking for a
constraint depending on an index for the given relation, either the FK or a
PK can appears first depending on various conditions. It is then possible
to trick it make a FK constraint a parent of a PK...

Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
be to have get_relation_idx_constraint_oid ignore any constraints that
are not unique or primary keys. I tried your scenario with the attached
and it seems to work correctly. Can you confirm? (I only ran the
pg_regress tests, not anything else for now.)

If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.

I was naively wondering about such a patch, but was worrying about potential
side effects on ATExecAttachPartitionIdx(), DetachPartitionFinalize() and
DefineIndex() where I didn't had a single glance. Did you had a look?

I did a quick ATTACH + DETACH test, and it seems DETACH partly fails with its
housecleaning:

DROP TABLE IF EXISTS parent, child1;

CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);

CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);

\C 'Before ATTACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;

ALTER TABLE parent ATTACH PARTITION child1 FOR VALUES IN ('1');

\C 'After ATTACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;

ALTER TABLE parent DETACH PARTITION child1;

\C 'After DETACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;

Before ATTACH
oid | conname | conparentid | conrelid | confrelid
-------+----------------------------+-------------+----------+-----------
24711 | parent_pkey | 0 | 24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 | 24706 | 24706
24721 | child1 | 0 | 24717 | 0
24723 | child1_pkey | 0 | 24717 | 0
(4 rows)

After ATTACH
oid | conname | conparentid | conrelid | confrelid
-------+-----------------------------+-------------+----------+-----------
24711 | parent_pkey | 0 | 24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 | 24706 | 24706
24721 | child1 | 0 | 24717 | 0
24723 | child1_pkey | 24711 | 24717 | 0
24724 | parent_id_abc_no_part_fkey1 | 24712 | 24706 | 24717
24727 | parent_id_abc_no_part_fkey | 24712 | 24717 | 24706
(6 rows)

After DETACH
oid | conname | conparentid | conrelid | confrelid
-------+----------------------------+-------------+----------+-----------
24711 | parent_pkey | 0 | 24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 | 24706 | 24706
24721 | child1 | 0 | 24717 | 0
24723 | child1_pkey | 0 | 24717 | 0
24727 | parent_id_abc_no_part_fkey | 0 | 24717 | 24706
(5 rows)

Looking for few minutes in ATExecDetachPartitionFinalize(), it seems it only
support removing the parental link on FK, not to clean the FKs added during the
ATTACH DDL anyway. That explains the FK child1->parent left behind. But in
fact, this let me wonder if this part of the code ever considered implication
of self-FK during the ATTACH and DETACH process? Why in the first place TWO FK
are created during the ATTACH DDL?

Regards,

On 2022-Aug-24, Jehan-Guillaume de Rorthais wrote:

I was naively wondering about such a patch, but was worrying about potential
side effects on ATExecAttachPartitionIdx(), DetachPartitionFinalize() and
DefineIndex() where I didn't had a single glance. Did you had a look?

No. But AFAIR all the code there is supposed to worry about unique
constraints and PK only, not FKs. So if something changes, then most
likely it was wrong to begin with.

I did a quick ATTACH + DETACH test, and it seems DETACH partly fails with its
housecleaning:

Ugh. More fixes required, then.

Looking for few minutes in ATExecDetachPartitionFinalize(), it seems it only
support removing the parental link on FK, not to clean the FKs added during the
ATTACH DDL anyway. That explains the FK child1->parent left behind. But in
fact, this let me wonder if this part of the code ever considered implication
of self-FK during the ATTACH and DETACH process?

No, or at least I don't remember thinking about self-referencing FKs.
If there are no tests for it, then that's likely what happened.

Why in the first place TWO FK are created during the ATTACH DDL?

That's probably a bug too.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Hi,

While studying and hacking on the parenting constraint issue, I found an
incoherent piece of code leading to badly chosen fk name. If a constraint
name collision is detected, while choosing a new name for the constraint,
the code uses fkconstraint->fk_attrs which is not yet populated:

/* No dice. Set up to create our own constraint */
fkconstraint = makeNode(Constraint);
if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
RelationGetRelid(partRel),
NameStr(constrForm->conname)))
fkconstraint->conname =
ChooseConstraintName(RelationGetRelationName(partRel),
ChooseForeignKeyConstraintNameAddition(
fkconstraint->fk_attrs), // <= WOO000OPS
"fkey",
RelationGetNamespace(partRel), NIL);
else
fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
fkconstraint->fk_upd_action = constrForm->confupdtype;
fkconstraint->fk_del_action = constrForm->confdeltype;
fkconstraint->deferrable = constrForm->condeferrable;
fkconstraint->initdeferred = constrForm->condeferred;
fkconstraint->fk_matchtype = constrForm->confmatchtype;
for (int i = 0; i < numfks; i++)
{
Form_pg_attribute att;

att = TupleDescAttr(RelationGetDescr(partRel),
mapped_conkey[i] - 1);
fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs, // <= POPULATING
makeString(NameStr(att->attname)));
}

The following SQL script showcase the bad constraint name:

DROP TABLE IF EXISTS parent, child1;

CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
CONSTRAINT dummy_constr FOREIGN KEY (id_abc, no_part)
REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);

CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT dummy_constr CHECK ((no_part = 1))
);

ALTER TABLE parent ATTACH PARTITION child1 FOR VALUES IN ('1');

SELECT conname
FROM pg_constraint
WHERE conrelid = 'child1'::regclass
AND contype = 'f';

DROP TABLE
CREATE TABLE
CREATE TABLE
ALTER TABLE

conname
--------------
child1__fkey
(1 row)

The resulting constraint name "child1__fkey" is missing the attributes name the
original code wanted to add. The expected name is "child1_id_abc_no_part_fkey".

Find in attachment a simple fix, moving the name assignation after the
FK attributes are populated.

Regards,

Hi there,

I believe this very small bug and its fix are really trivial and could be push
out of the way quite quickly. It's just about a bad constraint name fixed by
moving one assignation after the next one. This could easily be fixed for next
round of releases.

Well, I hope I'm not wrong :)

Regards,

On Thu, 1 Sep 2022 18:41:56 +0200
Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote:

On 2022-Sep-08, Jehan-Guillaume de Rorthais wrote:

Hi there,

I believe this very small bug and its fix are really trivial and could be push
out of the way quite quickly. It's just about a bad constraint name fixed by
moving one assignation after the next one. This could easily be fixed for next
round of releases.

Well, I hope I'm not wrong :)

I think you're right, so pushed, and backpatched to 12. I added the
test case to regression also.

For 11, I adjusted the test case so that it didn't depend on an FK
pointing to a partitioned table (which is not supported there); it turns
out that the old code is not smart enough to get into the problem in the
first place. Setup is

CREATE TABLE parted_fk_naming_pk (id bigint primary key);
CREATE TABLE parted_fk_naming (
id_abc bigint,
CONSTRAINT dummy_constr FOREIGN KEY (id_abc)
REFERENCES parted_fk_naming_pk (id)
)
PARTITION BY LIST (id_abc);
CREATE TABLE parted_fk_naming_1 (
id_abc bigint,
CONSTRAINT dummy_constr CHECK (true)
);

and then
ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR VALUES IN ('1');
throws this error:

ERROR: duplicate key value violates unique constraint "pg_constraint_conrelid_contypid_conname_index"
DETALLE: Key (conrelid, contypid, conname)=(686125, 0, dummy_constr) already exists.

It seems fair to say that this case, with pg11, is unsupported and
people should upgrade if they want better behavior.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&amp;cid=4647152)

On Thu, 8 Sep 2022 13:25:15 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Sep-08, Jehan-Guillaume de Rorthais wrote:

Hi there,

I believe this very small bug and its fix are really trivial and could be
push out of the way quite quickly. It's just about a bad constraint name
fixed by moving one assignation after the next one. This could easily be
fixed for next round of releases.

Well, I hope I'm not wrong :)

I think you're right, so pushed, and backpatched to 12. I added the
test case to regression also.

Great, thank you for the additional work on the regression test and the commit!

For 11, I adjusted the test case so that it didn't depend on an FK
pointing to a partitioned table (which is not supported there); it turns
out that the old code is not smart enough to get into the problem in the
first place. [...]
It seems fair to say that this case, with pg11, is unsupported and
people should upgrade if they want better behavior.

That works for me.

Thanks!

Hi,

On 2022-09-08 13:25:15 +0200, Alvaro Herrera wrote:

I think you're right, so pushed, and backpatched to 12. I added the
test case to regression also.

Something here doesn't look to be quite right. Starting with this commit CI
[1]: https://cirrus-ci.com/github/postgres/postgres/
also seen the crash on windows (CI run[3]https://cirrus-ci.com/task/6629440791773184, stack trace [4]https://api.cirrus-ci.com/v1/artifact/task/6629440791773184/crashlog/crashlog-postgres.exe_1468_2022-09-08_17-05-24-591.txt).

There does appear to be some probabilistic aspect, I also saw a run succeed.

Greetings,

Andres Freund

[1]: https://cirrus-ci.com/github/postgres/postgres/
[2]: https://api.cirrus-ci.com/v1/task/6180840047640576/logs/cores.log
[3]: https://cirrus-ci.com/task/6629440791773184
[4]: https://api.cirrus-ci.com/v1/artifact/task/6629440791773184/crashlog/crashlog-postgres.exe_1468_2022-09-08_17-05-24-591.txt

Andres Freund <andres@anarazel.de> writes:

Something here doesn't look to be quite right. Starting with this commit CI
[1] started to fail on freebsd (stack trace [2]), and in the meson branch I've
also seen the crash on windows (CI run[3], stack trace [4]).

The crash seems 100% reproducible if I remove the early-exit optimization
from GetForeignKeyActionTriggers:

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 53b0f3a9c1..112ca77d97 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10591,8 +10591,6 @@ GetForeignKeyActionTriggers(Relation trigrel,
            Assert(*updateTriggerOid == InvalidOid);
            *updateTriggerOid = trgform->oid;
        }
-       if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid))
-           break;
    }

if (!OidIsValid(*deleteTriggerOid))

With that in place, it's probabilistic whether the Asserts notice anything
wrong, and mostly they don't. But there are multiple matching triggers:

regression=# select oid, tgconstraint, tgrelid,tgconstrrelid, tgtype, tgname from pg_trigger where tgconstraint = 104301;
oid | tgconstraint | tgrelid | tgconstrrelid | tgtype | tgname
--------+--------------+---------+---------------+--------+-------------------------------
104302 | 104301 | 104294 | 104294 | 9 | RI_ConstraintTrigger_a_104302
104303 | 104301 | 104294 | 104294 | 17 | RI_ConstraintTrigger_a_104303
104304 | 104301 | 104294 | 104294 | 5 | RI_ConstraintTrigger_c_104304
104305 | 104301 | 104294 | 104294 | 17 | RI_ConstraintTrigger_c_104305
(4 rows)

I suspect that the filter conditions being applied are inadequate
for the case of a self-referential FK, which this evidently is
given that tgrelid and tgconstrrelid are equal.

I'd counsel dropping the early-exit optimization; it doesn't
save much I expect, and it evidently hides bugs. Or maybe
make it conditional on !USE_ASSERT_CHECKING.

regards, tom lane

On Fri, Sep 9, 2022 at 4:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Something here doesn't look to be quite right. Starting with this commit CI
[1] started to fail on freebsd (stack trace [2]), and in the meson branch I've
also seen the crash on windows (CI run[3], stack trace [4]).

The crash seems 100% reproducible if I remove the early-exit optimization
from GetForeignKeyActionTriggers:

Indeed, reproduced here.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 53b0f3a9c1..112ca77d97 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10591,8 +10591,6 @@ GetForeignKeyActionTriggers(Relation trigrel,
Assert(*updateTriggerOid == InvalidOid);
*updateTriggerOid = trgform->oid;
}
-       if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid))
-           break;
}

if (!OidIsValid(*deleteTriggerOid))

With that in place, it's probabilistic whether the Asserts notice anything
wrong, and mostly they don't. But there are multiple matching triggers:

regression=# select oid, tgconstraint, tgrelid,tgconstrrelid, tgtype, tgname from pg_trigger where tgconstraint = 104301;
oid | tgconstraint | tgrelid | tgconstrrelid | tgtype | tgname
--------+--------------+---------+---------------+--------+-------------------------------
104302 | 104301 | 104294 | 104294 | 9 | RI_ConstraintTrigger_a_104302
104303 | 104301 | 104294 | 104294 | 17 | RI_ConstraintTrigger_a_104303
104304 | 104301 | 104294 | 104294 | 5 | RI_ConstraintTrigger_c_104304
104305 | 104301 | 104294 | 104294 | 17 | RI_ConstraintTrigger_c_104305
(4 rows)

I suspect that the filter conditions being applied are inadequate
for the case of a self-referential FK, which this evidently is
given that tgrelid and tgconstrrelid are equal.

Yes, the loop in GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "action" triggers on the PK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_PK)
+           continue;

Likewise, GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "check" triggers on the FK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_FK)
+           continue;

We evidently missed this in f4566345cf40b0.

I'd counsel dropping the early-exit optimization; it doesn't
save much I expect, and it evidently hides bugs. Or maybe
make it conditional on !USE_ASSERT_CHECKING.

While neither of these functions are called in hot paths, I am
inclined to keep the early-exit bit in non-assert builds.

Attached a patch.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Sep-09, Amit Langote wrote:

Yes, the loop in GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "action" triggers on the PK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_PK)
+           continue;

Likewise, GetForeignKeyActionTriggers() needs this:

+       /* Only ever look at "check" triggers on the FK side. */
+       if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_FK)
+           continue;

We evidently missed this in f4566345cf40b0.

Ouch. Thank you, pushed.

On Fri, Sep 9, 2022 at 4:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd counsel dropping the early-exit optimization; it doesn't
save much I expect, and it evidently hides bugs. Or maybe
make it conditional on !USE_ASSERT_CHECKING.

While neither of these functions are called in hot paths, I am
inclined to keep the early-exit bit in non-assert builds.

I kept it that way.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

Hi,

Please, find in attachment a small serie of patch:

0001 fix the constraint parenting bug. Not much to say. It's basically your
patch we discussed with some more comments and the check on contype equals to
either primary, unique or exclusion.

0002 fix the self-FK being cloned twice on partitions

0003 add a regression test validating both fix.

I should confess than even with these fix, I'm still wondering about this code
sanity as we could still end up with a PK on a partition being parented with a
simple unique constraint from the table, on a field not even NOT NULL:

DROP TABLE IF EXISTS parted_self_fk, part_with_pk;

CREATE TABLE parted_self_fk (
id bigint,
id_abc bigint,
FOREIGN KEY (id_abc) REFERENCES parted_self_fk(id),
UNIQUE (id)
)
PARTITION BY RANGE (id);

CREATE TABLE part_with_pk (
id bigint PRIMARY KEY,
id_abc bigint,
CHECK ((id >= 0 AND id < 10))
);

ALTER TABLE parted_self_fk ATTACH
PARTITION part_with_pk FOR VALUES FROM (0) TO (10);

SELECT cr.relname, co.conname, co.contype, p.conname AS conparentrelname
FROM pg_catalog.pg_constraint co
JOIN pg_catalog.pg_class cr ON cr.oid = co.conrelid
LEFT JOIN pg_catalog.pg_constraint p ON p.oid = co.conparentid
WHERE cr.relname IN ('parted_self_fk', 'part_with_pk')
AND co.contype IN ('u', 'p');

DROP TABLE parted_self_fk;

DROP TABLE
CREATE TABLE
CREATE TABLE
ALTER TABLE
relname | conname | contype | conparentrelname
----------------+-----------------------+---------+-----------------------
parted_self_fk | parted_self_fk_id_key | u |
part_with_pk | part_with_pk_pkey | p | parted_self_fk_id_key
(2 rows)

Nothing forbid the partition to have stricter constraints than the parent
table, but it feels weird, so it might worth noting it here.

I wonder if AttachPartitionEnsureConstraints() should exists and take care of
comparing/cloning constraints before calling AttachPartitionEnsureIndexes()
which would handle missing index without paying attention to related
constraints?

Regards,

On Wed, 24 Aug 2022 12:49:13 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
wrote:

Hi,

Please, find in attachment a small serie of patch:

0001 fix the constraint parenting bug. Not much to say. It's basically
your
patch we discussed with some more comments and the check on contype
equals to
either primary, unique or exclusion.

0002 fix the self-FK being cloned twice on partitions

0003 add a regression test validating both fix.

I should confess than even with these fix, I'm still wondering about this
code
sanity as we could still end up with a PK on a partition being parented
with a
simple unique constraint from the table, on a field not even NOT NULL:

DROP TABLE IF EXISTS parted_self_fk, part_with_pk;

CREATE TABLE parted_self_fk (
id bigint,
id_abc bigint,
FOREIGN KEY (id_abc) REFERENCES parted_self_fk(id),
UNIQUE (id)
)
PARTITION BY RANGE (id);

CREATE TABLE part_with_pk (
id bigint PRIMARY KEY,
id_abc bigint,
CHECK ((id >= 0 AND id < 10))
);

ALTER TABLE parted_self_fk ATTACH
PARTITION part_with_pk FOR VALUES FROM (0) TO (10);

SELECT cr.relname, co.conname, co.contype, p.conname AS conparentrelname
FROM pg_catalog.pg_constraint co
JOIN pg_catalog.pg_class cr ON cr.oid = co.conrelid
LEFT JOIN pg_catalog.pg_constraint p ON p.oid = co.conparentid
WHERE cr.relname IN ('parted_self_fk', 'part_with_pk')
AND co.contype IN ('u', 'p');

DROP TABLE parted_self_fk;

DROP TABLE
CREATE TABLE
CREATE TABLE
ALTER TABLE
relname | conname | contype | conparentrelname

----------------+-----------------------+---------+-----------------------
parted_self_fk | parted_self_fk_id_key | u |
part_with_pk | part_with_pk_pkey | p | parted_self_fk_id_key
(2 rows)

Nothing forbid the partition to have stricter constraints than the parent
table, but it feels weird, so it might worth noting it here.

I wonder if AttachPartitionEnsureConstraints() should exists and take care
of
comparing/cloning constraints before calling AttachPartitionEnsureIndexes()
which would handle missing index without paying attention to related
constraints?

Regards,

On Wed, 24 Aug 2022 12:49:13 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Aug-24, Jehan-Guillaume de Rorthais wrote:

I was naively wondering about such a patch, but was worrying about

potential

side effects on ATExecAttachPartitionIdx(), DetachPartitionFinalize()

and

DefineIndex() where I didn't had a single glance. Did you had a look?

No. But AFAIR all the code there is supposed to worry about unique
constraints and PK only, not FKs. So if something changes, then most
likely it was wrong to begin with.

I did a quick ATTACH + DETACH test, and it seems DETACH partly fails

with

its housecleaning:

Ugh. More fixes required, then.

Looking for few minutes in ATExecDetachPartitionFinalize(), it seems

it only

support removing the parental link on FK, not to clean the FKs added

during

the ATTACH DDL anyway. That explains the FK child1->parent left

behind. But

in fact, this let me wonder if this part of the code ever considered
implication of self-FK during the ATTACH and DETACH process?

No, or at least I don't remember thinking about self-referencing FKs.
If there are no tests for it, then that's likely what happened.

Why in the first place TWO FK are created during the ATTACH DDL?

That's probably a bug too.

Hi,

+ * Self-Foreign keys are ignored as the index was preliminary
created

preliminary created -> primarily created

Cheers

On Fri, 30 Sep 2022 16:11:09 -0700
Zhihong Yu <zyu@yugabyte.com> wrote:

On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
wrote:

...

+ * Self-Foreign keys are ignored as the index was preliminary
created

preliminary created -> primarily created

Thank you! This is fixed and rebased on current master branch in patches
attached.

Regards,

On 2022-Oct-03, Jehan-Guillaume de Rorthais wrote:

Thank you! This is fixed and rebased on current master branch in patches
attached.

Thanks. As far as I can see this fixes the bugs that were reported.
I've been giving the patches a look and it caused me to notice two
additional bugs in the same area:

- FKs in partitions are sometimes marked NOT VALID. This is because of
missing initialization when faking up a Constraint node in
CloneFkReferencing. Easy to fix, have patch, running tests now.

- The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not
correctly propagated. This should be an easy fix also, haven't tried,
need to add a test case.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

Backpatching this to 12 shows yet another problem -- the topmost
relation acquires additional FK constraints, not yet sure why. I think
we must have fixed something in 13 that wasn't backpatched, but I can't
remember what it is and whether it was intentionally not backpatched.

Looking ...

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I can see support will not be a problem. 10 out of 10." (Simon Wittber)
(http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)

On 2022-Oct-05, Alvaro Herrera wrote:

Backpatching this to 12 shows yet another problem -- the topmost
relation acquires additional FK constraints, not yet sure why. I think
we must have fixed something in 13 that wasn't backpatched, but I can't
remember what it is and whether it was intentionally not backpatched.

This was actually a mismerge. Once I fixed that, it worked properly.

However, there was another bug, which only showed up when I did a
DETACH, ATTACH, and repeat. The problem is that when we detach, the
no-longer-partition retains an FK constraint to the partitioned table.
This is good -- we want that one -- but when we reattach, then we see
that the partitioned table is being referenced from outside, so we
consider that another constraint that we need to add the partition to,
*in addition to the constraint that we need to clone*. So we need to
ignore both a self-referencing FK that goes to the partitioned table, as
well as a self-referencing one that comes from the partition-to-be.
When we do that, then the clone correctly uses that one as the
constraint to retain and attach into the hierarchy of constraints, and
everything [appears to] work correctly.

So I've pushed this, and things are now mostly good. Two problems
remain, though I don't think either of them is terribly serious:

1. one of the constraints in the final hierarchy is marked as not
validated. I mentioned this before.

2. (only in 15) There are useless pg_depend rows for the pg_trigger
rows, which make them depend on their parent pg_trigger rows. This is
not related to self-referencing foreign keys, but I just happened to
notice because I was examining the catalog contents with the added test
case. I think this breakage is due to f4566345cf40. I couldn't find
any actual misbehavior caused by these extra pg_depend entries, but we
should not be creating them anyway.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Por suerte hoy explotó el califont porque si no me habría muerto
de aburrido" (Papelucho)

On 2022-Oct-05, Alvaro Herrera wrote:

I've been giving the patches a look and it caused me to notice two
additional bugs in the same area:

- FKs in partitions are sometimes marked NOT VALID. This is because of
missing initialization when faking up a Constraint node in
CloneFkReferencing. Easy to fix, have patch, running tests now.

I have pushed the fix for this now.

- The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not
correctly propagated. This should be an easy fix also, haven't tried,
need to add a test case.

There was no bug here actually: it's true that the struct member is left
uninitialized, but in practice that doesn't matter, because the set of
columns is propagated separately from the node.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes" (Germán Poo)

On Thu, 3 Nov 2022 20:44:16 +0100
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Oct-05, Alvaro Herrera wrote:

I've been giving the patches a look and it caused me to notice two
additional bugs in the same area:

- FKs in partitions are sometimes marked NOT VALID. This is because of
missing initialization when faking up a Constraint node in
CloneFkReferencing. Easy to fix, have patch, running tests now.

I have pushed the fix for this now.

Thank you Alvaro!