sql: support for RETURNING *

[RaduBerinde]

This change adds support for RETURNING * in inserts and deletes. For updates we
currently only expand the updated columns; this is related to an existing
problem (we can't return other columns even explicitly; filed #4645).

Fixes #4593.

@knz @mjibson

[tamird]

Haven't reviewed the implementation yet, will do that this afternoon. Glad to see this fixed, though!

---

Reviewed 4 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

sql/logic_test.go, line 555 [r1] (raw file):
why do you need to do this? seems odd to split column names on whitespace.

---

sql/testdata/insert, line 362 [r1] (raw file):
is this different from the INSERT INTO return VALUES (1) RETURNING * case?

---

sql/testdata/update, line 190 [r1] (raw file):
nit: multiple spaces here

---

sql/testdata/update, line 197 [r1] (raw file):
here too

---

Comments from the review on Reviewable.io

[RaduBerinde]

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

---

sql/logic_test.go, line 555 [r1] (raw file):
To support whitespaces in column names, it's the same odd way we support whitespaces in results (copied from below). Without this we get an error like

    logic_test.go:654: testdata/insert:355: expected:
        "a"  "b"  "a"  
        "+"  "b"  "1"  
        "2"  "3"  "3"  
        "4"  "7"  
        but found:
        "a"  "b"  "a + b"  
        "1"  "2"  "3"      
        "3"  "4"  "7"      

---

sql/testdata/insert, line 362 [r1] (raw file):
In that one we are only inserting to one column. I added the VALUES (1) one to check if this is a problem (like it is for updates).

---

Comments from the review on Reviewable.io

[knz]

LGTM

---

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

---

Comments from the review on Reviewable.io

[tamird]

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

---

sql/logic_test.go, line 555 [r1] (raw file):
OK. Can you update the comment to refer to whitespace in column names rather than results?

---

sql/returning.go, line 38 [r1] (raw file):
super nit: we usually name functions that return values (rather than pointers) makeFoo rather than newFoo

---

sql/returning.go, line 53 [r1] (raw file):
remove this newline

---

sql/select.go, line 373 [r1] (raw file):
s/prefix/suffix/

nit: golint doesn't care because this isn't exported, but for consistency please start the comment with the function name.

---

sql/select.go, line 378 [r1] (raw file):
the named returns here serve as nice documentation, but it makes me nervous about naked returns. doesn't look like you're using any, but I just wanted to point out the possibility for abuse.

---

sql/select.go, line 379 [r1] (raw file):
nit: remove this newline

---

sql/select.go, line 399 [r1] (raw file):
nit: doesn't look like this is used beyond the next if block, could be nice to reduce its lifetime:

if tablename := ...; tablename != ""....

---

sql/testdata/insert, line 362 [r1] (raw file):
OK. Maybe put these cases next to each other, since it's not clear why they're ordered as they are now.

---

Comments from the review on Reviewable.io

[tamird]

LGTM

---

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

---

Comments from the review on Reviewable.io

[knz]

Wait please add a test using Prepare as well. The lesson learned with #4610 is that prepare != execute.

---

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

---

Comments from the review on Reviewable.io

[RaduBerinde]

I can't until #4635 is merged

---

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

---

sql/select.go, line 379 [r1] (raw file):
I'd like to keep it, it separates the function declaration from the code. I wish gofmt would append two tabs instead of one when we break up a declaration line.

---

sql/testdata/insert, line 362 [r1] (raw file):
Moved and added a comment.

---

Comments from the review on Reviewable.io

[maddyblue]

LGTM

---

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

---

sql/select.go, line 382 [r2] (raw file):
Any problem with using a naked return here? Some people dislike them, which is a fine argument.

---

sql/select.go, line 384 [r2] (raw file):
Could omit the : here because err is already declared and in a parent scope, then use a naked return below. Or not, I don't feel strongly.

---

Comments from the review on Reviewable.io

[tamird]

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

---

sql/select.go, line 379 [r1] (raw file):
I appreciate that, but I would prefer to stick to the pervasive style for now. We should discuss this in the context of the upcoming style guide. cc @bdarnell

---

Comments from the review on Reviewable.io

[RaduBerinde]

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

---

sql/select.go, line 379 [r1] (raw file):
The situation of a multi-line function definition is anything but pervasive, but ok. I think we're wasting too much time with these nits btw.

---

sql/select.go, line 382 [r2] (raw file):
I think they can be error prone (I put variable names in just for documentation). The official go tour says they should be only used in short functions.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 8 of 9 files reviewed at latest revision, 4 unresolved discussions.

---

sql/select.go, line 379 [r1] (raw file):
Noted. FWIW, I agree with Radu: a blank line can add clarity when the declaration spans multiple lines (unless gofmt were changed to double-indent continuation lines).

---

Comments from the review on Reviewable.io