Обсуждение: Self FK oddity when attaching a partition
Hi all, While studying the issue discussed in thread "Detaching a partition with a FK on itself is not possible"[1], I stumbled across an oddity while attaching a partition having the same multiple self-FK than the parent table. Only one of the self-FK is found as a duplicate. Find in attachment some SQL to reproduce the scenario. Below the result of this scenario (constant from v12 to commit 7e367924e3). Why "child1_id_abc_no_part_fkey" is found duplicated but not the three others? From pg_constraint, only "child1_id_abc_no_part_fkey" has a "conparentid" set. conname | conparentid | conrelid | confrelid -----------------------------+-------------+----------+----------- child1_id_abc_no_part_fkey | 16901 | 16921 | 16921 child1_id_def_no_part_fkey | 0 | 16921 | 16921 child1_id_ghi_no_part_fkey | 0 | 16921 | 16921 child1_id_jkl_no_part_fkey | 0 | 16921 | 16921 parent_id_abc_no_part_fkey | 16901 | 16921 | 16894 parent_id_abc_no_part_fkey | 0 | 16894 | 16894 parent_id_abc_no_part_fkey1 | 16901 | 16894 | 16921 parent_id_def_no_part_fkey | 16906 | 16921 | 16894 parent_id_def_no_part_fkey | 0 | 16894 | 16894 parent_id_def_no_part_fkey1 | 16906 | 16894 | 16921 parent_id_ghi_no_part_fkey | 0 | 16894 | 16894 parent_id_ghi_no_part_fkey | 16911 | 16921 | 16894 parent_id_ghi_no_part_fkey1 | 16911 | 16894 | 16921 parent_id_jkl_no_part_fkey | 0 | 16894 | 16894 parent_id_jkl_no_part_fkey | 16916 | 16921 | 16894 parent_id_jkl_no_part_fkey1 | 16916 | 16894 | 16921 (16 rows) Table "public.child1" [...] Partition of: parent FOR VALUES IN ('1') Partition constraint: ((no_part IS NOT NULL) AND (no_part = '1'::smallint)) Indexes: "child1_pkey" PRIMARY KEY, btree (id, no_part) Check constraints: "child1" CHECK (no_part = 1) Foreign-key constraints: "child1_id_def_no_part_fkey" FOREIGN KEY (id_def, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT "child1_id_ghi_no_part_fkey" FOREIGN KEY (id_ghi, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT "child1_id_jkl_no_part_fkey" FOREIGN KEY (id_jkl, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_abc_no_part_fkey" FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_def_no_part_fkey" FOREIGN KEY (id_def, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_ghi_no_part_fkey" FOREIGN KEY (id_ghi, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_jkl_no_part_fkey" FOREIGN KEY (id_jkl, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT Referenced by: TABLE "child1" CONSTRAINT "child1_id_def_no_part_fkey" FOREIGN KEY (id_def, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "child1" CONSTRAINT "child1_id_ghi_no_part_fkey" FOREIGN KEY (id_ghi, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "child1" CONSTRAINT "child1_id_jkl_no_part_fkey" FOREIGN KEY (id_jkl, no_part) REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_abc_no_part_fkey" FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_def_no_part_fkey" FOREIGN KEY (id_def, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_ghi_no_part_fkey" FOREIGN KEY (id_ghi, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT TABLE "parent" CONSTRAINT "parent_id_jkl_no_part_fkey" FOREIGN KEY (id_jkl, no_part) REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT Regards, [1] https://www.postgresql.org/message-id/flat/20220321113634.68c09d4b%40karst#83c0880a1b4921fcd00d836d4e6bceb3
Вложения
[BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
От
Jehan-Guillaume de Rorthais
Дата:
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.
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
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
От
Jehan-Guillaume de Rorthais
Дата:
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,
Вложения
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
От
Jehan-Guillaume de Rorthais
Дата:
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: > 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,
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&cid=4647152)
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
От
Jehan-Guillaume de Rorthais
Дата:
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] 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]). 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)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
От
Jehan-Guillaume de Rorthais
Дата:
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. >
Вложения
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
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
От
Jehan-Guillaume de Rorthais
Дата:
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)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
От
Jehan-Guillaume de Rorthais
Дата:
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!