Enforce PRIMARY KEY NOT NULL and give every read path one row image - #310
Merged
Conversation
An absent field and an explicit NULL both satisfy IS NULL, but each execution path resolves the field independently. Assert the scan, aggregate, DELETE, and UPDATE paths agree on the same fixture, and that IS NULL / IS NOT NULL counts partition the collection.
A declared PRIMARY KEY implies NOT NULL on every engine, since row identity derives from the primary-key value at plan time. Assert 23502 (not_null_violation) across document schemaless/strict, KV, and columnar, and across INSERT, UPSERT, object-literal, INSERT ... SELECT, and UPDATE paths. Cover the empty-string counterpart, where the value is present and a repeated insert collides as a duplicate key instead.
A declared PRIMARY KEY implies NOT NULL, but identity derivation stringified the key and read the empty string as "no identity", so a NULL or omitted key minted a fresh surrogate and committed a row no uniqueness check could see. Return a typed DocId that separates a present key from an explicit NULL and from an absent column, and refuse the latter two when the catalog records a declared key. The guard reads the declared column, not the resolved one: schemaless, columnar, and spatial collections resolve the key name to id by convention, which says nothing about what the DDL declared. Map RejectedConstraint to a SQLSTATE by its constraint kind. Both mappers hardcoded unique_violation and ignored the field.
The key is a KV row's identity, so it carries the same NOT NULL obligation a declared PRIMARY KEY does. An omitted key column was substituted with an empty string, which stored a row under a key no lookup names. Substitute NULL instead, and refuse a NULL key ahead of the intent match so every write intent is covered.
UPDATE could null out a declared primary key, and the identity choke point shared by INSERT ... SELECT, MERGE, and UPDATE ... FROM minted a fresh surrogate whenever the target key was NULL or absent. Both stored a row whose declared key no longer identifies it. Refuse a literal NULL assignment to the declared key before the engine dispatch, so every engine answers alike, and refuse a missing key value at the shared assignment point. Carry the declared flag on TargetPk so an undeclared id-by-convention column keeps minting as before. Key an empty string like any other value there. Treating it as missing let two rows share it.
A schemaless row's identity lives in the storage key when the body carries no id field. Readers that materialize a row inject it, but predicate evaluation did not, so id read as absent: SELECT ... WHERE id IS NULL returned every row while count(*) over the same predicate returned none, and DELETE ... WHERE id IS NULL emptied the collection. Require the identity at the shared predicate primitive and inject it there, so a caller cannot evaluate a row without one. The scan primitives pass the row key to their predicates to supply it. Emit rows through the same conversion, so a projection of id returns the value the predicate matched on. Route the update path's row image through the shared decode, so its write gate judges the same document the delete path's gate does.
Recursive CTE predicates, the RLS check on a point get, and a MERGE arm's AND condition each built a row image from the stored body alone, so a schemaless row whose identity lives only in its storage key was judged without one. A policy or condition naming id saw it as absent. Build all three images the way every other reader does. The MERGE fix also reaches the bodies the write gate and the commit-time expanders consume.
A write event's payload, the plan-time row-level-security write image, and the materializing scan each carried a schemaless body without the identity that lives in its storage key. A trigger WHEN clause, a policy, or an INSERT ... SELECT filter naming id judged a row that appeared to have none. Inject the storage key at each source, so every consumer reads the same string a query returns. The write gate and the event payload use that key rather than the plan's resolved document id, which for a minted identity is a different encoding of the same surrogate.
A declared PRIMARY KEY implies NOT NULL, but a non-literal right-hand side resolves only once the row is in hand, so the plan-time check cannot see it. UPDATE t SET id = NULLIF(v, v) stored a NULL key. Carry the declared key on the update plans and refuse a post-image that nulls it, at each site that builds one. The key travels on the replicated write record too: the apply rebuilds its plan from that record, so a guard reading it off the plan alone never fires. Strict collections need no guard. Their tuple encoding already refuses a NULL for a non-nullable column, and a declared key is non-nullable.
A refusal names its constraint kind, and the pgwire mappers read that to answer 23502 or 23505. The cross-node encoder flattened every constraint refusal into a generic internal error carrying one numeric code, so a not-null violation raised on a remote shard reached the client as a unique violation, and the coordinator dropped the code entirely. Carry the collection and the kind on the typed cluster error and on ErrorDetails, so a forwarded refusal reconstructs as the verdict the refusing shard reached and the existing mappers answer correctly. The Data Plane's own conversion passed the kind where the collection belonged, naming the constraint as the collection.
Minting a row identity and refusing a NULL key were two calls, so a write path that resolved identity without the separate guard reproduced the hole. Fold the check into the function that mints, which reads the declared column while identity keys on the resolved one. Enforce it on the native protocol, whose plan builders bypass SQL planning and carried no declared key, and on the point-update binary merge path, which returns before the post-image guard and so admitted a literal NULL.
farhan-syah
added
the
run-ci
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
1 participant
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.
Fixes #293.
A declared
PRIMARY KEYon the document (schemaless) and key-value engines acceptedNULLand omitted values. Separately,IS NULLon the identity column returned different row sets on the scan path and the aggregate path, andDELETE ... WHERE id IS NULLmatched every row.Changes
NULLor omitted primary key with SQLSTATE23502across insert, upsert, object-literal insert,INSERT ... SELECT,MERGE,UPDATE ... FROM, andUPDATE, on the document, key-value, and columnar engines.NULL, at the point each update path builds its post-image.DELETE,UPDATE, point get, index fetch, materializing scan, transaction overlay, recursive CTE,UPDATE ... FROM,MERGE, write events, and the row-level-security write gate.23505for every constraint.Behavior changes
idis injected on read paths that omitted itid.id IS NULLmatches nothing.ErrorDetails::ConstraintViolationgainsconstraint23502, not23505.ReplicatedWrite::PointUpdateandBulkDmlgaindeclared_primary_keyBulkDmlcarriesNoneon its delete arm.Compatibility
The replicated write record gained a field. Pre-1.0, so no migration path is provided for existing WAL or replication logs.
Scope limit
The columnar engine does not enforce uniqueness of a declared non-
idprimary key. Two rows carrying the same value in that column both persist when noUNIQUE INDEXexists. Found while verifying this work, not fixed here. Tracked in #309.Validation
cargo nextest run --workspace --exclude nodedb-cluster-tests --all-featurescargo nextest run -p nodedb-cluster-tests --all-featurescargo clippy --workspace --all-targets -- -D warningsCoverage added in
sql_primary_key_nullability.rs,sql_null_predicate_parity.rs,native_primary_key_nullability.rs, andwrite_verdict_sqlstate.rs.