Using returning clause causes errors

[bluebrown]

Hi, I noticed different types of errors when using the returning clause.

Sometimes I see this:

server: src/vfs.c:1701: vfsFileShmLock: Assertion `wal->n_tx == 0' failed

And sometimes this:

error: another row available

These errors happen with the below code after the app is ready and the DB has been created.

if _, err := db.ExecContext(ctx, "create table if not exists test (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, value TEXT NOT NULL)"); err != nil {
	return err
}

for i := 0; i < 10; i++ {
	result, err :=  db.ExecContext(ctx, "INSERT INTO test (value) VALUES (?) RETURNING *",  i)
	if err != nil {
		return err
	}
	id, err := result.LastInsertId()
	if err != nil {
		return err
	}
	log.Printf("inserted %d", id)
}

It works OK, when removing the returning clause.

I have installed the c libraries with this script: https://gist.github.com/bluebrown/85e1b39980f50c66682743afe0d8b316.

[bluebrown]

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)
	}
}

Expand to see Dockerfile

# shared deps
FROM golang as base
RUN apt-get -y update
RUN apt-get -y --no-install-recommends install autoconf automake make \
    libtool liblz4-dev libuv1-dev libsqlite3-dev

# raft & dqlite
FROM base as dqlite
# raft
RUN git clone https://github.com/canonical/raft.git /tmp/raft
WORKDIR /tmp/raft
RUN autoreconf -i
RUN ./configure
RUN make
RUN make install
# dqlite
RUN git clone https://github.com/canonical/dqlite.git /tmp/dqlite
WORKDIR /tmp/dqlite
RUN autoreconf -i
RUN ./configure
RUN make
RUN make install

# sqlite3
FROM base as sqlite3
RUN curl -fsSL https://www.sqlite.org/2022/sqlite-autoconf-3380300.tar.gz | tar -xz -C /tmp
WORKDIR /tmp/sqlite-autoconf-3380300
RUN autoreconf -i
RUN ./configure
RUN make
RUN make install

# consolidate
FROM dqlite as builder-base
COPY --from=sqlite3 /usr/local/lib /usr/local/lib
RUN ldconfig
ENV CGO_LDFLAGS_ALLOW="-Wl,-z,now"

# build go source
FROM builder-base as builder
WORKDIR /workspace
COPY go.mod go.sum ./
RUN go mod download
COPY main.go ./
RUN go build -tags libsqlite3 -o server .

# final image
FROM debian:bullseye-slim
RUN apt -y update && apt -y install libuv1-dev
COPY --from=builder /usr/local/lib /usr/local/lib
RUN ldconfig
WORKDIR /app
COPY --from=builder /workspace/server ./server
CMD [ "/app/server" ]

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

[MathieuBordere]

Thanks that's interesting information. As it's a duplicate of #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

[MathieuBordere]

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.

[MathieuBordere]

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?

[freeekanayaka]

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.

[bluebrown]

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.

[freeekanayaka]

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.

[bluebrown]

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.

[bluebrown]

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.

[freeekanayaka]

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.

[freeekanayaka]

[freeekanayaka]

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.

[Zxilly]

I also met this error when I use dqlite with ent. It uses returing on every query and update.

[freeekanayaka]

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.

[Zxilly]

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 have made a fork and do this, but I still hope for a better solution.

[bluebrown]

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.

[freeekanayaka]

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.

[freeekanayaka]

Actually @cole-miller's PR canonical/dqlite#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.