Re: addFkRecurseReferencing use unassigned fkconstraint->fk_with_period value

On Mon, Sep 28, 2026 at 9:17 AM Andrew Krylosov
<krylosov(dot)andrew(at)gmail(dot)com> wrote:
>
> Hi,
>
> Haibo Yan wrote:
> > The fix is to set conwithperiod from the correct source:
>
> I tested v1 on 3c5d9d914f with assertions enabled on macos.
> without_overlaps and the full regress and isolation suites pass.
> With only the tablecmds.c changes reverted, the new tests fail at all
> four expected rejection cases: VALIDATE, ENFORCED, and both ATTACH
> paths.
>
> The changes look right to me. In addFkRecurseReferencing(), with_period
> is correct for both the parser path and the partially reconstructed
> Constraint from CloneFkReferencing(). The other two assignments take it
> from the catalog row being validated. I found no other producer of a
> foreign-key NewConstraint.
>
> I tried multiranges with gaps in coverage, partitioned tables on both
> sides of the FK, recursive validation and enforcement, attaching a
> partitioned table with a different column order, and reusing a NOT VALID
> FK during ADD FOREIGN KEY. The affected operations accept uncovered
> rows on the base revision and reject them with v1; the corresponding
> covered rows pass with v1.
>
> One point for the commit message: constraints that went through one of
> these paths on 18.x may already be marked valid with uncovered rows.
> The fix does not recheck them, and VALIDATE CONSTRAINT is a no-op for
> a valid constraint. Such rows can be found with a query modeled on the
> RI check; alternatively, with the fix, the constraint can be dropped and
> re-added as NOT VALID in one transaction and then validated separately,
> which keeps it enforced for new rows if validation fails.
>
> For a separate cleanup, RI_Initial_Check() could return false when
> riinfo->hasperiod is true. It already fetches that information, so
> this could avoid making callers maintain another copy of the flag.
> I think the minimal fix in v1 is suitable for backpatching as it stands.
>
> Two small test nits: "with pre-existing rows" would read better than
> "with rows already", and the NOT VALID/NOT ENFORCED block would fit
> better after the pg_get_constraintdef checks, keeping those checks
> next to the constraint they inspect.
>
> I think this is ready for a committer.
>
> Best regards,
> Andrew Krylosov

Hi Andrew,

Thanks for the review and additional testing.

I addressed both test comments in v2: changed the wording to "with pre-existing
rows" and moved the NOT VALID / NOT ENFORCED tests after the
pg_get_constraintdef checks.

There are no code changes from v1.

Thanks,
Haibo