Fix constraint validation when merging with a NOT ENFORCED constraint#81
Draft
NikolayS wants to merge 1 commit into
Draft
Fix constraint validation when merging with a NOT ENFORCED constraint#81NikolayS wants to merge 1 commit into
NikolayS wants to merge 1 commit into
Conversation
When a local ENFORCED check constraint was added to a legacy-inheritance child table having an identically named inherited NOT ENFORCED constraint, MergeWithExistingConstraint() marked the merged constraint as both enforced and validated without checking the table's existing rows. Rows violating the constraint could therefore remain in the table while the catalogs claimed the constraint was valid, and the planner would then trust the false metadata (e.g. for constraint exclusion), silently returning wrong query results. To fix, have MergeWithExistingConstraint() set convalidated according to the new constraint's initially-valid status instead of unconditionally setting it to true, and have ATAddCheckNNConstraint() queue a Phase 3 verification scan of the existing rows when such a merge promotes a NOT ENFORCED constraint to enforced, like ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED does. As a side effect, a constraint added with NOT VALID is no longer silently marked validated when merged this way; it keeps its not-valid state and can be validated later with ALTER TABLE ... VALIDATE CONSTRAINT. Oversight in the introduction of NOT ENFORCED constraints in PostgreSQL 18. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01WKfageiP8Th6TgzZuyCpfQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a correctness bug where
ALTER TABLE child ADD CONSTRAINTon a legacy-inheritance child could mark an inheritedNOT ENFORCEDCHECK constraint as both enforced and validated without scanning existing rows, leaving trusted-but-false catalog metadata. Withconstraint_exclusion = on, the planner then trusted the false constraint and silently returned wrong query results (rows filtered out that actually exist in the table).Root cause
MergeWithExistingConstraint()insrc/backend/catalog/heap.c, when merging a local ENFORCED constraint into an existing inherited NOT ENFORCED one, unconditionally set:No Phase 3 verification scan was ever queued, so rows violating the constraint survived while
pg_constraintclaimed the constraint was validated.Fix
Two coordinated changes:
src/backend/catalog/heap.c—MergeWithExistingConstraint()now setsconvalidatedfrom the new constraint'sinitially_validflag instead of unconditionally totrue. A merge with aNOT VALIDconstraint now also correctly leaves the constraint NOT VALID (previously the not-valid state was silently ignored), to be validated later withALTER TABLE ... VALIDATE CONSTRAINT.src/backend/commands/tablecmds.c—ATAddCheckNNConstraint()records, before callingAddRelationNewConstraints(), whether an identically-named NOT ENFORCED check constraint exists that the merge would promote to enforced. If the merge happens and the new constraint is initially valid, it queues a Phase 3 verification scan of the existing rows — mirroring whatALTER TABLE ... ALTER CONSTRAINT ... ENFORCEDdoes. If existing rows violate the constraint, the ALTER now fails withcheck constraint ... is violated by some rowand the catalog change rolls back.Behavior after the fix
ALTER TABLE child ADD CONSTRAINT iso CHECK (tenant = 1)over an inherited NOT ENFORCED constraint with violating rows → error, catalog unchanged, query results stay correct.... NOT VALIDvariant → merge succeeds without a scan; constraint becomes enforced but stays NOT VALID;VALIDATE CONSTRAINTthen behaves as expected.ALTER COLUMN ... TYPEre-add path re-verifies rows through the same mechanism.Testing
src/test/regress/sql/inherit.sqlcovering the violating-row error, the NOT VALID merge state, and the successful verified merge; expected output updated (including the pre-existinginh_check_constraint10case, whose NOT VALID state is now preserved instead of ignored).make check, assert-enabled build).A mailing-list-ready
format-patchof this commit is available for posting to pgsql-hackers.🤖 Generated with Claude Code
https://claude.ai/code/session_01WKfageiP8Th6TgzZuyCpfQ
Generated by Claude Code