Hi.
While trying to make ALTER COLUMN SET EXPRESSION just validate the
existing constraint instead of dropping and recreating constraints,
(https://commitfest.postgresql.org/patch/7117).
I find the following code suspicious.
In CloneFkReferencing we have:
{
/* No dice. Set up to create our own constraint */
fkconstraint = makeNode(Constraint);
fkconstraint->contype = CONSTRAINT_FOREIGN;
/* ->conname determined below */
fkconstraint->deferrable = constrForm->condeferrable;
fkconstraint->initdeferred = constrForm->condeferred;
fkconstraint->location = -1;
fkconstraint->pktable = NULL;
/* ->fk_attrs determined below */
fkconstraint->pk_attrs = NIL;
fkconstraint->fk_matchtype = constrForm->confmatchtype;
fkconstraint->fk_upd_action = constrForm->confupdtype;
fkconstraint->fk_del_action = constrForm->confdeltype;
fkconstraint->fk_del_set_cols = NIL;
fkconstraint->old_conpfeqop = NIL;
fkconstraint->old_pktable_oid = InvalidOid;
fkconstraint->is_enforced = constrForm->conenforced;
fkconstraint->skip_validation = false;
fkconstraint->initially_valid = constrForm->convalidated;
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,
makeString(NameStr(att->attname)));
}
indexOid = constrForm->conindid;
with_period = constrForm->conperiod;
/* Create the pg_constraint entry at this level */
address = addFkConstraint(addFkReferencingSide,
NameStr(constrForm->conname), fkconstraint,
partRel, pkrel, indexOid, parentConstrOid,
numfks, confkey,
mapped_conkey, conpfeqop,
conppeqop, conffeqop,
numfkdelsetcols, confdelsetcols,
false, with_period);
}
And in
addFkRecurseReferencing
{
tab = ATGetQueueEntry(wqueue, rel);
newcon = palloc0_object(NewConstraint);
....
newcon->conwithperiod = fkconstraint->fk_with_period;
newcon->qual = (Node *) fkconstraint;
tab->constraints = lappend(tab->constraints, newcon);
}
------------------------------------
The preceding code shows that in CloneFkReferencing
{
addFkConstraint
....
addFkRecurseReferencing
}
We not set fkconstraint->fk_with_period value and just use it in
addFkRecurseReferencing,
and validateForeignKeyConstraint require NewConstraint->conwithperiod
to set properly.
So I asked Claude to confirm this; the Claude response is attached.
Looking at src/test/regress/sql/without_overlaps.sql, there is no such
test case:
ATTACH PARTITION where the partition has data.
The minimum reproducible example:
drop table if exists tp, tfk, tfk2;
CREATE TABLE tp (
id int4range,
valid_at daterange,
CONSTRAINT tp_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
INSERT INTO tp VALUES ('[1,2)', daterange('2000-01-01', '2000-02-01'));
CREATE TABLE tfk (
id int4range,
parent_id int4range,
valid_at daterange,
CONSTRAINT tfk_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES tp (id, PERIOD valid_at)
) PARTITION BY LIST (id);
CREATE TABLE tfk2 (LIKE tfk including all);
INSERT INTO tfk2(id, parent_id, valid_at) VALUES ('[2,3)', '[1,2)',
daterange('2000-01-01', '2010-01-01'));
ALTER TABLE tfk ATTACH PARTITION tfk2 FOR VALUES IN ('[2,3)'); --
expect error, but no error now.
INSERT INTO tfk2(id, parent_id, valid_at) VALUES ('[2,3)', '[1,2)',
daterange('2000-01-01', '2010-01-01')); -- error as expected.
--
jian
https://www.enterprisedb.com/
src/backend/commands/tablecmds.c:
addFkRecurseReferencing
if (wqueue && !old_check_ok && !fkconstraint->skip_validation &&
fkconstraint->is_enforced)
{
NewConstraint *newcon;
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, rel);
newcon = palloc0_object(NewConstraint);
newcon->name = get_constraint_name(parentConstr);
newcon->contype = CONSTR_FOREIGN;
newcon->refrelid = RelationGetRelid(pkrel);
newcon->refindid = indexOid;
newcon->conid = parentConstr;
newcon->conwithperiod = fkconstraint->fk_with_period;
newcon->qual = (Node *) fkconstraint;
tab->constraints = lappend(tab->constraints, newcon);
}
CloneFkReferencing:
/* No dice. Set up to create our own constraint */
fkconstraint = makeNode(Constraint);
fkconstraint->contype = CONSTRAINT_FOREIGN;
/* ->conname determined below */
fkconstraint->deferrable = constrForm->condeferrable;
fkconstraint->initdeferred = constrForm->condeferred;
fkconstraint->location = -1;
fkconstraint->pktable = NULL;
/* ->fk_attrs determined below */
fkconstraint->pk_attrs = NIL;
fkconstraint->fk_matchtype = constrForm->confmatchtype;
fkconstraint->fk_upd_action = constrForm->confupdtype;
fkconstraint->fk_del_action = constrForm->confdeltype;
fkconstraint->fk_del_set_cols = NIL;
fkconstraint->old_conpfeqop = NIL;
fkconstraint->old_pktable_oid = InvalidOid;
fkconstraint->is_enforced = constrForm->conenforced;
fkconstraint->skip_validation = false;
fkconstraint->initially_valid = constrForm->convalidated;
CloneFkReferencing
does not assign the fkconstraint->fk_with_period properly then
``
newcon->conwithperiod = fkconstraint->fk_with_period;
`` is wrong?
if wrong, make a minmium test case demo it.
HINT: validateForeignKeyConstraint use this boolean;
validateForeignKeyConstraint(fkconstraint->conname, rel, refrel,
con->refindid,
con->conid,
con->conwithperiod);
----------------------------------------------------------------------------------------------------
Yes, that's a real bug, and your instinct about `validateForeignKeyConstraint`
is exactly the right place to look.
## Confirming it
`CloneFkReferencing` builds the `Constraint` node with `makeNode` (11750),
which zeroes it, and the explicit assignments at 11751–11767 never touch
`fk_with_period` or `pk_with_period`. So the node says `false`. The function
then reads the truth from the catalog separately — `with_period =
constrForm->conperiod;` (11779) — and threads *that* into `addFkConstraint`
(11789) and `addFkRecurseReferencing` (11813). So `pg_constraint.conperiod` on
the cloned child constraint is correct; only the node is stale.
Then `addFkRecurseReferencing` has `with_period` right there as a parameter
(11183) but line 11236 reads the node instead. Result: `newcon->conwithperiod
== false` for a temporal FK.
The fix is one line:
```c
- newcon->conwithperiod = fkconstraint->fk_with_period;
+ newcon->conwithperiod = with_period;
```
And that's safe for every caller, not just the clone path. In
`ATAddForeignKeyConstraint`, `with_period = fk_with_period || pk_with_period`
(10307) and 10308 immediately errors out if `with_period && !fk_with_period` —
so there the two are provably equal. The parameter is always at least as
correct as the node field, and in the one path where they diverge the node
field is the wrong one.
## Why the symptom is the bad direction
`hasperiod=false` makes 14258 take the `RI_Initial_Check` LEFT JOIN shortcut
instead of the per-row trigger loop. And the join is built from `conpfeqop`,
where the period column's operator is *overlaps*, not equality — see
`for_overlaps = with_period && i == numpks - 1` at 10497. So you get:
```sql
... FROM ONLY tfk2 fk LEFT JOIN ONLY tp pk
ON (pk.id = fk.parent_id AND pk.valid_at && fk.valid_at)
WHERE pk.id IS NULL AND ...
```
That asks "does this row overlap *some* PK row?" Temporal FK semantics require
"is this row's period contained in the *union* of the matching PK rows'
periods" — which is what the trigger path computes via `range_agg`. Overlap is
strictly weaker than containment, so the buggy check **under-reports**: rows
that poke out past the union pass. You end up attaching a partition full of
FK-violating rows, with the constraint left marked validated.
Also worth noting: `without_overlaps.sql` has zero `ATTACH PARTITION` coverage,
which is why this survived.
## Minimal repro
I couldn't execute this — no flex/bison in this sandbox and no root to install
them — so treat it as untested. Run as superuser with no RLS, otherwise
`RI_Initial_Check` bails out on permissions (1738/1746) and silently falls
through to the *correct* trigger loop, masking the bug.
```sql
CREATE TABLE tp (
id int4range,
valid_at daterange,
CONSTRAINT tp_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
INSERT INTO tp VALUES ('[1,2)', daterange('2000-01-01', '2000-02-01'));
CREATE TABLE tfk (
id int4range,
valid_at daterange,
parent_id int4range,
CONSTRAINT tfk_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES tp (id, PERIOD valid_at)
) PARTITION BY LIST (id);
-- Baseline: the runtime trigger correctly rejects this row.
CREATE TABLE tfk1 PARTITION OF tfk FOR VALUES IN ('[1,2)');
INSERT INTO tfk VALUES ('[1,2)', daterange('2000-01-01', '2010-01-01'),
'[1,2)');
-- ERROR: insert or update on table "tfk1" violates foreign key constraint
"tfk_fk"
-- Now the same row via ATTACH PARTITION, which goes through CloneFkReferencing.
CREATE TABLE tfk2 (LIKE tfk); -- plain LIKE: no FK copied, so we clone
rather than attach
INSERT INTO tfk2 VALUES ('[2,3)', daterange('2000-01-01', '2010-01-01'),
'[1,2)');
ALTER TABLE tfk ATTACH PARTITION tfk2 FOR VALUES IN ('[2,3)');
-- expected: ERROR, same violation
-- actual: succeeds -- the row *overlaps* [2000-01-01,2000-02-01) so the LEFT
JOIN finds a match
```
Then show the catalog is now lying:
```sql
SELECT conname, conperiod, convalidated FROM pg_constraint
WHERE conrelid = 'tfk2'::regclass AND contype = 'f';
-- conperiod = t, convalidated = t
UPDATE tfk2 SET parent_id = parent_id; -- fires the real RI check
-- ERROR -- the row ATTACH just validated is in fact a violation
```
That last step is the money shot for a bug report: the identical row is
accepted by the validation scan and rejected by the enforcement trigger, which
can only mean the scan used the wrong semantics.
Affects 18 and up — temporal FKs went in with 89f908a6d0a (2024-09-17) — so
it's back-patchable to REL_18_STABLE. Worth adding the `ATTACH PARTITION` cases
to `without_overlaps.sql` in the same patch.
