canonical/go-dqlite

RowsAffected not returning correct value for UPDATE

SimonRichardson opened this issue · 4 comments

SimonRichardson commented

Running a simple update statement doesn't return the rows affected in a transaction. This does work in sqlite, but not dqlite. The interesting thing from my logging is that the commit does contain the right number of rows affected.

dqlite.driver driver.go:410 result: BEGIN protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: UPDATE change_log_witness SET
        lower_bound = ?,
        upper_bound = ?,
        updated_at = datetime()
WHERE id = ?;
 protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: COMMIT protocol.Result{LastInsertID:0x0, RowsAffected:0x1}

Running the following within the same transaction does return the right values:

row := tx.QueryRowContext(ctx, "SELECT changes(), total_changes();")
            var changes, totalChanges int64
            if err := row.Scan(&changes, &totalChanges); err != nil {
                return errors.Annotate(err, "recording watermark")
            }

This might be an oddity of sqlite or a bug in dqlite, but you also get the number of rows affected for subsequent requests (notice the COMMIT/BEGIN even though there are no rows affected for those things):

dqlite.driver driver.go:410 result: BEGIN protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: UPDATE change_log_witness SET
        lower_bound = ?,
        upper_bound = ?,
        updated_at = datetime()
WHERE id = ?;
 protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: COMMIT protocol.Result{LastInsertID:0x0, RowsAffected:0x1}
dqlite.driver driver.go:410 result: BEGIN protocol.Result{LastInsertID:0x0, RowsAffected:0x1}
dqlite.driver driver.go:410 result: COMMIT protocol.Result{LastInsertID:0x0, RowsAffected:0x1}
dqlite.driver driver.go:410 result: BEGIN protocol.Result{LastInsertID:0x0, RowsAffected:0x1}
dqlite.driver driver.go:410 result: DELETE FROM lease WHERE uuid in (
    SELECT l.uuid
    FROM   lease l LEFT JOIN lease_pin p ON l.uuid = p.lease_uuid
    WHERE  p.uuid IS NULL
    AND    l.expiry < datetime('now')
) protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: COMMIT protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
dqlite.driver driver.go:410 result: BEGIN protocol.Result{LastInsertID:0x0, RowsAffected:0x0}
cole-miller commented

Thanks Simon, I'll have a look at this.

cole-miller commented

This might be an oddity of sqlite or a bug in dqlite, but you also get the number of rows affected for subsequent requests (notice the COMMIT/BEGIN even though there are no rows affected for those things)

I think this part is expected behavior in some sense, sqlite3_changes returns the number of rows affected by the last UPDATE/INSERT/DELETE, not the number of rows affected by the last statement full stop, and

Executing any other type of SQL statement does not modify the value returned by these functions.

If you look at the database/sql documentation for sql.Result, it pretty much echoes what SQLite says, which I'm sure is intentional.

The first part, about getting a funny Result for an UPDATE that runs in the middle of a transaction, is more suspicious. In a simple case you get the expected results (verified by running a test locally):

EXEC("CREATE TABLE test (n INT)")
EXEC("INSERT INTO test (n) VALUES (17)") => {rows_affected=1, ..}
EXEC("UPDATE test SET n = 2 WHERE n = 1") => {rows_affected=0, ..}
EXEC("BEGIN") => {rows_affected=0, ..}
EXEC("UPDATE test SET n = 18 WHERE n = 17") => {rows_affected=1, ..}
EXEC("COMMIT") => {rows_affected=1, ..}

But it seems like you're seeing behavior that's inconsistent with that, which I haven't managed to reproduce yet. In particular this sequence should never happen:

EXEC("BEGIN")
EXEC("UPDATE blah blah blah") => {rows_affected=0, ..}
EXEC("COMMIT") => {rows_affected=1, ..}
cole-miller commented

This was fixed (unintentionally) in canonical/dqlite@a869e94, just needs a new dqlite release and a regression test.