{185333845}: reject type-incompatible foreign-key constraints at sche… - #6100
Open
chands10 wants to merge 1 commit into
Open
{185333845}: reject type-incompatible foreign-key constraints at sche…#6100chands10 wants to merge 1 commit into
chands10 wants to merge 1 commit into
Conversation
…ma-change time comdb2 matches foreign-key columns to the referenced key positionally (not by name), converting only the first min(local, parent) columns via stag_to_stag_buf_ckey. If a positionally-corresponding pair is type-incompatible -- e.g. a swapped key order that lines an int up against a cstring -- the conversion returns -1 at runtime: every write to the child table is rejected and existing rows fail verify with 'error loading rc = -1'. Nothing caught this at schema-change time: verify_constraints_exist only checked column counts, and even that is short-circuited when prefix_foreign_keys and superset_foreign_keys are both on (the legacy default). Add a per-column type-class compatibility check to verify_constraints_exist so such a constraint is rejected up front on add/alter, guarded by a new runtime tunable fk_constraint_type_check (default ON). Startup is unaffected: existing tables load via populate_reverse_constraints, which is not touched, so a db that already has such a constraint still boots. Add tests/fk_type_check.test and update tests/tunables.test per AGENTS.md. Signed-off-by: Salil Chandra <schandra107@bloomberg.net>
roborivers
suggested changes
Jul 31, 2026
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Error ⚠.
Regression testing: Success ✓.
The first 10 failing tests are:
noresetgen
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
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
2 participants
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.
…ma-change time
comdb2 matches foreign-key columns to the referenced key positionally (not by name), converting only the first min(local, parent) columns via stag_to_stag_buf_ckey. If a positionally-corresponding pair is type-incompatible -- e.g. a swapped key order that lines an int up against a cstring -- the conversion returns -1 at runtime: every write to the child table is rejected and existing rows fail verify with 'error loading rc = -1'. Nothing caught this at schema-change time: verify_constraints_exist only checked column counts, and even that is short-circuited when prefix_foreign_keys and superset_foreign_keys are both on (the legacy default).
Add a per-column type-class compatibility check to verify_constraints_exist so such a constraint is rejected up front on add/alter, guarded by a new runtime tunable fk_constraint_type_check (default ON). Startup is unaffected: existing tables load via populate_reverse_constraints, which is not touched, so a db that already has such a constraint still boots.
Add tests/fk_type_check.test and update tests/tunables.test per AGENTS.md.