-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: fix erroneous NOT NULL constraint violations in UPSERTs and refactor upsert logic #133671
Conversation
I'm only planning on backporting the first commit. |
c98c078
to
9752f71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, 5 of 5 files at r2, 5 of 5 files at r3, 6 of 6 files at r4, 2 of 2 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)
pkg/sql/tablewriter.go
line 50 at r6 (raw file):
// err := tw.finalize() // // Handle err. type tableWriter interface {
Nice cleanup!
pkg/sql/upsert.go
line 138 at r6 (raw file):
func (n *upsertNode) processSourceRow(params runParams, rowVals tree.Datums) error { // Check for NOT NULL constraint violations. if n.run.tw.canaryOrdinal != -1 && rowVals[n.run.tw.canaryOrdinal] != tree.DNull {
Oh, this happened because the canary column check in the upsert code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, 5 of 5 files at r2, 5 of 5 files at r3, 6 of 6 files at r4, 2 of 2 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @michae2)
pkg/sql/tablewriter_update.go
line 31 at r4 (raw file):
// rowForUpdate performs an update. // // The passed Datums is not used after `row` returns.
nit: s/row/rowForUpdate/
.
pkg/sql/upsert.go
line 139 at r1 (raw file):
// Check for NOT NULL constraint violations. if n.run.tw.canaryOrdinal != -1 && rowVals[n.run.tw.canaryOrdinal] != tree.DNull { // When there is a canary column and its values is not NULL, then an
nit: s/its values/its value/
.
pkg/sql/logictest/testdata/logic_test/upsert
line 1361 at r1 (raw file):
1 2 10 subtest regression_133145
nit: s/133145/133146
.
pkg/sql/logictest/testdata/logic_test/upsert
line 1376 at r1 (raw file):
# This should not cause a not-null constraint violation of column "a" because # the value of "a" is not being update to NULL in the existing row.
nit: s/being update/being updated/
.
Fixes cockroachdb#133146 Release note (bug fix): A bug has been fixed that caused incorrect NOT NULL constraint violation errors on `UPSERT` and `INSERT .. ON CONFLICT .. DO UPDATE` statements when those statements updated an existing row and a subset of columns which did not include a `NOT NULL` column of the table. This bug has been present since at least version 20.1.0.
This commit removes the unused `expressionCarrier` interface and the implementations of its single method, `walkExprs`. Release note: None
This commit removes the unused `desc` method from the `tableWriter` interface and its implementations. Release note: None
The `tableWriter` interface was never used. All types implementing this interface were always used directly. Release note: None
The `enforceNotNullConstraints` function now has an assertion enforcing the lengths of its input to prevent incorrect usage. Release note: None
This commit refactors the upsert table writer which had names and references to logic implementation details that no longer exist because they were made obsolete by the cost-based optimizer. Release note: None
9752f71
to
8294317
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2)
pkg/sql/tablewriter.go
line 50 at r6 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nice cleanup!
Thanks!
pkg/sql/tablewriter_update.go
line 31 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/row/rowForUpdate/
.
Done.
pkg/sql/upsert.go
line 139 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/its values/its value/
.
Done.
pkg/sql/upsert.go
line 138 at r6 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Oh, this happened because the canary column check in the upsert code path.
What happened?
pkg/sql/logictest/testdata/logic_test/upsert
line 1361 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/133145/133146
.
Done.
pkg/sql/logictest/testdata/logic_test/upsert
line 1376 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/being update/being updated/
.
Done.
TFTRs! bors r+ |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from fece3ae to blathers/backport-release-23.1-133671: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. error creating merge commit from fece3ae to blathers/backport-release-23.2-133671: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. error creating merge commit from fece3ae to blathers/backport-release-24.2-133671: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.2.x failed. See errors above. error creating merge commit from 936b9e7 to blathers/backport-release-24.3-133671: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.3.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
sql: fix erroneous NOT NULL constraint violations in UPSERTs
Fixes #133146
Release note (bug fix): A bug has been fixed that caused incorrect NOT
NULL constraint violation errors on
UPSERT
andINSERT .. ON CONFLICT .. DO UPDATE
statements when those statements updated an existing rowand a subset of columns which did not include a
NOT NULL
column of thetable. This bug has been present since at least version 20.1.0.
sql: remove
expressionCarrier
interfaceThis commit removes the unused
expressionCarrier
interface and theimplementations of its single method,
walkExprs
.Release note: None
sql: remove unused
desc
method fromtableWriter
interfaceThis commit removes the unused
desc
method from thetableWriter
interface and its implementations.
Release note: None
sql: remove
tableWriter
interfaceThe
tableWriter
interface was never used. All types implementing thisinterface were always used directly.
Release note: None
sql: add assertion for
enforceNotNullConstraints
The
enforceNotNullConstraints
function now has an assertion enforcingthe lengths of its input to prevent incorrect usage.
Release note: None
sql: refactor upsert table writer
This commit refactors the upsert table writer which had names and
references to logic implementation details that no longer exist because
they were made obsolete by the cost-based optimizer.
Release note: None