On Mon, Sep 28, 2026 at 9:17 AM Andrew Krylosov <[email protected]> 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
v2-0001-Fix-loss-of-PERIOD-semantics-when-validating-temp.patch
Description: Binary data
