sql: make ParseQualifiedTableName do what it's meant to do

[knz]

Prior to this patch, ParseQualifiedTableName() was using
ParseTableNameWithIndex and thus was overly generous in the syntax it
accepted. Worse than that, it would even silently drop the index part,
so one could e.g. compute nextval('someseq@invalidindex') without
error.

This patch corrects this oversight by ensuring the method only parses
table names.

Release note (bug fix): various primitives that expect table names as
argument now properly reject invalid table names.

[cockroach-teamcity]

This change is 

[m-schneider]

pkg/sql/parser/parse.go, line 105 at r1 (raw file):

ALTER TABLE %s RENAME TO x
Slightly confused abut this, it's renaming the table passed in to x?

---

Comments from Reviewable

[knz]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/parser/parse.go, line 105 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

ALTER TABLE %s RENAME TO x
Slightly confused abut this, it's renaming the table passed in to x?

The idea is that we're merely parsing that statement. since the SQL language (and thus its grammar) do not support using a table name at the top level, we need a well-known grammar rule that accepts table name in its syntax, and then after parsing extract that part.

See ParseTableNameWithIndex above which does the same thing but for an index name.

Does this clarify?

---

Comments from Reviewable

[m-schneider]

pkg/sql/parser/parse.go, line 105 at r1 (raw file):

Previously, knz (kena) wrote…

The idea is that we're merely parsing that statement. since the SQL language (and thus its grammar) do not support using a table name at the top level, we need a well-known grammar rule that accepts table name in its syntax, and then after parsing extract that part.

See ParseTableNameWithIndex above which does the same thing but for an index name.

Does this clarify?

Yep makes sense!

---

Comments from Reviewable

[m-schneider]

---

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

---

Comments from Reviewable

[knz]

Thanks!

[andreimatei]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/parse.go, line 105 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

Yep makes sense!

I know you've copied this from somewhere, but this obviously needed a comment :)
Do you reckon there's some different way to use the parser generated code to parse a table name more narrowly?

---

Comments from Reviewable

[knz]

ALTER TABLE RENAME is the smallest syntax element that contains a table name. Every other rule involving a table name is much more complex to parse. I don't understand your concern?

[andreimatei]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/parse.go, line 105 at r1 (raw file):
Copying your answer here. Please don't reply to a code comment with a top-level comment; the discussion threads make no sense :)

ALTER TABLE RENAME is the smallest syntax element that contains a table name. Every other rule involving a table name is much more complex to parse. I don't understand your concern?

A qualified name is also a "syntax element"; perhaps not a top-level one. I'm asking if you think there's a way to ask the yacc-generated parser to parse using a smaller grammar - basically to tell it what rule we want it to use.
I don't have a particular "concern", but this code is clearly bizarre.

---

Comments from Reviewable

[andreimatei]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/parse.go, line 105 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Copying your answer here. Please don't reply to a code comment with a top-level comment; the discussion threads make no sense :)

ALTER TABLE RENAME is the smallest syntax element that contains a table name. Every other rule involving a table name is much more complex to parse. I don't understand your concern?

A qualified name is also a "syntax element"; perhaps not a top-level one. I'm asking if you think there's a way to ask the yacc-generated parser to parse using a smaller grammar - basically to tell it what rule we want it to use.
I don't have a particular "concern", but this code is clearly bizarre.

sent #24132 adding a small comment

---

Comments from Reviewable

[knz]

Op 21-03-18 om 23:14 schreef Andrei Matei:

A qualified name is also a "syntax element"

No. "Qualified names" do not exist in the 2.0 grammar any more. For good reason.

I'm asking if you think there's a way to ask the yacc-generated parser to parse using a smaller grammar - basically to tell it what rule we want it to use.

No this is not possible. The LALR generator underlying go-yacc is unable to do this. If you want a different grammar, you'd need to make a separate .y file containing just the sub-rule that you want. There'd need to be Makefile magic to ensure there's no duplication of code between the two .y files (go-yacc does not support include directives). Ugly all around.

I don't have a particular "concern", but this code is clearly bizarre.

This is clearly common in the world of LALR parser generators. I have a couple good books to refer you to if you're interested.

…

-- Raphael 'kena' Poss