Re: addFkRecurseReferencing use unassigned fkconstraint->fk_with_period value

On Mon, Aug 10, 2026 at 3:09 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> 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/

Hi Jian,

Thanks for the report and the reproducer. I can confirm the issue on master.

The problem is that CloneFkReferencing() reconstructs a Constraint node from
the catalog but doesn’t set fk_with_period, so addFkRecurseReferencing()
initializes NewConstraint.conwithperiod as false even when the constraint is
actually a PERIOD FK. That leads phase 3 validation to incorrectly take the
RI_Initial_Check() fast path, which is insufficient for PERIOD semantics: it
only checks for some overlapping referenced row, rather than requiring the
referencing period to be covered by the aggregate of matching referenced
periods.

The fix is to set conwithperiod from the correct source:

* In addFkRecurseReferencing(), use the with_period argument directly.
* In QueueFKConstraintValidation() and ATExecAlterFKConstrEnforceability(),
read it from pg_constraint.conperiod.

I’ve also added regression tests covering ATTACH PARTITION, VALIDATE
CONSTRAINT, and ALTER CONSTRAINT … ENFORCED, including cases with pre-existing
violating rows.

Passes make check-world with assertions enabled.

Best,
Haibo