Skip to content
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: INSERT ON CONFLICT can insert NULLs into NOT NULL columns #35040

Closed
idubrov opened this issue Feb 18, 2019 · 10 comments · Fixed by #35371
Closed

sql: INSERT ON CONFLICT can insert NULLs into NOT NULL columns #35040

idubrov opened this issue Feb 18, 2019 · 10 comments · Fixed by #35371
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2 Medium-high impact: many users impacted, risks of availability and difficult-to-fix data errors

Comments

@idubrov
Copy link

idubrov commented Feb 18, 2019

Describe the problem

The following sequence crashes CockroachDB:

CREATE TABLE test(
  key STRING NOT NULL,
  pos INT NOT NULL,
  token STRING NOT NULL,
  PRIMARY KEY(key)
);

PREPARE X AS
INSERT INTO test(key, pos, token)
VALUES ($1, $2, $3)
ON CONFLICT (key)
DO UPDATE SET pos = CASE
  WHEN excluded.token = test.token THEN excluded.pos
  ELSE NULL
END
RETURNING NOTHING;

EXECUTE X('key', 123, 'hello');
EXECUTE X('key', 123, 'bye');
SELECT * FROM test;

Backtrace:

*
* ERROR: [n1,client=[::1]:60292,user=root] a SQL panic has occurred while executing "SELECT * FROM test": Non-nullable column "test:pos" with no value! Index scanned was "primary" with the index key columns (key) and the values ('key')
*
*
* ERROR: [n1,client=[::1]:60292,user=root] a panic has occurred!
*
panic while executing 1 statements: SELECT * FROM _; caused by Non-nullable column "test:pos" with no value! Index scanned was "primary" with the index key columns (key) and the values ('key')

goroutine 90 [running]:
runtime/debug.Stack(0x6d171e0, 0xc0052a4400, 0x3)
	/usr/local/Cellar/go/1.11.4/libexec/src/runtime/debug/stack.go:24 +0xa7
github.com/cockroachdb/cockroach/pkg/util/log.ReportPanic(0x6d171e0, 0xc0052a4400, 0xc0005b5980, 0x64db4c0, 0xc00612f860, 0x1)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:213 +0xa6
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).closeWrapper(0xc005dbc000, 0x6d171e0, 0xc0052a4400, 0x61f4de0, 0xc0068db560)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:645 +0x2df
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1(0xc005dbc000, 0x6d171e0, 0xc0052a4400)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:387 +0x61
panic(0x61f4de0, 0xc0068db560)
	/usr/local/Cellar/go/1.11.4/libexec/src/runtime/panic.go:513 +0x1b9
github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*RowFetcher).finalizeRow(0xc0069b7c98, 0x6d172a0, 0xc00612f530)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/rowfetcher.go:1247 +0xa37
github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*RowFetcher).NextRow(0xc0069b7c98, 0x6d172a0, 0xc00612f530, 0xc00612f560, 0x1, 0x1, 0x0, 0x10001, 0x0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/rowfetcher.go:997 +0x19a
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*rowFetcherWrapper).Next(0xc0078b15c0, 0x6d172a0, 0xc00612f530, 0xc006cf2c60, 0xc00612f4d0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/tablereader.go:144 +0x4a
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*tableReader).Next(0xc0069b7800, 0x0, 0x6d172a0, 0x6d172a0, 0xc00612f530)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/tablereader.go:247 +0x4c
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.Run(0x6d172a0, 0xc00612f3b0, 0x6d20660, 0xc0069b7800, 0x6cf43a0, 0xc005044380)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/base.go:172 +0x35
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*ProcessorBase).Run(0xc0069b7800, 0x6d172a0, 0xc00612f3b0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/processors.go:731 +0x96
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*Flow).StartSync(0xc0062ea8c0, 0x6d172a0, 0xc00612f3b0, 0x6785fb8, 0xc000253880, 0x6cf4060)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:607 +0x19a
github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).Run(0xc0007d1600, 0xc006bd0d20, 0xc005024e10, 0xc0075f29e8, 0xc005c31680, 0xc005dbc4b0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:253 +0x8cb
github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRun(0xc0007d1600, 0x6d172a0, 0xc005e2d1d0, 0xc005dbc4b0, 0xc006bd0d20, 0xc005024e10, 0x6d0a3a0, 0xc000775500, 0xc005c31680)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:758 +0x24c
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithDistSQLEngine(0xc005dbc000, 0x6d172a0, 0xc005e2d1d0, 0xc005dbc418, 0x3, 0xab9c398, 0xc005025050, 0x1, 0x0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:982 +0x26b
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine(0xc005dbc000, 0x6d172a0, 0xc005e2d1d0, 0x6d1a920, 0xc00690d300, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:824 +0xa6a
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState(0xc005dbc000, 0x6d172a0, 0xc005e2d1d0, 0x6d1a920, 0xc00690d300, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:402 +0xaa7
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt(0xc005dbc000, 0x6d172a0, 0xc005e2d1d0, 0x6d1a920, 0xc00690d300, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:96 +0x333
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run(0xc005dbc000, 0x6d171e0, 0xc0052a4400, 0xc00063f998, 0x5400, 0x15000, 0xc00063fa30, 0xc0004c90f0, 0x0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1118 +0x2140
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn(0xc000830870, 0x6d171e0, 0xc0052a4400, 0xc005dbc000, 0x5400, 0x15000, 0xc00063fa30, 0xc0004c90f0, 0x0, 0x0)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:389 +0xce
github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl.func4(0xc000830870, 0x6d171e0, 0xc0052a4400, 0xc005dbc000, 0x5400, 0x15000, 0xc00063fa30, 0xc0004c90f0, 0xc0004c9100, 0xc0004c6da4)
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:313 +0x81
created by github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl
	/private/tmp/cockroach-20190121-61877-1tcqxrf/cockroach-v2.1.4/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:312 +0x1018

Please describe the issue you observed, and any steps we can take to reproduce it:

To Reproduce

What did you do? Describe in your own words.

Run the set of statements posted above.

Expected behavior
A clear and concise description of what you expected to happen.

Second EXECUTE X should fail to insert NULL value into a NON-NULL column.

Environment:

  • CockroachDB version: CCL v2.1.4 @ 2019/01/21 23:00:37 (go1.11.4)
  • Server OS: macOS
  • Client app cocroach sql
@jordanlewis
Copy link
Member

Great find. Thank you, we'll fix this.

@jordanlewis
Copy link
Member

cc @vivekmenezes - this is not just a schema change problem. We should remove this panic.

@jordanlewis jordanlewis changed the title CRDB crashes when reading table with NON-NULL constraint but NULL value smuggled in sql: INSERT ON CONFLICT can insert NULLs into NOT NULL columns Feb 20, 2019
@jordanlewis
Copy link
Member

I filed #35083 to address the "over-eager panic" here. This issue represents a fix for the on conflict smuggling that you found.

@jordanlewis jordanlewis added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2 Medium-high impact: many users impacted, risks of availability and difficult-to-fix data errors labels Feb 28, 2019
@jordanlewis
Copy link
Member

Minimized repro:

CREATE TABLE test(a int primary key, b int not null);
insert into test values(0,0) on conflict(a) do update set b=null;
insert into test values(0,0) on conflict(a) do update set b=null;
select * from test;

@jordanlewis
Copy link
Member

Here's a similar problem:

[email protected]:52658/defaultdb> create table z (a int2 primary key);
CREATE TABLE

[email protected]:52658/defaultdb> insert into z values(111111111111111111);
pq: integer out of range for type SMALLINT (column "a")
[email protected]:52658/defaultdb> insert into z values(1);
INSERT 1

Time: 2.296ms

[email protected]:52658/defaultdb> insert into z values(1) on conflict(a) do update set a=111111111111111111;
INSERT 1

@knz
Copy link
Contributor

knz commented Feb 28, 2019

I think this is a regression. It used to work fine.

@knz
Copy link
Contributor

knz commented Feb 28, 2019

So the logic to check nullability is properly shared between INSERT and UPSERT, because it's performed in GenerateInsertRow. It's UPDATE that's the special case here (and also has this code).

However there's something funky going on in GenerateInsertRow: the nullability check is only performed for a subset of the columns in the table, maybe not all columns for which a new value is inserted. I think this is where the bug is. However before I fix that I need to figure out why that subset was selected.

@knz
Copy link
Contributor

knz commented Mar 4, 2019

Found the bug: GenerateInsertRow validates the row coming from the data source, ahead of the mutation and upsert conflict resolution. It's too early.

@vivekmenezes
Copy link
Contributor

#34323 can be caused by this issue.

craig bot pushed a commit that referenced this issue Mar 7, 2019
35091: sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints r=knz a=knz

Fixes #32555. For TypeORM compat, see discussion on #22298.

Release note (sql change): This patch changes RENAME COLUMN to become
a "table command", which can be used alongside other table commands in
a single ALTER TABLE statement. This makes it possible to e.g. atomically
add a computed column based on an existing column, and rename the
columns so that the computed column "replaces" the original column.

Release note (sql change): CockroachDB now supports ALTER TABLE RENAME
CONSTRAINT for compatibility with PostgreSQL. This feature is limited
to *named* constraints, where the name of the constraints is preserved
in the table metadata. This currently includes CHECK, UNIQUE and
FOREIGN KEY constraints, and does not include other
constraints (DEFAULT, NULL etc) otherwise supported by PostgreSQL. For
UNIQUE constraint, only supporting indexes that are not depended on by
views can be renamed.

35121: sql: add support for pg_catalog.{current_setting,set_config} r=knz a=knz

Fixes #35108. Needed for Flowable compatibility.

Release note (sql change): The SQL built-in functions
`pg_catalog.current_setting()` and `pg_catalog.set_config()` are now
supported for compatibility with PostgreSQL. Note that only
session-scoped configuration changes remain supported (`set_config(_,
_, false)`).

35359: roachtest: backup: use AOST time slightly in the past r=mjibson a=mjibson

Avoids problems where it's sometimes in the future according to the
AOST logic.

Fixes #34817

Release note: None

35371: sql: ensure column constraints are validated after SET for UPSERT r=knz a=knz

Fixes #35040.

Release note (bug fix): CockroachDB now properly applies column width
and nullability constraints on the result of conflict resolution in
UPSERT and INSERT ON CONFLICT.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Matt Jibson <[email protected]>
@craig craig bot closed this as completed in #35371 Mar 7, 2019
@vivekmenezes
Copy link
Contributor

vivekmenezes commented Mar 7, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2 Medium-high impact: many users impacted, risks of availability and difficult-to-fix data errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants