Skip to content

Conversation

uptimeDBA
Copy link
Contributor

@uptimeDBA uptimeDBA commented May 17, 2016

A bit of a moving target this one, and I broke something using git but I think I fixed it.
Some links are dependent on the constraints documentation that is still under review.


This change is Reviewable

@dt dt mentioned this pull request May 17, 2016
@@ -1,22 +1,111 @@
---
title: CREATE TABLE
toc: false
toc_not_nested: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the contributing.md file asked for toc_not_nested at some point, but it's no longer needed.

@jseldess
Copy link
Contributor

Thanks for this work, @uptimeDBA! I added some comments and will find the right devs to review for you. As with the other PR, once this is good-to-go from a correctness standpoint, I'll merge and do a copy edit in another PR.

@knz
Copy link
Contributor

knz commented May 17, 2016

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


create-table.md, line 31 [r9] (raw file):

The [name](data-definition.html#identifiers) (optionally schema-qualified) of the table to be created. Table names are required to be unique within a database.

The `UPSERT` and `INSERT INTO ... ON CONFLICT` statements make use of a table called `EXCLUDED` to handle data conflicts during execution. It's therefore not recommended to use the name `EXCLUDED` for any of your tables.

This is a relatively obscure feature for most readers. If you want to make this point, please support it with examples (and/or counter-examples).


create-table.md, line 87 [r9] (raw file):

As the syntax diagram suggests, you can create a table without any columns like you can in PostgreSQL. The table does have a column called `rowid` that was added because the table does not have a `PRIMARY KEY` or `UNIQUE` defined.

You have already refered above to the fact that a rowid column is implicitly created if there is no primary key. This is clear. However it is not clear here why the UNIQUE constraint makes a difference. Please clarify more.

Also it would be interesting to make a comment as to why/how a table with no columns could be useful.


Comments from Reviewable

@bdarnell
Copy link
Contributor

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


create-table.md, line 31 [r9] (raw file):

Previously, knz (kena) wrote…

This is a relatively obscure feature for most readers. If you want to make this point, please support it with examples (and/or counter-examples).

If you do have a table named "excluded", can you work around the conflict with `insert into excluded as excluded_table on conflict foo do update set excluded_table.x=excluded.x`? As long as that works it might be better to document this workaround in the `insert on conflict` docs instead of a much more prominent warning against tables named "excluded"

create-table.md, line 87 [r9] (raw file):

Previously, knz (kena) wrote…

You have already refered above to the fact that a rowid column is implicitly created if there is no primary key. This is clear. However it is not clear here why the UNIQUE constraint makes a difference. Please clarify more.

Also it would be interesting to make a comment as to why/how a table with no columns could be useful.

The rowid column is created whenever there is no primary key; it doesn't matter whether there are unique columns.
root@:26257> create table t2 (a int unique);
CREATE TABLE
root@:26257> show columns from t2;
+-------+------+-------+----------------+
| Field | Type | Null  |    Default     |
+-------+------+-------+----------------+
| a     | INT  | true  | NULL           |
| rowid | INT  | false | unique_rowid() |
+-------+------+-------+----------------+

A table with no columns is useful for finding bugs in client libraries (i just found one in the go lib/pq driver), but i'm not sure if it's ever legitimately useful (it's also possible to create such a table by dropping the last column).

root@:26257> create table t (a int);
CREATE TABLE
root@:26257> insert into t values (1);
INSERT 1
root@:26257> alter table t drop column a;
ALTER TABLE
root@:26257> select * from t;
pq: unexpected DataRow in simple query execution

Comments from Reviewable

@uptimeDBA
Copy link
Contributor Author

Review status: 0 of 33 files reviewed at latest revision, 7 unresolved discussions.


create-table.md, line 4 [r9] (raw file):

Previously, jseldess wrote…

I know the contributing.md file asked for toc_not_nested at some point, but it's no longer needed.

Done.

create-table.md, line 20 [r9] (raw file):

Previously, jseldess wrote…

Because the sql diagram parameters do not always have specific, helpful names, I've been avoiding this type of list in favor of a "Usage" section. But the parameter names are sufficiently clear in this case, so I understand why you've built in this section.

For consistency, though, please rename it to "Parameters" and try to do this as a table.

I've converted the Semantics section into a table and renamed it Parameters. Looks ok in html but it's very messy in markdown as the formatting options seem limited inside a table. Take a look and see what you think.

create-table.md, line 31 [r9] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

If you do have a table named "excluded", can you work around the conflict with insert into excluded as excluded_table on conflict foo do update set excluded_table.x=excluded.x? As long as that works it might be better to document this workaround in the insert on conflict docs instead of a much more prominent warning against tables named "excluded"

Good work around. Should probably be mentioned as part of the `INSERT ... ON CONFLICT DO UPDATE` documentation.

create-table.md, line 49 [r9] (raw file):

Previously, jseldess wrote…

Since we're defining the parameters above, I don't think we need a "Usage" section. I'd rather have more detailed examples (see next comment). The notes about primary key and replication zones are helpful, so perhaps just add them to the general description at the top?

"Usage" section removed with examples added to the Examples section. Replication Zone note added to the top description.

create-table.md, line 87 [r9] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The rowid column is created whenever there is no primary key; it doesn't matter whether there are unique columns.

root@:26257> create table t2 (a int unique);
CREATE TABLE
root@:26257> show columns from t2;
+-------+------+-------+----------------+
| Field | Type | Null  |    Default     |
+-------+------+-------+----------------+
| a     | INT  | true  | NULL           |
| rowid | INT  | false | unique_rowid() |
+-------+------+-------+----------------+

A table with no columns is useful for finding bugs in client libraries (i just found one in the go lib/pq driver), but i'm not sure if it's ever legitimately useful (it's also possible to create such a table by dropping the last column).

root@:26257> create table t (a int);
CREATE TABLE
root@:26257> insert into t values (1);
INSERT 1
root@:26257> alter table t drop column a;
ALTER TABLE
root@:26257> select * from t;
pq: unexpected DataRow in simple query execution
I've removed the no-column table example. I was intrigued that you could even do this. In 20+ years as an operational DBA/architect, I've never seen a no-column table. As you say, it's so left-field, it's not worth a mention.

create-table.md, line 109 [r9] (raw file):

Previously, jseldess wrote…

Broken link here. Though I don't think we have a page for SHOW CREATE TABLE yet, so you should probably just remove this one.

Done.

Comments from Reviewable

@jseldess
Copy link
Contributor

LGTM, Paul. I'll merge now so we can include in this week's release notes. I'll open a separate PR to tweak some language/formatting, but nothing urgent.

@jseldess jseldess merged commit 3a356f2 into cockroachdb:gh-pages May 19, 2016
@uptimeDBA uptimeDBA deleted the 256-create-table branch June 1, 2016 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants