Binary Protocol Clenaup

[1st1]

Headers & Message Layout

Our binary protocol message structure is inherited from PostgreSQL. The motivation for that is to avoid repacking data messages coming from Postgres to our own format. Back when we designed the very first version of our protocol we made a decision to add "headers" to basically all protocol messages. The intention was to use headers to include auxiliary data, like tracing & debug information. We could introduce such headers without requiring all clients to bump the protocol version.

At some point, however, we started to abuse the message headers, adding headers that can change the output structure, control what capabilities the query is allowed to use, etc. That blurs the difference between headers & message fields and makes both the client and the server code unnecessarily messy. Most importantly, we still had to bump the protocol version and change the parsing logic with every new header we've introduced so far. And lastly, headers use their own ad-hoc value serialization format, different from the rest of the protocol.

I propose to:

• Fold the following headers into message fields: ALLOW_CAPABILITIES, IMPLICIT_LIMIT, IMPLICIT_TYPENAMES, IMPLICIT_TYPEIDS, EXPLICIT_OBJECTIDS.

• Declare that we never again use headers for purposes other than sending auxiliary data (such as metrics and debug information).

• When adding new capabilities to existing protocol messages we will add them as message fields to the end of the message, bumping the protocol version.

While we are there, the CommandComplete message does not need the CAPABILITIES header. Instead, OptimisticExecute must have the capabilities field, which must be matched against the cached query.

For example, this is what we have now:

struct Prepare {
  // Message type ('P').
  uint8           mtype = 0x50;

  // Length of message contents in bytes,
  // including self.
  uint32          message_length;

  // A set of message headers.
  uint16          num_headers;
  Header          headers[num_headers];

  // Data I/O format.
  uint8<IOFormat> io_format;

  // Expected result cardinality
  uint8<Cardinality> expected_cardinality;

  // Prepared statement name. Currently must
  // be empty.
  bytes           statement_name;

  // Command text.
  string          command;
};

And this is what we should have:

struct Prepare {
  // Message type ('P').
  uint8           mtype = 0x50;

  // Length of message contents in bytes,
  // including self.
  uint32          message_length;

  // A set of message headers.
  uint16          num_headers;
  Header          headers[num_headers];

  // Prepared statement name. Currently must
  // be empty.
  bytes           statement_name;

  // Data I/O format.
  uint8<IOFormat> io_format;

  // Expected result cardinality
  uint8<Cardinality> expected_cardinality;

  int32              implicit_limit;
  uint64             allowed_capabilities;
  uint64             implicit_fields_mask;

  // Command text.
  string          command;
};

Drop ExecuteScript. Enhance the Parse/Execute Flow.

Historically, our ExecuteScript message was based on Postgres' SimpleQuery, basically inheriting all of its warts to this day:

• Does not support arguments.
• Does not allow to "prepare" the script or use the optimistic flow for it.
• Until recently allowed to mix transaction control commands with other commands in the script.

Over the course of working with the protocol it became apparent that the right solution would be to simply allow our Prepare and OptimisticExecute messages to accept EdgeQL scripts. Scripts will have some limitations:

• Scripts cannot return data.
• Scripts cannot mix transaction control commands with other commands or have more than one transaction command.

Other than that, we can implement the very similar flow we have for regular commands with Prepare/Execute and OptimisticExecute. Both Prepare and OptimisticExecute will get a new boolean field: script_mode, which when set would allow them to accept a sequence of commands.

Backwards Compatibility

Let me state the obvious: the first part of this proposal will be somewhat painful to implement. We'll have to update all of our binding libraries (JS, Python, Go) and our CLI. The protocol version will have to be bumped. The server-side protocol parsing code will blow up in size as we'll have to support both the old protocol and the new protocol for at least one version.

The second part of the proposal, however, does not have any backwards compatibility issues. Drivers will stop using the deprecated ExecuteScript message and allow users to pass arguments to their scripts (a new feature). As for script-mode restrictions, we already implement them in ExecuteScript.

[tailhook]

Dropping ExecuteScript is fine.

Regarding header changes, I think this is not justified in the form stated here. On the contrary, I would gather all the parameters that has to be submit back and compared into a single "dictionary". I.e. we could split "headers" into "metadata" and "extensions". Adding individual struct fields "to the end of the message" is weird in such places as Prepare where new fields will come after command (right?).

Also current approach is a little bit lazy on our side as we don't keep backwards compatibility in the protocol at client side. As long as we start doing that, adding features such as capabilities is much easier to do by negotiating "extension" at the handshake and using header for new thing, instead of bumping protocol version. Or even if we do bump version, supporting extra header is easier than extra field presence of which depends on message length and protocol version.

[1st1]

Adding individual struct fields "to the end of the message" is weird in such places as Prepare where new fields will come after command (right?).

That's solvable by making the command field first.

As long as we start doing that, adding features such as capabilities is much easier to do by negotiating "extension" at the handshake and using header for new thing, instead of bumping protocol version.

Which I think means more work for both the clients and the server. I'm not saying that what you say is wrong, I'm just saying that bumping the protocol version and adding one extra field read is easier.

Or even if we do bump version, supporting extra header is easier than extra field presence of which depends on message length and protocol version.

Hm, I don't see why. We generally don't care about message length in all three of our drivers, we just use it to detect over reads and under reads. That check is generic and works for all messages. So reading an extra field is literally just a matter of three lines (in pseudo code):

if protocol_version > x {
   y = (int32)read(buffer, 4)
}

[tailhook]

Here are a couple of things I think worth investing into instead:

1. Get rid of Describe. We can add a flag to Prepare to disable sending descriptors, but otherwise send them by default. Until we have real prepared statements (i.e. multiple statements could be prepared simultaneously) there is no reason for not using OptimisticExecute instead.
2. If we do (1) we also should consider making output_typedesc_id in OptimisticExecute optional. So if there are no arguments in the query there is no need for Prepare+Describe, you can just send OptimisticExecute and receive the new output type descriptor and data in a single roundtrip.
3. Get rid of Sync.
4. Allow multiplexing and pipelining (and essentially multiple prepared statements). Perhaps by adding request ids (the latter will also help with (3)).
5. Work on new error message serialization.
6. Work on warnings from server
7. Standardize on the format of headers (perhaps with self-descriptive codec).

[1st1]

Get rid of Describe. We can add a flag to Prepare to disable sending descriptors, but otherwise send them by default. Until we have real prepared statements (i.e. multiple statements could be prepared simultaneously) there is no reason for not using OptimisticExecute instead.

Hm, potentially we can do this, yes.

If we do (1) we also should consider making output_typedesc_id in OptimisticExecute optional. So if there are no arguments in the query there is no need for Prepare+Describe, you can just send OptimisticExecute and receive the new output type descriptor and data in a single roundtrip.

IMO the use case for this is very narrow. For commands like START TRANSACTION we'll be able cache the descriptors in after the first Prepare, and use OptimisticExecute after.

Get rid of Sync.

Sync is now used solely for the purpose of recovering the protocol state when an error happens. But if we implement request IDs we can indeed drop it, which is a very lucrative idea.

As we've just discussed on a call, we can attach current DB status (in transaction, in error, OK) to Error and CommandComplete message.

Allow multiplexing and pipelining (and essentially multiple prepared statements). Perhaps by adding request ids (the latter will also help with (3)).

Just for extra context, I also proposed adding request IDs in #1852, although that was for cancellation purposes.

Perhaps if we go as far as refactoring headers/fields we can add a field for request ID to every message.

Work on new error message serialization.
Work on warnings from server

Yeah, absolutely. I'll update this "pre-RFC".

Standardize on the format of headers (perhaps with self-descriptive codec).

If we reserve headers for auxiliary info, maybe we don't need that much. If we decide to keep using the headers the way we use them now, then absolutely, we need to fix the mess with codecs.

[tailhook]

Another thing left on my to do list is idle timeouts and pings. If you still want pings come from clients, then we have to announce timeout in server-side handshake (and enforce it).