sql: Initial support for creating and dropping views

[a-robinson]

There's still quite a bit of work needed to finish off #2971, but this adds the basic framework for creating and dropping views.

The main TODOs outstanding are:

• Manage references between views and their dependencies to support proper CASCADE/RESTRICT behavior
• Handle views in queries
• ALTER VIEW and CREATE OR REPLACE VIEW
• Tests (beyond the basic parsing tests)

I'm sending this out now since it can somewhat stand on its own to avoid sending out a giant PR later, but let me know if you'd prefer to just see it all at once later.

cc @paperstreet @dt @nvanbenschoten - I'm just guessing at who to assign. Feel free to reassign, hopefully no hard feelings :)

---

This change is

[dt]

I'd love to see a testdata/view with some logic tests too:
creating/dropping views, name collisions with tables, mismatched col counts, etc

Also, I know this is a first-step, but I'm a little worried about adding writing of table descriptors to where they can be found and retrieved by other code paths that don't know about / handle them yet. Off the top of my head, does ALTER need to also check IsTable and throw errors? CREATE INDEX?

Does it make sense to add a IsTable check and error to the common table descriptor accessors (getTableLease, etc) and whitelist only the callsites where we do want to resolve views?

---

Reviewed 11 of 13 files at r1, 8 of 9 files at r2, 1 of 1 files at r3.
Review status: 6 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

  }

  if desc.Adding() {

fwiw, afaik, this will never be true right now, since neither makeViewTableDesc nor any code above sets the state to ADD (though when you get to adding back-refs that'll change)

---

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

  }

  // TODO(a-robinson): Is this sufficient to retain the correctness of the

Yeah, when saving an expr to disk (e.g. in CHECK) we use Format (vs the raw original verbatim) -- one benefit of that is that we can then always parse them as Traditional syntax, without having to store/plumb the session syntax that created them.

---

sql/create.go, line 974 [r3] (raw file):

  for _, colRes := range resultColumns {
      colType, _ := parser.DatumTypeToColumnType(colRes.Typ)
      columnTableDef := parser.ColumnTableDef{Name: parser.Name(colRes.Name), Type: colType}

Does this not need to use ToCols ?

---

sql/sqlbase/structured.go, line 874 [r3] (raw file):

  // Only validate the indexes if this is actually a table, not if it's
  // just a view.
  if desc.ViewQuery == "" {

nit: could reuse the IsTable helper above

---

Comments from Reviewable

[a-robinson]

Yeah, we should add a check to the common accessors, since this could definitely cause some trickiness. I'll work on that next - it might take a little longer than expected since I'll have to figure out what/where all the accessors are.

I got parsing and logic tests added.

Thanks for the review!

---

Review status: 6 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, dt (David Taylor) wrote…

fwiw, afaik, this will never be true right now, since neither makeViewTableDesc nor any code above sets the state to ADD (though when you get to adding back-refs that'll change)

Ack. Should I take this out until I have back-refs added or just leave it in?

---

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

Previously, dt (David Taylor) wrote…

Yeah, when saving an expr to disk (e.g. in CHECK) we use Format (vs the raw original verbatim) -- one benefit of that is that we can then always parse them as Traditional syntax, without having to store/plumb the session syntax that created them.

Thanks, removed the TODO and kept the Format call (switched from FormatNode to calling Format directly).

---

sql/create.go, line 974 [r3] (raw file):

Previously, dt (David Taylor) wrote…

Does this not need to use ToCols ?

Good catch, done.

---

sql/sqlbase/structured.go, line 874 [r3] (raw file):

Previously, dt (David Taylor) wrote…

nit: could reuse the IsTable helper above

Done.

---

Comments from Reviewable

[a-robinson]

PTAL @dt.

I've updated the code that gets table descriptors to not return views unless asked and prevented anyone from taking a lease on a view descriptor for the time being, although IIUC that part will have to change as I add support for querying from and/or altering views.

---

Review status: 4 of 26 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[dt]

---

Reviewed 2 of 13 files at r7, 8 of 9 files at r8, 1 of 1 files at r9, 2 of 3 files at r11, 1 of 2 files at r12, 8 of 8 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

sql/create.go, line 312 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Ack. Should I take this out until I have back-refs added or just leave it in?

I'm fine with leaving it.

---

Comments from Reviewable

[a-robinson]

Thanks for the review! I've rebased on top of your #9327 which made a couple similar cleanups and switched to your new pattern of calling makeTableDesc only after checking for already exists errors and generating an ID.

---

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

---

Comments from Reviewable