Implement new GENEXEC request

[cole-miller]

This implements a new request type, GENEXEC, that propagates changes to the database like EXEC but supports returning rows like QUERY. It's intended to be the backend for implementing a dqlite_step function for the C client.

I realize GENEXEC isn't a great name, so bring on the bikeshedding.

Closes #471

Signed-off-by: Cole Miller cole.miller@canonical.com

[codecov[bot]]

Codecov Report

Merging #472 (9f1aa3b) into master (57d7590) will decrease coverage by 0.38%. The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   59.12%   58.74%   -0.38%     
==========================================
  Files          33       33              
  Lines        5876     5934      +58     
  Branches     1748     1770      +22     
==========================================
+ Hits         3474     3486      +12     
- Misses       1353     1382      +29     
- Partials     1049     1066      +17     

Impacted Files	Coverage Δ
src/client/protocol.c	33.27% <0.00%> (-0.55%)	:arrow_down:
src/gateway.c	43.81% <41.46%> (-0.22%)	:arrow_down:
src/query.c	32.35% <53.84%> (+2.35%)	:arrow_up:
src/transport.c	46.10% <0.00%> (-5.20%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[freeekanayaka]

Thinking about it, I guess Go SQL doesn't care about DELETE FROM RETURNING because it's a PostgreSQL extension, adopted by SQLite as well.

[cole-miller]

Am I correct that the response is always a rows response ?

Sorry, I missed this before. Yes, we always send a rows response.

[freeekanayaka]

What about calling the new method QUERY_OR_EXEC? In the sense that depending on the statement the result of the operation may or may not return rows, and hence the response type may vary.

It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

[MathieuBordere]

What about calling the new method QUERY_OR_EXEC? In the sense that depending on the statement the result of the operation may or may not return rows, and hence the response type may vary.

It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

The response is always a rows response in the implementation proposed in this PR.

[freeekanayaka]

What about calling the new method QUERY_OR_EXEC? In the sense that depending on the statement the result of the operation may or may not return rows, and hence the response type may vary. It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

The response is always a rows response in the implementation proposed in this PR.

Ah, thanks, I didn't realize that indeed.

It might be problematic if a client wants to know the ID of the last inserted row, or the number of rows affected (both of them are included in the RESULT response of an EXEC request). That's a typical use case, inserting a row and immediately getting the ID, for example to insert another row in another table which references it.

With plain SQLite you use sqlite3_last_insert_rowid() and sqlite3_changes(), see also here the dqlite code that fills the RESULT response for an EXEC request.

I'm not entirely sure what we could do about that. I guess there should be a dqlite_last_insert_rowid() and dqlite_changes() equivalent, although that would probably mean additional request types, round trips and server state.

[MathieuBordere]

What about calling the new method QUERY_OR_EXEC? In the sense that depending on the statement the result of the operation may or may not return rows, and hence the response type may vary. It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

The response is always a rows response in the implementation proposed in this PR.

Ah, thanks, I didn't realize that indeed.

It might be problematic if a client wants to know the ID of the last inserted row, or the number of rows affected (both of them are included in the RESULT response of an EXEC request). That's a typical use case, inserting a row and immediately getting the ID, for example to insert another row in another table which references it.

With plain SQLite you use sqlite3_last_insert_rowid() and sqlite3_changes(), see also here the dqlite code that fills the RESULT response for an EXEC request.

I'm not entirely sure what we could do about that. I guess there should be a dqlite_last_insert_rowid() and dqlite_changes() equivalent, although that would probably mean additional request types, round trips and server state.

The client dealing with 2 response types could handle that I think, saving the last_insert_rowid and changes on the dqlite object returned from dqlite_open. But I guess we need both if a statement both returns rows and makes changes to the DB, so a new result message then, the union of a rows result and an exec result?

[freeekanayaka]

What about calling the new method QUERY_OR_EXEC? In the sense that depending on the statement the result of the operation may or may not return rows, and hence the response type may vary. It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

The response is always a rows response in the implementation proposed in this PR.

Ah, thanks, I didn't realize that indeed. It might be problematic if a client wants to know the ID of the last inserted row, or the number of rows affected (both of them are included in the RESULT response of an EXEC request). That's a typical use case, inserting a row and immediately getting the ID, for example to insert another row in another table which references it. With plain SQLite you use sqlite3_last_insert_rowid() and sqlite3_changes(), see also here the dqlite code that fills the RESULT response for an EXEC request. I'm not entirely sure what we could do about that. I guess there should be a dqlite_last_insert_rowid() and dqlite_changes() equivalent, although that would probably mean additional request types, round trips and server state.

The client dealing with 2 response types could handle that I think, saving the last_insert_rowid and changes on the dqlite object returned from dqlite_open. But I guess we need both if a statement both returns rows and makes changes to the DB, so a new result message then, the union of a rows result and an exec result?

Yeah, basically it feels like this new request type needs a new response type too. At least if we want to accommodate both Go-style model (Query + Exec) and SQLite model (step-oriented).

Alternatively, we could introduce a new request type for obtaining the last inserted ID and/or affected rows count: the implementation of dqlite_last_insert_rowid() and dqlite_changes() would use that new request type.

[MathieuBordere]

Ah yeah, I was thinking dqlite_last_insert_rowid() and dqlite_changes() would be able to work with state saved in the application-local dqlite object, but they would indeed require a request to the leader.

[freeekanayaka]

Given all what we said, I believe that the new GENEXEC request can be simply dropped and instead we can modify the existing QUERY request to also handle non-readonly statements? Basically the current implementation of handle_query() would be replaced by the implementation of handle_genexec(), roughly speaking.

It should not have any impact on existing clients, since they already assume that QUERY is for read-only statements. But from now on, QUERY would also be able to handle non-readonly statements, that possibly also yield rows.

Regarding dqlite_last_insert_rowid() and dqlite_changes() we could either introduce a new request type or modify the schema of the ROWS response to also include those values.

[cole-miller]

My preference would definitely be to introduce the new request type instead of changing how QUERY is handled -- less risk to existing clients, clearer separation between SQLite-style and database/sql usage patterns, and I'd much rather incorporate last_insert_id and rows_affected into a new combined response type (paired with GENEXEC/QUERY_OR_EXEC) than introduce a new version of the ROWS response.

It'd be nice to update https://dqlite.io/docs/reference/wire-protocol too once everything is done.

Yes, will do.

[freeekanayaka]

My preference would definitely be to introduce the new request type instead of changing how QUERY is handled -- less risk to existing clients, clearer separation between SQLite-style and database/sql usage patterns, and I'd much rather incorporate last_insert_id and rows_affected into a new combined response type (paired with GENEXEC) than introduce a new version of the ROWS response.

Regarding the risk for existing clients, there's none, because if you try to run an INSERT statement using a QUERY request you'd get an error at best or some other kind of undefined behavior. Certainly it's not going to succeed. And I believe there isn't actually a separation between SQLite-style and database/sql style, given what you said above after reading the go/sql docs: basically in go/sql you can use QUERY any time you are interested in getting back rows, regardless of whether the statement is read-only or not, but you also have the option of using EXEC if you know upfront that you don't care about rows. In dqlite_step() we'd just use QUERY all the times.

Anyone reading the protocol documentation would wonder why QUERY is there at all, given that GENEXEC is functionally exactly the same as QUERY plus the ability of handling non-readonly statements. By enhancing QUERY to handle non-readonly statements we have a less ad-hoc/ambiguous protocol and we also make it possible for the Go driver to run things like DELETE FROM RETURNING, which is currently unsupported. When someone writes a new driver for a new language it will be clear that it can use QUERY for everything, and EXEC as a small optimization if rows are not important.

[freeekanayaka]

Another way to look at this is, what would we do if someone asks to support DELETE FROM RETURNING in the Go driver? And I think the answer would be to modify QUERY to be essentially like GENEXEC.

Probably the mistake was to have the EXEC method at all, as it doesn't offer much value. If anything, perhaps we should deprecate EXEC and leave only QUERY.

[cole-miller]

It is possible currently to send a modifying QUERY and have the server respond with success, e.g. this passes on master (add to test_gateway.c):

TEST_CASE(query, modifying, NULL)
{
    struct query_fixture *f = data;
    uint64_t stmt_id;
    (void)params;
    PREPARE("INSERT INTO test (n) VALUES (17) RETURNING *");
    f->request.db_id = 0;
    f->request.stmt_id = stmt_id;
    ENCODE(&f->request, query);
    HANDLE(QUERY);
    ASSERT_CALLBACK(0, ROWS);
    return MUNIT_OK;
}

The only reason we generally get an error without RETURNING is this check, where n will typically be 0 for a CREATE/INSERT/DELETE/UPDATE/etc.:

https://github.com/canonical/dqlite/blob/57d7590d445baae419e2c3119342fe65c1926f79/src/query.c#L107-L110

I admit it's not likely to be something anyone is doing in practice. I'm just leery of changing the observable behavior of existing well-formed requests in any way, when it's pretty simple to just add a new request type. But I'll defer to you.

[MathieuBordere]

It is possible currently to send a modifying QUERY and have the server respond with success, e.g. this passes on master (add to test_gateway.c):

TEST_CASE(query, modifying, NULL)
{
  struct query_fixture *f = data;
  uint64_t stmt_id;
  (void)params;
  PREPARE("INSERT INTO test (n) VALUES (17) RETURNING *");
  f->request.db_id = 0;
  f->request.stmt_id = stmt_id;
  ENCODE(&f->request, query);
  HANDLE(QUERY);
  ASSERT_CALLBACK(0, ROWS);
  return MUNIT_OK;
}

The only reason we generally get an error without RETURNING is this check, where n will typically be 0 for a CREATE/INSERT/DELETE/UPDATE/etc.:

https://github.com/canonical/dqlite/blob/57d7590d445baae419e2c3119342fe65c1926f79/src/query.c#L107-L110

I admit it's not likely to be something anyone is doing in practice. I'm just leery of changing the observable behavior of existing well-formed requests in any way, when it's pretty simple to just add a new request type. But I'll defer to you.

This will never be replicated no? Looks like a serious bug then.

[cole-miller]

Right, it won't be replicated.

[MathieuBordere]

Right, it won't be replicated.

So I wouldn't bother much about breaking that behavior (good catch), I think Free's suggestion to modify query makes sense.

[cole-miller]

Okay, working on a v2 that modifies the handling of QUERY and QUERY_SQL.

[freeekanayaka]

Right, it won't be replicated.

Exactly, that's what I meant with "undefined behavior", I was not sure what was going to happen (server returning an error or not), but I was sure that it wouldn't actually work. So the behavior is broken already and it's legitimate to change it.

[freeekanayaka]

Note also that I believe it's fine for now to not worry about the last inserted row ID / affected rows count, and just modify QUERY to handle non-readonly queries, leaving the ROWS response as it is.

There are probably many other SQLite APIs similar to sqlite3_last_inserted_rowid() and sqlite3_changes() that might require a dqlite_ analog, and we might want to think a bit more holistically about them, especially to maintain a balance between granularity and efficiency in the request types, like trying to minimize the number of round trips that typical use cases would require, so that a single dqlite_ call doesn't always map to a single request. Exactly like dqlite_step(), where the QUERY request will yield a batch of rows, not a single one, so follow-up dqlite_step() calls won't need an expensive request round trip, at least until the batch is exhausted.

[cole-miller]

There are probably many other SQLite APIs similar to sqlite3_last_inserted_rowid() and sqlite3changes() that might require a dqlite analog, and we might want to think a bit more holistically about them [...]

Yes, agreed. The number of functions is not that enormous; it would be feasible to go through all of them and categorize them by the level of support we'd eventually like to offer in dqlite. I'll try to do that if I get the chance.

[freeekanayaka]

There are probably many other SQLite APIs similar to sqlite3_last_inserted_rowid() and sqlite3changes() that might require a dqlite analog, and we might want to think a bit more holistically about them [...]

Yes, agreed. The number of functions is not that enormous; it would be feasible to go through all of them and categorize them by the level of support we'd eventually like to offer in dqlite. I'll try to do that if I get the chance.

That's a sensible approach indeed, that will allow to plan accordingly. It feels like a medium/long term goal, given the API surface.

[cole-miller]

Sorry for the delay on v2, I got caught up trying to simplify/refactor some parts of the gateway that that were making it tricky to make the updates to QUERY and QUERY_SQL.

[cole-miller]

Now working on top of #476 to implement handling of modifying QUERY and QUERY_SQL -- but running into a bug where the modifications seem not to be propagated despite going through leader__exec. I'll figure out what's going on.

Edit: it seems that if we prepare a statement like DELETE FROM test RETURNING *, VfsPoll will not pick up any changes to the database after the first call to sqlite3_step. This is surprising, because the SQLite documentation is explicit that

When a DELETE, INSERT, or UPDATE statement with a RETURNING clause is run, all of the database changes occur during the first invocation of sqlite3_step().

So maybe this is a bug in our VFS -- let me see if I can reduce it to a failing test.

[cole-miller]

Edit: it seems that if we prepare a statement like DELETE FROM test RETURNING *, VfsPoll will not pick up any changes to the database after the first call to sqlite3_step. This is surprising, because [snip]

I posted on the SQLite forum and got some clarification about this -- the "database changes occur during the first sqlite3_step" statement does not mean the changes will get flushed to the WAL at that point, they might remain in cache up until autocommit happens just before the sqlite3_step call that returns SQLITE_DONE. That makes things a bit difficult for us, since we want to VfsPoll and raft_apply before yielding any rows to the client. We could buffer all the rows up in memory, but that pretty much defeats the purpose of sending them to the client in batches. Alternatively, we could force a cache flush immediately after the first sqlite3_step.

[freeekanayaka]

Edit: it seems that if we prepare a statement like DELETE FROM test RETURNING *, VfsPoll will not pick up any changes to the database after the first call to sqlite3_step. This is surprising, because [snip]

I posted on the SQLite forum and got some clarification about this -- the "database changes occur during the first sqlite3_step" statement does not mean the changes will get flushed to the WAL at that point, they might remain in cache up until autocommit happens just before the sqlite3_step call that returns SQLITE_DONE. That makes things a bit difficult for us, since we want to VfsPoll and raft_apply before yielding any rows to the client. We could buffer all the rows up in memory, but that pretty much defeats the purpose of sending them to the client in batches. Alternatively, we could force a cache flush immediately after the first sqlite3_step.

If forcing a cache flush works, perhaps it's the easiest solution, although I'm not entirely sure of all the ramifications of that, so it'd need a careful look.

Alternatively, we could just say that DELETE FROM RETURNING is currently not supported because of this technical limitation. If dqlite users find that they need it, they could notify us about that (for example we could write this in the docs and point to an open GitHub issue where people can post).

[freeekanayaka]

I would tend to prefer the choice of not supporting DELETE FROM RETURNING for now, since we never had a need for it, so I'd be wary of complicating things for something which is currently not an issue in real-world usages of dqlite.

[cole-miller]

Closing in favor of #477.