marcboeker / go-duckdb

go-duckdb provides a database/sql driver for the DuckDB database engine.
MIT License
583 stars 96 forks source link

Silent primary key violation in the appender #210

Closed taniabogatsch closed 1 month ago

taniabogatsch commented 2 months ago

Currently, a constraint violation in the appender goes unnoticed, i.e., a.Close() succeeds. The values aren't appended, but there is no feedback that a violation happened.

When appending the same value to a primary key column twice, the second a.AppendRow(int32(1)) cannot return an error. That is because we are constructing the data on the go-duckdb side first (without constraint context). We detect a constraint violation only when calling either Flush or Close. Currently, there is a bug causing these functions not to return the constraint violation error, leaving the user in the dark about the unsuccessful append.

I believe this is a bug on the duckdb side itself, but I am opening a mirror issue here to make people aware until this is fixed. After fixing this (in duckdb), we should add respective tests for errAppenderClose and errAppenderFlush. For now, I'll push a FIXME with the fix for #197.

Reproduction in go-duckdb.

package main

import (
    "context"
    "database/sql"
    "fmt"
    "github.com/marcboeker/go-duckdb"
    _ "github.com/marcboeker/go-duckdb"
)

func panicOnErr(err error) {
    if err != nil {
        panic(err.Error())
    }
}

func main() {
    c, err := duckdb.NewConnector("", nil)
    panicOnErr(err)
    defer c.Close()

    _, err = sql.OpenDB(c).Exec(`
        CREATE TABLE test (
            C1 INTEGER PRIMARY KEY         
        )`)
    panicOnErr(err)

    con, err := c.Connect(context.Background())
    panicOnErr(err)
    defer con.Close()

    a, err := duckdb.NewAppenderFromConn(con, "", "test")
    panicOnErr(err)

    err = a.AppendRow(int32(1))
    panicOnErr(err)
    err = a.AppendRow(int32(1))
    panicOnErr(err)

    err = a.Close()
    panicOnErr(err)

    // Verify results.
    res, err := sql.OpenDB(c).QueryContext(context.Background(), `SELECT * FROM test`)
    panicOnErr(err)

    i := 0
    for res.Next() {
        i++
    }
    fmt.Println(i)
}

@marcboeker, feel free to go ahead and assign me!

taniabogatsch commented 2 months ago

go-duckdb test reproduction.

func TestAppenderPK(t *testing.T) {
    c, con, a := prepareAppender(t, `
        CREATE TABLE test (
            c1 INTEGER PRIMARY KEY            
        )`)

    require.NoError(t, a.AppendRow(int32(1)))
    require.NoError(t, a.AppendRow(int32(1)))

    require.ErrorContains(t, a.Close(), "TODOpk violation")
    cleanupAppender(t, c, con, a)
}
taniabogatsch commented 2 months ago

Update: https://github.com/duckdb/duckdb/pull/12051