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: automatically add index for fk constraint on empty table #32234

Merged
merged 1 commit into from Nov 20, 2018

Conversation

Projects
None yet
5 participants
@lucy-zhang
Copy link
Contributor

lucy-zhang commented Nov 12, 2018

Adding a foreign key constraint requires that the referencing column(s)
be indexed, so previously, an ALTER TABLE statement to add a foreign key
constraint for an existing table would always fail, even if the
referencing table were empty. This change automatically adds the needed
index for the foreign key constraint when the table is empty. This will
improve compatibility with some ORMs, which create tables before adding
foreign key constraints.

Fixes #26738

Release note (sql change): An ALTER TABLE statement to add a foreign key
constraint now automatically creates the necessary index if the
referencing table is empty, if the index does not already exist.

@lucy-zhang lucy-zhang requested a review from dt Nov 12, 2018

@lucy-zhang lucy-zhang requested review from cockroachdb/sql-bulk-prs as code owners Nov 12, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 12, 2018

This change is Reviewable

@lucy-zhang lucy-zhang force-pushed the lucy-zhang:fk-add-index branch from 9dd9fe5 to fa3379f Nov 13, 2018

@dt dt requested a review from BramGruneir Nov 14, 2018

@dt

dt approved these changes Nov 14, 2018

Copy link
Member

dt left a comment

Nice!

Overall LGTM. Given the debugging we went though, we might also want to add a couple test cases for ALTER and CREATE in same and separate txns, but I'll leave any further review to the current expert on FKs -- Bram.

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/logictest/testdata/logic_test/fk, line 769 at r1 (raw file):

DROP TABLE b

statement ok

we might also want to add a case to the schema_change_in_txn file to exercise these changes in both the CREATE and ALTER paths inside and not a txn (and since we spent some time digging into it, maybe also in same txn that creates table vs different txn).

@lucy-zhang lucy-zhang force-pushed the lucy-zhang:fk-add-index branch 2 times, most recently from 951d606 to e3128c3 Nov 14, 2018

@lucy-zhang
Copy link
Contributor

lucy-zhang left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/logictest/testdata/logic_test/fk, line 769 at r1 (raw file):

Previously, dt (David Taylor) wrote…

we might also want to add a case to the schema_change_in_txn file to exercise these changes in both the CREATE and ALTER paths inside and not a txn (and since we spent some time digging into it, maybe also in same txn that creates table vs different txn).

I added some tests in schema_change_in_txn, and red/greened create_and_add_fk_in_same_txn with the earlier commit that was using InternalExecutor and would hang when the CREATE TABLE was in the same transaction as the ALTER TABLE.

@BramGruneir
Copy link
Member

BramGruneir left a comment

Code looks good. Just more testing needed.

Reviewed 4 of 5 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/create_table.go, line 616 at r2 (raw file):

	}

	var added sqlbase.IndexDescriptor

Just move this into the the if statement and return inside it after the panic check.

Then you can remove the else scope and just return without declaring added at all aft the end of the other case.


pkg/sql/logictest/testdata/logic_test/fk, line 769 at r2 (raw file):

DROP TABLE b

statement ok

Please put these in a subtest. Perhaps at the end of the file.


pkg/sql/logictest/testdata/logic_test/fk, line 770 at r2 (raw file):


statement ok
CREATE TABLE nonempty_a (id SERIAL NOT NULL, self_id INT, b_id INT NOT NULL, PRIMARY KEY (id))

Please add some comments on what each main statement is doing. What are you testing exactly? Looks like you did this a bit in the other test file.

Also, could you add a test that uses a composite fk, to make sure that index is also added.

Are there any cases where an index can't be added?
I can think of two:

  • duplicate columns in an index
  • index name already exists (or do we get around that?)
    Please add tests to ensure these still fail (or work).

And of course, also add these tests to the transaction tests as well.


pkg/sql/logictest/testdata/logic_test/fk, line 799 at r2 (raw file):

ALTER TABLE nonempty_a ADD CONSTRAINT fk_b FOREIGN KEY (b_id) REFERENCES nonempty_b;

statement ok

drop table statements can be combined

@BramGruneir
Copy link
Member

BramGruneir left a comment

Oh, and this will either fix or allow a number of hibernate tests to now pass. Please run those tests and adjust the hibernate blacklist accordingly.

Take a look at the 2.2 blacklist in https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/hibernate_blacklist.go. Look for #26738 of course.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

