Skip to content

Fix determine_update_operation to correctly detect INSERT with explicit rowid#26

Open
russellromney wants to merge 2 commits intoasg017:mainfrom
russellromney:fix-determine-update-operation-insert
Open

Fix determine_update_operation to correctly detect INSERT with explicit rowid#26
russellromney wants to merge 2 commits intoasg017:mainfrom
russellromney:fix-determine-update-operation-insert

Conversation

@russellromney
Copy link

Summary

Fixes a bug in determine_update_operation that caused INSERT operations with explicit rowid to panic.

The existing code checked argv[1] == NULL to detect INSERT operations, but according to SQLite xUpdate documentation, argv[0] == NULL indicates INSERT:

If argc is greater than 1 then an insert occurs when argv[0] is an SQL NULL.

The Bug

When inserting with an explicit rowid:

INSERT INTO vtab(rowid, col) VALUES (1, 'value')
  1. SQLite calls xUpdate with argv[0]=NULL (indicating INSERT) and argv[1]=1 (the rowid)
  2. The code incorrectly checked argv[1] for NULL
  3. Since argv[1]=1 (not NULL), it fell through to the unimplemented todo!() branch

The Fix

  • Check argv[0] for NULL to detect INSERT (matches SQLite docs)
  • Compare argv[0]/argv[1] values (not pointers) for UPDATE detection
  • Remove todo!() by treating rowid-change UPDATE as regular UPDATE

Testing

Tested with sqlite-tantivy, a Tantivy-powered FTS virtual table that uses explicit rowids:

CREATE VIRTUAL TABLE articles USING tantivy(title TEXT, body TEXT);
INSERT INTO articles(rowid, title, body) VALUES (1, 'Hello', 'World');
-- Previously panicked, now works correctly

All 31 tests pass (25 unit + 6 integration).

The existing code checked argv[1] == NULL to detect INSERT operations,
but according to SQLite documentation, argv[0] == NULL indicates INSERT.

From SQLite xUpdate docs:
- DELETE: argc=1, argv[0] is the rowid to delete
- INSERT: argc>1, argv[0]=NULL, argv[1] is rowid (NULL for auto-generate)
- UPDATE: argc>1, argv[0]≠NULL, argv[0]=argv[1] (same rowid)

The bug caused INSERT with explicit rowid to fail (hit todo!()) because:
1. Code checked argv[1] for INSERT detection (should be argv[0])
2. With explicit rowid, argv[1] is the rowid value, not NULL
3. Falls through to unimplemented UPDATE-with-rowid-change branch

This fix:
- Checks argv[0] for NULL to detect INSERT (matches docs)
- Compares argv[0]/argv[1] values (not pointers) for UPDATE detection
- Removes todo!() by treating rowid-change as regular UPDATE

Fixes explicit rowid INSERT: INSERT INTO vtab(rowid, col) VALUES (1, 'x')
russellromney added a commit to russellromney/sqlite-tantivy that referenced this pull request Jan 18, 2026
SQLite extension providing Tantivy-powered full-text search with
FTS5-compatible MATCH syntax.

Features:
- Full-text search with Tantivy engine
- FTS5-compatible CREATE VIRTUAL TABLE and MATCH syntax
- Support for TEXT, TAG, INTEGER, FLOAT field types
- In-SQLite storage (no external index files)
- Phrase search, stemming, and tokenizer options

Includes patch for sqlite-loadable-rs to fix INSERT with explicit rowid.
See: asg017/sqlite-loadable-rs#26
@russellromney russellromney marked this pull request as ready for review January 18, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant