adapter: firm up prohibited statements in /api/sql

[sploiselle]

/api/sql cannot handle many types of ExecuteResponses; however,
it only receives statements as input. To make sure we don't
accept statements whose responses we can't handle, this commit
introduces a method to try to formalize the types of responses
each statement can generate.

Motivation

This PR adds a known-desirable feature. MaterializeInc/database-issues#4025

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered.

• This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-protobuf label.

• This PR includes the following user-facing behavior changes:

  • there are no user-facing behavior changes

[benesch]

Neat! Can you add a soft_assert! somewhere in the execute path that verifies that observed execute response for all real queries is contained in the set declared by expected_statement_responses? That will ensure that expected_statement_responses doesn't get out of sync.

[sploiselle]

This approach is blocked on rust-lang/rust-clippy#9037 making it into stable because:

error: this lifetime isn't used in the impl
   --> src/adapter/src/command.rs:161:22
    |
161 | #[derive(Derivative, EnumKind)]
    |                      ^^^^^^^^
    |
    = note: requested on the command line with `-D clippy::extra-unused-lifetimes`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes
    = note: this error originates in the derive macro `EnumKind` (in Nightly builds, run with -Z macro-backtrace for more info)

[maddyblue]

I'm not convinced this is a good direction to solve this problem. It relies on us correctly keeping multiple big lists of enums to be correct in a way that people adding or changing syntax might not know they need to go and poke at. For example, UPDATE in this PR not being told it might return rows someday. If we left UPDATE as is here, then when it was taught to return rows, there would be no test or compile failure. If a dev adds or changes an ExecuteResponse return from some statement execution, it's totally unknown that they need to also update these functions.

An idea (maybe bad): have the simple query function parse statements and have a single match statement that errors if the parsed statement type can't be executed by it. Then the response serializer can call unreachable for unexpected responses. This concentrates the logic for "can this be run over the web" into a single place directly where it's needed. It's still brittle, but I think less brittle than this approach. It does suffer from the same big class of flaw here, though: if a dev adds or changes the ExecuteResponse produced by a Statement variant, there's no test or compile error that would inform them they also need to change this code. Hmm.

[maddyblue]

This might also return rows someday. Statement::Delete has these.

[sploiselle]

A more robust option is:

1. Do a Statement -> Plan -> Vec mapping
2. When sequencing, stick the permitted ExecuteResponse in the ClientTransmitter
3. When sending, soft_assert that the value is of the permitted type

This would catch anything that fell out of date as long as it ran through some test somewhere.

This has the disadvantage of:

• Requiring more correlating enums
• Not necessarily updated if statements "lose" valid ExecuteResponses

@mjibson if this is more amenable to you, lmk, otherwise I'll just close the issue as not something worth completing.

[benesch]

FWIW, I'm very pro doing something like this to improve the current code. The current situation sketches me out.

[maddyblue]

We spoke over slack, and I agree with the above proposal.

[sploiselle]

Closing in favor of #14508