sql: automatically add index for fk constraint on empty table
Adding a foreign key constraint requires that the referencing column(s)
be indexed, so previously, an ALTER TABLE statement to add a foreign key
constraint for an existing table would always fail, even if the
referencing table were empty. This change automatically adds the needed
index for the foreign key constraint when the table is empty. This will
improve compatibility with some ORMs, which create tables before adding
foreign key constraints.

Fixes #26738

Release note (sql change): An ALTER TABLE statement to add a foreign key
constraint now automatically creates the necessary index if the
referencing table is empty, if the index does not already exist.

@lucy-zhang lucy-zhang force-pushed the lucy-zhang:fk-add-index branch from e3128c3 to d67ac45 Nov 20, 2018

@lucy-zhang
Copy link
Contributor

lucy-zhang left a comment

I updated the Hibernate blacklist. Some of the tests labeled with this issue still fail for unrelated reasons, so I updated the issue labels for those.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/create_table.go, line 616 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Just move this into the the if statement and return inside it after the panic check.

Then you can remove the else scope and just return without declaring added at all aft the end of the other case.

Done.


pkg/sql/logictest/testdata/logic_test/fk, line 769 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Please put these in a subtest. Perhaps at the end of the file.

Done.


pkg/sql/logictest/testdata/logic_test/fk, line 770 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Please add some comments on what each main statement is doing. What are you testing exactly? Looks like you did this a bit in the other test file.

Also, could you add a test that uses a composite fk, to make sure that index is also added.

Are there any cases where an index can't be added?
I can think of two:

  • duplicate columns in an index
  • index name already exists (or do we get around that?)
    Please add tests to ensure these still fail (or work).

And of course, also add these tests to the transaction tests as well.

Done. (I did not try to get around the issue where the auto-created index can't be created because a different index with the same name already exists; in that case it just fails, as the test shows.)


pkg/sql/logictest/testdata/logic_test/fk, line 799 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

drop table statements can be combined

Done.

@BramGruneir
Copy link
Member

BramGruneir left a comment

:lgtm:

There may still be other changes to the hibernate_blacklist, but let's just wait for the nightly run to address them.

Reviewed 5 of 5 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@BramGruneir

This comment has been minimized.

Copy link
Member

BramGruneir commented Nov 20, 2018

@rmloveland This change needs to be reflected in our Docs, but it should be easier to describe.

@lucy-zhang

This comment has been minimized.

Copy link
Contributor

lucy-zhang commented Nov 20, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 20, 2018

Merge #32234
32234: sql: automatically add index for fk constraint on empty table r=lucy-zhang a=lucy-zhang

Adding a foreign key constraint requires that the referencing column(s)
be indexed, so previously, an ALTER TABLE statement to add a foreign key
constraint for an existing table would always fail, even if the
referencing table were empty. This change automatically adds the needed
index for the foreign key constraint when the table is empty. This will
improve compatibility with some ORMs, which create tables before adding
foreign key constraints.

Fixes #26738

Release note (sql change): An ALTER TABLE statement to add a foreign key
constraint now automatically creates the necessary index if the
referencing table is empty, if the index does not already exist.

Co-authored-by: Lucy Zhang <lucy-zhang@users.noreply.github.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 20, 2018

Build succeeded

@craig craig bot merged commit d67ac45 into cockroachdb:master Nov 20, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 20, 2018

cc @awoods187 this was one of the roadmap items on front-end. Is now completed by schema change team.

@knz knz added this to Triage in SQL Front-end, Lang & Semantics via automation Nov 21, 2018

@knz knz moved this from Triage to Finished (m2.2-2) in SQL Front-end, Lang & Semantics Nov 21, 2018

BramGruneir added a commit to BramGruneir/cockroach that referenced this pull request Nov 26, 2018

roachtest: updated the hibernate blacklist for 2.2
Thanks to the update to allow adding FK constraints on empty tables cockroachdb#32234, the
blacklist needs to be updated.  Sadly, 2 tests now pass, but 4 others now fail
due to cockroachdb#32610.

Fixes cockroachdb#32532.

Release note: None

craig bot pushed a commit that referenced this pull request Nov 26, 2018

Merge #32611
32611: roachtest: updated the hibernate blacklist for 2.2 r=BramGruneir a=BramGruneir

Thanks to the update to allow adding FK constraints on empty tables #32234, the
blacklist needs to be updated.  Sadly, 2 tests now pass, but 4 others now fail
due to #32610.

Fixes #32532.

Release note: None

Co-authored-by: Bram Gruneir <bram@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment