Reviewed 1 of 1 files at r1, 2 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @knz)

-- commits, line 86 at r2:
Why is this a concern on this endpoint in particular, and not on all the existing APIs?
Using cookies is much more ergonomic for a browser app so I'd definitely prefer for this API to support that as well.

pkg/server/api_v2_sql.go, line 190 at r2 (raw file):

	// Parse the parameters.
	if err := r.ParseForm(); err != nil {

might be nice to also take args as a JSON object.

pkg/server/api_v2_sql.go, line 237 at r2 (raw file):

	// we saw and call it a day.
	if r.PostForm.Get("execute") == "" {
		w.Header().Add("Content-Type", "text/plain; charset=utf-8")

nit: probably nicer to support JSON on all responses for consistency.

Because the other endpoints do not modify data in the database. They only retrieve it.

And this one is unrestricted in its ability to modify the database. It's the most sensitive thing we've ever exposed over HTTP.

bdarnell can you chime in here? the question at hand is whether we should continue to use a separate header for CSRF protection (as in the PR currently) or whether we can make-do with a session cookie.

I'd need someone from the security team to weigh in here. In the past, cookies were not sufficient and an HTTP header was required. The introduction of the samesite cookie attribute was intended to make it feasible to use cookies without worrying about CSRF, but I'm not sure about current real-world deployment.

The entire APIv2 tree uses a separate header (and not a cookie) to avoid CSRF. I'm following pattern here.

I think this is a sufficient justification for sticking with the separate header even if samesite cookies are also a solution. We should be consistent with the rest of APIv2 and (potentially) introduce a browser-friendly mode of API access for everything at once.

The original argument still holds: the question is really who pays for storing the serialized session state and how long.

I think there are two distinct cases: idle session state (mainly SET variables) and transactional state. For transactional state, we don't want to make it too easy to have long-running transactions that hold on to locks, so I think it's fine if open transactions keep the SQL pod alive.

For session variables, it makes sense to keep these on the client side. But it may make even more sense to model them not as a cookie-like blob that the server returns but a set of parameters to the request. If you could just pass session variables in the HTTP request you could use them the same way whether it's the first request of a "session" or not and you don't need to parse the results and retain anything.

If we let the end-user's web browser do it, then the session state goes away when they close their browser's tab.

But we don't reliably find out when they close the browser, which is important for cleaning up transactional state. An API for transactions from the browser should perhaps use websockets instead of HTTP req/resp.

Reviewed 5 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @knz)

pkg/server/api_v2_sql.go line 49 at r2 (raw file):

// implicit transaction.
//
// If multiple SQL statements are specified, they are executed using a

Multiple SQL statements can make it easier to exploit SQL injection; many drivers place restrictions on the ability to use multiple statements in one string for this reason. We may want to add an argument multi_statement= (default false) so that a client must opt in to this multi-statement mode for a little extra safety.

pkg/server/api_v2_sql.go line 59 at r2 (raw file):

// ---
// parameters:
// - name: sql

Possible future extension: allow the sql string to contain placeholders and have a separate argument array for their values, to allow for safe parameterization without client-side knowledge of the sql grammar.

pkg/server/api_v2_sql.go line 190 at r2 (raw file):

Be consistent with the rest of APIv2

pkg/server/api_v2_sql.go line 224 at r2 (raw file):

		dbName = "defaultdb"
	}
	showCols := r.PostForm.Get("show_columns_always") != ""

Why is this an option? Seems to me that it's better to always return the columns even if there are no rows.

Nits: return instead of show, parse it as a bool or 1/0 instead of using empty vs non-empty.

pkg/server/api_v2_sql.go line 236 at r2 (raw file):

	// If the client did not request execution, just print what
	// we saw and call it a day.
	if r.PostForm.Get("execute") == "" {

It's weird that execution defaults to off.

pkg/server/api_v2_sql.go line 237 at r2 (raw file):

Agreed (including all errors)

pkg/server/api_v2_sql.go line 254 at r2 (raw file):

	// We can't use json.Marshal for the outer shell of the results, because
	// we want to stream the results and avoid accumulating results in RAM.
	fmt.Fprintf(w, "{\"num_statements\": %d,\n \"execution\": {\n", len(statements))

Constructing json by hand here makes me very nervous, especially when considering unwinding the stack to output errors in the json payload. You've already got a size limit with a small default; I'd prefer to buffer everything and encode normally. We can consider streaming when/if we decide we need to handle larger result sets (at which time we'll also need to consider how the client will be consuming them - many clients also have difficulties parsing json in a streaming fashion. I think a format other than json may be appropriate at that point, or maybe one of the json wrappers in https://en.wikipedia.org/wiki/JSON_streaming).

pkg/server/api_v2_sql.go line 318 at r2 (raw file):

					}()

					it, err := a.admin.ie.QueryIteratorEx(ctx, "run-query-via-api", txn,

Have we ever used the internal executor for user-specified code before? This seems like a risk of crashing because the user stumbles across something not supported by the internal executor, or invalid security assumptions, etc.

pkg/server/api_v2_sql.go line 334 at r2 (raw file):

							// NB: we need to place the number of rows affected inside
							// quotes because json integers must be 53 bits or less and
							// the row affected count may be 64 bits.

Can the row affected count really be more than 53 bits? That's an awfully big number.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @knz)

pkg/server/api_v2_sql.go line 59 at r2 (raw file):

After a little more thought, this is really important. I wouldn't feel comfortable shipping this without supporting placeholder-based usage.

I think there are two distinct cases: idle session state (mainly SET variables) and transactional state.

I agree on the distinction between idle and live txn. I also believe that the requesters of this feature only care about preserving the idle session state (the same case we already implemented session serialization for).

However, you're incorrect that idle sessions are only about SET variables. They also contain:

• prepared statements.
• sequence cache.

Additionally, I'm not sure all the remaining fields in the in-memory (and serializable) session state can be re-populated using SET statements. Someone from XP would need to confirm.

If you could just pass session variables in the HTTP request you could use them the same way whether it's the first request of a "session" or not and you don't need to parse the results and retain anything.

I agree we should support setting session variables in the HTTP request. It's not done here yet because we're still using the InternalExecutor, but as soon as that changes we can do session variable overrides.

However if we wish to use this interface to e.g. support a SQL shell, we need capturing more of the session state.

But we don't reliably find out when they close the browser, which is important for cleaning up transactional state.

I'm pretty sure we're not interested in offering a HTTP interface that leaves transactions open (or adds more statements on an open txn).

I've also taken note of your other review comments. I agree with all of them and hope to address them in a later revision to the PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @dhartunian, and @knz)

pkg/server/api_v2_sql.go line 254 at r2 (raw file):

The choice I made here is because we're converting the SQL datums to pkg/util/json.JSON, which do not implement go's json.Marshaler interface. So to do what you say, we'd need to do this:

1. convert datum to crdbjson.JSON
2. serialize crdbjson.JSON to string
3. parse string via gojson.Unmarshal
4. add resulting datum into result tree
5. at the very end, re-serialize everything

The current implementations skip steps 3-5.

We can do what you say by adding proper json marshaling support to crdb's util/json package. I think it's out of scope for this MVP.

pkg/server/api_v2_sql.go line 318 at r2 (raw file):

there's no panic inside the internal executor, all errors flow out.

pkg/server/api_v2_sql.go line 334 at r2 (raw file):

Are you interested to make that call now and set it in stone in the API spec? I wasn't comfortable doing so and thought the string syntax was more generic.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @dhartunian, and @knz)

pkg/server/api_v2_sql.go line 49 at r2 (raw file):

Instead of semicolons the interface now requires the statements to be passed as separate JSON payloads. So this point is moot.

pkg/server/api_v2_sql.go line 59 at r2 (raw file):

Done.

pkg/server/api_v2_sql.go line 190 at r2 (raw file):

Done.

pkg/server/api_v2_sql.go line 224 at r2 (raw file):

Removed.

pkg/server/api_v2_sql.go line 236 at r2 (raw file):

I found it easier to use to troubleshoot interactively this way. I don't have hard feelings either way.

pkg/server/api_v2_sql.go line 237 at r2 (raw file):

Done.

Are folk happy with the interface / UX? If so, I will implement some unit tests and mark the PR ready for review.

👍 I'd like to merge this and then port one of our features to it. I successfully prototyped a SQL-execution UI feature in DB Console using an earlier iteration of the PR and found the API easy to work with.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @dhartunian)

-- commits line 86 at r2:

Retracting this. I understand the motivation provided for using the header and we can keep it.

@knz just pushed tests up. Looks good to merge unless you have more cases for me to cover.

One thing that I was a bit surprised by during testing was having a top-level "error" field and a per-statement error. In the case of SQL errors, the object is repeated once at the top of the JSON blob and again inside the statement result. Feels a bit redundant. I understand the purpose of each since we can have non-statement errors and am wondering if in the case of SQL errors we should only have them within the result.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @dhartunian)

LGTM modulo the lint nit that CI is telling you about.

In the case of SQL errors, the object is repeated once at the top of the JSON blob and again inside the statement result.

The design snag this addresses is that there can be multiple statements executed, some of them succesful followed by a problem. For example, select 1; select 2; select foo can return two results and then an error.

The current error presentation ensures that it's clear which of the statement the error pertains to; and it also lets the statements before it return an error.

I'm interested in your opinion on how to present this differently.

Reviewed 1 of 5 files at r4, 3 of 5 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @dhartunian)

pkg/server/api_v2_sql_test.go line 85 at r6 (raw file):

				return fmt.Sprintf("%s|%s", er.Error.Code, er.Error.Message)
			}
			return string(r)

can I suggest you unmarshal the response in every case,
then re-marshal it with indentation for the expected output. This will make the expected output much more readable.

- thanks for adding tests to help drive this home @dhartunian

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @dhartunian)

I'm interested in your opinion on how to present this differently.

My only idea was to omit a top level error in the case where the errors are attributable to a statement and only show the error in one place. But I think this is fine to keep for now. Once there's more usage by teams we can see if it causes confusion.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell, @dhartunian, and @knz)

pkg/server/api_v2_sql_test.go line 85 at r6 (raw file):

Done.

👏 👏

Reviewed 1 of 5 files at r4, 3 of 3 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell)