Open bluebrown opened 2 years ago
Here is a way to reproduce the issue
package main
import (
"context"
"fmt"
"log"
"os"
"github.com/canonical/go-dqlite/app"
)
func main() {
ctx := context.Background()
exitIf(os.MkdirAll("data", 0755))
dqlite, err := app.New("data")
exitIf(err)
exitIf(dqlite.Ready(ctx))
db, err := dqlite.Open(ctx, "test")
exitIf(err)
exitIf(db.Ping())
_, err = db.ExecContext(ctx, "create table if not exists test (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, value TEXT NOT NULL)")
exitIf(err)
var (
id int64
value string
)
for i := 0; i < 10; i++ {
exitIf(db.QueryRowContext(ctx, "INSERT INTO test (value) VALUES (?) RETURNING *", i).Scan(&id, &value))
log.Printf("inserted %d, %s", id, value)
}
}
func exitIf(err error) {
if err != nil {
fmt.Printf("error: %v\n", err)
os.Exit(1)
}
}
If you build and run this, you get the error consistently:
$ docker build -t test .
$ docker run --rm test
2022/05/05 19:44:32 inserted 1, 0
server: src/vfs.c:1701: vfsFileShmLock: Assertion `wal->n_tx == 0' failed.
I noticed that it works when using a transaction:
for i := 0; i < 10; i++ {
txn, err := db.BeginTx(ctx, nil)
exitIf(err)
exitIf(txn.QueryRowContext(ctx, "INSERT INTO test (value) VALUES (?) RETURNING *", i).Scan(&id, &value))
exitIf(txn.Commit())
log.Printf("inserted %d, %s", id, value)
}
$ docker build -t test .
$ docker run --rm test
2022/05/05 19:47:08 inserted 1, 0
2022/05/05 19:47:08 inserted 2, 1
2022/05/05 19:47:08 inserted 3, 2
2022/05/05 19:47:08 inserted 4, 3
2022/05/05 19:47:08 inserted 5, 4
2022/05/05 19:47:08 inserted 6, 5
2022/05/05 19:47:08 inserted 7, 6
2022/05/05 19:47:08 inserted 8, 7
2022/05/05 19:47:08 inserted 9, 8
2022/05/05 19:47:08 inserted 10, 9
Thanks that's interesting information. As it's a duplicate of https://github.com/canonical/go-dqlite/issues/141, I will close this, but I'll have a look at it. Feel free to add extra information though, I'll see it.
edit: makes more sense to keep this one open
I more or less have an idea what's happening.
Statements like ExecContext(ctx, "INSERT INTO test (value) VALUES (?) RETURNING *", i)
will not work because an Exec
is not expected to return result rows see https://pkg.go.dev/database/sql#DB.ExecContext
To return result rows you have to use DB.QueryContext
and the likes. The problem is that dqlite
, when handling a Query command, assumes that the Query is read only and will not write to the database. In the case of your example
for i := 0; i < 10; i++ {
exitIf(db.QueryRowContext(ctx, "INSERT INTO test (value) VALUES (?) RETURNING *", i).Scan(&id, &value))
log.Printf("inserted %d, %s", id, value)
}
that assumption clearly doesn't hold and the internal logic breaks down.
It's interesting that it works within a transaction, still need to dig a bit in the code to verify this case is actually not causing issues. So, for the time being, I suggest to avoid using RETURNING
until we fix this.
I think sqlite3_stmt_readonly
can provide us a way to determine if a Query statement will write to the DB and then we can clean up our internal logic. I'll start by protecting against this case, before implementing an actual fix which will take more work, and I will do that at a later stage.
It also looks like RETURNING
is only supported in SQLite since version 3.35.0 (2021-03-12).
libsqlite3 on Ubuntu Bionic Beaver https://launchpad.net/ubuntu/bionic/amd64/libsqlite3-dev/3.22.0-1 and Focal Fossa https://launchpad.net/ubuntu/focal/amd64/libsqlite3-dev/3.31.1-4 both don't support it.
What do you think @stgraber, I guess dqlite shouldn't be able to support RETURNING
then?
Indeed it looks like support for RETURNING
is new, and it breaks dqlite's assumption. Using sqlite3_stmt_readonly
as protection seems fine.
The use cases for RETURNING
are somehow narrow, since usually people just want the ID and that's supported and can be obtained with Result.LastInsertID()
. Unless some user comes up with a real-world need for it, I'd probably would leave things like they are.
Some ORMs and such build on top of it though. I.e. https://github.com/volatiletech/sqlboiler.
This would make the dqlite library kind of incompatible with some popular existing abstractions on top of the std sql package.
Please can you point where RETURNING
is used in sqlboiler
? And possibly make real-world examples where it's needed.
The RETURNING
statement is not standard SQL, and I'm not even sure if mysql support it (at least it wasn't a while ago). So I'd be suprised that ORMs depend on it, also because the use cases are usually narrow.
It's using returning on the first class insert method.
var m models.MyModel
m.Name = "foo"
m.Insert(r.Context(), db, boil.Infer())
Thats the most straight forward and probably suggested way to use the models. You can get around my using rawQuery and things like that, but I would think those methods are for edge cases. model.Insert is for primary use.
You can see it for example here:: https://github.com/volatiletech/sqlboiler/blob/master/templates/main/15_insert.go.tpl#L95
Maybe there are some other ways to disable it, but it's the default behaviour AFAIK. I don't know that package well, it's just one I know of that is using returning.
I think returning makes also sense when dealing with default values and partial updates.
For example you have a http patch request handler. The user sends only 1 column to update but you want to respond with the full row. You would use returning after the update statement, to get the full row and respond with it.
It's using returning on the first class insert method.
var m models.MyModel m.Name = "foo" m.Insert(r.Context(), db, boil.Infer())
Thats the most straight forward and probably suggested way to use the models. You can get around my using rawQuery and things like that, but I would think those methods are for edge cases. model.Insert is for primary use.
You can see it for example here:: https://github.com/volatiletech/sqlboiler/blob/master/templates/main/15_insert.go.tpl#L95
Maybe there are some other ways to disable it, but it's the default behaviour AFAIK. I don't know that package well, it's just one I know of that is using returning.
It looks like setting the UseInsertID
flag to true
in the sqlboiler's sqlite3 driver should do the trick, or alternatively adding a dedicated dqlite
driver with that flag set to true
.
I think returning makes also sense when dealing with default values and partial updates.
For example you have a http patch request handler. The user sends only 1 column to update but you want to respond with the full row. You would use returning after the update statement, to get the full row and respond with it.
Fair enough, I didn't think about that use case. You can still just do a SELECT
after the UPDATE
, so it's not a deal breaker. The performance impact of that should not be meaningful to a lot of applications.
Having said that, dqlite could eventually support RETURNING
, for example if it turns out to be critical to the performance of a certain use case. However I feel it's not going to be trivial to do that, and for now I'm not sure it's worth the effort and added complexity.
I also met this error when I use dqlite with ent. It uses returing
on every query and update.
I also met this error when I use dqlite with ent. It uses
returing
on every query and update.
The easiest workaround would probably be to add dqlite as a new ent dialect, and have it behave the same as the sqlite dialect, except that it would not support RETURNING
. The ent SQL generator here should then avoid using RETURNING
, as it does for the mysql dialect.
I also met this error when I use dqlite with ent. It uses
returing
on every query and update.The easiest workaround would probably be to add dqlite as a new ent dialect, and have it behave the same as the sqlite dialect, except that it would not support
RETURNING
. The ent SQL generator here should then avoid usingRETURNING
, as it does for the mysql dialect.
I have made a fork and do this, but I still hope for a better solution.
I still think, it would be good to aim for full feature parity with sqlite3. Otherwise, it's difficult to use dqlite as a drop-in replacement for existing code.
I agree this is annoying and should be fixed. It's not going to be easy though, because it most probably requires a wire protocol change.
Since we have now one more ORM using RETURNING
I think it'd be fair to raise the priority of this issue.
Actually @cole-miller's PR https://github.com/canonical/dqlite/pull/477 should be a good first step for solving this. I believe what we'd need is to also return rows for non read-only statements submitted using the QUERY
or QUERY_SQL
protocol method. So actually we may not need a protocol change.
Hi, I noticed different types of errors when using the returning clause.
Sometimes I see this:
And sometimes this:
These errors happen with the below code after the app is ready and the DB has been created.
It works OK, when removing the returning clause.
I have installed the c libraries with this script: https://gist.github.com/bluebrown/85e1b39980f50c66682743afe0d8b316.