Skip to content

Commit 96fb74b

Browse files
committed
internal/ctlog: fix SQLite LockBackend for nil values
When passing nil to Create or Replace, it was stored as NULL, but then was fetched as []byte("") by Fetch, which then didn't compare equal to NULL in Replace, making it always fail. This was made harder to debug by two things: 1. in the CLI, NULL and empty are both shown as empty by default; 2. I tried to fix it by running "UPDATE checkpoints SET body = '' WHERE body IS NULL;" which put empty TEXT in those columns, which does not compare equal to empty BLOB. They were both allowed in the same column by the non-STRICT table.
1 parent baa5c6b commit 96fb74b

File tree

4 files changed

+15
-6
lines changed

4 files changed

+15
-6
lines changed

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ This database must be global and it **must never be changed or modified** as an
5252
The database must already exist, to prevent misconfigurations. Create it with
5353
5454
```
55-
sqlite3 checkpoints.db "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body TEXT)"
55+
sqlite3 checkpoints.db "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body BLOB NOT NULL) STRICT"
5656
```
5757

5858
Sunlight can alternatively use DynamoDB or S3-compatible object storage with `ETag` and `If-Match` support (such as Tigris) as global lock backends.

cmd/sunlight/sunlight.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ type Config struct {
9696
// The database must already exist to protect against accidental
9797
// misconfiguration. Create the table with:
9898
//
99-
// $ sqlite3 checkpoints.db "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body TEXT)"
99+
// $ sqlite3 checkpoints.db "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body BLOB NOT NULL) STRICT"
100100
//
101101
Checkpoints string
102102

internal/ctlog/cache.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ func initCache(path string) (readConn, writeConn *sqlite.Conn, err error) {
2020
if err := sqlitex.ExecTransient(writeConn, `
2121
CREATE TABLE IF NOT EXISTS cache (
2222
key BLOB PRIMARY KEY,
23-
timestamp INTEGER,
24-
leaf_index INTEGER
25-
) WITHOUT ROWID;`, nil); err != nil {
23+
timestamp INTEGER NOT NULL,
24+
leaf_index INTEGER NOT NULL
25+
) WITHOUT ROWID, STRICT;`, nil); err != nil {
2626
writeConn.Close()
2727
return nil, nil, err
2828
}

internal/ctlog/sqlite.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ func NewSQLiteBackend(ctx context.Context, path string, l *slog.Logger) (*SQLite
3434

3535
conn, err := sqlite.OpenConn(path, sqlite.OpenFlagsDefault & ^sqlite.SQLITE_OPEN_CREATE)
3636
if err != nil {
37-
return nil, fmt.Errorf(`failed to open SQLite lock database (hint: to avoid misconfiguration, the lock database must be created manually with "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body TEXT)"): %w`, err)
37+
return nil, fmt.Errorf(`failed to open SQLite lock database (hint: to avoid misconfiguration, the lock database must be created manually with "CREATE TABLE checkpoints (logID BLOB PRIMARY KEY, body BLOB NOT NULL) STRICT"): %w`, err)
3838
}
3939
if err := sqlitex.ExecTransient(conn, "PRAGMA synchronous = FULL", nil); err != nil {
4040
conn.Close()
@@ -86,13 +86,18 @@ func (b *SQLiteBackend) Replace(ctx context.Context, old LockedCheckpoint, new [
8686
defer prometheus.NewTimer(b.duration).ObserveDuration()
8787
b.mu.Lock()
8888
defer b.mu.Unlock()
89+
if new == nil {
90+
// NULL does *not* compare equal to an empty blob!
91+
new = []byte{}
92+
}
8993
o := old.(*sqliteCheckpoint)
9094
err := sqlitex.Exec(b.conn, "UPDATE checkpoints SET body = ? WHERE logID = ? AND body = ?",
9195
nil, new, o.logID[:], o.body)
9296
if err != nil {
9397
return nil, fmtErrorf("failed to update SQLite checkpoint: %w", err)
9498
}
9599
if b.conn.Changes() == 0 {
100+
// TODO: this also hits if new == old.body, and shouldn't.
96101
return nil, fmtErrorf("SQLite checkpoint not found or has changed")
97102
}
98103
return &sqliteCheckpoint{logID: o.logID, body: new}, nil
@@ -101,6 +106,10 @@ func (b *SQLiteBackend) Replace(ctx context.Context, old LockedCheckpoint, new [
101106
func (b *SQLiteBackend) Create(ctx context.Context, logID [sha256.Size]byte, new []byte) error {
102107
b.mu.Lock()
103108
defer b.mu.Unlock()
109+
if new == nil {
110+
// NULL does *not* compare equal to an empty blob!
111+
new = []byte{}
112+
}
104113
err := sqlitex.Exec(b.conn, `INSERT INTO checkpoints (logID, body) VALUES (?, ?)
105114
ON CONFLICT(logID) DO NOTHING`, nil, logID[:], new)
106115
if err != nil {

0 commit comments

Comments
 (0)