sql: allow creation of tables with no primary key #3286

maddyblue · 2015-12-02T18:28:29Z

fixes #2081

CLAassistant · 2015-12-02T18:28:30Z

All committers have accepted the CLA.

tamird · 2015-12-02T18:29:37Z

Can you file the other bugs you found? I'd really like for this to go in with the associated logic test enablement.

tamird · 2015-12-02T18:30:28Z

sql/create.go

@@ -126,7 +126,28 @@ func (p *planner) CreateTable(n *parser.CreateTable) (planNode, error) {
 	// Inherit permissions from the database descriptor.
 	desc.Privileges = dbDesc.GetPrivileges()

-	if err := desc.AllocateIDs(); err != nil {
+	err = desc.AllocateIDs()
+	if err == errMissingPrimaryKey {


instead of relying on this error, can you check for the presence of a primary key directly?

I tried that, and it sadly doesn't work. I duplicated the check in Validate for no primary keys, but the fields it relies on to do that are populated near the end of AllocateIDs, right before it calls Validate itself. Hence the double call to AllocateIDs in this function.

I don't follow; you can't do if len(desc.PrimaryIndex.ColumnNames) == 0? The check in Validate is checking that the IDs were assigned, but you can just look at the names here.

maddyblue · 2015-12-02T18:31:27Z

Sure, I'll file the other bugs.

petermattis · 2015-12-02T18:57:25Z

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

sql/create.go, line 132 [r1] (raw file):
Is this the right builtin to be using? experimental_unique_int() would be faster, though the ordering might not be desirable.

sql/create.go, line 134 [r1] (raw file):
Is there precedent for the leading underscore? Perhaps just ROWID for this column name. Btw, column names are case-insensitive and we generally use lowercase.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
If you added a new column to the table via an ALTER TABLE t ADD c INT statement, then the internal columns would be a, b, _rowid, c. Subsequent insertions would not work as expected. I think you need to tag the _rowid ColumnDescriptor as auto-generated so that it can be ignored for insertions and other operations.

sql/testdata/no_primary_key, line 26 [r1] (raw file):
You can do something like SELECT length(_rowid) FROM t to verify that the _rowid column exists.

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T19:35:01Z

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Done.

sql/create.go, line 134 [r1] (raw file):
Done.

sql/testdata/no_primary_key, line 26 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

tbg · 2015-12-02T19:36:59Z

sql/create.go

+	err = desc.AllocateIDs()
+	if err == errMissingPrimaryKey {
+		// Ensure a Primary Key exists.
+		s := "experimental_unique_int()"


nit: const s = "experimental_unique_int()"

ah, taking the address. That probably ruins it.

tamird · 2015-12-02T19:39:37Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
We should extract a constant, so that when this method is no longer "experimental", this doesn't rot.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T19:41:10Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
The test would fail if we removed the experimental_ prefix from the builtin without fixing this code. I fail to see how rot would occur.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T19:42:18Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
I'm fine with addressing this in a future PR, but it would good to add a test case demonstrating the problem to this PR.

Comments from the review on Reviewable.io

tamird · 2015-12-02T19:43:02Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
"Rot" was the wrong word.

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T19:49:44Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 130 [r1] (raw file):

cockroach/sql/structured.go

Line 246 in 28a6c07

index.ColumnIDs = append(index.ColumnIDs, 0)

Is where desc.PrimaryIndex.ColumnIDs is appended inside of the AllocateIDs func. The create function could go through and manually examine all indicies to see if any are primary, but it may be better to keep that logic in one place in AllocateIDs.

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T19:52:15Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
Where should I make this test case? If I make it in this PR, it'll have to be commented out with a comment explaining why. What about an open issue/PR with the breaking test case, that we can merge with the fix?

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T19:54:01Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
The address taking is indeed why you can't use a const here. I have no opinion on moving the string to a common place so it doesn't break during rename. I'm inclined to leave it as is.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T19:56:45Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
I'd add the test case to the end of this file. You don't have to comment it out, just expect the error (statement error ...) instead of success and add a TODO(mjibson): this should succeed comment.

Comments from the review on Reviewable.io

tamird · 2015-12-02T20:04:34Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 130 [r1] (raw file):
I'm not sure what that has to do with anything; why does it prevent you from checking ColumnNames?

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T20:05:46Z

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

tbg · 2015-12-02T20:07:51Z

Reviewed 1 of 2 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
hm, I thought we added this mostly for the sql tests and now it seems like we worry about performance in a way that would rather have us speed up ID generation ever so slightly, but instead have write hotspots for all such tables. I'm not invested in this, just curious why that's a good choice.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T20:10:52Z

LGTM

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
I'm not sure what the right choice is here. Certainly, the user shouldn't expect any ordering of the rows if they didn't specify a primary key. ID generation speed seems of small importance. The size of the ID field (16-bytes for the UUID vs 8-bytes for an INT) seems more important, but still relatively minor.

Comments from the review on Reviewable.io

maddyblue · 2015-12-02T20:43:18Z

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 130 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

tbg · 2015-12-02T20:59:34Z

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Suppose we could always have a version of experimental_unique_int() which reverses the bit order so that it should be distributed enough if we ever have high-performance users of this.

Comments from the review on Reviewable.io

tbg · 2015-12-02T20:59:46Z

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T21:09:17Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/testdata/no_primary_key, line 38 [r4] (raw file):
Not a big deal, but you could make the INSERT fail by giving column c the type STRING.

Comments from the review on Reviewable.io

petermattis · 2015-12-02T21:09:55Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Yeah. Might be worth having an internal_rowid() function.

Comments from the review on Reviewable.io

Fixes #2081

maddyblue · 2015-12-03T06:21:51Z

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

sql/create.go, line 132 [r1] (raw file):
I don't understand the problem with experimental_unique_int and why reversing the bit order matters.

sql/testdata/no_primary_key, line 38 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

petermattis · 2015-12-03T13:30:50Z

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
experimental_unique_int is composed of a timestamp+node-id. The timestamp occupies the high bits and thus causes the generated IDs to be mostly sequential (modulo clock skew between different nodes). Sequential IDs create a hotspot since insertion of rows into a table will all be directed at the same range. If we reversed the bits the writes would be scattered and could take advantage of more write bandwidth. On the downside, reversing the bits would enforce writes being scattered so the insertion of 2 rows would necessarily hit 2 ranges. I'm not sure if there is a best answer here.

Comments from the review on Reviewable.io

maddyblue · 2015-12-03T18:41:46Z

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
Makes sense. Are there objections to merging this PR, and we can choose to change the implementation later? I don't care one way or the other, but if we don't know which is better, maybe it would work to merge this and see how things go, then change it if it's bad.

Comments from the review on Reviewable.io

petermattis · 2015-12-03T18:43:10Z

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
I have no objection to merging as-is.

Comments from the review on Reviewable.io

sql: allow creation of tables with no primary key

tamird · 2015-12-03T19:05:19Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

tamird reviewed Dec 2, 2015
View reviewed changes

tbg reviewed Dec 2, 2015
View reviewed changes

maddyblue mentioned this pull request Dec 2, 2015

sql: hide rowid column from select and inserts #3293

Closed

sql: allow creation of tables with no primary key

949a538

Fixes #2081

maddyblue added a commit that referenced this pull request Dec 3, 2015

Merge pull request #3286 from mjibson/sql-no-primary-key

929047b

sql: allow creation of tables with no primary key

maddyblue merged commit 929047b into cockroachdb:master Dec 3, 2015

maddyblue deleted the sql-no-primary-key branch December 3, 2015 18:45

benesch added the first-pr Use to mark the first PR sent by a contributor / team member. Reviewers should be mindful of this. label May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: allow creation of tables with no primary key #3286

sql: allow creation of tables with no primary key #3286

maddyblue commented Dec 2, 2015

CLAassistant commented Dec 2, 2015

tamird commented Dec 2, 2015

tamird Dec 2, 2015

maddyblue Dec 2, 2015

tamird Dec 2, 2015

maddyblue commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg Dec 2, 2015

tbg Dec 2, 2015

tamird commented Dec 2, 2015

petermattis commented Dec 2, 2015

petermattis commented Dec 2, 2015

tamird commented Dec 2, 2015

maddyblue commented Dec 2, 2015

maddyblue commented Dec 2, 2015

maddyblue commented Dec 2, 2015

petermattis commented Dec 2, 2015

tamird commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg commented Dec 2, 2015

tbg commented Dec 2, 2015

petermattis commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 3, 2015

petermattis commented Dec 3, 2015

maddyblue commented Dec 3, 2015

petermattis commented Dec 3, 2015

tamird commented Dec 3, 2015

sql: allow creation of tables with no primary key #3286

sql: allow creation of tables with no primary key #3286

Conversation

maddyblue commented Dec 2, 2015

CLAassistant commented Dec 2, 2015

tamird commented Dec 2, 2015

tamird Dec 2, 2015

Choose a reason for hiding this comment

maddyblue Dec 2, 2015

Choose a reason for hiding this comment

tamird Dec 2, 2015

Choose a reason for hiding this comment

maddyblue commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg Dec 2, 2015

Choose a reason for hiding this comment

tbg Dec 2, 2015

Choose a reason for hiding this comment

tamird commented Dec 2, 2015

petermattis commented Dec 2, 2015

petermattis commented Dec 2, 2015

tamird commented Dec 2, 2015

maddyblue commented Dec 2, 2015

maddyblue commented Dec 2, 2015

maddyblue commented Dec 2, 2015

petermattis commented Dec 2, 2015

tamird commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 2, 2015

tbg commented Dec 2, 2015

tbg commented Dec 2, 2015

petermattis commented Dec 2, 2015

petermattis commented Dec 2, 2015

maddyblue commented Dec 3, 2015

petermattis commented Dec 3, 2015

maddyblue commented Dec 3, 2015

petermattis commented Dec 3, 2015

tamird commented Dec 3, 2015