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: race condition with fast path for insert #43969

Closed
rafiss opened this issue Jan 14, 2020 · 2 comments · Fixed by #44031
Closed

sql: race condition with fast path for insert #43969

rafiss opened this issue Jan 14, 2020 · 2 comments · Fixed by #44031
Assignees
Labels
A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@rafiss
Copy link
Collaborator

rafiss commented Jan 14, 2020

#42914 added a specialized execution operator that allows running foreign key checks before (or in parallel with) a mutation.

This seems to have created a race condition, which I discovered because of a test that started failing in the Hibernate repo. The test is org.hibernate.test.annotations.cid.CompositeIdTest.testSecondaryTableWithCompositeId, but here is how to reproduce the issue with just SQL from the CLI interface.

Run the following all at once by copy-pasting into the CLI:

CREATE TABLE presenter (
                name VARCHAR(255) NOT NULL,
                CONSTRAINT "primary" PRIMARY KEY (name ASC),
                FAMILY "primary" (name)
              );

CREATE TABLE channel (
                  id INT4 NOT NULL,
                  name VARCHAR(255) NULL,
                  CONSTRAINT "primary" PRIMARY KEY (id ASC),
                  FAMILY "primary" (id, name)
              );

CREATE TABLE tvprogram (
                  "time" TIME NULL,
                  chan_id INT4 NOT NULL,
                  presenter_name VARCHAR(255) NOT NULL,
                  CONSTRAINT "primary" PRIMARY KEY (chan_id ASC, presenter_name ASC),
                  CONSTRAINT fkoeti4r4k3x1hm4rb2r8o15ce2 FOREIGN KEY (chan_id) REFERENCES channel(id),
                  CONSTRAINT fk96yxppjh6mrelfbgspmfr0i2r FOREIGN KEY (presenter_name) REFERENCES presenter(name),
                  INDEX tvprogram_auto_index_fk96yxppjh6mrelfbgspmfr0i2r (presenter_name ASC),
                  FAMILY "primary" ("time", chan_id, presenter_name)
              );

CREATE TABLE tv_program_ext (
                      txt VARCHAR(255) NULL,
                      channel_id INT4 NOT NULL,
                      presenter_name VARCHAR(255) NOT NULL,
                      CONSTRAINT "primary" PRIMARY KEY (channel_id ASC, presenter_name ASC),
                      CONSTRAINT fktms6bhx8rws68estjr0qvh0tk FOREIGN KEY (channel_id, presenter_name) REFERENCES tvprogram(chan_id, presenter_name),
                      FAMILY "primary" (txt, channel_id, presenter_name)
                  );

insert into Channel (name, id) values (null, 8);
insert into Presenter (name) values ('tim russett');
insert into TvProgram ("time", chan_id, presenter_name) values ('2020-01-13T17:40:02-5:00', 8, 'tim russett');
insert into TV_PROGRAM_EXT (TXT, CHANNEL_ID, PRESENTER_NAME) values ('Award Winning Programming', 8, 'tim russett');

The following error occurs

ERROR: internal error: 2 FK cols, only 1 cols in index
SQLSTATE: XX000
DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/sql/insert_fast_path.go:103: init()
github.com/cockroachdb/cockroach/pkg/sql/insert_fast_path.go:235: startExec()
github.com/cockroachdb/cockroach/pkg/sql/plan.go:337: func2()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:142: func1()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:679: visitInternal()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:122: visitConcrete()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:567: visitInternal()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:109: visit()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:73: walkPlan()
github.com/cockroachdb/cockroach/pkg/sql/plan.go:340: startExec()
github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:117: Start()
github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:742: Run()
github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:373: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:374: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:976: PlanAndRun()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:823: execWithDistSQLEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:715: dispatchToExecutionEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:418: execStmtInOpenState()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:100: execStmt()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1283: execCmd()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1212: run()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:451: ServeConn()
github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:566: func1()
runtime/asm_amd64.s:1337: goexit()

However, if I run the above statements one-by-one, with some time in between each one, it seems to work. Also, usually if I retry running the last statement (inserting into TV_PROGRAM_EXT), then it gets executed successfully.

@RaduBerinde RaduBerinde self-assigned this Jan 14, 2020
@RaduBerinde
Copy link
Member

The change happens because statistics kick in and for such small tables, the FK checks end up being hash joins (because of #41701). The failure would show up again with more data in the tables. This should be 100% reproducible if the auto stats are turned off.

@rafiss rafiss added A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Jan 14, 2020
@RaduBerinde
Copy link
Member

The problem is that the optimizer chooses the tvprogram_auto_index_fk96yxppjh6mrelfbgspmfr0i2r index for lookup instead of the PK (which makes sense), but in this index the second FK column is implicit. The fast path insert code, as well as EncodePartialIndexKey doesn't support that correctly.

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 15, 2020
The insert fast path doesn't work correctly when the FK checks use a
non-unique index. This is rare, but it can happen when 1) the key is
composite; 2) it is the primary key in the parent table; and 3) there
is a non-unique secondary index on a subset of the primary key
columns. In this case the secondary index is preferred because it
contains fewer columns.

The problem is that the lookup involves implicit columns and neither
the fast path code nor the `EncodePartialIndexKey` code were equipped
to handle this. This change adds logic to handle implicit columns.

Note that the legacy path doesn't have this problem because it is
always hardwired to use the unique index.

Fixes cockroachdb#43969.

Release note (bug fix): fixed internal error of the form "x FK cols, only y cols in
index" in some cases when inserting into a table with foreign key
references.
craig bot pushed a commit that referenced this issue Jan 16, 2020
43960: importccl: move OnSuccess callback to Resume r=pbardea a=pbardea

OnSuccess is not guaranteed to be called only once. In an effort to move
away from using this callback the logic previously contained in
OnSuccess for import is being moved into Resume with logic to ensure
that it is only run once.e

Release note (bug fix): Ensure the import cleanup is only run once.

44025: roachtest: update libpq blacklist r=rafiss a=rafiss

A new test was added that is failing, and a different test was fixed
upstream so it passes now.

fixes #43345
Release note: none

44031: opt: fix insert fast path when using a non-unique index r=RaduBerinde a=RaduBerinde

The insert fast path doesn't work correctly when the FK checks use a
non-unique index. This is rare, but it can happen when 1) the key is
composite; 2) it is the primary key in the parent table; and 3) there
is a non-unique secondary index on a subset of the primary key
columns. In this case the secondary index is preferred because it
contains fewer columns.

The problem is that the lookup involves implicit columns and neither
the fast path code nor the `EncodePartialIndexKey` code were equipped
to handle this. This change adds logic to handle implicit columns.

Note that the legacy path doesn't have this problem because it is
always hardwired to use the unique index.

Fixes #43969.

Release note (bug fix): fixed internal error of the form "x FK cols, only y cols in
index" in some cases when inserting into a table with foreign key
references.

Co-authored-by: Paul Bardea <pbardea@gmail.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@craig craig bot closed this as completed in 7b80836 Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants