diff --git a/build-linux.sh b/build-linux.sh index 625ed10a..2e7acd53 100755 --- a/build-linux.sh +++ b/build-linux.sh @@ -1,6 +1,6 @@ #!/bin/sh -# Build Marmot v2.9.15-beta for Linux (static binary) +# Build Marmot v2.9.16-beta for Linux (static binary) # Requires musl cross-compiler: brew install FiloSottile/musl-cross/musl-cross CC=x86_64-linux-musl-gcc \ diff --git a/cfg/config.go b/cfg/config.go index 4edb378e..3b12d832 100644 --- a/cfg/config.go +++ b/cfg/config.go @@ -94,6 +94,18 @@ type TransactionConfiguration struct { HeartbeatTimeoutSeconds int `toml:"heartbeat_timeout_seconds"` // Transaction timeout without heartbeat ConflictWindowSeconds int `toml:"conflict_window_seconds"` // LWW conflict resolution window LockWaitTimeoutSeconds int `toml:"lock_wait_timeout_seconds"` // How long to wait for locks (MySQL: innodb_lock_wait_timeout) + + // DDLImplicitCommit makes DDL end an open transaction before it runs, as + // MySQL does. MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly + // commit the active transaction, then run on their own, leaving the session + // with no transaction open. + // + // With this enabled (the default), a transaction that mixes DDL and DML + // behaves as it would on MySQL, so DML can see a column added earlier in + // the same transaction. Disable it to keep DDL inside the transaction and + // replicate it atomically with the surrounding statements, at the cost of + // DML in that transaction not seeing the pending schema change. + DDLImplicitCommit bool `toml:"ddl_implicit_commit"` } // MetaStoreConfiguration controls PebbleDB metadata storage @@ -291,9 +303,10 @@ var Config = &Configuration{ }, Transaction: TransactionConfiguration{ - HeartbeatTimeoutSeconds: 10, // Timeout transactions after 10s without heartbeat - ConflictWindowSeconds: 10, // 10 second window for LWW conflict resolution - LockWaitTimeoutSeconds: 50, // MySQL default: innodb_lock_wait_timeout + HeartbeatTimeoutSeconds: 10, // Timeout transactions after 10s without heartbeat + ConflictWindowSeconds: 10, // 10 second window for LWW conflict resolution + LockWaitTimeoutSeconds: 50, // MySQL default: innodb_lock_wait_timeout + DDLImplicitCommit: true, // MySQL semantics: DDL commits the open transaction }, MetaStore: MetaStoreConfiguration{ diff --git a/cfg/ddl_implicit_commit_default_test.go b/cfg/ddl_implicit_commit_default_test.go new file mode 100644 index 00000000..5d3a4a1f --- /dev/null +++ b/cfg/ddl_implicit_commit_default_test.go @@ -0,0 +1,15 @@ +package cfg + +import "testing" + +// TestDDLImplicitCommitDefault pins the shipped default. MySQL has no +// transactional DDL: schema changes implicitly commit the open transaction, and +// clients written against MySQL depend on that, so it is the default here. +// +// This lives in cfg because Config holds the defaults directly; a test that +// changes the flag would otherwise be reading its own mutation back. +func TestDDLImplicitCommitDefault(t *testing.T) { + if !Config.Transaction.DDLImplicitCommit { + t.Fatal("ddl_implicit_commit must default to true to match MySQL semantics") + } +} diff --git a/config.toml b/config.toml index 7aaec0f4..a298678c 100644 --- a/config.toml +++ b/config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta Configuration +# Marmot v2.9.16-beta Configuration # Leaderless SQLite Replication # ============================================================================== @@ -26,6 +26,13 @@ conflict_window_seconds = 10 # Lock wait timeout (seconds) - matches MySQL innodb_lock_wait_timeout lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + # ============================================================================== # CLUSTER MEMBERSHIP # ============================================================================== diff --git a/config.toml.example b/config.toml.example index d4333b1b..4efd1dd0 100644 --- a/config.toml.example +++ b/config.toml.example @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta Configuration +# Marmot v2.9.16-beta Configuration # Leaderless SQLite Replication # ============================================================================== @@ -26,6 +26,13 @@ conflict_window_seconds = 10 # Lock wait timeout (seconds) - matches MySQL innodb_lock_wait_timeout lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + # ============================================================================== # CLUSTER MEMBERSHIP # ============================================================================== diff --git a/coordinator/commit_lost_pinned_state_test.go b/coordinator/commit_lost_pinned_state_test.go new file mode 100644 index 00000000..ffb140b1 --- /dev/null +++ b/coordinator/commit_lost_pinned_state_test.go @@ -0,0 +1,148 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package coordinator_test + +// Regression test for handleCommit's empty-transaction fast path silently +// masquerading a lost pinned transaction as a successful empty COMMIT. +// +// TakeAndReleasePinnedStateForTest (coordinator/vec_testexport_test.go) +// simulates the pinned state being taken and released by something other +// than this COMMIT - e.g. a concurrent forward-session eviction calling +// CoordinatorHandler.CloseSession - which is exactly the class of loss the +// grpc.closeRemovedForwardSession execMu fix now prevents in production, +// but which this fast path must also refuse to paper over as defense in +// depth. + +import ( + "context" + "database/sql" + "testing" + "time" + + "github.com/maxpert/marmot/coordinator" + "github.com/maxpert/marmot/db" + "github.com/maxpert/marmot/hlc" + "github.com/maxpert/marmot/protocol" + "github.com/stretchr/testify/require" +) + +type lostPinnedStateReplicator struct{} + +func (*lostPinnedStateReplicator) ReplicateTransaction( + _ context.Context, + _ uint64, + _ *coordinator.ReplicationRequest, +) (*coordinator.ReplicationResponse, error) { + return &coordinator.ReplicationResponse{Success: true}, nil +} + +type lostPinnedStateSetup struct { + handler *coordinator.CoordinatorHandler + session *protocol.ConnectionSession + conn *sql.DB +} + +func setupLostPinnedState(t *testing.T) *lostPinnedStateSetup { + t.Helper() + + tmpDir := t.TempDir() + clock := hlc.NewClock(1) + + dbMgr, err := db.NewDatabaseManager(tmpDir, 1, clock) + require.NoError(t, err) + t.Cleanup(func() { dbMgr.Close() }) + + const dbName = "lostpinned" + require.NoError(t, dbMgr.CreateDatabase(dbName)) + + systemDB, err := dbMgr.GetDatabase(db.SystemDatabaseName) + require.NoError(t, err) + schemaVersionMgr := db.NewSchemaVersionManager(systemDB.GetMetaStore()) + + nodeProvider := coordinator.NewMockNodeProvider([]uint64{1}) + writeCoord := coordinator.NewWriteCoordinator( + 1, + nodeProvider, + &lostPinnedStateReplicator{}, + db.NewLocalReplicator(1, dbMgr, clock), + 10*time.Second, + clock, + ) + readCoord := coordinator.NewReadCoordinator(1, nodeProvider, db.NewLocalReader(dbMgr), 10*time.Second) + + handler := coordinator.NewCoordinatorHandler( + 1, + writeCoord, + readCoord, + clock, + dbMgr, + coordinator.NewDDLLockManager(30*time.Second), + schemaVersionMgr, + noopNodeRegistry{}, + ) + + session := &protocol.ConnectionSession{ + ConnID: 1, + CurrentDatabase: dbName, + TranspilationEnabled: true, + } + + _, err = handler.HandleQuery(session, "CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)", nil) + require.NoError(t, err) + + conn, err := dbMgr.GetDatabaseConnection(dbName) + require.NoError(t, err) + + return &lostPinnedStateSetup{handler: handler, session: session, conn: conn} +} + +// TestCommitFailsLoudWhenPinnedStateLost pins the fix: if eager DML pinned +// state for this transaction but that state is gone by the time COMMIT +// reads it - with no buffered statements and no pinned state left, exactly +// what a legitimately empty transaction looks like - COMMIT must return an +// error, never a silent OK, because the write may have already executed and +// be unrecoverably lost. +func TestCommitFailsLoudWhenPinnedStateLost(t *testing.T) { + s := setupLostPinnedState(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('lost')", nil) + require.NoError(t, err) + + // Simulate a concurrent eviction taking and discarding the pinned state + // out from under this transaction, without going through this session's + // own COMMIT/ROLLBACK - the same effect grpc/forward_session.go's old, + // unsynchronized closeRemovedForwardSession had on an in-flight COMMIT. + took := s.handler.TakeAndReleasePinnedStateForTest(s.session.ConnID) + require.True(t, took, "INSERT must have pinned transaction state") + + require.True(t, s.session.InTransaction(), "the race leaves COMMIT still seeing an open transaction") + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.Error(t, err, "COMMIT must fail loud when its pinned state vanished instead of silently reporting OK") + + require.False(t, s.session.InTransaction(), "COMMIT must still end the session's transaction even when it errors") + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'lost'").Scan(&count)) + require.Equal(t, 0, count, "the discarded write must not have been applied") +} + +// TestCommitEmptyTransactionStillNoop guards that a transaction which never +// pinned any state (BEGIN immediately followed by COMMIT, or one that only +// ran no-op DML) keeps working exactly as before: COMMIT is a real no-op, +// not an error. +func TestCommitEmptyTransactionStillNoop(t *testing.T) { + s := setupLostPinnedState(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + res, err := s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err, "a transaction that never pinned any state must commit as a plain no-op") + require.Nil(t, res) + require.False(t, s.session.InTransaction()) +} diff --git a/coordinator/ddl_defects_test.go b/coordinator/ddl_defects_test.go new file mode 100644 index 00000000..fdfbe0fc --- /dev/null +++ b/coordinator/ddl_defects_test.go @@ -0,0 +1,112 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package coordinator_test + +import ( + "database/sql" + "testing" + + "github.com/maxpert/marmot/protocol" + "github.com/stretchr/testify/require" +) + +// TestUnparseableDDLReturnsCleanError pins the LLDAP 0.6.3 regression: a DDL +// statement with an unquoted, hyphenated constraint name is not valid MySQL +// syntax (unquoted identifiers cannot contain '-'). Vitess's DDL fallback used +// to swallow the resulting syntax error and hand back a partially-parsed AST +// (e.g. "ALTER TABLE t ADD CONSTRAINT unique-user-email UNIQUE (email)" +// degraded to just "ALTER TABLE t"), which Marmot then forwarded into 2PC, +// where SQLite's PREPARE failed with a confusing "incomplete input" error. +// The statement must instead be rejected immediately with a clean MySQL +// syntax error, and the table must be left completely untouched. +func TestUnparseableDDLReturnsCleanError(t *testing.T) { + s := setupNoopDML(t) + + badDDL := "alter table t add CONSTRAINT unique-user-email UNIQUE (email)" + rs, err := s.handler.HandleQuery(s.session, badDDL, nil) + require.Nil(t, rs, "no result set for a rejected statement") + require.Error(t, err, "unquoted hyphenated identifier is not valid MySQL syntax") + + mysqlErr, ok := err.(*protocol.MySQLError) + require.Truef(t, ok, "expected *protocol.MySQLError, got %T: %v", err, err) + require.Equal(t, protocol.ErrCodeParseError, mysqlErr.Code) + + // The table must be exactly what setupNoopDML created - no truncated DDL + // ("ALTER TABLE t") must have been silently applied. + rows, err := s.conn.Query("PRAGMA table_info(t)") + require.NoError(t, err) + defer rows.Close() + + var cols []string + for rows.Next() { + var cid int + var name, colType string + var notNull, pk int + var dflt sql.NullString + require.NoError(t, rows.Scan(&cid, &name, &colType, ¬Null, &dflt, &pk)) + cols = append(cols, name) + } + require.Equal(t, []string{"id", "name"}, cols, "the malformed DDL must not have altered the table") +} + +// TestAddConstraintUniqueUsesGeneratedIndex verifies the properly-quoted +// equivalent of the LLDAP statement (a well-formed MySQL "ADD CONSTRAINT ... +// UNIQUE" with a hyphenated name) transpiles to a real SQLite UNIQUE INDEX +// that actually enforces uniqueness, end to end through HandleQuery. +func TestAddConstraintUniqueUsesGeneratedIndex(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "ALTER TABLE t ADD COLUMN email TEXT", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, + "ALTER TABLE t ADD CONSTRAINT `unique-user-email` UNIQUE (email)", nil) + require.NoError(t, err, "well-formed ADD CONSTRAINT ... UNIQUE must transpile and apply") + + _, err = s.handler.HandleQuery(s.session, + "UPDATE t SET email = 'a@example.com' WHERE id = 1", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, + "INSERT INTO t (id, name, email) VALUES (2, 'dup', 'a@example.com')", nil) + require.Error(t, err, "the generated unique index must reject a duplicate email") +} + +// TestSubqueryHavingSurvivesTranspilation pins the second LLDAP regression: an +// IN-subquery with GROUP BY/HAVING was mangled by transpilation because the +// serializer treated every *sqlparser.Where node as a WHERE clause, including +// ones that were actually HAVING (Vitess represents both with the same Where +// struct, distinguished only by its Type field). That turned "GROUP BY email +// HAVING COUNT(email) > ?" into "GROUP BY email WHERE COUNT(email) > ?", +// which SQLite's PREPARE rejected with "near \"WHERE\": syntax error". +func TestSubqueryHavingSurvivesTranspilation(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "ALTER TABLE t ADD COLUMN email TEXT", nil) + require.NoError(t, err) + + for i, row := range []struct { + id int + name string + email string + }{ + {2, "b", "dup@example.com"}, + {3, "c", "dup@example.com"}, + {4, "d", "unique@example.com"}, + } { + _, err := s.handler.HandleQuery(s.session, + "INSERT INTO t (id, name, email) VALUES (?, ?, ?)", + []interface{}{row.id, row.name, row.email}) + require.NoErrorf(t, err, "insert row %d", i) + } + + sql := "SELECT `id`, `email` FROM `t` WHERE `email` IN " + + "(SELECT `email` FROM `t` GROUP BY `email` HAVING COUNT(`email`) > ?) " + + "ORDER BY `id` ASC" + rs, err := s.handler.HandleQuery(s.session, sql, []interface{}{1}) + require.NoError(t, err, "GROUP BY/HAVING subquery must survive transpilation") + require.Len(t, rs.Rows, 2, "only the two duplicate-email rows qualify") + require.Equal(t, "dup@example.com", rs.Rows[0][1]) + require.Equal(t, "dup@example.com", rs.Rows[1][1]) +} diff --git a/coordinator/ddl_implicit_commit_test.go b/coordinator/ddl_implicit_commit_test.go new file mode 100644 index 00000000..88cc68b7 --- /dev/null +++ b/coordinator/ddl_implicit_commit_test.go @@ -0,0 +1,184 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package coordinator_test + +import ( + "testing" + + "github.com/maxpert/marmot/cfg" + "github.com/maxpert/marmot/coordinator" + "github.com/maxpert/marmot/protocol" + "github.com/stretchr/testify/require" +) + +// withDDLImplicitCommit sets the flag for one test and restores it after. +func withDDLImplicitCommit(t *testing.T, enabled bool) { + t.Helper() + prev := cfg.Config.Transaction.DDLImplicitCommit + cfg.Config.Transaction.DDLImplicitCommit = enabled + t.Cleanup(func() { cfg.Config.Transaction.DDLImplicitCommit = prev }) +} + +// TestDDLInTransactionSeesSchemaChange is the LLDAP migration shape: DDL and +// dependent DML in one transaction. On MySQL the DDL commits first, so the +// UPDATE sees the new column. +func TestDDLInTransactionSeesSchemaChange(t *testing.T) { + withDDLImplicitCommit(t, true) + s := setupNoopDML(t) + + for _, q := range []string{ + "BEGIN", + "ALTER TABLE t ADD COLUMN temp_name TEXT", + "UPDATE t SET temp_name = name", + "COMMIT", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoErrorf(t, err, "query %q", q) + } + + var got string + require.NoError(t, s.conn.QueryRow("SELECT temp_name FROM t WHERE id = 1").Scan(&got)) + require.Equal(t, "a", got, "DML must see the column added earlier in the transaction") +} + +// TestDDLImplicitCommitPersistsPriorWrites pins that statements buffered before +// the DDL are committed by it, rather than discarded or deferred. +func TestDDLImplicitCommitPersistsPriorWrites(t *testing.T) { + withDDLImplicitCommit(t, true) + s := setupNoopDML(t) + + for _, q := range []string{ + "BEGIN", + "INSERT INTO t (id, name) VALUES (2, 'b')", + "ALTER TABLE t ADD COLUMN note TEXT", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoErrorf(t, err, "query %q", q) + } + + // The INSERT is durable before any COMMIT is sent, as on MySQL. + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE id = 2").Scan(&count)) + require.Equal(t, 1, count, "DDL must commit the statements buffered before it") + + require.False(t, s.session.InTransaction(), + "the transaction is ended by the DDL, not left open") +} + +// TestCommitAfterImplicitCommitIsNoop covers the trailing COMMIT every ORM +// sends after its transaction body; MySQL accepts it silently. +func TestCommitAfterImplicitCommitIsNoop(t *testing.T) { + withDDLImplicitCommit(t, true) + s := setupNoopDML(t) + + for _, q := range []string{ + "BEGIN", + "ALTER TABLE t ADD COLUMN note TEXT", + "COMMIT", + "ROLLBACK", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoErrorf(t, err, "query %q", q) + } +} + +// TestDDLInTransactionBufferedWhenDisabled pins the opt-out: DDL stays inside +// the transaction and is applied at COMMIT, so nothing lands before then. +func TestDDLInTransactionBufferedWhenDisabled(t *testing.T) { + withDDLImplicitCommit(t, false) + s := setupNoopDML(t) + + for _, q := range []string{ + "BEGIN", + "INSERT INTO t (id, name) VALUES (2, 'b')", + "ALTER TABLE t ADD COLUMN note TEXT", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoErrorf(t, err, "query %q", q) + } + + require.True(t, s.session.InTransaction(), + "with the flag off, DDL must not end the transaction") + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE id = 2").Scan(&count)) + require.Equal(t, 0, count, "buffered statements must not land before COMMIT") + + _, err := s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE id = 2").Scan(&count)) + require.Equal(t, 1, count, "COMMIT applies the buffered statements") +} + +// TestDDLInTransactionFailsWithoutImplicitCommit documents the cost of opting +// out: DDL stays in the transaction and is applied at COMMIT, so DML in that +// same transaction cannot see the schema change. This is the failure that makes +// MySQL-written migrations (LLDAP's, for one) unable to run. +func TestDDLInTransactionFailsWithoutImplicitCommit(t *testing.T) { + withDDLImplicitCommit(t, false) + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + _, err = s.handler.HandleQuery(s.session, "ALTER TABLE t ADD COLUMN temp_name TEXT", nil) + require.NoError(t, err, "the DDL itself buffers fine") + + _, err = s.handler.HandleQuery(s.session, "UPDATE t SET temp_name = name", nil) + if err == nil { + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + } + require.Error(t, err, "DML cannot see a column added earlier in the same transaction") + require.Contains(t, err.Error(), "temp_name") +} + +// TestDMLInTransactionStillBuffers guards the change from widening: only schema +// changes trigger an implicit commit. +func TestDMLInTransactionStillBuffers(t *testing.T) { + withDDLImplicitCommit(t, true) + s := setupNoopDML(t) + + for _, q := range []string{ + "BEGIN", + "INSERT INTO t (id, name) VALUES (2, 'b')", + "INSERT INTO t (id, name) VALUES (3, 'c')", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoErrorf(t, err, "query %q", q) + } + + require.True(t, s.session.InTransaction(), "DML must not end the transaction") + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t").Scan(&count)) + require.Equal(t, 1, count, "DML stays buffered until COMMIT") +} + +// TestCausesImplicitCommitClassification pins which statements end a +// transaction, matching MySQL's implicit-commit list. +func TestCausesImplicitCommitClassification(t *testing.T) { + ends := []protocol.StatementCode{ + protocol.StatementDDL, + protocol.StatementCreateDatabase, + protocol.StatementDropDatabase, + protocol.StatementCreateVectorIndex, + protocol.StatementDropVectorIndex, + protocol.StatementReindexVectorIndex, + } + keeps := []protocol.StatementCode{ + protocol.StatementInsert, + protocol.StatementUpdate, + protocol.StatementDelete, + protocol.StatementReplace, + protocol.StatementSelect, + } + for _, code := range ends { + require.Truef(t, coordinator.CausesImplicitCommit(protocol.Statement{Type: code}), + "statement type %d should end a transaction", code) + } + for _, code := range keeps { + require.Falsef(t, coordinator.CausesImplicitCommit(protocol.Statement{Type: code}), + "statement type %d should not end a transaction", code) + } +} diff --git a/coordinator/eager_txn_test.go b/coordinator/eager_txn_test.go new file mode 100644 index 00000000..87c61f93 --- /dev/null +++ b/coordinator/eager_txn_test.go @@ -0,0 +1,232 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package coordinator_test + +// Tests for eager execution of DML inside an explicit BEGIN...COMMIT/ROLLBACK +// transaction (as opposed to buffering statements until COMMIT). +// +// Requirements pinned here (see task brief for full context): +// - Each DML inside a txn returns REAL rows-affected and REAL last_insert_id. +// - Reads inside the txn on a pinned database see the txn's own uncommitted +// writes (read-your-own-writes), while other connections do not. +// - COMMIT replicates via the same 2PC path as autocommit DML (CDC msgpack, +// never raw SQL). +// - ROLLBACK, and disconnect/session-close with an open txn, discard +// everything and release the writer so later transactions are not stuck. +// - Zero-row DML inside a txn reports RowsAffected 0, not a fake 1. + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestEagerInsertReturnsRealLastInsertId pins requirement 1: an INSERT inside +// an explicit transaction must report the real auto-increment id it produced, +// not the old fake RowsAffected:1/LastInsertId:0 buffered-statement response. +func TestEagerInsertReturnsRealLastInsertId(t *testing.T) { + s := setupNoopDML(t) + + for _, q := range []string{"BEGIN"} { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoError(t, err) + } + + res, err := s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('x')", nil) + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, int64(1), res.RowsAffected) + require.NotZero(t, res.LastInsertId, "eager INSERT must report the real last_insert_id") + + _, err = s.handler.HandleQuery(s.session, "ROLLBACK", nil) + require.NoError(t, err) +} + +// TestEagerReadSeesOwnUncommittedWrite pins requirement 2: a SELECT after a +// DML in the same still-open transaction must see that DML's write, while an +// independent connection to the same database must not. +func TestEagerReadSeesOwnUncommittedWrite(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('own-write')", nil) + require.NoError(t, err) + + // Read-your-own-writes: same session, same still-open transaction. + res, err := s.handler.HandleQuery(s.session, "SELECT name FROM t WHERE name = 'own-write'", nil) + require.NoError(t, err) + require.Len(t, res.Rows, 1, "SELECT inside the open transaction must see its own uncommitted INSERT") + + // Isolation: an independent connection must not see the uncommitted write. + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'own-write'").Scan(&count)) + require.Equal(t, 0, count, "an independent connection must not see the uncommitted write") + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'own-write'").Scan(&count)) + require.Equal(t, 1, count, "COMMIT must persist the write") +} + +// TestEagerParentChildInsertUsingReturnedId is the LLDAP shape: a parent +// INSERT whose real last_insert_id feeds a child INSERT in the same +// transaction, both surviving COMMIT. +func TestEagerParentChildInsertUsingReturnedId(t *testing.T) { + s := setupNoopDML(t) + + for _, q := range []string{ + "CREATE TABLE parent (id INTEGER PRIMARY KEY, name TEXT)", + "CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id INTEGER, name TEXT)", + } { + _, err := s.handler.HandleQuery(s.session, q, nil) + require.NoError(t, err) + } + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + parentRes, err := s.handler.HandleQuery(s.session, "INSERT INTO parent (name) VALUES ('p1')", nil) + require.NoError(t, err) + require.NotZero(t, parentRes.LastInsertId) + + childSQL := fmt.Sprintf("INSERT INTO child (parent_id, name) VALUES (%d, 'c1')", parentRes.LastInsertId) + childRes, err := s.handler.HandleQuery(s.session, childSQL, nil) + require.NoError(t, err) + require.Equal(t, int64(1), childRes.RowsAffected) + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + + var parentCount, childCount int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM parent WHERE name = 'p1'").Scan(&parentCount)) + require.Equal(t, 1, parentCount) + require.NoError(t, s.conn.QueryRow( + fmt.Sprintf("SELECT COUNT(*) FROM child WHERE parent_id = %d AND name = 'c1'", parentRes.LastInsertId), + ).Scan(&childCount)) + require.Equal(t, 1, childCount, "child row must link to the parent's real last_insert_id") +} + +// TestEagerRollbackDiscardsWrites pins requirement 4: ROLLBACK discards +// everything written eagerly inside the transaction, and releases the writer +// so a later statement on the same connection succeeds (the writer is not +// left stuck holding the SQLite write lock). +func TestEagerRollbackDiscardsWrites(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('rollback-me')", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "ROLLBACK", nil) + require.NoError(t, err) + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'rollback-me'").Scan(&count)) + require.Equal(t, 0, count, "ROLLBACK must discard the eagerly-executed INSERT") + + // The writer must not be stuck: a fresh statement must succeed. + res, err := s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('after-rollback')", nil) + require.NoError(t, err, "writer must be released after ROLLBACK") + require.Equal(t, int64(1), res.RowsAffected) +} + +// TestEagerZeroRowUpdateReportsZero pins requirement 7: a zero-row DML inside +// a transaction reports RowsAffected 0, not the old fake RowsAffected:1, and +// still commits cleanly as a no-op (no 2PC round for the no-op statement). +func TestEagerZeroRowUpdateReportsZero(t *testing.T) { + s := setupNoopDML(t) + before := s.replicator.prepares + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + res, err := s.handler.HandleQuery(s.session, "UPDATE t SET name = 'z' WHERE id = 999", nil) + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, int64(0), res.RowsAffected, "no-op DML inside a transaction must report zero rows") + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + require.Equal(t, before, s.replicator.prepares, "an all-no-op transaction must not run 2PC") +} + +// TestEagerCommitReplicatesViaCDC pins requirement 3: COMMIT drives the exact +// same 2PC path handleCommit uses today, and the write is durable afterward. +// +// It cannot observe this via countingReplicator (the seam noop_dml_test.go's +// negative "2PC did not run" assertions use): WriteCoordinator dispatches the +// coordinator's own node through wc.localReplicator, never through the +// injected wc.replicator (coordinator/write_coordinator.go:750,756) - that +// only fires for genuine remote peers, and setupNoopDML's fixture is a +// single-node cluster with none. Durable presence in an independent +// connection is proof enough: a PinnedSession's underlying SQLite +// transaction is only ever rolled back (see PinnedSession's doc comment in +// pinned_txn.go), never committed directly, so the write can only have +// landed via CDC replay through WriteTransaction/2PC. +func TestEagerCommitReplicatesViaCDC(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('y')", nil) + require.NoError(t, err) + + res, err := s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + require.NotNil(t, res) + require.NotZero(t, res.CommittedTxnId, "COMMIT must report the committed txn id") + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'y'").Scan(&count)) + require.Equal(t, 1, count, "COMMIT must persist the write via CDC replay, since the pinned SQLite txn is only ever rolled back") +} + +// TestEagerSessionCloseWithOpenTxnRollsBack pins requirement 4's disconnect +// case: a client that vanishes with an open transaction must not leave its +// eager writes applied, nor leave the SQLite writer stuck. CloseSession is +// the seam protocol/server.go's connection-cleanup path calls. +func TestEagerSessionCloseWithOpenTxnRollsBack(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('leaked')", nil) + require.NoError(t, err) + + s.handler.CloseSession(s.session) + + require.False(t, s.session.InTransaction(), "CloseSession must end the session's transaction") + + var count int + require.NoError(t, s.conn.QueryRow("SELECT COUNT(*) FROM t WHERE name = 'leaked'").Scan(&count)) + require.Equal(t, 0, count, "a session closed mid-transaction must not leave its writes applied") + + // The writer must not be stuck: a fresh autocommit statement must succeed. + res, err := s.handler.HandleQuery(s.session, "INSERT INTO t (name) VALUES ('after-close')", nil) + require.NoError(t, err, "writer must be released after CloseSession") + require.Equal(t, int64(1), res.RowsAffected) +} + +// TestEagerEmptyTransactionCommitNoop guards requirement 7: BEGIN immediately +// followed by COMMIT, with no pinned session ever created, must still behave +// (no panic, no error, no 2PC round). +func TestEagerEmptyTransactionCommitNoop(t *testing.T) { + s := setupNoopDML(t) + before := s.replicator.prepares + + _, err := s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + require.Equal(t, before, s.replicator.prepares, "an empty transaction must not run 2PC") +} diff --git a/coordinator/handler.go b/coordinator/handler.go index b6055dde..3ecf478e 100644 --- a/coordinator/handler.go +++ b/coordinator/handler.go @@ -66,6 +66,12 @@ type ReplicatedDatabaseProvider interface { GetSchemaCache() interface{} // Returns *SchemaCache (using interface{} to avoid import cycle) // DescribeResultColumns reports the columns a query returns without running it. DescribeResultColumns(ctx context.Context, query string) ([]common.ResultColumn, error) + // BeginPinnedSession starts a PinnedSession for eager DML execution inside + // an explicit transaction (see PinnedSession doc). ctx governs the pinned + // SQLite transaction's entire lifetime: the caller must not cancel it + // until after calling Release, matching ExecuteLocalWithHooks's existing + // cancelHookCtx convention. + BeginPinnedSession(ctx context.Context, txnID uint64) (PinnedSession, error) } // ExecutionRequest for local-only execution - never replicated @@ -167,6 +173,18 @@ func getDDLValidationTimeout() time.Duration { return 60 * time.Second } +// pinnedSessionTimeout bounds how long a pinned eager-execution SQLite +// transaction's context stays alive - reuses the existing hook-execution lock +// wait timeout (same knob ExecuteLocalWithHooks's autocommit path already +// uses for hookDB sessions), applied here for its naturally longer eager +// duration instead of a new speculative config value. +func pinnedSessionTimeout() time.Duration { + if cfg.Config != nil && cfg.Config.Transaction.LockWaitTimeoutSeconds > 0 { + return time.Duration(cfg.Config.Transaction.LockWaitTimeoutSeconds) * time.Second + } + return 50 * time.Second +} + // writeTimeoutForStatements returns the deadline budget WriteTransaction needs // for this transaction: the DDL validation timeout if it carries any DDL // statement (PREPARE will execute-and-rollback the real statement for each @@ -226,6 +244,15 @@ type CoordinatorHandler struct { draining atomic.Bool vecGoRankTemplates sync.Map + // pinnedTxns tracks the pinned eager-execution SQLite transactions for + // each connection's currently open explicit transaction (see + // pinned_txn.go). Keyed by ConnectionSession.ConnID; a connection has at + // most one open explicit transaction at a time, so ConnID alone is a + // sufficient key. Entries are created lazily on the first DML inside a + // transaction and always removed via takePinnedState at COMMIT, ROLLBACK, + // or CloseSession. + pinnedTxns sync.Map + // vecEngine is the VectorUDFProvider used by the vector-query rewriter // (see coordinator/vec_rewrite.go). It is set once at startup via // SetVectorEngine. Nil disables the rewrite hook (handler falls through @@ -359,6 +386,13 @@ func (h *CoordinatorHandler) HandleQuery(session *protocol.ConnectionSession, sq Bool("transpilation", session.TranspilationEnabled). Msg("PARSED") + // Fail fast on statements the parser/transpiler could not handle (e.g. a DDL + // statement Vitess only partially parsed) instead of forwarding the mangled + // SQL into the read/2PC path, where it produces a confusing downstream error. + if stmt.Type == protocol.StatementUnsupported && stmt.Error != "" { + return nil, protocol.NewMySQLError(protocol.ErrCodeParseError, protocol.SQLStateSyntax, stmt.Error) + } + // Handle SET commands: extract @@marmot_vec_* vars via Vitess AST; ignore others. if stmt.Type == protocol.StatementSet { return h.handleSetCommand(session, sql) @@ -454,7 +488,19 @@ func (h *CoordinatorHandler) HandleQuery(session *protocol.ConnectionSession, sq // If in explicit transaction, buffer mutations instead of immediate 2PC if inTransaction && isMutation { - return h.bufferStatement(session, stmt) + // MySQL has no transactional DDL: DDL ends the open transaction before + // it runs. Honouring that lets DML later in the transaction see a + // column the DDL added, which buffering to COMMIT cannot do. + if causesImplicitCommit(stmt) && cfg.Config.Transaction.DDLImplicitCommit { + if err := h.implicitCommitBeforeDDL(session, stmt); err != nil { + return nil, err + } + inTransaction = false + } else if protocol.IsDML(stmt) { + return h.executeEagerDML(session, stmt, params) + } else { + return h.bufferStatement(session, stmt) + } } // Normal path - immediate execution (for YCSB and auto-commit clients) @@ -480,6 +526,19 @@ func (h *CoordinatorHandler) HandleQuery(session *protocol.ConnectionSession, sq } } + // Read-your-own-writes: a plain SELECT inside an explicit transaction + // that already pinned this database reads from the pinned session so it + // observes the transaction's own uncommitted writes. + if stmt.Type == protocol.StatementSelect && inTransaction { + if rs, handled, readErr := h.tryPinnedRead(session, stmt, params); handled { + if readErr != nil { + return nil, readErr + } + h.processFoundRowsResult(session, rs) + return rs, nil + } + } + rs, err := h.handleRead(stmt, params, consistency) if err != nil { return nil, err @@ -600,11 +659,9 @@ func (h *CoordinatorHandler) handleMutation(stmt protocol.Statement, params []in } ctx, cancel := context.WithTimeout(context.Background(), hookTimeout) cancelHookCtx = cancel // Store for later - DO NOT call yet - // Use extracted params from literal extraction, or wire protocol params - execParams := params - if len(execParams) == 0 && len(stmt.ExtractedParams) > 0 { - execParams = stmt.ExtractedParams - } + // Merge extracted-literal params with wire protocol params in + // serialization order (see protocol.Statement.MergeExecParams). + execParams := stmt.MergeExecParams(params) req := ExecutionRequest{SQL: stmt.SQL, Params: execParams} pendingExec, err = replicatedDB.ExecuteLocalWithHooks(ctx, uint64(txnID), []ExecutionRequest{req}) @@ -737,6 +794,87 @@ func (h *CoordinatorHandler) handleMutation(stmt protocol.Statement, params []in return rs, nil } +// executeEagerDML runs one DML statement inside an explicit transaction +// eagerly, against a pinned (or newly-pinned) SQLite transaction for +// stmt.Database, and returns the REAL rows-affected/last-insert-id SQLite +// reports - mirroring handleMutation's autocommit pattern but keeping the +// transaction open (via PinnedSession) instead of committing through 2PC +// immediately. The write only becomes durable when the caller later runs +// handleCommit, which replays every pinned session's captured CDC entries +// through the same 2PC path autocommit DML uses. +func (h *CoordinatorHandler) executeEagerDML(session *protocol.ConnectionSession, stmt protocol.Statement, params []interface{}) (*protocol.ResultSet, error) { + txnState := session.GetTransaction() + if txnState == nil { + return nil, fmt.Errorf("no active transaction") + } + if stmt.Database == "" { + return nil, fmt.Errorf("no database selected for statement inside transaction") + } + + replicatedDB, err := h.dbManager.GetReplicatedDatabase(stmt.Database) + if err != nil { + return nil, fmt.Errorf("failed to get database %s: %w", stmt.Database, err) + } + + st := h.getOrCreatePinnedState(session.ConnID, txnState.TxnID) + pinned, err := st.getOrPin(stmt.Database, func() (PinnedSession, context.CancelFunc, error) { + ctx, cancel := context.WithTimeout(context.Background(), pinnedSessionTimeout()) + pinnedSession, err := replicatedDB.BeginPinnedSession(ctx, txnState.TxnID) + if err != nil { + cancel() + return nil, nil, err + } + return pinnedSession, cancel, nil + }) + if err != nil { + return nil, fmt.Errorf("failed to pin transaction on database %s: %w", stmt.Database, err) + } + session.MarkPinnedStateActive() + + execParams := stmt.MergeExecParams(params) + + execCtx, cancel := context.WithTimeout(context.Background(), pinnedSessionTimeout()) + defer cancel() + rowsAffected, lastInsertId, err := pinned.ExecuteStatement(execCtx, stmt.SQL, execParams) + if err != nil { + return nil, fmt.Errorf("DML execution failed: %w", err) + } + + rs := &protocol.ResultSet{RowsAffected: rowsAffected} + if stmt.Type == protocol.StatementInsert || stmt.Type == protocol.StatementReplace { + rs.LastInsertId = lastInsertId + } + return rs, nil +} + +// tryPinnedRead routes a SELECT to the pinned session for stmt.Database, if +// one exists on this connection's open transaction, so it observes the +// transaction's own uncommitted writes (read-your-own-writes). handled is +// false when there is no pinned session for this database (or no open +// transaction), meaning the caller should fall through to the normal read +// path - reads before any DML, or on a database nothing has written to yet. +func (h *CoordinatorHandler) tryPinnedRead(session *protocol.ConnectionSession, stmt protocol.Statement, params []interface{}) (*protocol.ResultSet, bool, error) { + st := h.lookupPinnedState(session.ConnID) + if st == nil { + return nil, false, nil + } + pinned, ok := st.get(stmt.Database) + if !ok { + return nil, false, nil + } + + execParams := stmt.MergeExecParams(params) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + columns, rows, err := pinned.Query(ctx, stmt.SQL, execParams) + if err != nil { + return nil, true, err + } + + return buildResultSetFromRows(columns, rows), true, nil +} + func (h *CoordinatorHandler) handleVectorControlMutation( stmt protocol.Statement, txnID uint64, @@ -858,14 +996,61 @@ func (h *CoordinatorHandler) handleVectorControlMutation( return &protocol.ResultSet{RowsAffected: 0, CommittedTxnId: txnID}, nil } +// buildResultSetFromRows converts column names and row maps (the shape both +// ReadCoordinator.ReadTransaction and PinnedSession.Query return) into a +// protocol.ResultSet, inferring column types from the values themselves. +func buildResultSetFromRows(columns []string, rows []map[string]interface{}) *protocol.ResultSet { + rs := &protocol.ResultSet{ + Columns: make([]protocol.ColumnDef, 0), + Rows: make([][]interface{}, 0), + } + + if len(rows) == 0 && len(columns) == 0 { + return rs + } + + // Use columns from response if available (preserves order) + if len(columns) > 0 { + for _, colName := range columns { + rs.Columns = append(rs.Columns, protocol.ColumnDef{Name: colName}) + } + } else if len(rows) > 0 { + // Fallback: Infer columns from first row (sorted for consistent order) + firstRow := rows[0] + colNames := make([]string, 0, len(firstRow)) + for colName := range firstRow { + colNames = append(colNames, colName) + } + sort.Strings(colNames) + for _, colName := range colNames { + rs.Columns = append(rs.Columns, protocol.ColumnDef{Name: colName}) + } + } + + for _, rowMap := range rows { + row := make([]interface{}, len(rs.Columns)) + for i, col := range rs.Columns { + row[i] = rowMap[col.Name] + } + rs.Rows = append(rs.Rows, row) + } + + // Column types come from the values themselves: SQLite types values, not + // columns, and strict clients refuse to decode a number out of a column + // declared as text. + for i, t := range protocol.InferColumnTypes(rs.Rows, len(rs.Columns)) { + rs.Columns[i].Type = t + } + + return rs +} + func (h *CoordinatorHandler) handleRead(stmt protocol.Statement, params []interface{}, consistency protocol.ConsistencyLevel) (*protocol.ResultSet, error) { queryStart := time.Now() - // Use extracted params from literal extraction, or wire protocol params - execParams := params - if len(execParams) == 0 && len(stmt.ExtractedParams) > 0 { - execParams = stmt.ExtractedParams - } + // Merge extracted-literal params with wire protocol params in + // serialization order (see protocol.Statement.MergeExecParams). + execParams := stmt.MergeExecParams(params) req := &ReadRequest{ Query: stmt.SQL, @@ -905,45 +1090,7 @@ func (h *CoordinatorHandler) handleRead(stmt protocol.Statement, params []interf } // Convert to protocol.ResultSet - rs := &protocol.ResultSet{ - Columns: make([]protocol.ColumnDef, 0), - Rows: make([][]interface{}, 0), - } - - if len(resp.Rows) > 0 || len(resp.Columns) > 0 { - // Use columns from response if available (preserves order) - if len(resp.Columns) > 0 { - for _, colName := range resp.Columns { - rs.Columns = append(rs.Columns, protocol.ColumnDef{Name: colName}) - } - } else if len(resp.Rows) > 0 { - // Fallback: Infer columns from first row (sorted for consistent order) - firstRow := resp.Rows[0] - colNames := make([]string, 0, len(firstRow)) - for colName := range firstRow { - colNames = append(colNames, colName) - } - sort.Strings(colNames) - for _, colName := range colNames { - rs.Columns = append(rs.Columns, protocol.ColumnDef{Name: colName}) - } - } - - for _, rowMap := range resp.Rows { - row := make([]interface{}, len(rs.Columns)) - for i, col := range rs.Columns { - row[i] = rowMap[col.Name] - } - rs.Rows = append(rs.Rows, row) - } - - // Column types come from the values themselves: SQLite types values, not - // columns, and strict clients refuse to decode a number out of a column - // declared as text. - for i, t := range protocol.InferColumnTypes(rs.Rows, len(rs.Columns)) { - rs.Columns[i].Type = t - } - } + rs := buildResultSetFromRows(resp.Columns, resp.Rows) // Record success metrics telemetry.QueriesTotal.With("select", "success").Inc() @@ -1165,9 +1312,31 @@ func (h *CoordinatorHandler) handleCommit(session *protocol.ConnectionSession) ( } txnState := session.GetTransaction() - if txnState == nil || len(txnState.Statements) == 0 { - // Empty transaction - just clear and return OK + pinnedState := h.takePinnedState(session.ConnID) + + bufferedCount := 0 + hadPinnedState := false + if txnState != nil { + bufferedCount = len(txnState.Statements) + hadPinnedState = txnState.HadPinnedState + } + if bufferedCount == 0 && (pinnedState == nil || pinnedState.isEmpty()) { session.EndTransaction() + if hadPinnedState { + // This transaction did pin eager-execution state at some point + // (executeEagerDML ran and called MarkPinnedStateActive), but + // that state is gone now without this COMMIT having read it - + // e.g. a concurrent forward-session eviction took and released + // it out from under an in-flight COMMIT. Reporting OK here would + // silently drop an already-executed write, so fail loud instead + // of taking the empty-transaction fast path. + log.Error(). + Uint64("conn_id", session.ConnID). + Uint64("txn_id", txnState.TxnID). + Msg("COMMIT: pinned transaction state missing at commit time; refusing to report success") + return nil, fmt.Errorf("transaction state for conn_id=%d txn_id=%d was lost before commit", session.ConnID, txnState.TxnID) + } + // Empty transaction - just clear and return OK log.Debug(). Uint64("conn_id", session.ConnID). Msg("COMMIT: Empty transaction") @@ -1177,75 +1346,57 @@ func (h *CoordinatorHandler) handleCommit(session *protocol.ConnectionSession) ( log.Debug(). Uint64("conn_id", session.ConnID). Uint64("txn_id", txnState.TxnID). - Int("stmt_count", len(txnState.Statements)). + Int("stmt_count", bufferedCount). Msg("COMMIT: Executing batched transaction via 2PC") - // Execute DML statements with hooks to capture CDC data - // This is required for 2PC replication - without CDC data, intents cannot be created - enrichedStatements := make([]protocol.Statement, 0, len(txnState.Statements)) + // Gather CDC entries already captured by every pinned (eager) session, + // plus whatever non-DML statements are still buffered (DDL when the + // implicit-commit flag is off, DCL, LOAD DATA, etc.) - DML never reaches + // txnState.Statements anymore, it executed eagerly via executeEagerDML. + enrichedStatements := make([]protocol.Statement, 0, bufferedCount) var totalRowsAffected int64 allCDCEntries := make([]common.CDCEntry, 0) // Collect all CDC entries for publishing - hookTimeout := 50 * time.Second - if cfg.Config != nil && cfg.Config.Transaction.LockWaitTimeoutSeconds > 0 { - hookTimeout = time.Duration(cfg.Config.Transaction.LockWaitTimeoutSeconds) * time.Second - } - - for i := 0; i < len(txnState.Statements); { - stmt := txnState.Statements[i] - if protocol.IsDML(stmt) && stmt.Database != "" { - replicatedDB, err := h.dbManager.GetReplicatedDatabase(stmt.Database) - if err != nil { - session.EndTransaction() - h.recentTxnIDs.Delete(txnState.TxnID) - return nil, fmt.Errorf("failed to get database %s: %w", stmt.Database, err) - } - - requests := make([]ExecutionRequest, 0, 1) - for i < len(txnState.Statements) { - nextStmt := txnState.Statements[i] - if !protocol.IsDML(nextStmt) || nextStmt.Database != stmt.Database { - break - } - requests = append(requests, ExecutionRequest{ - SQL: nextStmt.SQL, - Params: nextStmt.ExtractedParams, - }) - i++ + if pinnedState != nil { + for _, database := range pinnedState.databasesSorted() { + pinned, ok := pinnedState.get(database) + if !ok { + continue } - - ctx, cancel := context.WithTimeout(context.Background(), hookTimeout) - pendingExec, err := replicatedDB.ExecuteLocalWithHooks(ctx, txnState.TxnID, requests) - if err != nil { - cancel() - session.EndTransaction() - h.recentTxnIDs.Delete(txnState.TxnID) - return nil, fmt.Errorf("DML execution failed: %w", err) + cdcEntries := pinned.CDCEntries() + if len(cdcEntries) == 0 { + continue } - cancel() - - totalRowsAffected += pendingExec.GetTotalRowCount() - - // Convert CDC entries directly to statements - cdcEntries := pendingExec.GetCDCEntries() - if len(cdcEntries) > 0 { - allCDCEntries = append(allCDCEntries, cdcEntries...) - for _, entry := range cdcEntries { - cdcStmt := ConvertToStatement(entry) - cdcStmt.SQL = stmt.SQL - cdcStmt.Database = stmt.Database - enrichedStatements = append(enrichedStatements, cdcStmt) - } + allCDCEntries = append(allCDCEntries, cdcEntries...) + for _, entry := range cdcEntries { + cdcStmt := ConvertToStatement(entry) + cdcStmt.Database = database + enrichedStatements = append(enrichedStatements, cdcStmt) } - // No captured rows means every statement in the group matched none. - // They contribute nothing to replicate and are dropped; DML must - // never fall back to raw SQL replication. - } else { - enrichedStatements = append(enrichedStatements, stmt) - i++ + totalRowsAffected += int64(len(cdcEntries)) } } + for _, stmt := range txnState.Statements { + enrichedStatements = append(enrichedStatements, stmt) + } + + // Release every pinned SQLite transaction now that its CDC entries have + // been read out - mirrors ExecuteLocalWithHooks's hookDB, which is rolled + // back immediately after capturing CDC data and well before 2PC's local + // commit runs (db/db_integration.go ExecuteLocalWithHooks). SQLite allows + // only one writer for the whole file: leaving the pinned transaction open + // through WriteTransaction below would contend with 2PC's own local write + // (TransactionManager.applyCDCEntries) on a different connection and + // deadlock until SQLite's busy timeout gives up with "database is + // locked" - confirmed by TestNoopDMLInExplicitTransaction failing that + // way when release was ordered after WriteTransaction as originally + // specified. Never commits - see PinnedSession's doc comment; the actual + // write only becomes durable via CDC replay in WriteTransaction below. + if pinnedState != nil { + pinnedState.releaseAll() + } + // Create 2PC transaction with CDC-enriched statements txn := &Transaction{ ID: txnState.TxnID, @@ -1329,6 +1480,12 @@ func (h *CoordinatorHandler) handleRollback(session *protocol.ConnectionSession) txnID = txnState.TxnID } + // Release every pinned eager-execution session; PinnedSession.Release + // always rolls back, so this discards their writes. + if pinnedState := h.takePinnedState(session.ConnID); pinnedState != nil { + pinnedState.releaseAll() + } + // Just discard the buffer - no network activity needed session.EndTransaction() // Cleanup from recentTxnIDs to prevent memory growth @@ -1344,7 +1501,63 @@ func (h *CoordinatorHandler) handleRollback(session *protocol.ConnectionSession) return nil, nil // OK response } +// CloseSession releases any pinned eager-execution transaction state left +// open when a connection ends without an explicit COMMIT or ROLLBACK - a +// disconnected client, or (via write forwarding) an evicted/expired forward +// session. Satisfies protocol.SessionCloser. +func (h *CoordinatorHandler) CloseSession(session *protocol.ConnectionSession) { + if session == nil { + return + } + if pinnedState := h.takePinnedState(session.ConnID); pinnedState != nil { + pinnedState.releaseAll() + } + if session.InTransaction() { + txnState := session.GetTransaction() + session.EndTransaction() + if txnState != nil { + h.recentTxnIDs.Delete(txnState.TxnID) + } + } +} + // bufferStatement adds a mutation to the active transaction buffer +// causesImplicitCommit reports whether a statement ends an open transaction +// before it runs, as MySQL's "statements that cause an implicit commit" do. +// Schema changes qualify; DML does not. +func causesImplicitCommit(stmt protocol.Statement) bool { + switch stmt.Type { + case protocol.StatementDDL, + protocol.StatementCreateDatabase, + protocol.StatementDropDatabase, + protocol.StatementCreateVectorIndex, + protocol.StatementDropVectorIndex, + protocol.StatementReindexVectorIndex: + return true + default: + return false + } +} + +// implicitCommitBeforeDDL commits the statements buffered so far and closes the +// transaction, so the DDL that follows runs on its own. +// +// The commit happens first and its failure is reported without running the DDL, +// matching MySQL: the transaction is ended by the attempt either way, so the +// session is left with no transaction open regardless of the outcome. +func (h *CoordinatorHandler) implicitCommitBeforeDDL(session *protocol.ConnectionSession, stmt protocol.Statement) error { + log.Debug(). + Uint64("conn_id", session.ConnID). + Int("stmt_type", int(stmt.Type)). + Msg("DDL in transaction: committing buffered statements first") + + // handleCommit ends the transaction whether or not anything was buffered. + if _, err := h.handleCommit(session); err != nil { + return fmt.Errorf("implicit commit before DDL failed: %w", err) + } + return nil +} + func (h *CoordinatorHandler) bufferStatement(session *protocol.ConnectionSession, stmt protocol.Statement) (*protocol.ResultSet, error) { session.AddStatement(stmt) diff --git a/coordinator/noop_dml_test.go b/coordinator/noop_dml_test.go index cc84f17e..8e12291f 100644 --- a/coordinator/noop_dml_test.go +++ b/coordinator/noop_dml_test.go @@ -101,7 +101,14 @@ func setupNoopDML(t *testing.T) *noopDMLSetup { noopNodeRegistry{}, ) - session := &protocol.ConnectionSession{ConnID: 1, CurrentDatabase: dbName} + // Real connections enable transpilation (protocol/server.go), and without it + // BEGIN does not parse as transaction control, so a test meaning to exercise + // the explicit-transaction path would silently run in autocommit. + session := &protocol.ConnectionSession{ + ConnID: 1, + CurrentDatabase: dbName, + TranspilationEnabled: true, + } _, err = handler.HandleQuery(session, "CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)", nil) require.NoError(t, err) @@ -207,7 +214,7 @@ func TestAllNoopTransactionSkips2PC(t *testing.T) { func TestUnknownDatabaseDMLReportsDatabase(t *testing.T) { s := setupNoopDML(t) - session := &protocol.ConnectionSession{ConnID: 2, CurrentDatabase: "nosuchdb"} + session := &protocol.ConnectionSession{ConnID: 2, CurrentDatabase: "nosuchdb", TranspilationEnabled: true} _, err := s.handler.HandleQuery(session, "DELETE FROM t WHERE id = 1", nil) require.Error(t, err) diff --git a/coordinator/pinned_txn.go b/coordinator/pinned_txn.go new file mode 100644 index 00000000..c365ea6c --- /dev/null +++ b/coordinator/pinned_txn.go @@ -0,0 +1,206 @@ +package coordinator + +import ( + "context" + "sort" + "sync" + + "github.com/maxpert/marmot/common" + "github.com/rs/zerolog/log" +) + +// PinnedSession is a real SQLite transaction held open on a dedicated +// connection for the lifetime of an explicit BEGIN...COMMIT/ROLLBACK, used to +// execute DML eagerly: every statement gets the REAL rows-affected/ +// last-insert-id SQLite reports (not the fake buffered-statement response), +// and reads on the same database within the transaction observe the +// transaction's own uncommitted writes. +// +// The underlying SQLite transaction is NEVER committed directly. The actual +// write lands in the database only through CDC replay during 2PC, exactly as +// it does today for ExecuteLocalWithHooks (see that method's doc comment): +// PREPARE persists the captured CDC entries as durable intents, and COMMIT +// applies them to the write connection via TransactionManager. +// applyCDCEntries. Release always rolls the pinned SQLite transaction back - +// on an explicit ROLLBACK, on a 2PC rejection, and even after a successful +// COMMIT (whose CDC entries were already read out via CDCEntries before +// Release runs) - so the write is never applied twice. +type PinnedSession interface { + // ExecuteStatement runs one DML statement on the pinned transaction. It + // captures CDC data for the statement and acquires the same per-row + // CDC/intent locks ExecuteLocalWithHooks does, immediately rather than + // deferred to Release/COMMIT (so a later statement in the same or another + // transaction that touches the same row blocks/conflicts from this point + // on, not just from COMMIT). + ExecuteStatement(ctx context.Context, sql string, params []interface{}) (rowsAffected int64, lastInsertId int64, err error) + + // Query runs a read on the pinned transaction so it observes the + // transaction's own uncommitted writes. columns/rows use the same shape + // as ReplicatedDatabase.ExecuteSnapshotRead. + Query(ctx context.Context, sql string, params []interface{}) (columns []string, rows []map[string]interface{}, err error) + + // CDCEntries returns every CDC entry captured so far (already row-locked), + // across all ExecuteStatement calls on this session, in the order the + // statements ran. + CDCEntries() []common.CDCEntry + + // Release rolls back the pinned SQLite transaction and releases its row + // locks and connection. Safe to call exactly once; the caller must not + // use the session afterward. Never commits - see type doc. + Release() error +} + +// pinnedEntry pairs a PinnedSession with the context that owns its SQLite +// transaction's lifetime. Go's database/sql auto-rolls-back a transaction +// when the context passed to BeginTx is cancelled, so this context must stay +// alive until Release() has been called - cancel is invoked right after, +// mirroring the cancelHookCtx pattern already used for the autocommit path +// in handleMutation. +type pinnedEntry struct { + session PinnedSession + cancel context.CancelFunc +} + +// connPinnedState holds the pinned sessions for one connection's currently +// open explicit transaction, keyed by database name. A transaction that +// touches more than one database gets one pinned SQLite transaction per +// database, all committed (via 2PC + CDC replay) or rolled back together at +// COMMIT/ROLLBACK. +type connPinnedState struct { + mu sync.Mutex + txnID uint64 + sessions map[string]pinnedEntry +} + +func newConnPinnedState(txnID uint64) *connPinnedState { + return &connPinnedState{ + txnID: txnID, + sessions: make(map[string]pinnedEntry), + } +} + +// get returns the pinned session for database, if one exists. +func (st *connPinnedState) get(database string) (PinnedSession, bool) { + st.mu.Lock() + defer st.mu.Unlock() + entry, ok := st.sessions[database] + if !ok { + return nil, false + } + return entry.session, true +} + +// getOrPin returns the existing pinned session for database, or creates one +// via begin and registers it. begin is called at most once per database per +// transaction. +func (st *connPinnedState) getOrPin(database string, begin func() (PinnedSession, context.CancelFunc, error)) (PinnedSession, error) { + st.mu.Lock() + if entry, ok := st.sessions[database]; ok { + st.mu.Unlock() + return entry.session, nil + } + st.mu.Unlock() + + // begin runs outside the lock: it does real I/O (opens a connection, + // starts a SQLite transaction) and must not block other databases in the + // same multi-database transaction from pinning concurrently. + session, cancel, err := begin() + if err != nil { + return nil, err + } + + st.mu.Lock() + defer st.mu.Unlock() + if entry, ok := st.sessions[database]; ok { + // Lost a race with a concurrent pin of the same database; discard + // the one we just created and use the winner's. + cancel() + _ = session.Release() + return entry.session, nil + } + st.sessions[database] = pinnedEntry{session: session, cancel: cancel} + return session, nil +} + +// databasesSorted returns the pinned databases in a deterministic (sorted) +// order, so the CDC entries fed into 2PC have a reproducible statement order +// instead of depending on Go's randomized map iteration. +func (st *connPinnedState) databasesSorted() []string { + st.mu.Lock() + defer st.mu.Unlock() + names := make([]string, 0, len(st.sessions)) + for name := range st.sessions { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// isEmpty reports whether any database has been pinned yet. +func (st *connPinnedState) isEmpty() bool { + st.mu.Lock() + defer st.mu.Unlock() + return len(st.sessions) == 0 +} + +// releaseAll rolls back every pinned session and releases its context. Safe +// to call once, after which the state must be discarded (see +// CoordinatorHandler.takePinnedState). +func (st *connPinnedState) releaseAll() { + st.mu.Lock() + defer st.mu.Unlock() + for database, entry := range st.sessions { + if err := entry.session.Release(); err != nil { + log.Error().Err(err).Str("database", database).Uint64("txn_id", st.txnID). + Msg("Failed to release pinned session") + } + entry.cancel() + } + st.sessions = nil +} + +// getOrCreatePinnedState returns the connPinnedState for connID, creating one +// for txnID if none exists. If a stale state from a different (already +// finished) transaction is somehow still present - COMMIT/ROLLBACK/ +// CloseSession always remove it via takePinnedState, so this only guards a +// bug elsewhere - it is discarded rather than reused. +func (h *CoordinatorHandler) getOrCreatePinnedState(connID, txnID uint64) *connPinnedState { + if v, ok := h.pinnedTxns.Load(connID); ok { + st := v.(*connPinnedState) + st.mu.Lock() + sameTxn := st.txnID == txnID + st.mu.Unlock() + if sameTxn { + return st + } + log.Warn().Uint64("conn_id", connID).Uint64("stale_txn_id", st.txnID).Uint64("txn_id", txnID). + Msg("Discarding stale pinned transaction state") + st.releaseAll() + } + + st := newConnPinnedState(txnID) + h.pinnedTxns.Store(connID, st) + return st +} + +// lookupPinnedState returns the connPinnedState for connID without removing +// it, or nil if none exists. Used by in-transaction read routing, which must +// not disturb COMMIT/ROLLBACK's ownership of the state. +func (h *CoordinatorHandler) lookupPinnedState(connID uint64) *connPinnedState { + v, ok := h.pinnedTxns.Load(connID) + if !ok { + return nil + } + return v.(*connPinnedState) +} + +// takePinnedState removes and returns the connPinnedState for connID, or nil +// if none exists. The caller takes ownership and must call releaseAll (or +// rely on it already being empty) exactly once. +func (h *CoordinatorHandler) takePinnedState(connID uint64) *connPinnedState { + v, ok := h.pinnedTxns.LoadAndDelete(connID) + if !ok { + return nil + } + return v.(*connPinnedState) +} diff --git a/coordinator/prepared_autoinc_params_test.go b/coordinator/prepared_autoinc_params_test.go new file mode 100644 index 00000000..1e221c57 --- /dev/null +++ b/coordinator/prepared_autoinc_params_test.go @@ -0,0 +1,170 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package coordinator_test + +// Reproduces the LLDAP boot failure: a prepared INSERT into a table with an +// auto-increment PK, executed with wire-supplied params, failed with +// "not enough args to execute query: want 6 got 5". Root cause: +// transform.ExtractLiterals rewrites the auto-increment id the pipeline +// injects into the AST (a *sqlparser.Literal) into its own `?` placeholder, +// so the final SQL carries 6 placeholders (5 caller + 1 injected) while the +// coordinator's execParams selection picked only ONE of the two param +// sources (wire params XOR stmt.ExtractedParams) instead of merging them per +// stmt.ParamOrder. See protocol.Statement.MergeExecParams. + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestPreparedAutoIncrementInsertWithWireParams pins the exact LLDAP shape: +// a prepared INSERT naming every non-PK column, with 5 wire params, inside an +// explicit transaction, against a table with an auto-increment PK. +func TestPreparedAutoIncrementInsertWithWireParams(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "CREATE TABLE groups ("+ + "group_id INTEGER PRIMARY KEY, "+ + "display_name TEXT, "+ + "lowercase_display_name TEXT, "+ + "creation_date TEXT, "+ + "uuid TEXT, "+ + "modified_date TEXT)", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "BEGIN", nil) + require.NoError(t, err) + + sql := "INSERT INTO groups (display_name, lowercase_display_name, creation_date, uuid, modified_date) " + + "VALUES (?, ?, ?, ?, ?)" + params := []interface{}{"Admins", "admins", "2026-01-01", "uuid-1", "2026-01-01"} + + res, err := s.handler.HandleQuery(s.session, sql, params) + require.NoError(t, err, "prepared INSERT with wire params must not fail on placeholder/arg count mismatch") + require.NotNil(t, res) + require.Equal(t, int64(1), res.RowsAffected) + require.NotZero(t, res.LastInsertId, "auto-increment id must still be generated and returned") + + _, err = s.handler.HandleQuery(s.session, "COMMIT", nil) + require.NoError(t, err) + + var displayName, uuid string + var groupID int64 + require.NoError(t, s.conn.QueryRow( + "SELECT group_id, display_name, uuid FROM groups WHERE display_name = 'Admins'", + ).Scan(&groupID, &displayName, &uuid)) + require.Equal(t, "Admins", displayName) + require.Equal(t, "uuid-1", uuid) + require.NotZero(t, groupID) + require.Equal(t, groupID, res.LastInsertId) +} + +// TestPreparedAutoIncrementInsertAutocommit pins the same shape outside an +// explicit transaction, exercising the autocommit DML path. +func TestPreparedAutoIncrementInsertAutocommit(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "CREATE TABLE groups ("+ + "group_id INTEGER PRIMARY KEY, "+ + "display_name TEXT, "+ + "lowercase_display_name TEXT, "+ + "creation_date TEXT, "+ + "uuid TEXT, "+ + "modified_date TEXT)", nil) + require.NoError(t, err) + + sql := "INSERT INTO groups (display_name, lowercase_display_name, creation_date, uuid, modified_date) " + + "VALUES (?, ?, ?, ?, ?)" + params := []interface{}{"Users", "users", "2026-01-01", "uuid-2", "2026-01-01"} + + res, err := s.handler.HandleQuery(s.session, sql, params) + require.NoError(t, err, "autocommit prepared INSERT with wire params must not fail") + require.NotNil(t, res) + require.Equal(t, int64(1), res.RowsAffected) + require.NotZero(t, res.LastInsertId) + + var displayName string + require.NoError(t, s.conn.QueryRow( + "SELECT display_name FROM groups WHERE uuid = 'uuid-2'", + ).Scan(&displayName)) + require.Equal(t, "Users", displayName) +} + +// TestUpdateLiteralBeforeWireParamOrdering proves values land in the right +// columns when a literal precedes a wire placeholder in the serialized SQL, +// not merely that execution succeeds. +func TestUpdateLiteralBeforeWireParamOrdering(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "CREATE TABLE items ("+ + "id INTEGER PRIMARY KEY, "+ + "status TEXT, "+ + "name TEXT)", nil) + require.NoError(t, err) + + _, err = s.handler.HandleQuery(s.session, "INSERT INTO items (id, status, name) VALUES (1, 'pending', 'orig')", nil) + require.NoError(t, err) + + // The auto-increment rule doesn't apply here (id is supplied), but this + // still exercises the general literal-before-wire-param serialization + // order that transform.ExtractLiterals/MergeExecParams must preserve + // whenever ExtractLiterals runs alongside caller-supplied `?` marks. + sql := "UPDATE items SET status = 'active', name = ? WHERE id = ?" + params := []interface{}{"renamed", int64(1)} + + res, err := s.handler.HandleQuery(s.session, sql, params) + require.NoError(t, err) + require.Equal(t, int64(1), res.RowsAffected) + + var status, name string + require.NoError(t, s.conn.QueryRow("SELECT status, name FROM items WHERE id = 1").Scan(&status, &name)) + require.Equal(t, "active", status, "the literal-valued column must get the literal, not a wire param") + require.Equal(t, "renamed", name, "the wire-param column must get the wire param, not the literal") +} + +// TestTextProtocolInsertAllLiterals is a regression check: an INSERT with no +// wire params at all (text protocol, everything a literal) must still work. +func TestTextProtocolInsertAllLiterals(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "CREATE TABLE plain ("+ + "id INTEGER PRIMARY KEY, "+ + "name TEXT)", nil) + require.NoError(t, err) + + res, err := s.handler.HandleQuery(s.session, "INSERT INTO plain (name) VALUES ('literal-only')", nil) + require.NoError(t, err) + require.Equal(t, int64(1), res.RowsAffected) + require.NotZero(t, res.LastInsertId) + + var name string + require.NoError(t, s.conn.QueryRow("SELECT name FROM plain WHERE id = ?", res.LastInsertId).Scan(&name)) + require.Equal(t, "literal-only", name) +} + +// TestPreparedInsertNoAutoIncrementColumn pins that a wire-param INSERT into +// a table with no auto-increment column (no id injection, so no +// ExtractedParams/ParamOrder in play at all) is unaffected by the merge. +func TestPreparedInsertNoAutoIncrementColumn(t *testing.T) { + s := setupNoopDML(t) + + _, err := s.handler.HandleQuery(s.session, "CREATE TABLE logs ("+ + "seq INTEGER, "+ + "message TEXT)", nil) + require.NoError(t, err) + + sql := "INSERT INTO logs (seq, message) VALUES (?, ?)" + params := []interface{}{int64(42), "hello"} + + res, err := s.handler.HandleQuery(s.session, sql, params) + require.NoError(t, err) + require.Equal(t, int64(1), res.RowsAffected) + + var seq int64 + var message string + require.NoError(t, s.conn.QueryRow("SELECT seq, message FROM logs").Scan(&seq, &message)) + require.Equal(t, int64(42), seq) + require.Equal(t, "hello", message) +} diff --git a/coordinator/setup_test.go b/coordinator/setup_test.go new file mode 100644 index 00000000..a9c544b2 --- /dev/null +++ b/coordinator/setup_test.go @@ -0,0 +1,22 @@ +package coordinator + +import ( + "os" + "testing" + + "github.com/maxpert/marmot/id" + "github.com/maxpert/marmot/protocol" +) + +// TestMain initializes the query pipeline before any test runs. +// +// Without it, statements are parsed in a degraded path: BEGIN never opens a +// transaction and MySQL-dialect syntax is not transpiled, so tests that mean to +// exercise the explicit-transaction path silently run in autocommit and pass +// for the wrong reason. +func TestMain(m *testing.M) { + if err := protocol.InitializePipeline(10000, id.NewCompactGenerator(1)); err != nil { + panic("failed to initialize query pipeline for tests: " + err.Error()) + } + os.Exit(m.Run()) +} diff --git a/coordinator/vec_handler.go b/coordinator/vec_handler.go index 850feafe..d80c190f 100644 --- a/coordinator/vec_handler.go +++ b/coordinator/vec_handler.go @@ -28,10 +28,7 @@ func (h *CoordinatorHandler) maybeRewriteVectorSelect( return nil, nil, nil } - execParams := params - if len(execParams) == 0 && len(stmt.ExtractedParams) > 0 { - execParams = stmt.ExtractedParams - } + execParams := stmt.MergeExecParams(params) querySession := &connQuerySession{vars: &session.VecVars} templateKey := makeGoRankTemplateKey(stmt.SQL, session.CurrentDatabase, querySession) @@ -129,17 +126,13 @@ func (h *CoordinatorHandler) executeVectorPlan( return nil, err } if info.FallbackOn && rs != nil && len(rs.Rows) < info.K && info.FallbackSQL != "" { - fb := stmt - fb.SQL = info.FallbackSQL - fb.ExtractedParams = args + fb := stmt.WithResolvedParams(info.FallbackSQL, args) return h.handleRead(fb, args, consistency) } return rs, nil } - primary := stmt - primary.SQL = info.PrimarySQL - primary.ExtractedParams = args + primary := stmt.WithResolvedParams(info.PrimarySQL, args) rs, err := h.handleRead(primary, args, consistency) if err != nil { @@ -148,9 +141,7 @@ func (h *CoordinatorHandler) executeVectorPlan( // §7.5 short-result fallback: only post-filter with explicit FallbackOn. if info.FallbackOn && info.Plan == PlanPostFilter && rs != nil && len(rs.Rows) < info.K && info.FallbackSQL != "" { - fb := stmt - fb.SQL = info.FallbackSQL - fb.ExtractedParams = args + fb := stmt.WithResolvedParams(info.FallbackSQL, args) fbRS, fbErr := h.handleRead(fb, args, consistency) if fbErr != nil { return nil, fbErr diff --git a/coordinator/vec_testexport_test.go b/coordinator/vec_testexport_test.go index 88ed6c66..b843b791 100644 --- a/coordinator/vec_testexport_test.go +++ b/coordinator/vec_testexport_test.go @@ -42,3 +42,27 @@ func (h *CoordinatorHandler) ExecuteVectorPlan( ) (*protocol.ResultSet, error) { return h.executeVectorPlan(stmt, info, args, consistency) } + +// CausesImplicitCommit exposes the implicit-commit classification to external +// test packages. +func CausesImplicitCommit(stmt protocol.Statement) bool { + return causesImplicitCommit(stmt) +} + +// TakeAndReleasePinnedStateForTest takes and releases the pinned transaction +// state for connID, simulating it being torn down by something other than +// that transaction's own COMMIT/ROLLBACK - e.g. a concurrent forward-session +// eviction calling CoordinatorHandler.CloseSession. Unlike CloseSession, it +// does not touch the session's transaction state, so tests can reproduce the +// exact race window handleCommit's empty-transaction fast path must guard +// against: pinned state gone, but the session's ConnectionSession still +// believes it is mid-transaction. Returns false if there was no pinned state +// to take. +func (h *CoordinatorHandler) TakeAndReleasePinnedStateForTest(connID uint64) bool { + st := h.takePinnedState(connID) + if st == nil { + return false + } + st.releaseAll() + return true +} diff --git a/db/cdc_applier.go b/db/cdc_applier.go index dd26c751..4deef619 100644 --- a/db/cdc_applier.go +++ b/db/cdc_applier.go @@ -5,6 +5,8 @@ import ( "fmt" "sort" "strings" + + "github.com/rs/zerolog/log" ) // CDCExecutor abstracts sql.DB and sql.Tx - both have identical Exec signature. @@ -53,8 +55,10 @@ func ApplyCDCValues(exec CDCExecutor, schema CDCSchemaProvider, opType OpType, t // ApplyCDCInsert performs INSERT OR REPLACE using CDC row data. // Columns are sorted alphabetically to ensure deterministic SQL generation. -// Values are deserialized from msgpack and []byte values are converted to strings -// to preserve TEXT type affinity in SQLite. +// Values are deserialized from msgpack via unmarshalCDCValue: TEXT-affinity +// columns were encoded as msgpack Str at capture time and decode as string; +// BLOB-affinity columns were encoded as msgpack Bin and decode as []byte, so +// they bind via sqlite3_bind_blob and keep their BLOB storage class. func ApplyCDCInsert(exec CDCExecutor, tableName string, newValues map[string][]byte) error { if len(newValues) == 0 { return fmt.Errorf("ApplyCDCInsert %s: no values to insert", tableName) @@ -153,10 +157,11 @@ func ApplyCDCUpdate(exec CDCExecutor, schema CDCSchemaProvider, tableName string strings.Join(setClauses, ", "), strings.Join(whereClauses, " AND ")) - _, err = exec.Exec(sqlStmt, values...) + result, err := exec.Exec(sqlStmt, values...) if err != nil { return fmt.Errorf("ApplyCDCUpdate %s: %w", tableName, err) } + logZeroRowsAffected(result, "ApplyCDCUpdate", tableName) return nil } @@ -197,13 +202,27 @@ func ApplyCDCDelete(exec CDCExecutor, schema CDCSchemaProvider, tableName string quoteSQLiteIdent(tableName), strings.Join(whereClauses, " AND ")) - _, err = exec.Exec(sqlStmt, values...) + result, err := exec.Exec(sqlStmt, values...) if err != nil { return fmt.Errorf("ApplyCDCDelete %s: %w", tableName, err) } + logZeroRowsAffected(result, "ApplyCDCDelete", tableName) return nil } +// logZeroRowsAffected logs at Debug level when a CDC UPDATE/DELETE matched no +// rows. This is not necessarily a bug - a legitimate FK ON DELETE CASCADE +// no-op looks identical to a real divergence (e.g. bad PK encoding) - so it is +// only logged, not treated as an error, but it must be visible for diagnosing +// replication divergence. Debug level keeps it out of default log output. +func logZeroRowsAffected(result sql.Result, opName, tableName string) { + n, err := result.RowsAffected() + if err != nil || n != 0 { + return + } + log.Debug().Str("table", tableName).Str("op", opName).Msg("CDC apply matched no rows") +} + func cdcPrimaryKeyPredicate(opName, tableName, pkCol string, pkBytes []byte, values []interface{}) (string, []interface{}, error) { value, err := unmarshalCDCValue(pkBytes) if err != nil { diff --git a/db/cdc_applier_test.go b/db/cdc_applier_test.go index c1ed90d1..a9665d41 100644 --- a/db/cdc_applier_test.go +++ b/db/cdc_applier_test.go @@ -568,7 +568,24 @@ func TestApplyCDCDelete_CompositePKWithNullComponent(t *testing.T) { } // TestUnmarshalCDCValue_BytesToString verifies []byte converts to string -func TestUnmarshalCDCValue_BytesToString(t *testing.T) { +// TestUnmarshalCDCValue_PreservesEncodedType verifies unmarshalCDCValue uses +// STRICT msgpack decoding: it returns exactly the Go type that was encoded, +// with no byte-slice-to-string coercion. A []byte value decodes back as +// []byte (msgpack Bin), preserving BLOB storage class on apply. +// +// This replaces the old "bytes always convert to string" contract: that +// conversion was previously done unconditionally here (and via +// encoding.Unmarshal's loose interface decoding before that), which silently +// corrupted BLOB columns - they were captured as raw []byte too, with no way +// to tell them apart from a TEXT column's []byte at this point. The decision +// of whether a []byte should round-trip as string now happens earlier, at +// capture time, using the column's declared TEXT affinity +// (encodeValuesWithSchema in preupdate_hook.go: TEXT-affinity columns are +// converted to string BEFORE encoding, so they arrive here already encoded +// as msgpack Str and decode as string; see TestEncodeValuesWithSchema_BlobVsText +// and the TestBlobFidelity_* tests in cdc_blob_fidelity_test.go for the +// full capture->apply round trip this enables). +func TestUnmarshalCDCValue_PreservesEncodedType(t *testing.T) { tests := []struct { name string input interface{} @@ -595,9 +612,9 @@ func TestUnmarshalCDCValue_BytesToString(t *testing.T) { expected: nil, }, { - name: "Byte slice (should convert to string)", - input: []byte("text_data"), - expected: "text_data", + name: "Byte slice (must stay []byte, not be coerced to string)", + input: []byte("blob_data"), + expected: []byte("blob_data"), }, } @@ -609,17 +626,16 @@ func TestUnmarshalCDCValue_BytesToString(t *testing.T) { t.Fatalf("unmarshalCDCValue failed: %v", err) } - // Special handling for []byte -> string conversion - if tt.name == "Byte slice (should convert to string)" { - if str, ok := result.(string); !ok { - t.Errorf("Expected string, got %T", result) - } else if str != tt.expected { - t.Errorf("Expected '%v', got '%v'", tt.expected, result) + if expectedBytes, ok := tt.expected.([]byte); ok { + resultBytes, ok := result.([]byte) + if !ok { + t.Fatalf("Expected []byte, got %T", result) } - } else { - if result != tt.expected { - t.Errorf("Expected %v (%T), got %v (%T)", tt.expected, tt.expected, result, result) + if string(resultBytes) != string(expectedBytes) { + t.Errorf("Expected %v, got %v", expectedBytes, resultBytes) } + } else if result != tt.expected { + t.Errorf("Expected %v (%T), got %v (%T)", tt.expected, tt.expected, result, result) } }) } @@ -664,3 +680,85 @@ func TestApplyCDC_EmptyValues(t *testing.T) { t.Error("Expected error for empty DELETE values, got nil") } } + +// TestApplyCDCUpdate_NoRowsMatched verifies an UPDATE whose WHERE clause +// matches no row is NOT treated as an error - it must return nil so a +// legitimate no-op (e.g. the row was already removed by an FK ON DELETE +// CASCADE that ran ahead of this CDC entry) doesn't abort replication. The +// row-affected count is only surfaced via a Debug log for diagnosability, not +// as a failure. +func TestApplyCDCUpdate_NoRowsMatched(t *testing.T) { + db, cleanup := setupCDCTestDB(t) + defer cleanup() + + _, err := db.Exec(`CREATE TABLE test_noop (id INTEGER PRIMARY KEY, name TEXT)`) + if err != nil { + t.Fatalf("Failed to create table: %v", err) + } + // Table is intentionally left empty - no row with id=999 exists. + + schema := &mockSchemaProvider{ + schemas: map[string][]string{ + "test_noop": {"id"}, + }, + } + + oldValues := map[string][]byte{"id": marshalValue(t, int64(999))} + newValues := map[string][]byte{"id": marshalValue(t, int64(999)), "name": marshalValue(t, "ghost")} + + err = ApplyCDCUpdate(db, schema, "test_noop", oldValues, newValues) + if err != nil { + t.Fatalf("ApplyCDCUpdate on a non-matching row must return nil, got: %v", err) + } +} + +// TestApplyCDCDelete_NoRowsMatched verifies a DELETE whose WHERE clause +// matches no row is likewise not an error (see TestApplyCDCUpdate_NoRowsMatched). +func TestApplyCDCDelete_NoRowsMatched(t *testing.T) { + db, cleanup := setupCDCTestDB(t) + defer cleanup() + + _, err := db.Exec(`CREATE TABLE test_noop_del (id INTEGER PRIMARY KEY, name TEXT)`) + if err != nil { + t.Fatalf("Failed to create table: %v", err) + } + + schema := &mockSchemaProvider{ + schemas: map[string][]string{ + "test_noop_del": {"id"}, + }, + } + + oldValues := map[string][]byte{"id": marshalValue(t, int64(999))} + + err = ApplyCDCDelete(db, schema, "test_noop_del", oldValues) + if err != nil { + t.Fatalf("ApplyCDCDelete on a non-matching row must return nil, got: %v", err) + } +} + +// TestLogZeroRowsAffected_OnlyLogsOnZero verifies the helper distinguishes a +// real zero-rows case from a normal affected-rows result and from a driver +// that can't report RowsAffected, without ever panicking or altering control +// flow (it has no return value to affect - this locks in that contract). +func TestLogZeroRowsAffected_OnlyLogsOnZero(t *testing.T) { + db, cleanup := setupCDCTestDB(t) + defer cleanup() + + _, err := db.Exec(`CREATE TABLE test_log (id INTEGER PRIMARY KEY)`) + if err != nil { + t.Fatalf("Failed to create table: %v", err) + } + res, err := db.Exec(`INSERT INTO test_log (id) VALUES (1)`) + if err != nil { + t.Fatalf("Failed to insert: %v", err) + } + // sql.Result from a normal driver Exec: must not panic regardless of count. + logZeroRowsAffected(res, "TestOp", "test_log") + + zeroRes, err := db.Exec(`DELETE FROM test_log WHERE id = 999`) + if err != nil { + t.Fatalf("Failed to exec no-op delete: %v", err) + } + logZeroRowsAffected(zeroRes, "TestOp", "test_log") +} diff --git a/db/cdc_blob_fidelity_test.go b/db/cdc_blob_fidelity_test.go new file mode 100644 index 00000000..921c8230 --- /dev/null +++ b/db/cdc_blob_fidelity_test.go @@ -0,0 +1,324 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestBlobFidelity_RoundTrip_Insert reproduces the LLDAP-shaped bug: a BLOB +// column (password hash / BINARY(16) UUID equivalent) must keep its BLOB +// storage class end to end through capture, msgpack encode/decode, and apply +// on a replica - not be silently coerced to TEXT. +func TestBlobFidelity_RoundTrip_Insert(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE creds (id INTEGER PRIMARY KEY, name TEXT, pwhash BLOB)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + blobVal := []byte{0x00, 0xFF, 0x10, 0xAB, 0x00, 0x01, 0x02, 0xDE, 0xAD, 0xBE, 0xEF} + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9001) + require.NoError(t, err) + defer session.Rollback() + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, + "INSERT INTO creds (id, name, pwhash) VALUES (?, ?, ?)", 1, "alice", blobVal) + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + require.Len(t, entries, 1) + + applyEntries(t, replica, entries) + + var typeofPwhash, typeofName string + var gotBlob []byte + var gotName string + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(pwhash), typeof(name), pwhash, name FROM creds WHERE id = 1`). + Scan(&typeofPwhash, &typeofName, &gotBlob, &gotName)) + + assert.Equal(t, "blob", typeofPwhash, "BLOB column must keep BLOB storage class on the replica") + assert.Equal(t, "text", typeofName, "TEXT column must stay TEXT") + assert.Equal(t, blobVal, gotBlob, "BLOB bytes must round-trip exactly") + assert.Equal(t, "alice", gotName) +} + +// TestBlobFidelity_RoundTrip_Update verifies an UPDATE that changes a BLOB +// column also preserves BLOB storage class on apply. +func TestBlobFidelity_RoundTrip_Update(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE creds (id INTEGER PRIMARY KEY, pwhash BLOB)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9102) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO creds (id, pwhash) VALUES (?, ?)", 1, []byte{0x01, 0x02}) + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + newBlob := []byte{0xCA, 0xFE, 0x00, 0xBA, 0xBE} + session2, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9103) + require.NoError(t, err) + require.NoError(t, session2.BeginTx(ctx)) + _, err = session2.ExecContext(ctx, "UPDATE creds SET pwhash = ? WHERE id = 1", newBlob) + require.NoError(t, err) + updateEntries, err := session2.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session2.Commit()) + require.Len(t, updateEntries, 1) + applyEntries(t, replica, updateEntries) + + var typeofPwhash string + var gotBlob []byte + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(pwhash), pwhash FROM creds WHERE id = 1`).Scan(&typeofPwhash, &gotBlob)) + assert.Equal(t, "blob", typeofPwhash) + assert.Equal(t, newBlob, gotBlob) +} + +// TestBlobFidelity_NullBlob verifies a NULL value in a BLOB column round trips +// as NULL, not as an empty string or empty blob. +func TestBlobFidelity_NullBlob(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE creds (id INTEGER PRIMARY KEY, pwhash BLOB)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9201) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO creds (id, pwhash) VALUES (?, NULL)", 1) + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofPwhash string + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(pwhash) FROM creds WHERE id = 1`).Scan(&typeofPwhash)) + assert.Equal(t, "null", typeofPwhash) +} + +// TestBlobFidelity_EmptyBlob verifies a zero-length BLOB round trips as an +// empty blob, not NULL and not an empty string. +func TestBlobFidelity_EmptyBlob(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE creds (id INTEGER PRIMARY KEY, pwhash BLOB)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9301) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO creds (id, pwhash) VALUES (?, ?)", 1, []byte{}) + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofPwhash string + var gotBlob []byte + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(pwhash), pwhash FROM creds WHERE id = 1`).Scan(&typeofPwhash, &gotBlob)) + assert.Equal(t, "blob", typeofPwhash) + assert.Len(t, gotBlob, 0) +} + +// TestBlobFidelity_VarcharAffinity verifies a VARCHAR-declared column (TEXT +// affinity via the CHAR substring rule, hence not BLOB affinity) round trips +// as text like a plain TEXT column, confirming isBlobAffinity's precedence. +func TestBlobFidelity_VarcharAffinity(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE users (id INTEGER PRIMARY KEY, email VARCHAR(255))` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9401) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO users (id, email) VALUES (?, ?)", 1, "a@example.com") + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofEmail, email string + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(email), email FROM users WHERE id = 1`).Scan(&typeofEmail, &email)) + assert.Equal(t, "text", typeofEmail) + assert.Equal(t, "a@example.com", email) +} + +// TestBlobFidelity_NumericAffinityHoldingText verifies that SQLite's dynamic +// typing case - a NUMERIC-affinity column (declared DECIMAL, which matches +// none of SQLite's INT/CHAR/CLOB/TEXT/BLOB/REAL/FLOA/DOUB substring rules and +// so falls to the NUMERIC catch-all) holding a value that doesn't parse as a +// number - replicates as TEXT storage class, not BLOB. NUMERIC affinity only +// converts well-formed numeric-looking text on INSERT; anything else is left +// as TEXT storage class untouched, so the preupdate hook's []byte for it must +// decode as string here, the same as for a declared TEXT column. +func TestBlobFidelity_NumericAffinityHoldingText(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE prices (id INTEGER PRIMARY KEY, amount DECIMAL)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9501) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO prices (id, amount) VALUES (?, ?)", 1, "call for price") + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofAmount, amount string + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(amount), amount FROM prices WHERE id = 1`).Scan(&typeofAmount, &amount)) + assert.Equal(t, "text", typeofAmount, "a DECIMAL (NUMERIC affinity) column holding non-numeric text must replicate as TEXT storage class") + assert.Equal(t, "call for price", amount) +} + +// TestBlobFidelity_IntegerAffinityHoldingText is the same case as +// TestBlobFidelity_NumericAffinityHoldingText for INTEGER affinity +// specifically (declared type containing "INT", SQLite's highest-precedence +// affinity rule). +func TestBlobFidelity_IntegerAffinityHoldingText(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE items (id INTEGER PRIMARY KEY, code INTEGER)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9502) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO items (id, code) VALUES (?, ?)", 1, "ABC-123") + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofCode, code string + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(code), code FROM items WHERE id = 1`).Scan(&typeofCode, &code)) + assert.Equal(t, "text", typeofCode, "an INTEGER-affinity column holding non-numeric text must replicate as TEXT storage class") + assert.Equal(t, "ABC-123", code) +} + +// TestBlobFidelity_UndeclaredTypeStaysBlob verifies a column with NO declared +// type at all (valid SQLite syntax, e.g. WordPress/ORM-generated schemas +// sometimes emit these) gets BLOB affinity per SQLite's rules and a genuine +// BLOB value in it still round-trips as BLOB storage class. +func TestBlobFidelity_UndeclaredTypeStaysBlob(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE untyped (id INTEGER PRIMARY KEY, data)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + schema, err := source.schemaCache.GetSchemaFor("untyped") + require.NoError(t, err) + dataIdx := -1 + for i, col := range schema.Columns { + if col == "data" { + dataIdx = i + } + } + require.GreaterOrEqual(t, dataIdx, 0) + require.True(t, schema.BlobAffinityCols[dataIdx], "a column with no declared type must get BLOB affinity") + + blobVal := []byte{0xDE, 0xAD, 0xBE, 0xEF} + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 9503) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, "INSERT INTO untyped (id, data) VALUES (?, ?)", 1, blobVal) + require.NoError(t, err) + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + applyEntries(t, replica, entries) + + var typeofData string + var gotData []byte + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT typeof(data), data FROM untyped WHERE id = 1`).Scan(&typeofData, &gotData)) + assert.Equal(t, "blob", typeofData) + assert.Equal(t, blobVal, gotData) +} + +// TestEncodeValuesWithSchema_CountMismatchErrors verifies encodeValuesWithSchema +// fails loud instead of silently truncating when the captured value count +// disagrees with the schema's column count (defect: stale schema vs. row). +func TestEncodeValuesWithSchema_CountMismatchErrors(t *testing.T) { + schema := &TableSchema{ + Columns: []string{"a", "b", "c"}, + BlobAffinityCols: []bool{false, false, false}, + } + _, err := encodeValuesWithSchema(schema, []interface{}{int64(1), int64(2)}) + require.Error(t, err) + assert.Contains(t, err.Error(), "out of range") +} + +// TestEncodeValuesWithSchema_BlobVsText verifies the affinity-driven encoding +// decision directly: a []byte value for a TEXT-affinity column is converted +// to string before encoding, while a []byte value for a BLOB-affinity column +// (or unknown/empty declared type) is preserved as bytes. +func TestEncodeValuesWithSchema_BlobVsText(t *testing.T) { + schema := &TableSchema{ + Columns: []string{"txt", "blob"}, + BlobAffinityCols: []bool{false, true}, + } + result, err := encodeValuesWithSchema(schema, []interface{}{ + []byte("hello"), + []byte{0x00, 0xFF}, + }) + require.NoError(t, err) + + decodedText, err := unmarshalCDCValue(result["txt"]) + require.NoError(t, err) + assert.Equal(t, "hello", decodedText, "TEXT-affinity column must decode as string") + + decodedBlob, err := unmarshalCDCValue(result["blob"]) + require.NoError(t, err) + assert.Equal(t, []byte{0x00, 0xFF}, decodedBlob, "BLOB-affinity column must decode as []byte") +} diff --git a/db/cdc_virtual_column_test.go b/db/cdc_virtual_column_test.go new file mode 100644 index 00000000..dd46f667 --- /dev/null +++ b/db/cdc_virtual_column_test.go @@ -0,0 +1,175 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestVirtualColumnCDC_CaptureRefusedNotCrashed reproduces (in a safe, fixed +// form) a confirmed go-sqlite3 v1.14.24 crash: its preupdate hook segfaults +// reading a GENERATED ALWAYS AS (...) VIRTUAL column's value, because +// sqlite3_preupdate_new/old return a NULL sqlite3_value* for a virtual +// column's index and row() dereferences it unconditionally via +// sqlite3_value_type. This was verified directly against the vendored +// go-sqlite3@v1.14.24 source (sqlite3_opt_preupdate_hook.go) and reproduced +// out of band in a throwaway program that registers a real preupdate hook on +// such a table and crashes the process with SIGSEGV in +// _Cfunc_sqlite3_value_type on INSERT - before hookCallback's guard existed. +// Embedding an actual crash in `go test` would take down the whole binary, so +// this test instead proves the FIX: hookCallback detects VIRTUAL columns via +// schema.VirtualColumns (populated from PRAGMA table_xinfo's hidden=2) and +// refuses capture with a clear conflict error before ever calling +// data.Old()/data.New() - so the crash can no longer be reached at all. +func TestVirtualColumnCDC_CaptureRefusedNotCrashed(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + const ddl = `CREATE TABLE t ( + id INTEGER PRIMARY KEY, + a TEXT, + b INTEGER GENERATED ALWAYS AS (id + 1) VIRTUAL + )` + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + schema, err := source.schemaCache.GetSchemaFor("t") + require.NoError(t, err) + require.Equal(t, []string{"b"}, schema.VirtualColumns, "schema must detect the VIRTUAL column via PRAGMA table_xinfo") + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 8001) + require.NoError(t, err) + defer session.Rollback() + require.NoError(t, session.BeginTx(ctx)) + + // This INSERT is exactly what crashed the vendored go-sqlite3 preupdate + // hook before the fix (New() reads the virtual column's index). It must + // now fail cleanly instead of segfaulting the process. + _, execErr := session.ExecContext(ctx, "INSERT INTO t (id, a) VALUES (1, 'hi')") + require.Error(t, execErr) + assert.Contains(t, execErr.Error(), "VIRTUAL") + assert.Contains(t, execErr.Error(), "b") + + // The process is still alive and the session's conflict state is set - + // proof there was no crash and the guard fired before Old()/New(). + assert.Error(t, session.GetConflictError()) +} + +// TestVirtualColumnCDC_UpdateAndDeleteAlsoRefused verifies the guard applies +// uniformly to UPDATE and DELETE, not just INSERT - both New() (UPDATE) and +// Old() (UPDATE/DELETE) can hit the same NULL sqlite3_value* for a virtual +// column's index. +func TestVirtualColumnCDC_UpdateAndDeleteAlsoRefused(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + const ddl = `CREATE TABLE t ( + id INTEGER PRIMARY KEY, + a TEXT, + b INTEGER GENERATED ALWAYS AS (id + 1) VIRTUAL + )` + // Insert the row directly (bypassing the hook) so UPDATE/DELETE have + // something to act on. + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + _, err = source.GetWriteDB().Exec("INSERT INTO t (id, a) VALUES (1, 'hi')") + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + ctx := context.Background() + + updateSession, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 8002) + require.NoError(t, err) + defer updateSession.Rollback() + require.NoError(t, updateSession.BeginTx(ctx)) + _, updateErr := updateSession.ExecContext(ctx, "UPDATE t SET a = 'bye' WHERE id = 1") + require.Error(t, updateErr) + assert.Contains(t, updateErr.Error(), "VIRTUAL") + require.NoError(t, updateSession.Rollback()) + + deleteSession, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 8003) + require.NoError(t, err) + defer deleteSession.Rollback() + require.NoError(t, deleteSession.BeginTx(ctx)) + _, deleteErr := deleteSession.ExecContext(ctx, "DELETE FROM t WHERE id = 1") + require.Error(t, deleteErr) + assert.Contains(t, deleteErr.Error(), "VIRTUAL") +} + +// TestVirtualColumnCDC_StoredGeneratedColumnIsFine verifies that a STORED +// generated column (the audit's other claim: unaffected) does not crash +// capture like VIRTUAL does. Unlike VIRTUAL, SQLite computes and stores a +// real value for it, so the preupdate hook can safely read it - but that +// value is intentionally NOT captured or replicated: SQLite rejects an +// explicit INSERT/UPDATE of a generated column ("cannot INSERT into +// generated column", verified directly), and the value is deterministically +// recomputed from the table's other captured columns by SQLite itself on +// every replica once the DDL (which carries the GENERATED ALWAYS AS +// expression) has replicated. The other real columns must still capture and +// apply normally, at their correct positions. +func TestVirtualColumnCDC_StoredGeneratedColumnIsFine(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE t ( + id INTEGER PRIMARY KEY, + a TEXT, + b INTEGER GENERATED ALWAYS AS (id + 1) STORED, + e TEXT + )` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + schema, err := source.schemaCache.GetSchemaFor("t") + require.NoError(t, err) + assert.Empty(t, schema.VirtualColumns, "STORED generated columns must not be treated as VIRTUAL") + assert.NotContains(t, schema.Columns, "b", "generated columns must be excluded from CDC capture") + assert.Contains(t, schema.Columns, "e", "a real column declared after the generated column must still be tracked") + + entries := captureEntries(t, source, 8004, "INSERT INTO t (id, a, e) VALUES (1, 'hi', 'world')") + require.Len(t, entries, 1) + assert.NotContains(t, entries[0].NewValues, "b", "generated column value must not be captured") + require.Contains(t, entries[0].NewValues, "e", "column declared after the generated column must still capture at its correct position") + + applyEntries(t, replica, entries) + + var a, e string + var b int64 + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT a, b, e FROM t WHERE id = 1`).Scan(&a, &b, &e)) + assert.Equal(t, "hi", a) + assert.Equal(t, int64(2), b, "SQLite must recompute the STORED generated column locally on the replica") + assert.Equal(t, "world", e) +} + +// TestSchemaLoad_FTS5HiddenPseudocolumnsExcluded is a regression guard for a +// finding made while fixing the VIRTUAL-column segfault: PRAGMA table_xinfo +// (needed to get generated columns' true positions correctly - see +// loadSchema) also exposes hidden == 1 rows for virtual tables' own +// pseudocolumns, which table_info silently excluded. Verified directly: an +// FTS5 table declared with 2 real columns reports 4 rows from table_xinfo +// (the 2 real columns plus a "docs"-named and a "rank" hidden == 1 +// pseudocolumn). These must stay excluded from Columns/FullColumns exactly +// as table_info excluded them, or CDC/publisher schema would gain two fake +// columns for every FTS5 table. Also verified directly that the preupdate +// hook never actually fires for an FTS5 virtual table's own name (it fires, +// if at all, for its real b-tree shadow tables), so this is a schema-loading +// correctness issue rather than a capture-path one. +func TestSchemaLoad_FTS5HiddenPseudocolumnsExcluded(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + _, err := source.GetWriteDB().Exec(`CREATE VIRTUAL TABLE docs USING fts5(title, body)`) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + schema, err := source.schemaCache.GetSchemaFor("docs") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"title", "body"}, schema.Columns, + "FTS5's hidden table-name/rank pseudocolumns must not appear as real columns") + assert.Empty(t, schema.VirtualColumns, "FTS5 hidden columns are not GENERATED ALWAYS VIRTUAL columns") +} diff --git a/db/db_integration.go b/db/db_integration.go index 77a1dc40..ee966f01 100644 --- a/db/db_integration.go +++ b/db/db_integration.go @@ -670,9 +670,11 @@ func (mdb *ReplicatedDatabase) ExecuteLocalWithHooks(ctx context.Context, txnID return nil, fmt.Errorf("failed to begin transaction: %w", err) } - // Execute each statement - hooks capture raw CDC data to Pebble + // Execute each statement - hooks capture raw CDC data to Pebble. + // rows-affected is not used here: this autocommit path computes it from + // the captured CDC entries via CompletedLocalExecution.GetTotalRowCount. for _, req := range requests { - if err := session.ExecContext(ctx, req.SQL, req.Params...); err != nil { + if _, err := session.ExecContext(ctx, req.SQL, req.Params...); err != nil { _ = session.Rollback() return nil, fmt.Errorf("failed to execute statement: %w", err) } @@ -706,6 +708,87 @@ func (mdb *ReplicatedDatabase) ExecuteLocalWithHooks(ctx context.Context, txnID }, nil } +// pinnedHookSession implements coordinator.PinnedSession over an +// EphemeralHookSession whose SQLite transaction is held open across multiple +// statements (BEGIN...COMMIT/ROLLBACK), instead of the one-shot +// execute-then-rollback flow ExecuteLocalWithHooks uses for autocommit +// statements. +type pinnedHookSession struct { + session *EphemeralHookSession +} + +var _ coordinator.PinnedSession = (*pinnedHookSession)(nil) + +// ExecuteStatement runs one DML statement on the pinned transaction, then +// captures and row-locks any newly captured CDC rows immediately so a +// conflicting transaction sees the lock from statement time, not just from +// Release. +func (p *pinnedHookSession) ExecuteStatement(ctx context.Context, sql string, params []interface{}) (int64, int64, error) { + rowsAffected, err := p.session.ExecContext(ctx, sql, params...) + if err != nil { + return 0, 0, err + } + if err := p.session.captureAndLockNewRows(); err != nil { + return 0, 0, err + } + return rowsAffected, p.session.GetLastInsertId(), nil +} + +func (p *pinnedHookSession) Query(ctx context.Context, sqlText string, params []interface{}) ([]string, []map[string]interface{}, error) { + rows, err := p.session.QueryContext(ctx, sqlText, params...) + if err != nil { + return nil, nil, err + } + defer rows.Close() + return scanRowsToMaps(rows) +} + +func (p *pinnedHookSession) CDCEntries() []common.CDCEntry { + entries := p.session.CapturedIntentEntries() + if len(entries) == 0 { + return nil + } + result := make([]common.CDCEntry, len(entries)) + for i, e := range entries { + result[i] = common.CDCEntry{ + Table: e.Table, + IntentKey: e.IntentKey, + Operation: e.Operation, + OldValues: e.OldValues, + NewValues: e.NewValues, + EncodedRow: e.EncodedRow, + EncodedCodec: e.EncodedCodec, + } + } + return result +} + +func (p *pinnedHookSession) Release() error { + return p.session.Rollback() +} + +// BeginPinnedSession starts a new PinnedSession: an EphemeralHookSession on +// hookDB (never writeDB - same deadlock-avoidance reasoning as +// ExecuteLocalWithHooks) with its SQLite transaction opened and left open. +// ctx must stay alive (not be cancelled) until the caller calls Release - see +// PinnedSession's doc comment in coordinator/pinned_txn.go. +// +// Unlike ExecuteLocalWithHooks, this does not fall back to statement-based +// execution when the preupdate hook build tag is absent: eager execution +// requires the hook build tag, consistent with how coordinator/pinned_txn.go +// and its tests are gated. +func (mdb *ReplicatedDatabase) BeginPinnedSession(ctx context.Context, txnID uint64) (coordinator.PinnedSession, error) { + session, err := StartEphemeralSession(ctx, mdb.hookDB, mdb.metaStore, mdb.schemaCache, txnID) + if err != nil { + return nil, fmt.Errorf("failed to start pinned session: %w", err) + } + if err := session.BeginTx(ctx); err != nil { + session.cleanup() + return nil, fmt.Errorf("failed to begin pinned transaction: %w", err) + } + return &pinnedHookSession{session: session}, nil +} + // ExecuteTransaction executes a transaction with distributed transaction semantics // This is the main entry point for application-level transactions func (mdb *ReplicatedDatabase) ExecuteTransaction(ctx context.Context, statements []protocol.Statement) error { @@ -785,6 +868,13 @@ func (mdb *ReplicatedDatabase) ExecuteSnapshotRead(ctx context.Context, query st } defer rows.Close() + return scanRowsToMaps(rows) +} + +// scanRowsToMaps drains rows into a (columns, row-maps) pair. []byte values +// are converted to string, matching SQLite's TEXT/BLOB ambiguity handling +// used elsewhere for query results. The caller owns closing rows. +func scanRowsToMaps(rows *sql.Rows) ([]string, []map[string]interface{}, error) { columns, err := rows.Columns() if err != nil { return nil, nil, err diff --git a/db/fk_cascade_cdc_test.go b/db/fk_cascade_cdc_test.go new file mode 100644 index 00000000..1e936321 --- /dev/null +++ b/db/fk_cascade_cdc_test.go @@ -0,0 +1,174 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestFKCascadeCDC_ParentDeleteReplicatesAndConverges is an integration-style +// regression test for the audit's code-read finding that ON DELETE CASCADE +// child deletes are captured as separate, seq-ordered CDC entries and that +// replaying them on a replica converges correctly (the replica's own FK +// cascade, if enabled, would re-delete an already-absent child harmlessly via +// ApplyCDCDelete's now-tolerant zero-rows-matched handling - see +// TestApplyCDCDelete_NoRowsMatched). +// +// SQLite foreign key enforcement is off by default per connection; it is +// turned on here explicitly on the single-connection hook pool (mirroring +// how a deployment that wants cascade behavior would configure it) rather +// than changed globally, since Marmot's default DSN does not enable it. +func TestFKCascadeCDC_ParentDeleteReplicatesAndConverges(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = ` + CREATE TABLE parent (id INTEGER PRIMARY KEY, name TEXT); + CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id INTEGER NOT NULL, label TEXT, + FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE CASCADE); + ` + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + _, err = replica.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, replica.ReloadSchema()) + + // Enable FK enforcement on the hook connection (single-conn pool, so this + // pragma sticks for every statement the capture session executes). + _, err = source.hookDB.Exec(`PRAGMA foreign_keys = ON`) + require.NoError(t, err) + + // Seed one parent with two children on the source, captured and applied + // to the replica first so both sides start in the same state. + seed := captureEntries(t, source, 7001, + `INSERT INTO parent (id, name) VALUES (1, 'acme')`, + `INSERT INTO child (id, parent_id, label) VALUES (10, 1, 'a')`, + `INSERT INTO child (id, parent_id, label) VALUES (11, 1, 'b')`, + ) + require.Len(t, seed, 3) + applyEntries(t, replica, seed) + + var childCount int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM child`).Scan(&childCount)) + require.Equal(t, 2, childCount, "seed data must land on the replica before the cascade test") + + // Deleting the parent must cascade-delete both children on the source, + // and the preupdate hook must capture ALL of it (parent + both children) + // as separate CDC entries in one transaction. + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 7002) + require.NoError(t, err) + defer session.Rollback() + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, `DELETE FROM parent WHERE id = 1`) + require.NoError(t, err) + cascadeEntries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + + require.Len(t, cascadeEntries, 3, "the parent delete plus both FK-cascaded child deletes must all be captured") + + deletedTables := map[string]int{} + for _, e := range cascadeEntries { + require.Equal(t, uint8(OpTypeDelete), e.Operation, "every captured row in this transaction must be a DELETE") + deletedTables[e.Table]++ + } + require.Equal(t, 1, deletedTables["parent"]) + require.Equal(t, 2, deletedTables["child"]) + + // Apply the cascade entries to the replica in the same seq order they + // were captured in - this is what real replication does. + applyEntries(t, replica, cascadeEntries) + + var parentCount, childCountAfter int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM parent`).Scan(&parentCount)) + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM child`).Scan(&childCountAfter)) + require.Equal(t, 0, parentCount, "replica must converge: parent gone") + require.Equal(t, 0, childCountAfter, "replica must converge: both children gone") +} + +// TestFKCascadeCDC_ReplicaOwnCascadeThenRedundantDeleteNoOps covers the other +// half of the audit's finding: when the REPLICA also enforces FK cascade, its +// own local cascade removes the children as soon as the parent-delete CDC +// entry is applied - so by the time the source's explicit per-child DELETE +// CDC entries are applied afterward, those rows are already gone. Applying +// them must be a harmless no-op (ApplyCDCDelete tolerates zero rows matched), +// not a replication failure. +func TestFKCascadeCDC_ReplicaOwnCascadeThenRedundantDeleteNoOps(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = ` + CREATE TABLE parent (id INTEGER PRIMARY KEY, name TEXT); + CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id INTEGER NOT NULL, label TEXT, + FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE CASCADE); + ` + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + _, err = replica.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, replica.ReloadSchema()) + + _, err = source.hookDB.Exec(`PRAGMA foreign_keys = ON`) + require.NoError(t, err) + // The replica enforces FK cascade too, on both connections it applies + // CDC through and reads back from. + _, err = replica.GetWriteDB().Exec(`PRAGMA foreign_keys = ON`) + require.NoError(t, err) + + seed := captureEntries(t, source, 7101, + `INSERT INTO parent (id, name) VALUES (1, 'acme')`, + `INSERT INTO child (id, parent_id, label) VALUES (10, 1, 'a')`, + ) + require.Len(t, seed, 2) + applyEntries(t, replica, seed) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 7102) + require.NoError(t, err) + defer session.Rollback() + require.NoError(t, session.BeginTx(ctx)) + _, err = session.ExecContext(ctx, `DELETE FROM parent WHERE id = 1`) + require.NoError(t, err) + cascadeEntries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + require.Len(t, cascadeEntries, 2, "parent delete plus the cascaded child delete") + + // Apply the parent delete first: the replica's own FK cascade removes + // the child as a side effect, before the child's own CDC delete entry + // (captured on the source) is ever applied. + var parentEntry, childEntry *IntentEntry + for _, e := range cascadeEntries { + if e.Table == "parent" { + parentEntry = e + } else { + childEntry = e + } + } + require.NotNil(t, parentEntry) + require.NotNil(t, childEntry) + + require.NoError(t, ApplyCDCEntry(replica.GetWriteDB(), &schemaCacheAdapter{cache: replica.schemaCache}, parentEntry)) + + var childCountAfterParentDelete int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM child`).Scan(&childCountAfterParentDelete)) + require.Equal(t, 0, childCountAfterParentDelete, "replica's own FK cascade must have already removed the child") + + // Now apply the source's redundant, explicit child-delete CDC entry: + // must be a harmless no-op, not an error. + require.NoError(t, ApplyCDCEntry(replica.GetWriteDB(), &schemaCacheAdapter{cache: replica.schemaCache}, childEntry)) + + var finalParentCount, finalChildCount int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM parent`).Scan(&finalParentCount)) + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM child`).Scan(&finalChildCount)) + require.Equal(t, 0, finalParentCount) + require.Equal(t, 0, finalChildCount) +} diff --git a/db/hook_capture_test.go b/db/hook_capture_test.go index 075f1db0..46cc13e9 100644 --- a/db/hook_capture_test.go +++ b/db/hook_capture_test.go @@ -57,7 +57,7 @@ func TestHookCapture_Insert(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Execute INSERT - err = session.ExecContext(ctx, "INSERT INTO test (id, name, value) VALUES (1, 'alice', 100)") + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name, value) VALUES (1, 'alice', 100)") require.NoError(t, err) // Verify capture WITHOUT calling ProcessCapturedRows @@ -120,7 +120,7 @@ func TestHookCapture_Update(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Execute UPDATE - err = session.ExecContext(ctx, "UPDATE test SET name = 'bob', value = 200 WHERE id = 1") + _, err = session.ExecContext(ctx, "UPDATE test SET name = 'bob', value = 200 WHERE id = 1") require.NoError(t, err) // Verify capture @@ -193,7 +193,7 @@ func TestHookCapture_Delete(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Execute DELETE - err = session.ExecContext(ctx, "DELETE FROM test WHERE id = 1") + _, err = session.ExecContext(ctx, "DELETE FROM test WHERE id = 1") require.NoError(t, err) // Verify capture @@ -254,11 +254,11 @@ func TestHookCapture_SkipsInternalTables(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Insert into internal table (should be skipped) - err = session.ExecContext(ctx, "INSERT INTO __marmot_internal (id, data) VALUES (1, 'internal')") + _, err = session.ExecContext(ctx, "INSERT INTO __marmot_internal (id, data) VALUES (1, 'internal')") require.NoError(t, err) // Insert into user table (should be captured) - err = session.ExecContext(ctx, "INSERT INTO user_table (id, name) VALUES (1, 'alice')") + _, err = session.ExecContext(ctx, "INSERT INTO user_table (id, name) VALUES (1, 'alice')") require.NoError(t, err) // Verify only user table was captured @@ -384,9 +384,12 @@ func TestHookCapture_MultipleOperations(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Multiple operations - require.NoError(t, session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (3, 'charlie')")) - require.NoError(t, session.ExecContext(ctx, "UPDATE test SET name = 'ALICE' WHERE id = 1")) - require.NoError(t, session.ExecContext(ctx, "DELETE FROM test WHERE id = 2")) + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (3, 'charlie')") + require.NoError(t, err) + _, err = session.ExecContext(ctx, "UPDATE test SET name = 'ALICE' WHERE id = 1") + require.NoError(t, err) + _, err = session.ExecContext(ctx, "DELETE FROM test WHERE id = 2") + require.NoError(t, err) // Verify captures captured := loadCapturedIntents(t, session) @@ -452,7 +455,7 @@ func TestHookCapture_ValuesRetrievable(t *testing.T) { } for _, td := range testData { - err = session.ExecContext(ctx, "INSERT INTO test (id, name, score) VALUES (?, ?, ?)", td.id, td.name, td.score) + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name, score) VALUES (?, ?, ?)", td.id, td.name, td.score) require.NoError(t, err) } @@ -511,7 +514,7 @@ func TestHookCapture_NoSchemaCache(t *testing.T) { require.NoError(t, session.BeginTx(ctx)) // Insert (schema was reloaded during StartEphemeralSession, so it should capture) - err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')") + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')") require.NoError(t, err) // Verify data was captured (schema reloaded during session start) @@ -522,3 +525,107 @@ func TestHookCapture_NoSchemaCache(t *testing.T) { assert.Equal(t, "test", captured[0].Table) assert.Equal(t, uint8(OpTypeInsert), captured[0].Operation) } + +// TestHookCapture_CaptureAndLockNewRows_AccumulatesAcrossCalls is a +// regression guard for the seq high-water-mark logic in +// captureAndLockNewRows: calling it after two separate ExecContext calls +// must accumulate entries from both statements, not just the most recent one. +func TestHookCapture_CaptureAndLockNewRows_AccumulatesAcrossCalls(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + metaPath := filepath.Join(tmpDir, "meta") + + os.MkdirAll(metaPath, 0755) + metaStore, err := NewPebbleMetaStore(metaPath, DefaultPebbleOptions()) + require.NoError(t, err) + defer metaStore.Close() + + clock := hlc.NewClock(1) + replicatedDB, err := NewReplicatedDatabase(dbPath, 1, clock, metaStore) + require.NoError(t, err) + defer replicatedDB.Close() + + _, err = replicatedDB.GetWriteDB().Exec(`CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`) + require.NoError(t, err) + require.NoError(t, replicatedDB.ReloadSchema()) + + ctx := context.Background() + txnID := uint64(1101) + session, err := StartEphemeralSession(ctx, replicatedDB.hookDB, metaStore, replicatedDB.schemaCache, txnID) + require.NoError(t, err) + defer session.Rollback() + require.NoError(t, session.BeginTx(ctx)) + + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')") + require.NoError(t, err) + require.NoError(t, session.captureAndLockNewRows()) + + entriesAfterFirst := session.CapturedIntentEntries() + require.Len(t, entriesAfterFirst, 1, "first captureAndLockNewRows call must capture the first statement's row") + + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (2, 'bob')") + require.NoError(t, err) + require.NoError(t, session.captureAndLockNewRows()) + + entriesAfterSecond := session.CapturedIntentEntries() + require.Len(t, entriesAfterSecond, 2, "second captureAndLockNewRows call must accumulate, not replace, the first statement's entry") + + var name0, name1 string + require.NoError(t, encoding.Unmarshal(entriesAfterSecond[0].NewValues["name"], &name0)) + require.NoError(t, encoding.Unmarshal(entriesAfterSecond[1].NewValues["name"], &name1)) + assert.Equal(t, "alice", name0) + assert.Equal(t, "bob", name1) +} + +// TestHookCapture_EagerMode_GuardsAgainstDoubleProcessing verifies that once +// captureAndLockNewRows has run (eagerCaptureUsed == true), GetIntentEntries +// and ProcessCapturedRows do not re-run collectCapturedRows - which would +// re-acquire already-held locks and double-append entries. +func TestHookCapture_EagerMode_GuardsAgainstDoubleProcessing(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + metaPath := filepath.Join(tmpDir, "meta") + + os.MkdirAll(metaPath, 0755) + metaStore, err := NewPebbleMetaStore(metaPath, DefaultPebbleOptions()) + require.NoError(t, err) + defer metaStore.Close() + + clock := hlc.NewClock(1) + replicatedDB, err := NewReplicatedDatabase(dbPath, 1, clock, metaStore) + require.NoError(t, err) + defer replicatedDB.Close() + + _, err = replicatedDB.GetWriteDB().Exec(`CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`) + require.NoError(t, err) + require.NoError(t, replicatedDB.ReloadSchema()) + + ctx := context.Background() + txnID := uint64(1102) + session, err := StartEphemeralSession(ctx, replicatedDB.hookDB, metaStore, replicatedDB.schemaCache, txnID) + require.NoError(t, err) + require.NoError(t, session.BeginTx(ctx)) + + _, err = session.ExecContext(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')") + require.NoError(t, err) + require.NoError(t, session.captureAndLockNewRows()) + require.True(t, session.eagerCaptureUsed) + + // GetIntentEntries must return the already-accumulated entries without + // re-collecting or erroring (which would happen if it tried to + // re-acquire the lock this txn already holds via a different code path). + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.Len(t, entries, 1) + + // Calling again must not change the count. + entriesAgain, err := session.GetIntentEntries() + require.NoError(t, err) + assert.Len(t, entriesAgain, 1) + + // ProcessCapturedRows must be a no-op under eager mode, not an error. + require.NoError(t, session.ProcessCapturedRows()) + assert.Len(t, session.CapturedIntentEntries(), 1) + + require.NoError(t, session.Rollback()) +} diff --git a/db/pinned_session_test.go b/db/pinned_session_test.go new file mode 100644 index 00000000..5af41ca7 --- /dev/null +++ b/db/pinned_session_test.go @@ -0,0 +1,170 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "context" + "testing" + + "github.com/maxpert/marmot/encoding" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPinnedSession_SequentialInsertsReturnRealResults verifies two sequential +// ExecuteStatement INSERTs on the same pinned session each report real, +// distinct lastInsertId and rowsAffected == 1 - not the fake buffered- +// statement response the old defer-to-COMMIT flow returned. +func TestPinnedSession_SequentialInsertsReturnRealResults(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20001) + require.NoError(t, err) + defer session.Release() + + rowsAffected1, lastInsertID1, err := session.ExecuteStatement(ctx, "INSERT INTO test (name) VALUES (?)", []interface{}{"alice"}) + require.NoError(t, err) + assert.Equal(t, int64(1), rowsAffected1) + assert.NotZero(t, lastInsertID1) + + rowsAffected2, lastInsertID2, err := session.ExecuteStatement(ctx, "INSERT INTO test (name) VALUES (?)", []interface{}{"bob"}) + require.NoError(t, err) + assert.Equal(t, int64(1), rowsAffected2) + assert.NotZero(t, lastInsertID2) + + assert.NotEqual(t, lastInsertID1, lastInsertID2) +} + +// TestPinnedSession_QuerySeesOwnUncommittedWrites verifies a Query on the +// pinned session observes a row written earlier in the same session, while a +// query from a separate *sql.DB connection to the same file does not - the +// core "eager execution" isolation guarantee. +func TestPinnedSession_QuerySeesOwnUncommittedWrites(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20002) + require.NoError(t, err) + defer session.Release() + + _, _, err = session.ExecuteStatement(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')", nil) + require.NoError(t, err) + + // Own session sees the uncommitted row. + cols, rows, err := session.Query(ctx, "SELECT id, name FROM test WHERE id = 1", nil) + require.NoError(t, err) + assert.Contains(t, cols, "name") + require.Len(t, rows, 1) + assert.Equal(t, "alice", rows[0]["name"]) + + // A separate connection (the read pool) to the same file does not see it, + // since the pinned session's SQLite transaction is still open and + // uncommitted (writer isolation). + var count int + require.NoError(t, source.GetReadDB().QueryRow("SELECT COUNT(*) FROM test WHERE id = 1").Scan(&count)) + assert.Equal(t, 0, count, "uncommitted write on the pinned session must not be visible to another connection") +} + +// TestPinnedSession_CDCEntriesAccumulateInOrder verifies CDCEntries() after +// two statements returns entries for both, in order, with correct +// Table/Operation/NewValues. +func TestPinnedSession_CDCEntriesAccumulateInOrder(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20003) + require.NoError(t, err) + defer session.Release() + + _, _, err = session.ExecuteStatement(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')", nil) + require.NoError(t, err) + _, _, err = session.ExecuteStatement(ctx, "INSERT INTO test (id, name) VALUES (2, 'bob')", nil) + require.NoError(t, err) + + entries := session.CDCEntries() + require.Len(t, entries, 2) + + assert.Equal(t, "test", entries[0].Table) + assert.Equal(t, uint8(OpTypeInsert), entries[0].Operation) + require.NotNil(t, entries[0].NewValues) + + assert.Equal(t, "test", entries[1].Table) + assert.Equal(t, uint8(OpTypeInsert), entries[1].Operation) + require.NotNil(t, entries[1].NewValues) + + var name0, name1 string + require.NoError(t, encoding.Unmarshal(entries[0].NewValues["name"], &name0)) + require.NoError(t, encoding.Unmarshal(entries[1].NewValues["name"], &name1)) + assert.Equal(t, "alice", name0) + assert.Equal(t, "bob", name1) +} + +// TestPinnedSession_ReleaseAlwaysRollsBack verifies Release never commits, +// even when CDCEntries() was read first - the "never double-apply" invariant. +func TestPinnedSession_ReleaseAlwaysRollsBack(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20004) + require.NoError(t, err) + + _, _, err = session.ExecuteStatement(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')", nil) + require.NoError(t, err) + + entries := session.CDCEntries() + require.Len(t, entries, 1) + + require.NoError(t, session.Release()) + + var count int + require.NoError(t, source.GetWriteDB().QueryRow("SELECT COUNT(*) FROM test WHERE id = 1").Scan(&count)) + assert.Equal(t, 0, count, "Release must always roll back, never commit, regardless of CDCEntries having been read") +} + +// TestPinnedSession_ZeroRowUpdateReturnsNoRowsNoEntries verifies a zero-row +// UPDATE returns rowsAffected == 0, err == nil, and contributes no +// CDCEntries(). +func TestPinnedSession_ZeroRowUpdateReturnsNoRowsNoEntries(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20005) + require.NoError(t, err) + defer session.Release() + + rowsAffected, _, err := session.ExecuteStatement(ctx, "UPDATE test SET name = 'nobody' WHERE id = 999", nil) + require.NoError(t, err) + assert.Equal(t, int64(0), rowsAffected) + assert.Empty(t, session.CDCEntries()) +} + +// TestPinnedSession_RowLockHeldFromStatementTime proves the pinned session's +// row lock is acquired immediately at ExecuteStatement time, not deferred to +// Release: after one INSERT on a not-yet-released pinned session, a +// concurrent AcquireCDCRowLock for the same table+intent key from a +// different txnID must fail. +func TestPinnedSession_RowLockHeldFromStatementTime(t *testing.T) { + source := newRowidTestDatabase(t, 1) + require.NoError(t, execAndReload(source, `CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)`)) + + ctx := context.Background() + session, err := source.BeginPinnedSession(ctx, 20006) + require.NoError(t, err) + defer session.Release() + + _, _, err = session.ExecuteStatement(ctx, "INSERT INTO test (id, name) VALUES (1, 'alice')", nil) + require.NoError(t, err) + + entries := session.CDCEntries() + require.Len(t, entries, 1) + + conflictErr := source.metaStore.AcquireCDCRowLock(99999, entries[0].Table, string(entries[0].IntentKey)) + assert.Error(t, conflictErr, "row lock for a row written by an unreleased pinned session must already be held") +} diff --git a/db/preupdate_hook.go b/db/preupdate_hook.go index 86643d7f..419dfce0 100644 --- a/db/preupdate_hook.go +++ b/db/preupdate_hook.go @@ -46,6 +46,9 @@ type EphemeralHookSession struct { intentEntries []*IntentEntry intentEntriesErr error + + lastProcessedSeq uint64 // high-water mark for captureAndLockNewRows + eagerCaptureUsed bool // true once captureAndLockNewRows has run at least once } type capturedRow struct { @@ -139,26 +142,114 @@ func (s *EphemeralHookSession) BeginTx(ctx context.Context) error { return nil } -// ExecContext executes a statement within the session's transaction -func (s *EphemeralHookSession) ExecContext(ctx context.Context, query string, args ...interface{}) error { +// ExecContext executes a statement within the session's transaction, and +// returns the real rows-affected count SQLite reports for it. Callers that +// need locks/CDC entries acquired incrementally (eager pinned-session +// execution) must follow this with captureAndLockNewRows; callers that defer +// all processing to Rollback (the autocommit ExecuteLocalWithHooks flow) do +// not need to. +func (s *EphemeralHookSession) ExecContext(ctx context.Context, query string, args ...interface{}) (int64, error) { if s.tx == nil { - return fmt.Errorf("no active transaction") + return 0, fmt.Errorf("no active transaction") } result, err := s.tx.ExecContext(ctx, query, args...) if err != nil { - return err + return 0, err } if conflictErr := s.GetConflictError(); conflictErr != nil { - return conflictErr + return 0, conflictErr } if id, err := result.LastInsertId(); err == nil && id != 0 { s.lastInsertId = id } + + rowsAffected, err := result.RowsAffected() + if err != nil { + return 0, err + } + return rowsAffected, nil +} + +// QueryContext runs a read on the session's still-open transaction, so it +// observes uncommitted writes made earlier in the same transaction. +func (s *EphemeralHookSession) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) { + if s.tx == nil { + return nil, fmt.Errorf("no active transaction") + } + return s.tx.QueryContext(ctx, query, args...) +} + +// captureAndLockNewRows processes any rows captured (via hookCallback) since +// the last call, converting them to IntentEntry and acquiring their CDC row +// locks immediately - unlike the existing one-shot ProcessCapturedRows/ +// GetIntentEntries path (used by the autocommit ExecuteLocalWithHooks flow), +// which defers lock acquisition until the whole hookDB transaction rolls +// back. A pinned session calls this after every statement so a conflicting +// transaction sees the lock from statement time, not just from COMMIT. +func (s *EphemeralHookSession) captureAndLockNewRows() error { + rows, err := s.captureSnapshot() + if err != nil { + return err + } + + s.mu.Lock() + lastSeq := s.lastProcessedSeq + s.mu.Unlock() + + newEntries := make([]*IntentEntry, 0) + maxSeq := lastSeq + for _, rowRef := range rows { + if rowRef.seq <= lastSeq { + continue + } + row, err := DecodeRow(rowRef.data) + if err != nil { + return fmt.Errorf("failed to decode captured row: %w", err) + } + + if ddlTxn, err := s.metaStore.GetCDCTableDDLLock(row.Table); err == nil && ddlTxn != 0 && ddlTxn != s.txnID { + return ErrCDCTableDDLInProgress{Table: row.Table, HeldByTxn: ddlTxn} + } + if err := s.metaStore.AcquireCDCRowLock(s.txnID, row.Table, string(row.IntentKey)); err != nil { + return err + } + + newEntries = append(newEntries, &IntentEntry{ + TxnID: s.txnID, + Seq: rowRef.seq, + Operation: row.Op, + Table: row.Table, + IntentKey: row.IntentKey, + OldValues: row.OldValues, + NewValues: row.NewValues, + EncodedRow: rowRef.data, + EncodedCodec: encodedCapturedRowCodecMsgpack, + }) + if rowRef.seq > maxSeq { + maxSeq = rowRef.seq + } + } + + s.mu.Lock() + s.intentEntries = append(s.intentEntries, newEntries...) + s.lastProcessedSeq = maxSeq + s.eagerCaptureUsed = true + s.mu.Unlock() return nil } +// CapturedIntentEntries returns the entries accumulated so far via +// captureAndLockNewRows, without triggering any collection. Used by the +// pinned-session wrapper to build CDC entries for 2PC at COMMIT time, before +// the session is rolled back. +func (s *EphemeralHookSession) CapturedIntentEntries() []*IntentEntry { + s.mu.Lock() + defer s.mu.Unlock() + return s.intentEntries +} + // Commit commits the transaction and closes the connection. // Note: CDC intent entries are NOT deleted here. They persist in MetaStore until // the distributed transaction completes (commit or abort). Cleanup happens in @@ -215,6 +306,13 @@ func (s *EphemeralHookSession) Rollback() error { // Encoding happens in hookCallback, so this does lock acquisition and conflict detection. func (s *EphemeralHookSession) ProcessCapturedRows() error { s.mu.Lock() + if s.eagerCaptureUsed { + // Entries and locks are already correct via captureAndLockNewRows; + // re-running collectCapturedRows would re-acquire locks already held + // and double-append entries. + s.mu.Unlock() + return nil + } if s.intentEntries != nil || s.intentEntriesErr != nil { err := s.intentEntriesErr s.mu.Unlock() @@ -295,6 +393,15 @@ func (s *EphemeralHookSession) GetIntentEntries() ([]*IntentEntry, error) { } s.mu.Lock() + if s.eagerCaptureUsed { + // Entries already accumulated via captureAndLockNewRows - "eager mode + // ran and captured zero rows" is a valid final state, not "not yet + // collected", so skip the collectCapturedRows(false) fallback branch + // entirely even when s.intentEntries is nil. + entries := s.intentEntries + s.mu.Unlock() + return entries, nil + } if s.intentEntries != nil || s.intentEntriesErr != nil { cached := s.intentEntries err := s.intentEntriesErr @@ -414,6 +521,36 @@ func (s *EphemeralHookSession) hookCallback(data sqlite3.SQLitePreUpdateData) { return } + // go-sqlite3's preupdate hook segfaults reading a VIRTUAL generated column's + // value: sqlite3_preupdate_new/old returns a NULL sqlite3_value* for it, and + // row() dereferences that pointer unconditionally (verified directly against + // go-sqlite3 v1.14.24; there is no way to skip just that column's index with + // the API as vendored). Refuse capture rather than crash the process. + // STORED generated columns are unaffected and are not in schema.VirtualColumns. + if len(schema.VirtualColumns) > 0 { + s.setConflictError(fmt.Errorf( + "cannot capture CDC for table %s: table has GENERATED ALWAYS AS (...) VIRTUAL "+ + "column(s) %s, which go-sqlite3's preupdate hook cannot safely read - "+ + "use STORED instead of VIRTUAL", + data.TableName, strings.Join(schema.VirtualColumns, ", "))) + return + } + + // Tables with no explicit PRIMARY KEY replicate their identity via SQLite's + // rowid (schema.PKIndices == [-1]). A user-declared column that shadows one + // of SQLite's rowid aliases would collide with the synthetic "rowid" CDC key + // used below, so refuse to capture rather than silently corrupting identity. + if isRowidSentinelSchema(schema) { + if shadow := findShadowedRowidColumn(schema.Columns); shadow != "" { + s.setConflictError(fmt.Errorf( + "cannot capture CDC for table %s: column %q shadows SQLite's rowid alias; "+ + "tables without an explicit PRIMARY KEY must not declare a column named "+ + "rowid, oid, or _rowid_ - add an explicit PRIMARY KEY instead", + data.TableName, shadow)) + return + } + } + // Determine operation type var opType uint8 switch data.Op { @@ -436,7 +573,16 @@ func (s *EphemeralHookSession) hookCallback(data sqlite3.SQLitePreUpdateData) { if data.Op == sqlite3.SQLITE_DELETE || data.Op == sqlite3.SQLITE_UPDATE { rawOld := make([]interface{}, colCount) if data.Old(rawOld...) == nil { - oldVals = encodeValuesWithSchema(schema.Columns, rawOld) + oldVals, err = encodeValuesWithSchema(schema, rawOld) + if err != nil { + s.setConflictError(fmt.Errorf("cannot capture CDC for table %s: %w", data.TableName, err)) + return + } + if isRowidSentinelSchema(schema) { + if encoded := encodeValue(data.OldRowID); encoded != nil { + oldVals[rowidColumnKey] = encoded + } + } if data.Op == sqlite3.SQLITE_DELETE { pkValues := extractPKFromValues(schema, rawOld, data.OldRowID) intentKey = filter.EncodeIntentKeyWithPrefix(schema.IntentKeyPrefix, pkValues) @@ -447,7 +593,16 @@ func (s *EphemeralHookSession) hookCallback(data sqlite3.SQLitePreUpdateData) { if data.Op == sqlite3.SQLITE_INSERT || data.Op == sqlite3.SQLITE_UPDATE { rawNew := make([]interface{}, colCount) if data.New(rawNew...) == nil { - newVals = encodeValuesWithSchema(schema.Columns, rawNew) + newVals, err = encodeValuesWithSchema(schema, rawNew) + if err != nil { + s.setConflictError(fmt.Errorf("cannot capture CDC for table %s: %w", data.TableName, err)) + return + } + if isRowidSentinelSchema(schema) { + if encoded := encodeValue(data.NewRowID); encoded != nil { + newVals[rowidColumnKey] = encoded + } + } pkValues := extractPKFromValues(schema, rawNew, data.NewRowID) intentKey = filter.EncodeIntentKeyWithPrefix(schema.IntentKeyPrefix, pkValues) } @@ -593,6 +748,36 @@ func (s *EphemeralHookSession) setConflictError(err error) { // Utility functions // ============================================================================= +// rowidColumnKey is the CDC map key used to carry a rowid-sentinel table's +// identity (SQLite's implicit rowid) through capture and apply, so replicas +// converge on the origin's rowid instead of assigning their own. +const rowidColumnKey = "rowid" + +// reservedRowidAliases are SQLite's built-in names for the rowid column. +// A user-declared column sharing one of these names would collide with +// rowidColumnKey in the CDC maps, so tables relying on the rowid sentinel +// must not declare any of them. +var reservedRowidAliases = [...]string{"rowid", "oid", "_rowid_"} + +// isRowidSentinelSchema reports whether a table has no explicit PRIMARY KEY, +// meaning its replication identity is SQLite's rowid (schema.PKIndices == [-1]). +func isRowidSentinelSchema(schema *TableSchema) bool { + return len(schema.PKIndices) == 1 && schema.PKIndices[0] == -1 +} + +// findShadowedRowidColumn returns the first declared column name that +// case-insensitively matches one of SQLite's rowid aliases, or "" if none do. +func findShadowedRowidColumn(columns []string) string { + for _, col := range columns { + for _, alias := range reservedRowidAliases { + if strings.EqualFold(col, alias) { + return col + } + } + } + return "" +} + // extractPKFromValues extracts PK values from raw values slice using schema indices. // Returns typed PK values in PK declaration order for binary encoding. func extractPKFromValues(schema *TableSchema, values []interface{}, rowID int64) []filter.TypedPKValue { @@ -615,17 +800,71 @@ func extractPKFromValues(schema *TableSchema, values []interface{}, rowID int64) } // encodeValuesWithSchema converts []interface{} to map[string][]byte using schema column names. -func encodeValuesWithSchema(columns []string, values []interface{}) map[string][]byte { +// +// SQLite's preupdate hook hands back TEXT and BLOB storage classes identically +// as Go []byte (see go-sqlite3's row(): both SQLITE_BLOB and SQLITE_TEXT go +// through sqlite3_value_bytes/GoBytes), so nothing about the raw value itself +// says which one it is. schema.BlobAffinityCols (precomputed at schema load +// from the declared column type) resolves that by column AFFINITY, not +// declared type name: per SQLite's dynamic typing (sqlite.org/datatype3.html +// #3.1), only BLOB affinity never coerces a stored value - every other +// affinity (TEXT, INTEGER, REAL, NUMERIC) converts a value that looks +// numeric on INSERT, but a TEXT value that doesn't parse as a number is left +// alone. So a []byte captured for a NUMERIC/INTEGER/REAL/TEXT-affinity +// column is TEXT storage class in the overwhelming common case (numbers +// arrive from the hook as int64/float64 already, never []byte) and is +// converted to string here, written as msgpack Str. A []byte for a +// BLOB-affinity column is left as-is and written as msgpack Bin. +// unmarshalCDCValue's strict decode preserves that choice on the way back +// out, so BLOB columns round trip as []byte -> sqlite3_bind_blob instead of +// being coerced to text. +// +// This is a deliberate, accepted lesser evil, not a complete solution: SQLite +// never coerces a genuine BLOB storage class value either, regardless of the +// column's declared affinity (e.g. inserted via a literal blob or an explicit +// CAST(... AS BLOB) into a NUMERIC/TEXT/etc-affinity column). Such a value +// would incorrectly round-trip as a string here, since nothing in the raw +// []byte or the static schema distinguishes it from ordinary TEXT storage +// class. This is intentionally the rarer case: BLOB-affinity columns are +// overwhelmingly used for genuine binary data (password hashes, UUIDs - the +// motivating bug), while non-BLOB-affinity columns overwhelmingly hold text +// or numbers, so defaulting non-BLOB affinities to string protects the +// common case in both directions. +// +// Columns are looked up by their true position (schema.ColumnPositions), not +// by their index within schema.Columns: whenever the table has a generated +// (STORED) column, schema.Columns excludes it but the preupdate hook's raw +// values array does not skip its slot, so index-in-Columns and +// index-into-values diverge (see loadSchema). +// +// Returns an error - rather than silently dropping data - if a column's +// position falls outside the captured values (a stale schema relative to +// this row) or if any value fails to encode, since a partial CDC row is +// worse than a loud failure. +func encodeValuesWithSchema(schema *TableSchema, values []interface{}) (map[string][]byte, error) { + columns := schema.Columns result := make(map[string][]byte, len(columns)) for i, col := range columns { - if i >= len(values) { - continue + pos := i + if i < len(schema.ColumnPositions) { + pos = schema.ColumnPositions[i] + } + if pos < 0 || pos >= len(values) { + return nil, fmt.Errorf("column %s: position %d out of range for %d captured values (stale schema?)", col, pos, len(values)) } - if encoded := encodeValue(values[i]); encoded != nil { - result[col] = encoded + + v := values[pos] + isBlobAffinity := i < len(schema.BlobAffinityCols) && schema.BlobAffinityCols[i] + if b, ok := v.([]byte); ok && !isBlobAffinity { + v = string(b) + } + encoded, err := encoding.Marshal(v) + if err != nil { + return nil, fmt.Errorf("failed to encode column %s: %w", col, err) } + result[col] = encoded } - return result + return result, nil } // encodeValue encodes a single value to msgpack bytes. diff --git a/db/rowid_sentinel_cdc_test.go b/db/rowid_sentinel_cdc_test.go new file mode 100644 index 00000000..8522f1f3 --- /dev/null +++ b/db/rowid_sentinel_cdc_test.go @@ -0,0 +1,278 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/maxpert/marmot/encoding" + "github.com/maxpert/marmot/hlc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newRowidTestDatabase creates a ReplicatedDatabase backed by its own temp dir +// and Pebble meta store, for use as either the capture ("source") or apply +// ("replica") side of a CDC round trip. +func newRowidTestDatabase(t *testing.T, nodeID uint64) *ReplicatedDatabase { + t.Helper() + + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + metaPath := filepath.Join(tmpDir, "meta") + require.NoError(t, os.MkdirAll(metaPath, 0755)) + + metaStore, err := NewPebbleMetaStore(metaPath, DefaultPebbleOptions()) + require.NoError(t, err) + + clock := hlc.NewClock(nodeID) + replicatedDB, err := NewReplicatedDatabase(dbPath, nodeID, clock, metaStore) + require.NoError(t, err) + + t.Cleanup(func() { + replicatedDB.Close() + metaStore.Close() + }) + return replicatedDB +} + +// captureEntries runs execStatements inside a single CDC capture transaction +// against source and returns the resulting intent entries in sequence order. +func captureEntries(t *testing.T, source *ReplicatedDatabase, txnID uint64, execStatements ...string) []*IntentEntry { + t.Helper() + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, txnID) + require.NoError(t, err) + defer session.Rollback() + + require.NoError(t, session.BeginTx(ctx)) + for _, stmt := range execStatements { + _, err = session.ExecContext(ctx, stmt) + require.NoError(t, err) + } + + // Intent entries must be read before Commit(): Commit() calls cleanup(), + // which clears the captured-row buffer (see EphemeralHookSession.cleanup). + entries, err := session.GetIntentEntries() + require.NoError(t, err) + require.NoError(t, session.Commit()) + return entries +} + +// applyEntries applies each entry to replica's write DB using its own schema cache. +func applyEntries(t *testing.T, replica *ReplicatedDatabase, entries []*IntentEntry) { + t.Helper() + adapter := &schemaCacheAdapter{cache: replica.schemaCache} + for _, entry := range entries { + require.NoError(t, ApplyCDCEntry(replica.GetWriteDB(), adapter, entry)) + } +} + +// TestRowidSentinelCDC_CaptureIncludesRowidKey verifies hookCallback embeds the +// SQLite rowid under the "rowid" CDC key for tables with no explicit PRIMARY KEY. +func TestRowidSentinelCDC_CaptureIncludesRowidKey(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + _, err := source.GetWriteDB().Exec(`CREATE TABLE metadata (version SMALLINT)`) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + entries := captureEntries(t, source, 2001, `INSERT INTO metadata (version) VALUES (1)`) + require.Len(t, entries, 1) + require.Equal(t, uint8(OpTypeInsert), entries[0].Operation) + require.Contains(t, entries[0].NewValues, "rowid") + + var rowid int64 + require.NoError(t, encoding.Unmarshal(entries[0].NewValues["rowid"], &rowid)) + assert.Equal(t, int64(1), rowid) + + entries = captureEntries(t, source, 2002, `UPDATE metadata SET version = 2 WHERE rowid = 1`) + require.Len(t, entries, 1) + require.Equal(t, uint8(OpTypeUpdate), entries[0].Operation) + require.Contains(t, entries[0].OldValues, "rowid") + require.Contains(t, entries[0].NewValues, "rowid") + + entries = captureEntries(t, source, 2003, `DELETE FROM metadata WHERE rowid = 1`) + require.Len(t, entries, 1) + require.Equal(t, uint8(OpTypeDelete), entries[0].Operation) + require.Contains(t, entries[0].OldValues, "rowid") + require.Nil(t, entries[0].NewValues) +} + +// TestRowidSentinelCDC_ApplyRoundTrip_Insert reproduces the LLDAP bug report: +// `CREATE TABLE metadata (version SMALLINT)` followed by writes must replicate. +// It also asserts the replica lands on the SAME rowid as the origin. +func TestRowidSentinelCDC_ApplyRoundTrip_Insert(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE metadata (version SMALLINT)` + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + _, err = replica.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, replica.ReloadSchema()) + + entries := captureEntries(t, source, 3001, `INSERT INTO metadata (version) VALUES (1)`) + require.Len(t, entries, 1) + + applyEntries(t, replica, entries) + + var rowid int64 + var version int64 + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT rowid, version FROM metadata`).Scan(&rowid, &version)) + assert.Equal(t, int64(1), rowid, "replica must land on origin's rowid") + assert.Equal(t, int64(1), version) +} + +// TestRowidSentinelCDC_ApplyRoundTrip_UpdateDelete verifies the exact failing +// sequence from the bug report - UPDATE and DELETE on a no-PK table - now +// replicates without the "primary key column rowid not found" error. +func TestRowidSentinelCDC_ApplyRoundTrip_UpdateDelete(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE metadata (version SMALLINT)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + insertEntries := captureEntries(t, source, 4001, `INSERT INTO metadata (version) VALUES (1)`) + applyEntries(t, replica, insertEntries) + + updateEntries := captureEntries(t, source, 4002, `UPDATE metadata SET version = 2`) + require.Len(t, updateEntries, 1) + applyEntries(t, replica, updateEntries) + + var version int64 + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT version FROM metadata`).Scan(&version)) + assert.Equal(t, int64(2), version) + + deleteEntries := captureEntries(t, source, 4003, `DELETE FROM metadata`) + require.Len(t, deleteEntries, 1) + applyEntries(t, replica, deleteEntries) + + var count int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM metadata`).Scan(&count)) + assert.Equal(t, 0, count) +} + +// TestRowidSentinelCDC_ApplyRoundTrip_MultiRow verifies multiple no-PK rows +// each preserve their own distinct origin rowid on the replica. +func TestRowidSentinelCDC_ApplyRoundTrip_MultiRow(t *testing.T) { + source := newRowidTestDatabase(t, 1) + replica := newRowidTestDatabase(t, 2) + + const ddl = `CREATE TABLE events (label TEXT)` + require.NoError(t, execAndReload(source, ddl)) + require.NoError(t, execAndReload(replica, ddl)) + + entries := captureEntries(t, source, 5001, + `INSERT INTO events (label) VALUES ('a')`, + `INSERT INTO events (label) VALUES ('b')`, + `INSERT INTO events (label) VALUES ('c')`, + ) + require.Len(t, entries, 3) + applyEntries(t, replica, entries) + + rows, err := replica.GetWriteDB().Query(`SELECT rowid, label FROM events ORDER BY rowid`) + require.NoError(t, err) + defer rows.Close() + + var got []struct { + rowid int64 + label string + } + for rows.Next() { + var r int64 + var l string + require.NoError(t, rows.Scan(&r, &l)) + got = append(got, struct { + rowid int64 + label string + }{r, l}) + } + require.NoError(t, rows.Err()) + require.Len(t, got, 3) + assert.Equal(t, int64(1), got[0].rowid) + assert.Equal(t, "a", got[0].label) + assert.Equal(t, int64(2), got[1].rowid) + assert.Equal(t, "b", got[1].label) + assert.Equal(t, int64(3), got[2].rowid) + assert.Equal(t, "c", got[2].label) + + // Delete the middle row and verify only it disappears on the replica. + deleteEntries := captureEntries(t, source, 5002, `DELETE FROM events WHERE rowid = 2`) + require.Len(t, deleteEntries, 1) + applyEntries(t, replica, deleteEntries) + + var remaining int + require.NoError(t, replica.GetWriteDB().QueryRow(`SELECT COUNT(*) FROM events`).Scan(&remaining)) + assert.Equal(t, 2, remaining) + var stillThere int + require.NoError(t, replica.GetWriteDB().QueryRow( + `SELECT COUNT(*) FROM events WHERE rowid = 2`).Scan(&stillThere)) + assert.Equal(t, 0, stillThere) +} + +// TestRowidSentinelCDC_ShadowedRowidColumn_CaptureFails verifies capture is +// refused with a clear error when a no-PK table declares a column named +// rowid, oid, or _rowid_, since it would collide with the CDC rowid key. +func TestRowidSentinelCDC_ShadowedRowidColumn_CaptureFails(t *testing.T) { + for _, shadowCol := range []string{"rowid", "oid", "_rowid_", "RowId"} { + t.Run(shadowCol, func(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + ddl := `CREATE TABLE bad (` + quoteSQLiteIdent(shadowCol) + ` TEXT, name TEXT)` + _, err := source.GetWriteDB().Exec(ddl) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + ctx := context.Background() + session, err := StartEphemeralSession(ctx, source.hookDB, source.metaStore, source.schemaCache, 6001) + require.NoError(t, err) + defer session.Rollback() + + require.NoError(t, session.BeginTx(ctx)) + _, execErr := session.ExecContext(ctx, `INSERT INTO bad (`+quoteSQLiteIdent(shadowCol)+`, name) VALUES ('x', 'y')`) + require.Error(t, execErr) + assert.Contains(t, execErr.Error(), "shadows SQLite's rowid alias") + }) + } +} + +// TestRowidSentinelCDC_WithoutRowidTable_NeverUsesSentinel confirms WITHOUT +// ROWID tables - which SQLite requires to declare an explicit PRIMARY KEY - +// never fall into the rowid-sentinel path, so the rowid CDC key is never +// synthesized for them. +func TestRowidSentinelCDC_WithoutRowidTable_NeverUsesSentinel(t *testing.T) { + source := newRowidTestDatabase(t, 1) + + _, err := source.GetWriteDB().Exec( + `CREATE TABLE kv (k TEXT PRIMARY KEY, v TEXT) WITHOUT ROWID`) + require.NoError(t, err) + require.NoError(t, source.ReloadSchema()) + + schema, err := source.schemaCache.GetSchemaFor("kv") + require.NoError(t, err) + assert.False(t, isRowidSentinelSchema(schema), "WITHOUT ROWID table must use its declared PK, not the rowid sentinel") + assert.Equal(t, []string{"k"}, schema.PrimaryKeys) + + entries := captureEntries(t, source, 7001, `INSERT INTO kv (k, v) VALUES ('key1', 'val1')`) + require.Len(t, entries, 1) + assert.NotContains(t, entries[0].NewValues, "rowid") +} + +func execAndReload(db *ReplicatedDatabase, stmt string) error { + if _, err := db.GetWriteDB().Exec(stmt); err != nil { + return err + } + return db.ReloadSchema() +} diff --git a/db/schema_cache.go b/db/schema_cache.go index bb024f23..a3b1e54b 100644 --- a/db/schema_cache.go +++ b/db/schema_cache.go @@ -33,10 +33,27 @@ import ( // Use view methods (ToPublisherSchema, GetColumnTypes) or adapters. type TableSchema struct { // Hot path fields - preupdate hook performance critical - Columns []string // Column names in declaration order - PrimaryKeys []string // PK column names in PK order - PKIndices []int // Indices into Columns for PKs (-1 for rowid) - IntentKeyPrefix []byte // Precomputed: version(1) + uvarint(tableLen) + table + Columns []string // Column names in declaration order, excluding generated (VIRTUAL/STORED) columns + ColumnPositions []int // Parallel to Columns: each column's TRUE ordinal position (PRAGMA table_xinfo cid), i.e. its index into the preupdate hook's raw value array. NOT the same as the index into Columns whenever the table has any generated column - see loadSchema. + PrimaryKeys []string // PK column names in PK order + PKIndices []int // Indices into the preupdate hook's raw value array for PKs (-1 for rowid); same true-cid space as ColumnPositions + IntentKeyPrefix []byte // Precomputed: version(1) + uvarint(tableLen) + table + BlobAffinityCols []bool // Parallel to Columns: true where the declared type has BLOB affinity. + // CDC capture cannot tell TEXT from BLOB storage class by value alone (the + // preupdate hook hands both back as Go []byte), so this precomputed lookup + // lets encodeValuesWithSchema pick msgpack Bin (BLOB affinity) vs. Str + // (every other affinity) without parsing the declared type on every row. + // Only BLOB affinity is singled out - per SQLite's affinity rules, it is + // the only one that never coerces a value on INSERT, so it is the only + // affinity where a captured []byte is more likely to be genuine BLOB + // storage class than TEXT. See encodeValuesWithSchema in preupdate_hook.go. + + // VirtualColumns lists GENERATED ALWAYS AS (...) VIRTUAL column names. + // go-sqlite3's preupdate hook segfaults reading a virtual column's value + // (sqlite3_preupdate_new/old returns NULL for it), so hookCallback must + // refuse to capture CDC for tables that have any. STORED generated columns + // are unaffected and are not included here. + VirtualColumns []string // Cold path fields - populated for CDC publisher, transpilation FullColumns []ColumnSchema // Full column metadata @@ -176,54 +193,98 @@ func (c *SchemaCache) Update(tableName string, schema *TableSchema) { } // loadSchema fetches schema from DB using the raw SQLite connection. -// Extracts ALL 6 columns from PRAGMA table_info: -// - cid: column index -// - name: column name -// - type: column type affinity -// - notnull: 1 if NOT NULL constraint -// - dflt_value: default value (ignored) -// - pk: primary key order (1-based, 0 if not PK) +// Uses PRAGMA table_xinfo (a superset of table_info) rather than table_info: +// - cid: column's TRUE ordinal position in the table, including hidden +// (generated) columns. This is the index the preupdate hook's +// Old()/New() use, and it does NOT match table_info's cid, which +// silently renumbers columns after excluding hidden ones (verified +// directly: a table with a generated column between two normal columns +// gets cid 0,1,2 from table_info but the normal columns' true positions +// are 0,2 - table_info's cid is useless for aligning against the +// preupdate hook's raw value array whenever any generated column exists). +// - name, type, notnull, pk: same as table_info. +// - hidden: 0 normal, 1 a virtual table's own hidden pseudocolumn (e.g. +// FTS5's table-name/rank columns), 2 GENERATED ALWAYS ... VIRTUAL, +// 3 ... STORED. +// +// Only hidden == 0 columns end up in Columns/FullColumns/PK tracking - the +// same set table_info already produced (it silently excludes every hidden +// kind), so this is not a behavior change for what counts as a "real" +// column. hidden == 3 (STORED) is excluded because SQLite rejects an +// explicit INSERT/UPDATE of a generated column ("cannot INSERT into +// generated column", verified directly) and its value is deterministically +// recomputed by SQLite from a row's other captured columns on every replica, +// so capturing and applying it would be both wrong and unnecessary. +// hidden == 1 pseudocolumns are query-only with no real storage to capture. +// VIRTUAL (hidden == 2) column names are recorded into VirtualColumns +// instead: go-sqlite3's preupdate hook cannot safely read their value at all +// (see hookCallback), so any table with one refuses CDC capture entirely +// rather than attempting a partial (and equally wrong) capture. func loadSchema(conn *sqlite3.SQLiteConn, tableName string) (*TableSchema, error) { - rows, err := conn.Query(fmt.Sprintf("PRAGMA table_info(%s)", tableName), nil) + rows, err := conn.Query(fmt.Sprintf("PRAGMA table_xinfo(%s)", tableName), nil) if err != nil { - return nil, fmt.Errorf("query table_info: %w", err) + return nil, fmt.Errorf("query table_xinfo: %w", err) } defer rows.Close() schema := &TableSchema{ - TableName: tableName, - Columns: make([]string, 0), - PrimaryKeys: make([]string, 0), - PKIndices: make([]int, 0), - FullColumns: make([]ColumnSchema, 0), + TableName: tableName, + Columns: make([]string, 0), + ColumnPositions: make([]int, 0), + PrimaryKeys: make([]string, 0), + PKIndices: make([]int, 0), + FullColumns: make([]ColumnSchema, 0), + BlobAffinityCols: make([]bool, 0), } // Track PK columns with their order for proper sorting type pkInfo struct { name string order int - index int // column index + index int // true cid: position into the preupdate hook's raw value array } var pkColumns []pkInfo - dest := make([]driver.Value, 6) - colIndex := 0 + const ( + hiddenNone = 0 + hiddenVirtual = 2 + ) + + dest := make([]driver.Value, 7) for { if err := rows.Next(dest); err != nil { if err == io.EOF { break } - return nil, fmt.Errorf("read table_info row: %w", err) + return nil, fmt.Errorf("read table_xinfo row: %w", err) } - // Extract all 6 PRAGMA columns + cid, _ := dest[0].(int64) name, _ := dest[1].(string) colType, _ := dest[2].(string) notNull, _ := dest[3].(int64) pk, _ := dest[5].(int64) + hidden, _ := dest[6].(int64) + + if hidden == hiddenVirtual { + schema.VirtualColumns = append(schema.VirtualColumns, name) + continue + } + if hidden != hiddenNone { + // hidden == 3 (GENERATED ... STORED): recomputed by SQLite itself + // from the row's other columns, so it is neither captured nor + // applied (see loadSchema's doc comment). + // hidden == 1 (a virtual table's own HIDDEN column, e.g. FTS5's + // table-name/rank pseudocolumns): query-only, no real storage to + // capture. Matches table_info, which already excluded these. + continue + } + // hidden == 0 (normal column): falls through and is captured below. // Hot path fields schema.Columns = append(schema.Columns, name) + schema.ColumnPositions = append(schema.ColumnPositions, int(cid)) + schema.BlobAffinityCols = append(schema.BlobAffinityCols, isBlobAffinity(colType)) // Cold path fields - full column metadata col := ColumnSchema{ @@ -240,10 +301,9 @@ func loadSchema(conn *sqlite3.SQLiteConn, tableName string) (*TableSchema, error pkColumns = append(pkColumns, pkInfo{ name: name, order: int(pk), - index: colIndex, + index: int(cid), }) } - colIndex++ } // Sort PKs by their order in composite key @@ -303,6 +363,35 @@ func loadSchema(conn *sqlite3.SQLiteConn, tableName string) (*TableSchema, error return schema, nil } +// isBlobAffinity reports whether a SQLite declared column type has BLOB +// affinity, using the exact precedence order from +// https://sqlite.org/datatype3.html#determination_of_column_affinity: +// 1. Contains "INT" -> INTEGER affinity (checked first, so e.g. "POINT" +// is INTEGER, not BLOB, even though it contains none of the BLOB +// markers - this rule must run before any of the others). +// 2. Contains "CHAR", "CLOB", or "TEXT" -> TEXT affinity. +// 3. Contains "BLOB", or no declared type at all -> BLOB affinity. +// 4. Contains "REAL", "FLOA", or "DOUB" -> REAL affinity. +// 5. Otherwise -> NUMERIC affinity. +// +// Only step 3 returns true here; every other affinity returns false. This +// intentionally is not a general affinity classifier (steps 4-5 are folded +// into a single "false"), because encodeValuesWithSchema only needs the +// BLOB/not-BLOB distinction. +func isBlobAffinity(declType string) bool { + d := strings.ToUpper(declType) + switch { + case strings.Contains(d, "INT"): + return false + case strings.Contains(d, "CHAR"), strings.Contains(d, "CLOB"), strings.Contains(d, "TEXT"): + return false + case strings.Contains(d, "BLOB"), d == "": + return true + default: + return false + } +} + // BuildDeterminismSchema creates a determinism.Schema from cached table metadata. // This is used for checking if DML statements are deterministic before execution. func (c *SchemaCache) BuildDeterminismSchema() *determinism.Schema { diff --git a/db/transaction.go b/db/transaction.go index b9dba18e..203d4034 100644 --- a/db/transaction.go +++ b/db/transaction.go @@ -513,10 +513,13 @@ func (tm *TransactionManager) applyNonDMLIntents(txnID uint64, intents []*WriteI log.Debug().Uint64("txn_id", txnID).Str("sql", intent.SQLStatement).Msg("DDL statement executed") } - // Reload schema cache after DDL operations. + // Reload schema cache after DDL operations. A failed reload leaves the + // cache stale, so subsequent preupdate hooks would silently drop CDC data + // for any column the DDL added/changed - fail the DDL apply instead of + // swallowing the error. if hasDDL && tm.schemaCache != nil { if err := tm.reloadSchemaCache(); err != nil { - log.Warn().Err(err).Uint64("txn_id", txnID).Msg("Failed to reload schema cache after DDL") + return fmt.Errorf("failed to reload schema cache after DDL (txn %d): %w", txnID, err) } } @@ -884,16 +887,21 @@ func (s *schemaCacheAdapter) GetPrimaryKeys(tableName string) ([]string, error) return schema.PrimaryKeys, nil } -// unmarshalCDCValue deserializes a msgpack-encoded value and converts []byte to string. -// SQLite returns TEXT values as []byte from preupdate hooks, so we convert back for proper type affinity. +// unmarshalCDCValue deserializes a msgpack-encoded CDC column value. +// +// SQLite's preupdate hook hands back TEXT and BLOB storage classes identically +// as Go []byte, so distinguishing them can only happen where the schema is +// known: capture (encodeValuesWithSchema in preupdate_hook.go) converts +// TEXT-affinity columns to string before encoding, so they are written as +// msgpack Str, while BLOB-affinity columns stay []byte and are written as +// msgpack Bin. UnmarshalStrict preserves that distinction on the way back out +// (Bin -> []byte, Str -> string) so BLOB columns bind via sqlite3_bind_blob +// instead of being coerced to text. Unmarshal's loose decoding must NOT be +// used here: it collapses Bin to string, silently corrupting BLOB columns. func unmarshalCDCValue(data []byte) (interface{}, error) { var value interface{} - if err := encoding.Unmarshal(data, &value); err != nil { + if err := encoding.UnmarshalStrict(data, &value); err != nil { return nil, err } - // Convert []byte to string - SQLite returns TEXT as []byte from preupdate hooks - if b, ok := value.([]byte); ok { - return string(b), nil - } return value, nil } diff --git a/db/transaction_schema_reload_test.go b/db/transaction_schema_reload_test.go new file mode 100644 index 00000000..fe0b048c --- /dev/null +++ b/db/transaction_schema_reload_test.go @@ -0,0 +1,101 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package db + +import ( + "database/sql" + "fmt" + "path/filepath" + "sync/atomic" + "testing" + + "github.com/mattn/go-sqlite3" + "github.com/maxpert/marmot/hlc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newSchemaReloadFaultDB opens a *sql.DB through a private driver registration +// whose ConnectHook fails every connection after the first. Combined with +// SetMaxIdleConns(0) (which forces the pool to open a brand-new connection - +// and so re-run the hook - for every operation instead of reusing one), this +// deterministically fails the SECOND connection a caller requests while +// letting the first succeed, with no timing/race dependency: both connection +// attempts happen strictly sequentially in the calling goroutine's own code. +func newSchemaReloadFaultDB(t *testing.T) (faultyDB *sql.DB, dbPath string) { + t.Helper() + dbPath = filepath.Join(t.TempDir(), "test.db") + + var connects atomic.Int32 + driverName := fmt.Sprintf("sqlite3_reload_fault_%p", t) + sql.Register(driverName, &sqlite3.SQLiteDriver{ + ConnectHook: func(conn *sqlite3.SQLiteConn) error { + if connects.Add(1) > 1 { + return fmt.Errorf("simulated connection failure (connect #%d)", connects.Load()) + } + return nil + }, + }) + + db, err := sql.Open(driverName, dbPath) + require.NoError(t, err) + t.Cleanup(func() { db.Close() }) + db.SetMaxIdleConns(0) // force a fresh connection (and ConnectHook call) per operation + return db, dbPath +} + +// TestApplyNonDMLIntents_PropagatesSchemaReloadFailure is a regression test +// for db/transaction.go's applyNonDMLIntents: previously, a failure to reload +// the schema cache after a successful DDL exec was only logged +// (log.Warn "Failed to reload schema cache after DDL") and the function +// returned nil, leaving the cache stale so subsequent preupdate hooks would +// silently drop CDC data for any column the DDL touched. It must now +// propagate the error and fail the DDL apply instead. +func TestApplyNonDMLIntents_PropagatesSchemaReloadFailure(t *testing.T) { + db, dbPath := newSchemaReloadFaultDB(t) + schemaCache := NewSchemaCache() + tm := NewTransactionManager(db, nil, hlc.NewClock(1), schemaCache) + + intents := []*WriteIntentRecord{ + { + IntentType: IntentTypeDDL, + SQLStatement: `CREATE TABLE t (id INTEGER PRIMARY KEY)`, + }, + } + + // The DDL exec itself is the first connection (succeeds); the schema + // reload that follows needs a second, fresh connection (fails). Every + // connection through this faulty driver after the first fails, so this + // is the last operation this test can perform against `db` itself. + err := tm.applyNonDMLIntents(1, intents) + require.Error(t, err, "a failed post-DDL schema reload must fail the DDL apply, not be silently logged and swallowed") + assert.Contains(t, err.Error(), "reload schema cache") + + // Sanity: the DDL itself really did succeed against the underlying file - + // this proves the test isolates the reload failure and isn't just + // failing because the connection was broken from the start. Checked via + // a plain, unfaulty connection since every further connection on `db` + // deliberately fails from here on. + plainDB, err := sql.Open(SQLiteDriverName, dbPath) + require.NoError(t, err) + defer plainDB.Close() + var name string + require.NoError(t, plainDB.QueryRow( + `SELECT name FROM sqlite_master WHERE type='table' AND name='t'`).Scan(&name)) + assert.Equal(t, "t", name) +} + +// TestApplyNonDMLIntents_NoDDLNeverReloads verifies a purely non-DDL intent +// list (e.g. only LOAD DATA) does not attempt a schema reload at all, so it +// is unaffected by this fix. +func TestApplyNonDMLIntents_NoDDLNeverReloads(t *testing.T) { + db, _ := newSchemaReloadFaultDB(t) + schemaCache := NewSchemaCache() + tm := NewTransactionManager(db, nil, hlc.NewClock(1), schemaCache) + + // No intents at all: hasDDL stays false, so reloadSchemaCache must never + // be called, and the second (failing) connection must never be needed. + err := tm.applyNonDMLIntents(1, nil) + require.NoError(t, err) +} diff --git a/docs/src/pages/reference.mdx b/docs/src/pages/reference.mdx index 31c623a0..6eda0932 100644 --- a/docs/src/pages/reference.mdx +++ b/docs/src/pages/reference.mdx @@ -20,8 +20,13 @@ data_dir = "./marmot-data" heartbeat_timeout_seconds = 10 # Transaction timeout without heartbeat conflict_window_seconds = 10 # Conflict resolution window lock_wait_timeout_seconds = 50 # Lock wait timeout (MySQL: innodb_lock_wait_timeout) +ddl_implicit_commit = true # DDL commits the open transaction, as MySQL does ``` +**`ddl_implicit_commit`** (default `true`): MySQL has no transactional DDL. `CREATE`, `ALTER`, and `DROP` implicitly commit the transaction a session has open, then run on their own, leaving no transaction open. Marmot follows that by default, so DML can see a column added earlier in the same transaction — the pattern schema migration tools rely on. + +Set it to `false` to keep DDL inside the transaction and replicate it atomically with the surrounding statements. Statements are then buffered until `COMMIT`, so DML in that transaction cannot see a schema change made earlier in it. + **Note**: Transaction log garbage collection is managed by the replication configuration to coordinate with anti-entropy. See `replication.gc_min_retention_hours` and `replication.gc_max_retention_hours`. ## Connection Pool diff --git a/encoding/msgpack.go b/encoding/msgpack.go index 521fee8b..a601b5bb 100644 --- a/encoding/msgpack.go +++ b/encoding/msgpack.go @@ -39,3 +39,15 @@ func Unmarshal(data []byte, v interface{}) error { return dec.Decode(v) } + +// UnmarshalStrict decodes msgpack data into interface{} WITHOUT loose interface +// decoding: msgpack Bin decodes as Go []byte, msgpack Str decodes as Go string. +// Use this where the encoder already chose Bin vs. Str deliberately (e.g. CDC +// column values, where BLOB columns are encoded as Bin and TEXT columns as Str) +// and that distinction must survive the round trip. Unmarshal's loose decoding +// would collapse both to string, which is correct for the general +// interface{}-decoding case documented on Unmarshal but corrupts BLOB data. +func UnmarshalStrict(data []byte, v interface{}) error { + dec := msgpack.NewDecoder(bytes.NewReader(data)) + return dec.Decode(v) +} diff --git a/examples/README.md b/examples/README.md index e6119ad4..e110a651 100644 --- a/examples/README.md +++ b/examples/README.md @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta Examples +# Marmot v2.9.16-beta Examples Quick start examples for running Marmot in different configurations. @@ -122,7 +122,7 @@ Each config includes: ### Full Database Replication -Marmot v2.9.15-beta uses **full database replication**, not partitioning: +Marmot v2.9.16-beta uses **full database replication**, not partitioning: - **Every node** has a complete copy of the database - **Writes** go to all nodes (broadcast) diff --git a/examples/cluster-with-replicas/node-1-config.toml b/examples/cluster-with-replicas/node-1-config.toml index be1c7016..3d53dedd 100644 --- a/examples/cluster-with-replicas/node-1-config.toml +++ b/examples/cluster-with-replicas/node-1-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Cluster Node 1 (Primary) +# Marmot v2.9.16-beta - Cluster Node 1 (Primary) # 2-node cluster with read-only replicas node_id = 1 @@ -9,6 +9,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "localhost:8081" diff --git a/examples/cluster-with-replicas/node-2-config.toml b/examples/cluster-with-replicas/node-2-config.toml index e7cd70e0..ddcba208 100644 --- a/examples/cluster-with-replicas/node-2-config.toml +++ b/examples/cluster-with-replicas/node-2-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Cluster Node 2 (Primary) +# Marmot v2.9.16-beta - Cluster Node 2 (Primary) # 2-node cluster with read-only replicas node_id = 2 @@ -9,6 +9,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "localhost:8082" diff --git a/examples/cluster-with-replicas/replica-1a-config.toml b/examples/cluster-with-replicas/replica-1a-config.toml index 441224af..cd9f2943 100644 --- a/examples/cluster-with-replicas/replica-1a-config.toml +++ b/examples/cluster-with-replicas/replica-1a-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Read-Only Replica 1A +# Marmot v2.9.16-beta - Read-Only Replica 1A # Follows cluster nodes for transparent failover node_id = 101 @@ -23,6 +23,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_port = 8091 diff --git a/examples/cluster-with-replicas/replica-2a-config.toml b/examples/cluster-with-replicas/replica-2a-config.toml index 90626ca3..a6a60428 100644 --- a/examples/cluster-with-replicas/replica-2a-config.toml +++ b/examples/cluster-with-replicas/replica-2a-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Read-Only Replica 2A +# Marmot v2.9.16-beta - Read-Only Replica 2A # Follows cluster nodes for transparent failover node_id = 102 @@ -23,6 +23,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_port = 8092 diff --git a/examples/cluster-with-replicas/run-cluster-with-replicas.sh b/examples/cluster-with-replicas/run-cluster-with-replicas.sh index bd20c1da..0b0d17d2 100755 --- a/examples/cluster-with-replicas/run-cluster-with-replicas.sh +++ b/examples/cluster-with-replicas/run-cluster-with-replicas.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Marmot v2.9.15-beta - 2-Node Cluster with Read-Only Replicas +# Marmot v2.9.16-beta - 2-Node Cluster with Read-Only Replicas # # Topology: # Node 1 (primary) ←→ Node 2 (primary) [2-node cluster with QUORUM writes] @@ -24,7 +24,7 @@ generate_secret() { CLUSTER_SECRET=$(generate_secret) REPLICA_SECRET=$(generate_secret) -echo "=== Marmot v2.9.15-beta Cluster with Replicas ===" +echo "=== Marmot v2.9.16-beta Cluster with Replicas ===" echo "Starting 2-node cluster + 2 read-only replicas" echo "" @@ -60,7 +60,7 @@ generate_node_config() { local config_file="/tmp/marmot-cluster/node-${node_id}.toml" cat > "$config_file" << EOF -# Marmot v2.9.15-beta - Node ${node_id} (auto-generated) +# Marmot v2.9.16-beta - Node ${node_id} (auto-generated) node_id = ${node_id} data_dir = "/tmp/marmot-cluster/node-${node_id}" @@ -143,7 +143,7 @@ generate_replica_config() { local config_file="/tmp/marmot-cluster/replica-${replica_id}a.toml" cat > "$config_file" << EOF -# Marmot v2.9.15-beta - Replica ${replica_id}A (auto-generated) +# Marmot v2.9.16-beta - Replica ${replica_id}A (auto-generated) node_id = ${replica_id}00 data_dir = "/tmp/marmot-cluster/replica-${replica_id}a" diff --git a/examples/node-1-config.toml b/examples/node-1-config.toml index 4a7f0f19..d4229c0b 100644 --- a/examples/node-1-config.toml +++ b/examples/node-1-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Example Node 1 +# Marmot v2.9.16-beta - Example Node 1 # Full Database Replication node_id = 1 @@ -9,6 +9,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 # How long to wait for locks (MySQL: innodb_lock_wait_timeout) +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "localhost:8081" diff --git a/examples/node-2-config.toml b/examples/node-2-config.toml index da8e6284..80a402c5 100644 --- a/examples/node-2-config.toml +++ b/examples/node-2-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Example Node 2 +# Marmot v2.9.16-beta - Example Node 2 # Full Database Replication node_id = 2 @@ -9,6 +9,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 # How long to wait for locks (MySQL: innodb_lock_wait_timeout) +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "localhost:8082" diff --git a/examples/node-3-config.toml b/examples/node-3-config.toml index 39246203..0fc7b9c1 100644 --- a/examples/node-3-config.toml +++ b/examples/node-3-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - Example Node 3 +# Marmot v2.9.16-beta - Example Node 3 # Full Database Replication node_id = 3 @@ -9,6 +9,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 # How long to wait for locks (MySQL: innodb_lock_wait_timeout) +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "localhost:8083" diff --git a/examples/run-cluster.sh b/examples/run-cluster.sh index b79546dd..007551da 100755 --- a/examples/run-cluster.sh +++ b/examples/run-cluster.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Marmot v2.9.15-beta - Example 3-Node Cluster +# Marmot v2.9.16-beta - Example 3-Node Cluster # Full Database Replication: ALL nodes get ALL data set -e @@ -37,7 +37,7 @@ while [[ $# -gt 0 ]]; do esac done -echo "=== Marmot v2.9.15-beta Example Cluster ===" +echo "=== Marmot v2.9.16-beta Example Cluster ===" echo "Starting 3-node cluster with full database replication" echo "" @@ -98,7 +98,7 @@ generate_node_config() { done cat > "$config_file" << EOF -# Marmot v2.9.15-beta - Node ${node_id} (auto-generated) +# Marmot v2.9.16-beta - Node ${node_id} (auto-generated) node_id = ${node_id} data_dir = "${data_dir}" diff --git a/examples/run-single-node.sh b/examples/run-single-node.sh index eaed961d..7a5d61aa 100755 --- a/examples/run-single-node.sh +++ b/examples/run-single-node.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Marmot v2.9.15-beta - Single Node Example +# Marmot v2.9.16-beta - Single Node Example # Simplest way to run Marmot set -e @@ -7,7 +7,7 @@ set -e SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" -echo "=== Marmot v2.9.15-beta Single Node ===" +echo "=== Marmot v2.9.16-beta Single Node ===" echo "" # Kill any existing marmot processes @@ -25,7 +25,7 @@ mkdir -p /tmp/marmot-single # Create config cat > /tmp/marmot-single/config.toml <<'TOML' -# Marmot v2.9.15-beta - Single Node Configuration (Optimized for Benchmarks) +# Marmot v2.9.16-beta - Single Node Configuration (Optimized for Benchmarks) node_id = 1 data_dir = "/tmp/marmot-single" diff --git a/examples/start-replica.sh b/examples/start-replica.sh index 2ea10f20..54ad7670 100755 --- a/examples/start-replica.sh +++ b/examples/start-replica.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Marmot v2.9.15-beta - Read-Only Replica with Transparent Failover +# Marmot v2.9.16-beta - Read-Only Replica with Transparent Failover # Streams from cluster nodes with automatic failover set -e @@ -29,7 +29,7 @@ generate_secret() { REPLICA_SECRET=${3:-${MARMOT_REPLICA_SECRET:-$(generate_secret)}} -echo "=== Marmot v2.9.15-beta Read-Only Replica ===" +echo "=== Marmot v2.9.16-beta Read-Only Replica ===" echo "Replica with transparent failover to cluster nodes" echo "" diff --git a/examples/wordpress-cluster/marmot-1-config.toml b/examples/wordpress-cluster/marmot-1-config.toml index 35295f43..750fb23d 100644 --- a/examples/wordpress-cluster/marmot-1-config.toml +++ b/examples/wordpress-cluster/marmot-1-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - WordPress Cluster Node 1 +# Marmot v2.9.16-beta - WordPress Cluster Node 1 node_id = 1 data_dir = "/data" @@ -8,6 +8,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "marmot-1:9181" diff --git a/examples/wordpress-cluster/marmot-2-config.toml b/examples/wordpress-cluster/marmot-2-config.toml index ae2b3ff4..794cc7bb 100644 --- a/examples/wordpress-cluster/marmot-2-config.toml +++ b/examples/wordpress-cluster/marmot-2-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - WordPress Cluster Node 2 +# Marmot v2.9.16-beta - WordPress Cluster Node 2 node_id = 2 data_dir = "/data" @@ -8,6 +8,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "marmot-2:9182" diff --git a/examples/wordpress-cluster/marmot-3-config.toml b/examples/wordpress-cluster/marmot-3-config.toml index 2f3512a7..9eec31be 100644 --- a/examples/wordpress-cluster/marmot-3-config.toml +++ b/examples/wordpress-cluster/marmot-3-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - WordPress Cluster Node 3 +# Marmot v2.9.16-beta - WordPress Cluster Node 3 node_id = 3 data_dir = "/data" @@ -8,6 +8,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "marmot-3:9183" diff --git a/examples/wordpress-cluster/run.sh b/examples/wordpress-cluster/run.sh index 8767b559..85f81ab6 100755 --- a/examples/wordpress-cluster/run.sh +++ b/examples/wordpress-cluster/run.sh @@ -15,7 +15,7 @@ NC='\033[0m' print_banner() { echo "" echo -e "${CYAN}╔══════════════════════════════════════════════════════════════╗${NC}" - echo -e "${CYAN}║${NC} ${BOLD}Marmot v2.9.15-beta + WordPress Cluster Demo${NC} ${CYAN}║${NC}" + echo -e "${CYAN}║${NC} ${BOLD}Marmot v2.9.16-beta + WordPress Cluster Demo${NC} ${CYAN}║${NC}" echo -e "${CYAN}║${NC} 3-Node Distributed SQLite powering 3 WordPress instances ${CYAN}║${NC}" echo -e "${CYAN}╚══════════════════════════════════════════════════════════════╝${NC}" echo "" @@ -54,7 +54,7 @@ start_stack() { echo -e "${YELLOW}[1/4]${NC} Cleaning up any existing containers..." docker compose down -v 2>/dev/null || true - echo -e "${YELLOW}[2/4]${NC} Building Marmot v2.9.15-beta image (this may take a few minutes on first run)..." + echo -e "${YELLOW}[2/4]${NC} Building Marmot v2.9.16-beta image (this may take a few minutes on first run)..." docker compose build echo -e "${YELLOW}[3/4]${NC} Starting services..." diff --git a/examples/wordpress/Dockerfile b/examples/wordpress/Dockerfile index 0c4600f9..d143f988 100644 --- a/examples/wordpress/Dockerfile +++ b/examples/wordpress/Dockerfile @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta Docker Image +# Marmot v2.9.16-beta Docker Image # Multi-stage build FROM golang:1.26-bookworm AS builder diff --git a/examples/wordpress/marmot-config.toml b/examples/wordpress/marmot-config.toml index 45e4f96c..75a0cb7f 100644 --- a/examples/wordpress/marmot-config.toml +++ b/examples/wordpress/marmot-config.toml @@ -1,4 +1,4 @@ -# Marmot v2.9.15-beta - WordPress Single Node Configuration +# Marmot v2.9.16-beta - WordPress Single Node Configuration node_id = 1 data_dir = "/data" @@ -8,6 +8,13 @@ heartbeat_timeout_seconds = 10 conflict_window_seconds = 10 lock_wait_timeout_seconds = 50 +# MySQL has no transactional DDL: CREATE/ALTER/DROP implicitly commit the open +# transaction, then run on their own. Keep true for MySQL-compatible behaviour so +# DML can see a schema change made earlier in the same transaction. +# Set false to keep DDL inside the transaction and replicate it atomically with +# the surrounding statements. +ddl_implicit_commit = true + [cluster] grpc_bind_address = "0.0.0.0" grpc_advertise_address = "marmot:8090" diff --git a/examples/wordpress/run.sh b/examples/wordpress/run.sh index 4145b6f9..0ef996c2 100755 --- a/examples/wordpress/run.sh +++ b/examples/wordpress/run.sh @@ -15,7 +15,7 @@ NC='\033[0m' print_banner() { echo "" echo -e "${CYAN}╔══════════════════════════════════════════════════════════════╗${NC}" - echo -e "${CYAN}║${NC} ${BOLD}Marmot v2.9.15-beta + WordPress Demo${NC} ${CYAN}║${NC}" + echo -e "${CYAN}║${NC} ${BOLD}Marmot v2.9.16-beta + WordPress Demo${NC} ${CYAN}║${NC}" echo -e "${CYAN}║${NC} Distributed SQLite powering WordPress ${CYAN}║${NC}" echo -e "${CYAN}╚══════════════════════════════════════════════════════════════╝${NC}" echo "" @@ -54,7 +54,7 @@ start_stack() { echo -e "${YELLOW}[1/4]${NC} Cleaning up any existing containers..." docker compose down -v 2>/dev/null || true - echo -e "${YELLOW}[2/4]${NC} Building Marmot v2.9.15-beta image (this may take a few minutes on first run)..." + echo -e "${YELLOW}[2/4]${NC} Building Marmot v2.9.16-beta image (this may take a few minutes on first run)..." docker compose build --quiet echo -e "${YELLOW}[3/4]${NC} Starting services..." @@ -137,7 +137,7 @@ print_success() { echo -e "${GREEN}╚══════════════════════════════════════════════════════════════╝${NC}" echo "" echo -e "${BOLD}Services Running:${NC}" - echo -e " ${CYAN}Marmot v2.9.15-beta${NC} - Distributed SQLite with MySQL protocol" + echo -e " ${CYAN}Marmot v2.9.16-beta${NC} - Distributed SQLite with MySQL protocol" echo -e " ${CYAN}WordPress${NC} - Connected to Marmot as its database backend" echo "" echo -e "${BOLD}Access Points:${NC}" @@ -153,7 +153,7 @@ print_success() { echo "" echo -e "${BOLD}Architecture:${NC}" echo -e " ┌─────────────┐ ┌──────────────────┐" - echo -e " │ WordPress │───>│ Marmot v2.9.15-beta │" + echo -e " │ WordPress │───>│ Marmot v2.9.16-beta │" echo -e " │ (port 8080)│ │ MySQL: 3316 │" echo -e " └─────────────┘ │ gRPC: 8090 │" echo -e " │ (internal only) │" diff --git a/grpc/forward_handler.go b/grpc/forward_handler.go index e23053b6..c275889b 100644 --- a/grpc/forward_handler.go +++ b/grpc/forward_handler.go @@ -29,6 +29,19 @@ type ForwardDBManager interface { // NewForwardHandler creates a new forward handler func NewForwardHandler(nodeID uint64, clock *hlc.Clock, sessionMgr *ForwardSessionManager, coordHandler *coordinator.CoordinatorHandler, dbManager ForwardDBManager) *ForwardHandler { + // A forward session can be evicted (idle timeout, or a replica dropping + // its StreamChanges connection) without the replica ever sending an + // explicit COMMIT/ROLLBACK for a transaction it started. Since forwarded + // transactions run through the same eager-execution pinning as a direct + // MySQL connection (see HandleForwardQuery below), an evicted session + // left mid-transaction would otherwise leak its pinned SQLite + // transaction and hold the writer locked forever. Wire eviction to the + // same CloseSession release path protocol/server.go uses on direct + // connection close. + if sessionMgr != nil && coordHandler != nil { + sessionMgr.SetSessionCloser(coordHandler.CloseSession) + } + return &ForwardHandler{ nodeID: nodeID, clock: clock, diff --git a/grpc/forward_session.go b/grpc/forward_session.go index 8d0aca21..e8357ad3 100644 --- a/grpc/forward_session.go +++ b/grpc/forward_session.go @@ -43,6 +43,15 @@ type ForwardSessionManager struct { mu sync.RWMutex sessionTimeout time.Duration stopCh chan struct{} + + // onSessionRemoved, if set, is invoked with a removed session's + // ConnSession whenever that session is evicted/removed from the manager + // (idle timeout, explicit removal, or replica disconnect) without going + // through an explicit COMMIT/ROLLBACK. This lets the coordinator release + // any pinned SQLite transaction (and its row locks/writer hold) that the + // session left open - see CoordinatorHandler.CloseSession, which mirrors + // the cleanup protocol/server.go performs on direct connection close. + onSessionRemoved func(*protocol.ConnectionSession) } // NewForwardSessionManager creates a new session manager and starts cleanup loop @@ -56,6 +65,16 @@ func NewForwardSessionManager(timeout time.Duration) *ForwardSessionManager { return m } +// SetSessionCloser registers a callback invoked with the ConnSession of every +// forward session removed from this manager, so any pinned transaction state +// held for that session's ConnID can be released. Must be called before the +// manager starts evicting sessions to avoid races with the cleanup loop. +func (m *ForwardSessionManager) SetSessionCloser(closer func(*protocol.ConnectionSession)) { + m.mu.Lock() + defer m.mu.Unlock() + m.onSessionRemoved = closer +} + // GetOrCreateSession retrieves an existing session or creates a new one func (m *ForwardSessionManager) GetOrCreateSession(key ForwardSessionKey, db string) *ForwardSession { m.mu.Lock() @@ -78,23 +97,39 @@ func (m *ForwardSessionManager) GetOrCreateSession(key ForwardSessionKey, db str return session } -// RemoveSession removes a specific session +// RemoveSession removes a specific session, closing any pinned transaction +// state left open on it. func (m *ForwardSessionManager) RemoveSession(key ForwardSessionKey) { m.mu.Lock() - defer m.mu.Unlock() - delete(m.sessions, key) + session, ok := m.sessions[key] + if ok { + delete(m.sessions, key) + } + closer := m.onSessionRemoved + m.mu.Unlock() + + if ok { + closeRemovedForwardSession(closer, session) + } } -// RemoveSessionsForReplica removes all sessions for a given replica node +// RemoveSessionsForReplica removes all sessions for a given replica node, +// closing any pinned transaction state left open on each of them. func (m *ForwardSessionManager) RemoveSessionsForReplica(replicaNodeID uint64) { m.mu.Lock() - defer m.mu.Unlock() - - for key := range m.sessions { + var removed []*ForwardSession + for key, session := range m.sessions { if key.ReplicaNodeID == replicaNodeID { + removed = append(removed, session) delete(m.sessions, key) } } + closer := m.onSessionRemoved + m.mu.Unlock() + + for _, session := range removed { + closeRemovedForwardSession(closer, session) + } } // startCleanupLoop runs periodic cleanup of expired sessions @@ -112,13 +147,13 @@ func (m *ForwardSessionManager) startCleanupLoop() { } } -// cleanupExpiredSessions removes sessions that have been inactive beyond timeout +// cleanupExpiredSessions removes sessions that have been inactive beyond +// timeout, closing any pinned transaction state left open on each of them. func (m *ForwardSessionManager) cleanupExpiredSessions() { m.mu.Lock() - defer m.mu.Unlock() now := time.Now() - expiredKeys := make([]ForwardSessionKey, 0) + expired := make([]*ForwardSession, 0) for key, session := range m.sessions { session.mu.Lock() @@ -126,20 +161,63 @@ func (m *ForwardSessionManager) cleanupExpiredSessions() { session.mu.Unlock() if now.Sub(lastActivity) > m.sessionTimeout { - expiredKeys = append(expiredKeys, key) + expired = append(expired, session) + delete(m.sessions, key) } } + closer := m.onSessionRemoved + m.mu.Unlock() - if len(expiredKeys) > 0 { - for _, key := range expiredKeys { - delete(m.sessions, key) + if len(expired) > 0 { + for _, session := range expired { + closeRemovedForwardSession(closer, session) } log.Debug(). - Int("count", len(expiredKeys)). + Int("count", len(expired)). Msg("cleaned up expired forward sessions") } } +// closeRemovedForwardSession invokes closer with session's ConnSession, if +// both are non-nil, so the coordinator can release any pinned SQLite +// transaction (and writer lock) the session left open. Runs outside the +// manager's lock since CloseSession may block on coordinator-side state. +// +// It holds session.execMu for the duration of the call, the same lock +// Execute holds for a whole forwarded statement (including COMMIT). Without +// this, eviction (idle timeout, explicit removal, or replica disconnect) can +// run concurrently with an in-flight COMMIT: the coordinator's CloseSession +// rolls back and discards the pinned transaction's captured CDC entries +// while handleCommit is still trying to read them, so the write is silently +// lost even though the client is told COMMIT succeeded. +// +// This cannot deadlock: closer (CoordinatorHandler.CloseSession) only takes +// coordinator-side pinned-transaction state and ends the session's +// transaction - it never calls back into the ForwardSessionManager or any +// ForwardSession (the coordinator package does not import grpc, so no such +// call path can exist). Nor is the manager's own mutex held while this runs +// - every caller (cleanupExpiredSessions, RemoveSession, +// RemoveSessionsForReplica) releases it before invoking +// closeRemovedForwardSession. So this can only block behind an in-flight +// Execute call on this same session, and always makes progress once that +// call returns. +func closeRemovedForwardSession(closer func(*protocol.ConnectionSession), session *ForwardSession) { + if closer == nil || session == nil { + return + } + + session.execMu.Lock() + defer session.execMu.Unlock() + + session.mu.Lock() + connSession := session.ConnSession + session.mu.Unlock() + + if connSession != nil { + closer(connSession) + } +} + // Stop signals the cleanup loop to stop func (m *ForwardSessionManager) Stop() { close(m.stopCh) diff --git a/grpc/forward_session_test.go b/grpc/forward_session_test.go index 315f7140..58b46210 100644 --- a/grpc/forward_session_test.go +++ b/grpc/forward_session_test.go @@ -2,6 +2,7 @@ package grpc import ( "context" + "sync/atomic" "testing" "time" @@ -146,6 +147,107 @@ func TestForwardSession_Touch(t *testing.T) { assert.True(t, session.LastActivity.After(oldTime)) } +// TestCloseRemovedForwardSession_InvokesCloserWhenIdle is a regression guard +// for the ordinary (non-racing) eviction path: a session with no in-flight +// Execute call must still have its closer invoked, so the execMu fix below +// does not accidentally turn eviction into a no-op. +func TestCloseRemovedForwardSession_InvokesCloserWhenIdle(t *testing.T) { + mgr := NewForwardSessionManager(time.Hour) + defer mgr.Stop() + + var closed atomic.Bool + mgr.SetSessionCloser(func(cs *protocol.ConnectionSession) { + closed.Store(true) + }) + + key := ForwardSessionKey{ReplicaNodeID: 1, SessionID: 200} + mgr.GetOrCreateSession(key, "testdb") + + mgr.RemoveSession(key) + + assert.True(t, closed.Load(), "RemoveSession must invoke the session closer for an idle session") +} + +// TestCloseRemovedForwardSession_SerializesAgainstInFlightExecute reproduces +// the forward-session eviction race: cleanupExpiredSessions/RemoveSession/ +// RemoveSessionsForReplica all reach closeRemovedForwardSession, which calls +// the coordinator's CloseSession to roll back and discard any pinned SQLite +// transaction (and its captured CDC entries) the session left open. Before +// the fix, that call was not synchronized against Execute's execMu at all, +// so eviction could run concurrently with an in-flight COMMIT: if eviction's +// CloseSession wins the race for the pinned state, the COMMIT sees no +// pinned state and no buffered statements and takes the "empty transaction" +// fast path, silently reporting success for a write that was never applied. +// A COMMIT merely being slow (lock contention, a busy 2PC round) is enough +// to make this reachable, since ForwardSession.LastActivity is set once at +// the start of Execute and does not move again until the call returns - see +// closeRemovedForwardSession's doc comment for the full deadlock analysis of +// the fix (taking session.execMu before invoking the closer). +// +// This test forces the interleaving deterministically with a fake closer and +// channels instead of sleeps/timing: it fails before the fix (the closer +// runs while Execute's callback is still parked mid-flight, simulating a +// slow COMMIT) and passes after (eviction blocks until Execute returns). +func TestCloseRemovedForwardSession_SerializesAgainstInFlightExecute(t *testing.T) { + mgr := NewForwardSessionManager(time.Hour) + defer mgr.Stop() + + execEntered := make(chan struct{}) + releaseExec := make(chan struct{}) + var closerRanDuringExecute atomic.Bool + + mgr.SetSessionCloser(func(*protocol.ConnectionSession) { + select { + case <-releaseExec: + // Execute's callback had already returned when the closer ran - + // correctly serialized after it. + default: + // Execute's callback is still parked mid-flight (simulated slow + // COMMIT): the closer ran concurrently with it. + closerRanDuringExecute.Store(true) + } + }) + + key := ForwardSessionKey{ReplicaNodeID: 1, SessionID: 300} + session := mgr.GetOrCreateSession(key, "testdb") + + execDone := make(chan struct{}) + go func() { + defer close(execDone) + _, _ = session.Execute("testdb", func(cs *protocol.ConnectionSession) (*ForwardQueryResponse, error) { + close(execEntered) + <-releaseExec // held open to simulate a slow in-flight COMMIT + return &ForwardQueryResponse{Success: true}, nil + }) + }() + + <-execEntered // Execute now holds execMu and is mid-flight + + evictDone := make(chan struct{}) + go func() { + mgr.RemoveSession(key) + close(evictDone) + }() + + // Bounded window for an unsynchronized eviction to run to completion. + // This does not gate correctness of the assertion below: releaseExec is + // not closed until after this window, so if eviction is unsynchronized + // and races ahead, its closer call is guaranteed to observe + // releaseExec still open and record the race regardless of exactly how + // much of the window it needed. + select { + case <-evictDone: + case <-time.After(100 * time.Millisecond): + } + + close(releaseExec) + <-execDone + <-evictDone + + assert.False(t, closerRanDuringExecute.Load(), + "session eviction must not invoke the session closer while an Execute call is still in flight on that session") +} + func TestForwardSessionManager_SessionTimeout(t *testing.T) { mgr := NewForwardSessionManager(50 * time.Millisecond) defer mgr.Stop() diff --git a/marmot.go b/marmot.go index 3e139a17..a0fd736b 100644 --- a/marmot.go +++ b/marmot.go @@ -119,7 +119,7 @@ func main() { // Branch based on operating mode if cfg.IsReplicaMode() { - log.Info().Msg("Marmot v2.9.15-beta - Read-Only Replica Mode") + log.Info().Msg("Marmot v2.9.16-beta - Read-Only Replica Mode") log.Info(). Strs("follow_addresses", cfg.Config.Replica.FollowAddresses). Msg("Following cluster nodes") @@ -135,7 +135,7 @@ func main() { } // Cluster mode - log.Info().Msg("Marmot v2.9.15-beta - Leaderless SQLite Replication") + log.Info().Msg("Marmot v2.9.16-beta - Leaderless SQLite Replication") // Warn if cluster authentication is not configured if !cfg.IsClusterAuthEnabled() { @@ -547,7 +547,7 @@ func main() { log.Info().Msg("Seed node fully initialized - now ALIVE") } - log.Info().Msg("Marmot v2.9.15-beta started successfully") + log.Info().Msg("Marmot v2.9.16-beta started successfully") log.Info(). Uint64("node_id", cfg.Config.NodeID). Int("grpc_port", cfg.Config.Cluster.GRPCPort). diff --git a/protocol/merge_exec_params_test.go b/protocol/merge_exec_params_test.go new file mode 100644 index 00000000..f32c15eb --- /dev/null +++ b/protocol/merge_exec_params_test.go @@ -0,0 +1,91 @@ +//go:build sqlite_preupdate_hook +// +build sqlite_preupdate_hook + +package protocol + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestMergeExecParams_StaleParamOrderMisinterleavesResolvedArgs reproduces +// the coordinator/vec_handler.go regression: a rewritten statement (e.g. a +// vector-search fallback/primary query) copied from an original prepared +// statement that itself mixed wire params with pipeline-extracted literals. +// The rewrite's args are already fully resolved and positional for the NEW +// SQL, but the copy - built by hand as `fb := stmt; fb.SQL = ...; +// fb.ExtractedParams = args` - kept the ORIGINAL statement's ParamOrder, +// which describes the ORIGINAL SQL's placeholder layout, not the rewrite's. +// Passing args as both wireParams and ExtractedParams (as the caller was +// forced to do, having only one resolved list) then makes MergeExecParams +// interleave args against itself instead of passing it through untouched. +func TestMergeExecParams_StaleParamOrderMisinterleavesResolvedArgs(t *testing.T) { + // Original prepared statement: one wire placeholder, one pipeline-extracted + // literal, in that order (mirrors a vec_match(...) query with a bound + // query vector alongside a literal WHERE clause the pipeline extracted). + original := Statement{ + SQL: "SELECT * FROM t WHERE vec_match(embedding, ?, 10) AND status = 'active'", + ExtractedParams: []interface{}{"active"}, + ParamOrder: []bool{true, false}, + } + + // The rewrite resolves everything (including the vector search result) + // into a single positional args list for a completely different query. + args := []interface{}{"resolved-a", "resolved-b"} + + // Pre-fix pattern: hand-copy the statement, replace SQL and + // ExtractedParams, but leave the stale ParamOrder in place. + stale := original + stale.SQL = "SELECT * FROM t WHERE id IN (?, ?)" + stale.ExtractedParams = args + + merged := stale.MergeExecParams(args) + + // BUG: [true, false] pulls args[0] for the wire slot, then + // stale.ExtractedParams[0] (== args[0] again, not args[1]) for the + // extracted slot - silently duplicating args[0] and dropping args[1] + // instead of passing args through untouched. + require.NotEqual(t, args, merged, + "demonstrates the stale-ParamOrder bug this test guards against: "+ + "a rewritten statement must not reuse the original's ParamOrder") + require.Equal(t, []interface{}{"resolved-a", "resolved-a"}, merged) +} + +// TestStatement_WithResolvedParams_FixesStaleParamOrder is the fix: building +// the rewritten copy via WithResolvedParams clears ParamOrder, so +// MergeExecParams falls back to the single-source path and passes the +// rewrite's already-resolved args straight through. +func TestStatement_WithResolvedParams_FixesStaleParamOrder(t *testing.T) { + original := Statement{ + SQL: "SELECT * FROM t WHERE vec_match(embedding, ?, 10) AND status = 'active'", + ExtractedParams: []interface{}{"active"}, + ParamOrder: []bool{true, false}, + } + args := []interface{}{"resolved-a", "resolved-b"} + + rewritten := original.WithResolvedParams("SELECT * FROM t WHERE id IN (?, ?)", args) + + require.Equal(t, "SELECT * FROM t WHERE id IN (?, ?)", rewritten.SQL) + require.Nil(t, rewritten.ParamOrder, "rewrite must not carry the original statement's placeholder layout") + require.Equal(t, args, rewritten.ExtractedParams) + + merged := rewritten.MergeExecParams(args) + require.Equal(t, args, merged, "resolved args must pass through untouched, not interleave with themselves") +} + +// TestStatement_WithResolvedParams_LeavesOriginalUntouched guards against a +// value-receiver mistake that mutates the caller's statement. +func TestStatement_WithResolvedParams_LeavesOriginalUntouched(t *testing.T) { + original := Statement{ + SQL: "SELECT 1", + ExtractedParams: []interface{}{"active"}, + ParamOrder: []bool{true, false}, + } + + _ = original.WithResolvedParams("SELECT 2", []interface{}{"x", "y"}) + + require.Equal(t, "SELECT 1", original.SQL) + require.Equal(t, []interface{}{"active"}, original.ExtractedParams) + require.Equal(t, []bool{true, false}, original.ParamOrder) +} diff --git a/protocol/parser.go b/protocol/parser.go index 21f6c279..5d7c5857 100644 --- a/protocol/parser.go +++ b/protocol/parser.go @@ -155,11 +155,13 @@ func ParseStatementWithOptions(sql string, opts ParseOptions) Statement { // Extract transpiled SQL from first statement transpiledSQL := "" var extractedParams []interface{} + var paramOrder []bool if len(ctx.Output.Statements) > 0 { transpiledSQL = ctx.Output.Statements[0].SQL if len(ctx.Output.Statements[0].Params) > 0 { extractedParams = ctx.Output.Statements[0].Params } + paramOrder = ctx.Output.Statements[0].ParamOrder } stmt := Statement{ @@ -168,6 +170,7 @@ func ParseStatementWithOptions(sql string, opts ParseOptions) Statement { Database: ctx.Output.Database, Error: errorString(ctx.Output.ValidationErr), ExtractedParams: extractedParams, + ParamOrder: paramOrder, } // Extract MySQL-specific metadata (if available) @@ -201,11 +204,13 @@ func ParseStatementWithSchema(sql string, schemaLookup SchemaLookupFunc) Stateme // Extract transpiled SQL from first statement transpiledSQL := "" var extractedParams []interface{} + var paramOrder []bool if len(ctx.Output.Statements) > 0 { transpiledSQL = ctx.Output.Statements[0].SQL if len(ctx.Output.Statements[0].Params) > 0 { extractedParams = ctx.Output.Statements[0].Params } + paramOrder = ctx.Output.Statements[0].ParamOrder } stmt := Statement{ @@ -214,6 +219,7 @@ func ParseStatementWithSchema(sql string, schemaLookup SchemaLookupFunc) Stateme Database: ctx.Output.Database, Error: errorString(ctx.Output.ValidationErr), ExtractedParams: extractedParams, + ParamOrder: paramOrder, } // Extract MySQL-specific metadata (if available) @@ -265,6 +271,7 @@ func buildStatement(ctx query.QueryContext, ts query.TranspiledStatement) Statem Database: ctx.Output.Database, Error: errorString(ctx.Output.ValidationErr), ExtractedParams: extractedParams, + ParamOrder: ts.ParamOrder, } // Extract MySQL-specific metadata (if available) diff --git a/protocol/prepared_stmt_test.go b/protocol/prepared_stmt_test.go index cf5b113f..a1e4b19e 100644 --- a/protocol/prepared_stmt_test.go +++ b/protocol/prepared_stmt_test.go @@ -9,7 +9,7 @@ import ( func TestParseParamValue_TINY(t *testing.T) { // MYSQL_TYPE_TINY = 0x01 payload := []byte{42} - offset, val, err := parseParamValue(payload, 0, 0x01) + offset, val, err := parseParamValue(payload, 0, 0x01, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -25,7 +25,7 @@ func TestParseParamValue_SHORT(t *testing.T) { // MYSQL_TYPE_SHORT = 0x02 payload := make([]byte, 2) binary.LittleEndian.PutUint16(payload, 1234) - offset, val, err := parseParamValue(payload, 0, 0x02) + offset, val, err := parseParamValue(payload, 0, 0x02, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -41,7 +41,7 @@ func TestParseParamValue_LONG(t *testing.T) { // MYSQL_TYPE_LONG = 0x03 payload := make([]byte, 4) binary.LittleEndian.PutUint32(payload, 123456) - offset, val, err := parseParamValue(payload, 0, 0x03) + offset, val, err := parseParamValue(payload, 0, 0x03, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -57,7 +57,7 @@ func TestParseParamValue_LONGLONG(t *testing.T) { // MYSQL_TYPE_LONGLONG = 0x08 payload := make([]byte, 8) binary.LittleEndian.PutUint64(payload, 9876543210) - offset, val, err := parseParamValue(payload, 0, 0x08) + offset, val, err := parseParamValue(payload, 0, 0x08, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -73,7 +73,7 @@ func TestParseParamValue_FLOAT(t *testing.T) { // MYSQL_TYPE_FLOAT = 0x04 payload := make([]byte, 4) binary.LittleEndian.PutUint32(payload, math.Float32bits(3.14)) - offset, val, err := parseParamValue(payload, 0, 0x04) + offset, val, err := parseParamValue(payload, 0, 0x04, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -93,7 +93,7 @@ func TestParseParamValue_DOUBLE(t *testing.T) { // MYSQL_TYPE_DOUBLE = 0x05 payload := make([]byte, 8) binary.LittleEndian.PutUint64(payload, math.Float64bits(3.14159265)) - offset, val, err := parseParamValue(payload, 0, 0x05) + offset, val, err := parseParamValue(payload, 0, 0x05, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -116,7 +116,7 @@ func TestParseParamValue_STRING(t *testing.T) { payload[0] = byte(len(str)) // Length prefix copy(payload[1:], str) - offset, val, err := parseParamValue(payload, 0, 0xFD) + offset, val, err := parseParamValue(payload, 0, 0xFD, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -139,7 +139,7 @@ func TestParseParamValue_STRING(t *testing.T) { func TestParseParamValue_NULL(t *testing.T) { // MYSQL_TYPE_NULL = 0x06 - offset, val, err := parseParamValue([]byte{}, 0, 0x06) + offset, val, err := parseParamValue([]byte{}, 0, 0x06, false) if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -207,7 +207,7 @@ func BenchmarkParseParamValue_LONGLONG(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - _, _, _ = parseParamValue(payload, 0, 0x08) + _, _, _ = parseParamValue(payload, 0, 0x08, false) } } diff --git a/protocol/query/context.go b/protocol/query/context.go index 24685d60..87f3ab97 100644 --- a/protocol/query/context.go +++ b/protocol/query/context.go @@ -112,6 +112,11 @@ type QueryOutput struct { type TranspiledStatement struct { SQL string Params []interface{} + // ParamOrder is set only when SQL mixes the caller's own bind + // placeholders with values the pipeline itself extracted (e.g. a + // server-injected auto-increment id) - see transform.ExtractLiterals and + // protocol.MergeExecParams. nil means Params is the only source needed. + ParamOrder []bool } // MySQLParseState holds MySQL-specific parsing state and metadata. @@ -143,6 +148,10 @@ type MySQLParseState struct { // SkipVitess signals that the statement was fully classified by pattern // and must not be passed to Vitess (e.g. vector index DDL, DROP INDEX). SkipVitess bool + // TranspiledSQL holds SQLite-syntax SQL built directly by the pattern classifier for + // statements Vitess cannot parse at all (SkipVitess == true). When set, the pipeline + // uses it instead of forwarding the original MySQL SQL unchanged. + TranspiledSQL string } // QueryContext holds all state for processing a single query through the pipeline. diff --git a/protocol/query/parser.go b/protocol/query/parser.go index b20d5354..11b6ef9b 100644 --- a/protocol/query/parser.go +++ b/protocol/query/parser.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" + "github.com/maxpert/marmot/protocol/query/transform" "vitess.io/vitess/go/vt/sqlparser" ) @@ -36,6 +37,20 @@ var ( // DDL patterns Vitess cannot parse dropIndexPattern = regexp.MustCompile(`(?i)^\s*DROP\s+INDEX\s+`) + // dropIndexIdent matches a MySQL identifier as either backtick-quoted (any character + // except a backtick, with `` as an escaped literal backtick - MySQL's own identifier + // quoting rule, e.g. `unique-user-email` or `has space`) or, unquoted, the strict + // [A-Za-z_][A-Za-z0-9_]* charset. This mirrors what AlterTableConstraintRule accepts + // for the equivalent ADD CONSTRAINT/ADD INDEX identifiers on the CREATE side. + dropIndexIdent = "(?:`(?:[^`]|``)*`|[A-Za-z_][A-Za-z0-9_]*)" + // dropIndexExtractPattern pulls the IF EXISTS flag and index name out of MySQL's + // "DROP INDEX [IF EXISTS] name ON table" - SQLite has no ON clause, so it must be + // stripped rather than forwarded as-is (SQLite's DROP INDEX rejects it outright). + // Group 1: "IF EXISTS " if present. Group 2: backtick-quoted name body (raw, with + // doubled backticks still escaped). Group 3: unquoted name. Exactly one of 2/3 matches. + dropIndexExtractPattern = regexp.MustCompile(`(?i)^\s*DROP\s+INDEX\s+(IF\s+EXISTS\s+)?` + + "(?:`((?:[^`]|``)*)`|([A-Za-z_][A-Za-z0-9_]*))" + + `\s+ON\s+` + dropIndexIdent) // Vector index DDL patterns (sqlite-vec extension, not parsed by Vitess) createVectorIndexPattern = regexp.MustCompile(`(?i)^\s*CREATE\s+VECTOR\s+INDEX\s+`) @@ -77,6 +92,19 @@ func (p *VitessParser) Parse(ctx *QueryContext) error { return err } + // Vitess's DDL fallback path returns a syntax error alongside a partially-parsed + // AST (SetFullyParsed(false)) instead of failing outright - see vitess sqlparser.Parse2. + // Forwarding that AST would silently truncate the statement (e.g. an unparseable + // "ALTER TABLE t ADD CONSTRAINT ..." degrading to "ALTER TABLE t"), which then fails + // downstream with a confusing SQLite error instead of a clean MySQL syntax error. + // Treat it as a parse failure so the caller returns a proper error to the client. + if ddl, ok := stmt.(sqlparser.DDLStatement); ok && !ddl.IsFullyParsed() { + return fmt.Errorf("syntax error in DDL statement: %s", ctx.Input.SQL) + } + if dbddl, ok := stmt.(sqlparser.DBDDLStatement); ok && !dbddl.IsFullyParsed() { + return fmt.Errorf("syntax error in DDL statement: %s", ctx.Input.SQL) + } + ctx.MySQLState.AST = stmt classifyStatement(ctx, stmt) extractMetadata(ctx, stmt) @@ -171,10 +199,35 @@ func classifyByPattern(ctx *QueryContext) { if dropIndexPattern.MatchString(sql) { ctx.Output.StatementType = StatementDDL ctx.MySQLState.SkipVitess = true + if idx := dropIndexExtractPattern.FindStringSubmatchIndex(sql); idx != nil { + ifExists := idx[2] != -1 + var name string + if idx[4] != -1 { + // Backtick-quoted: unescape MySQL's doubled-backtick literal-backtick rule. + name = strings.ReplaceAll(sql[idx[4]:idx[5]], "``", "`") + } else { + name = sql[idx[6]:idx[7]] + } + ctx.MySQLState.TranspiledSQL = buildDropIndexSQL(name, ifExists) + } return } } +// buildDropIndexSQL generates SQLite's "DROP INDEX [IF EXISTS] "name"" from a MySQL +// "DROP INDEX name ON table" statement. SQLite's DROP INDEX has no ON clause. The name is +// double-quoted (transform.QuoteIdentifier) since MySQL identifiers may contain characters +// (hyphens, spaces, embedded double quotes) that are not valid in an unquoted SQLite identifier. +func buildDropIndexSQL(indexName string, ifExists bool) string { + var sb strings.Builder + sb.WriteString("DROP INDEX ") + if ifExists { + sb.WriteString("IF EXISTS ") + } + sb.WriteString(transform.QuoteIdentifier(indexName)) + return sb.String() +} + func parseVectorWithClause(state *MySQLParseState, clause string) { kvPattern := regexp.MustCompile(`(?i)(\w+)\s*=\s*'?([^',)]+)'?`) for _, m := range kvPattern.FindAllStringSubmatch(clause, -1) { diff --git a/protocol/query/pipeline.go b/protocol/query/pipeline.go index a9d92db6..abc10f54 100644 --- a/protocol/query/pipeline.go +++ b/protocol/query/pipeline.go @@ -67,11 +67,17 @@ func (p *Pipeline) Process(ctx *QueryContext) error { return err } - // If the parser short-circuited (no AST produced), pass SQL through unchanged. - // This happens for statements like CREATE/DROP VECTOR INDEX that are classified - // by pattern but not understood by Vitess. + // If the parser short-circuited (no AST produced), the statement was classified by + // pattern rather than parsed by Vitess (e.g. CREATE/DROP VECTOR INDEX, DROP INDEX). + // Use the SQLite SQL the classifier built, if any; otherwise pass the SQL through + // unchanged (e.g. vector index DDL, which downstream code handles via extracted + // metadata rather than by executing the raw SQL against SQLite). if ctx.MySQLState != nil && ctx.MySQLState.AST == nil { - ctx.Output.Statements = []TranspiledStatement{{SQL: ctx.Input.SQL, Params: ctx.Input.Parameters}} + sql := ctx.Input.SQL + if ctx.MySQLState.TranspiledSQL != "" { + sql = ctx.MySQLState.TranspiledSQL + } + ctx.Output.Statements = []TranspiledStatement{{SQL: sql, Params: ctx.Input.Parameters}} return nil } diff --git a/protocol/query/pipeline_ddl_having_test.go b/protocol/query/pipeline_ddl_having_test.go new file mode 100644 index 00000000..0806626c --- /dev/null +++ b/protocol/query/pipeline_ddl_having_test.go @@ -0,0 +1,156 @@ +package query + +import ( + "database/sql" + "testing" + + _ "github.com/mattn/go-sqlite3" +) + +// TestPipeline_PartialDDLRejected pins the LLDAP 0.6.3 regression: Vitess's DDL +// fallback path ignores a syntax error and returns a partially-parsed AST +// (e.g. "ALTER TABLE t ADD CONSTRAINT unique-user-email UNIQUE (email)" +// degrades to just "ALTER TABLE t" because an unquoted identifier cannot +// contain a hyphen). Forwarding that AST would silently truncate the +// statement; the pipeline must instead report a parse failure. +func TestPipeline_PartialDDLRejected(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + sql := "alter table users add CONSTRAINT unique-user-email UNIQUE (email)" + ctx := NewContext(sql, nil) + if err := pipeline.Process(ctx); err == nil { + t.Fatalf("expected pipeline.Process to fail on partially-parsed DDL, got statements: %+v", ctx.Output.Statements) + } + if ctx.Output.IsValid { + t.Errorf("ctx.Output.IsValid must be false when parsing failed") + } +} + +// TestPipeline_WellFormedDDLStillParses is the control for the above: the same +// statement with the constraint name properly backtick-quoted (valid MySQL +// syntax) must parse and transpile normally, not be rejected. +func TestPipeline_WellFormedDDLStillParses(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + sql := "alter table users add CONSTRAINT `unique-user-email` UNIQUE (email)" + ctx := NewContext(sql, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed on well-formed DDL: %v", err) + } + want := `CREATE UNIQUE INDEX "unique-user-email" ON "users" ("email")` + if len(ctx.Output.Statements) != 1 || ctx.Output.Statements[0].SQL != want { + t.Fatalf("got statements %+v, want single statement %q", ctx.Output.Statements, want) + } +} + +// TestPipeline_AlterTableColumnRegressions locks in that plain ADD/DROP/RENAME +// COLUMN still pass through unchanged after introducing AlterTableConstraintRule. +func TestPipeline_AlterTableColumnRegressions(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + tests := []struct { + sql string + want string + }{ + {"ALTER TABLE users ADD COLUMN age INT", "alter table users add column age INT"}, + {"ALTER TABLE users DROP COLUMN age", "alter table users drop column age"}, + {"ALTER TABLE users RENAME COLUMN age TO years", "alter table users rename column age to years"}, + } + + for _, tt := range tests { + t.Run(tt.sql, func(t *testing.T) { + ctx := NewContext(tt.sql, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 1 || ctx.Output.Statements[0].SQL != tt.want { + t.Fatalf("got %+v, want single statement %q", ctx.Output.Statements, tt.want) + } + }) + } +} + +// TestPipeline_SubqueryHavingExecutesInSQLite is the exact LLDAP 0.6.3 +// COM_STMT_PREPARE statement that used to be corrupted by transpilation +// (HAVING inside the subquery was rewritten as WHERE, and SQLite's PREPARE +// rejected the result with "near \"WHERE\": syntax error"). This verifies the +// transpiled SQL both preserves the "?" placeholder and actually executes +// against a real SQLite database. +func TestPipeline_SubqueryHavingExecutesInSQLite(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + mysqlSQL := "SELECT `email`, `user_id` FROM `users` WHERE `email` IN " + + "(SELECT `email` FROM `users` GROUP BY `email` HAVING COUNT(`email`) > ?) " + + "ORDER BY `email` ASC, `user_id` ASC" + + ctx := NewContext(mysqlSQL, nil) + ctx.ExtractLiterals = true + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 1 { + t.Fatalf("expected 1 statement, got %d: %+v", len(ctx.Output.Statements), ctx.Output.Statements) + } + transpiled := ctx.Output.Statements[0].SQL + + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + defer db.Close() + + if _, err := db.Exec("CREATE TABLE users (email TEXT, user_id INTEGER)"); err != nil { + t.Fatalf("failed to create table: %v", err) + } + rows := []struct { + email string + userID int + }{ + {"dup@example.com", 1}, + {"dup@example.com", 2}, + {"unique@example.com", 3}, + } + for _, r := range rows { + if _, err := db.Exec("INSERT INTO users (email, user_id) VALUES (?, ?)", r.email, r.userID); err != nil { + t.Fatalf("failed to insert: %v", err) + } + } + + result, err := db.Query(transpiled, 1) + if err != nil { + t.Fatalf("transpiled SQL failed to execute: %v\nSQL: %s", err, transpiled) + } + defer result.Close() + + var got []int + for result.Next() { + var email string + var userID int + if err := result.Scan(&email, &userID); err != nil { + t.Fatalf("scan failed: %v", err) + } + if email != "dup@example.com" { + t.Errorf("unexpected email %q in results", email) + } + got = append(got, userID) + } + if len(got) != 2 || got[0] != 1 || got[1] != 2 { + t.Errorf("expected user_id [1 2] for the duplicated email, got %v", got) + } +} diff --git a/protocol/query/pipeline_index_ddl_test.go b/protocol/query/pipeline_index_ddl_test.go new file mode 100644 index 00000000..3b4d038b --- /dev/null +++ b/protocol/query/pipeline_index_ddl_test.go @@ -0,0 +1,265 @@ +package query + +import ( + "database/sql" + "testing" + + _ "github.com/mattn/go-sqlite3" + "github.com/maxpert/marmot/protocol/query/transform" +) + +// TestPipeline_CreateIndexTranspiles pins the LLDAP migration regression: standalone +// "CREATE [UNIQUE] INDEX name ON table (cols)" parses in Vitess to the exact same +// *AlterTable/AddIndexDefinition AST shape as "ALTER TABLE t ADD INDEX ..." (see +// create_index_prefix in vitess's sql.y), so without AlterTableConstraintRule handling +// plain (non-unique) index adds too, it fell through to the default serializer and printed +// invalid "ALTER TABLE t ADD INDEX ..." SQL that SQLite's PREPARE step rejects. Verifies +// both the transpiled SQL text and that it actually executes against real SQLite. +func TestPipeline_CreateIndexTranspiles(t *testing.T) { + tests := []struct { + name string + sql string + want string + table string + }{ + { + name: "plain index, unquoted, single column", + sql: "CREATE INDEX idx_email ON users (email)", + want: `CREATE INDEX "idx_email" ON "users" ("email")`, + table: "CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT, name TEXT)", + }, + { + name: "plain index, backtick-quoted, multi-column", + sql: "CREATE INDEX `idx_name_email` ON `users` (`name`, `email`)", + want: `CREATE INDEX "idx_name_email" ON "users" ("name", "email")`, + table: "CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT, name TEXT)", + }, + { + name: "unique index, unquoted, single column", + sql: "CREATE UNIQUE INDEX idx_email ON users (email)", + want: `CREATE UNIQUE INDEX "idx_email" ON "users" ("email")`, + table: "CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT, name TEXT)", + }, + { + name: "unique index, backtick-quoted, multi-column", + sql: "CREATE UNIQUE INDEX `idx_name_email` ON `users` (`name`, `email`)", + want: `CREATE UNIQUE INDEX "idx_name_email" ON "users" ("name", "email")`, + table: "CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT, name TEXT)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + ctx := NewContext(tt.sql, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 1 || ctx.Output.Statements[0].SQL != tt.want { + t.Fatalf("got statements %+v, want single statement %q", ctx.Output.Statements, tt.want) + } + + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + defer db.Close() + + if _, err := db.Exec(tt.table); err != nil { + t.Fatalf("failed to create table: %v", err) + } + if _, err := db.Exec(ctx.Output.Statements[0].SQL); err != nil { + t.Fatalf("transpiled SQL failed to execute: %v\nSQL: %s", err, ctx.Output.Statements[0].SQL) + } + }) + } +} + +// TestPipeline_DropIndexTranspiles pins the second LLDAP migration regression: MySQL's +// "DROP INDEX idx ON table" was classified as Vitess-incompatible (SkipVitess) and passed +// through completely untranspiled, but SQLite's DROP INDEX has no ON clause and rejects it. +func TestPipeline_DropIndexTranspiles(t *testing.T) { + tests := []struct { + name string + sql string + want string + createIndexOn string // the pre-existing SQLite index name the DROP must actually hit + }{ + { + name: "plain, unquoted", + sql: "DROP INDEX idx_email ON users", + want: `DROP INDEX "idx_email"`, + createIndexOn: "idx_email", + }, + { + name: "IF EXISTS preserved", + sql: "DROP INDEX IF EXISTS idx_email ON users", + want: `DROP INDEX IF EXISTS "idx_email"`, + createIndexOn: "idx_email", + }, + { + name: "backtick-quoted names", + sql: "DROP INDEX `idx_email` ON `users`", + want: `DROP INDEX "idx_email"`, + createIndexOn: "idx_email", + }, + { + name: "backtick-quoted hyphenated name", + sql: "DROP INDEX `unique-user-email` ON `users`", + want: `DROP INDEX "unique-user-email"`, + createIndexOn: "unique-user-email", + }, + { + name: "backtick-quoted name with spaces", + sql: "DROP INDEX `user email idx` ON `users`", + want: `DROP INDEX "user email idx"`, + createIndexOn: "user email idx", + }, + { + name: "IF EXISTS with backtick-quoted hyphenated name", + sql: "DROP INDEX IF EXISTS `unique-user-email` ON `users`", + want: `DROP INDEX IF EXISTS "unique-user-email"`, + createIndexOn: "unique-user-email", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + ctx := NewContext(tt.sql, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 1 || ctx.Output.Statements[0].SQL != tt.want { + t.Fatalf("got statements %+v, want single statement %q", ctx.Output.Statements, tt.want) + } + + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + defer db.Close() + + if _, err := db.Exec("CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT)"); err != nil { + t.Fatalf("failed to create table: %v", err) + } + createIndexSQL := `CREATE INDEX ` + transform.QuoteIdentifier(tt.createIndexOn) + ` ON users (email)` + if _, err := db.Exec(createIndexSQL); err != nil { + t.Fatalf("failed to create index: %v", err) + } + if _, err := db.Exec(ctx.Output.Statements[0].SQL); err != nil { + t.Fatalf("transpiled SQL failed to execute: %v\nSQL: %s", err, ctx.Output.Statements[0].SQL) + } + }) + } +} + +// TestPipeline_AlterAddColumnCharsetStripped pins the third LLDAP migration regression: +// ALTER TABLE ADD COLUMN with a MySQL CHARACTER SET/COLLATE clause has no SQLite +// equivalent - CreateTableRule already stripped these for CREATE TABLE columns, but ALTER +// TABLE ADD/MODIFY/CHANGE COLUMN went through the default serializer unstripped and SQLite's +// PREPARE step rejected the CHARACTER SET clause. +func TestPipeline_AlterAddColumnCharsetStripped(t *testing.T) { + tests := []struct { + name string + sql string + want string + }{ + { + name: "CHARACTER SET and COLLATE", + sql: "ALTER TABLE users ADD COLUMN bio VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", + want: "alter table users add column bio VARCHAR(255)", + }, + { + name: "COLLATE only", + sql: "ALTER TABLE users ADD COLUMN bio VARCHAR(255) COLLATE utf8mb4_unicode_ci", + want: "alter table users add column bio VARCHAR(255)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + ctx := NewContext(tt.sql, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 1 || ctx.Output.Statements[0].SQL != tt.want { + t.Fatalf("got statements %+v, want single statement %q", ctx.Output.Statements, tt.want) + } + + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + defer db.Close() + + if _, err := db.Exec("CREATE TABLE users (id INTEGER PRIMARY KEY)"); err != nil { + t.Fatalf("failed to create table: %v", err) + } + if _, err := db.Exec(ctx.Output.Statements[0].SQL); err != nil { + t.Fatalf("transpiled SQL failed to execute: %v\nSQL: %s", err, ctx.Output.Statements[0].SQL) + } + }) + } +} + +// TestPipeline_AlterAddColumnUniqueCombinedWithCharset verifies AlterTableColumnTypeRule +// (priority 8) runs before AlterTableConstraintRule (priority 10) so a combined ADD COLUMN +// (with charset) + ADD CONSTRAINT UNIQUE statement gets both fixes applied together. +func TestPipeline_AlterAddColumnUniqueCombinedWithCharset(t *testing.T) { + pipeline, err := NewPipeline(100, nil) + if err != nil { + t.Fatalf("Failed to create pipeline: %v", err) + } + defer pipeline.Close() + + sqlText := "ALTER TABLE users ADD COLUMN bio VARCHAR(255) CHARACTER SET utf8mb4, " + + "ADD CONSTRAINT uq_bio UNIQUE (bio)" + ctx := NewContext(sqlText, nil) + if err := pipeline.Process(ctx); err != nil { + t.Fatalf("pipeline.Process failed: %v", err) + } + if len(ctx.Output.Statements) != 2 { + t.Fatalf("expected 2 statements, got %d: %+v", len(ctx.Output.Statements), ctx.Output.Statements) + } + + wantAlter := "alter table users add column bio VARCHAR(255)" + if ctx.Output.Statements[0].SQL != wantAlter { + t.Errorf("first statement = %q, want %q", ctx.Output.Statements[0].SQL, wantAlter) + } + wantIndex := `CREATE UNIQUE INDEX "uq_bio" ON "users" ("bio")` + if ctx.Output.Statements[1].SQL != wantIndex { + t.Errorf("second statement = %q, want %q", ctx.Output.Statements[1].SQL, wantIndex) + } + + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + defer db.Close() + + if _, err := db.Exec("CREATE TABLE users (id INTEGER PRIMARY KEY)"); err != nil { + t.Fatalf("failed to create table: %v", err) + } + for _, stmt := range ctx.Output.Statements { + if _, err := db.Exec(stmt.SQL); err != nil { + t.Fatalf("transpiled SQL failed to execute: %v\nSQL: %s", err, stmt.SQL) + } + } +} diff --git a/protocol/query/transform/alter_table_column_type.go b/protocol/query/transform/alter_table_column_type.go new file mode 100644 index 00000000..a35d1188 --- /dev/null +++ b/protocol/query/transform/alter_table_column_type.go @@ -0,0 +1,55 @@ +package transform + +import ( + "vitess.io/vitess/go/vt/sqlparser" +) + +// AlterTableColumnTypeRule strips MySQL-specific column type attributes - CHARACTER SET, +// COLLATE, COMMENT, and integer display widths - from ALTER TABLE ADD COLUMN, MODIFY +// COLUMN, and CHANGE COLUMN definitions. SQLite's column type syntax doesn't support any +// of these, and without stripping them SQLite's PREPARE step rejects the statement (e.g. +// "ADD COLUMN c VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"). +// +// CreateTableRule already strips the same attributes for CREATE TABLE columns; both rules +// share the stripMySQLColumnType helper (table_utils.go) rather than duplicating the logic. +// +// This rule always mutates the AST in place and returns ErrRuleNotApplicable, deferring +// serialization to AlterTableConstraintRule (for statements that also add a constraint or +// index) or to the transpiler's default serialization pass otherwise - the same pattern +// IntTypeRule uses for CREATE TABLE column types. It must run before AlterTableConstraintRule +// (lower priority) so the stripped columns are visible whichever rule ends up serializing. +type AlterTableColumnTypeRule struct{} + +func (r *AlterTableColumnTypeRule) Name() string { + return "AlterTableColumnType" +} + +func (r *AlterTableColumnTypeRule) Priority() int { + return 8 +} + +func (r *AlterTableColumnTypeRule) Transform(stmt sqlparser.Statement, params []interface{}, schema SchemaProvider, database string, serializer Serializer) ([]TranspiledStatement, error) { + alter, ok := stmt.(*sqlparser.AlterTable) + if !ok { + return nil, ErrRuleNotApplicable + } + + for _, opt := range alter.AlterOptions { + switch o := opt.(type) { + case *sqlparser.AddColumns: + for _, col := range o.Columns { + stripMySQLColumnType(col.Type) + } + case *sqlparser.ModifyColumn: + if o.NewColDefinition != nil { + stripMySQLColumnType(o.NewColDefinition.Type) + } + case *sqlparser.ChangeColumn: + if o.NewColDefinition != nil { + stripMySQLColumnType(o.NewColDefinition.Type) + } + } + } + + return nil, ErrRuleNotApplicable +} diff --git a/protocol/query/transform/alter_table_constraint.go b/protocol/query/transform/alter_table_constraint.go new file mode 100644 index 00000000..ec2f91a4 --- /dev/null +++ b/protocol/query/transform/alter_table_constraint.go @@ -0,0 +1,136 @@ +package transform + +import ( + "strings" + + "vitess.io/vitess/go/vt/sqlparser" +) + +// AlterTableConstraintRule rewrites MySQL index-DDL AlterOptions - "ALTER TABLE t ADD +// [CONSTRAINT name] UNIQUE (cols)" and plain "ALTER TABLE t ADD INDEX ... (cols)" - into +// SQLite-compatible standalone CREATE [UNIQUE] INDEX statements. +// +// Vitess also parses bare "CREATE [UNIQUE] INDEX name ON table (cols)" into this exact same +// *AlterTable/AddIndexDefinition AST shape (see create_index_prefix in sql.y: it builds an +// AlterTable node with a single AddIndexDefinition option). So this rule is the single path +// that turns every MySQL index-DDL form - ALTER ADD CONSTRAINT/ADD INDEX and standalone +// CREATE INDEX alike - into SQLite syntax. +// +// SQLite's ALTER TABLE only supports RENAME TABLE/COLUMN, ADD COLUMN, and DROP COLUMN - there +// is no equivalent to MySQL's ADD CONSTRAINT/ADD INDEX alter options, and SQLite has no +// "ALTER TABLE ... ADD INDEX" at all. Without this rule the default serializer emits invalid +// SQLite syntax (e.g. "alter table t add index idx (col)") which SQLite's PREPARE step rejects. +type AlterTableConstraintRule struct { +} + +func (r *AlterTableConstraintRule) Name() string { + return "AlterTableConstraint" +} + +func (r *AlterTableConstraintRule) Priority() int { + return 10 +} + +func (r *AlterTableConstraintRule) Transform(stmt sqlparser.Statement, params []interface{}, schema SchemaProvider, database string, serializer Serializer) ([]TranspiledStatement, error) { + alter, ok := stmt.(*sqlparser.AlterTable) + if !ok { + return nil, ErrRuleNotApplicable + } + + var indexAdds []*sqlparser.IndexDefinition + var remaining []sqlparser.AlterOption + for _, opt := range alter.AlterOptions { + addIdx, ok := opt.(*sqlparser.AddIndexDefinition) + if !ok || addIdx.IndexDefinition == nil || addIdx.IndexDefinition.Info == nil { + remaining = append(remaining, opt) + continue + } + + switch addIdx.IndexDefinition.Info.Type { + case sqlparser.IndexTypeUnique, sqlparser.IndexTypeDefault: + indexAdds = append(indexAdds, addIdx.IndexDefinition) + default: + // FULLTEXT/SPATIAL indexes have no SQLite equivalent; leave them for the + // default serializer - unrelated pre-existing limitation, not this rule's concern. + remaining = append(remaining, opt) + } + } + + if len(indexAdds) == 0 { + return nil, ErrRuleNotApplicable + } + + tableName := alter.Table.Name.String() + var results []TranspiledStatement + + // If other alter options remain (e.g. ADD COLUMN alongside ADD CONSTRAINT), keep them + // in the ALTER TABLE statement; the default serializer already handles those correctly. + if len(remaining) > 0 { + alter.AlterOptions = remaining + results = append(results, TranspiledStatement{SQL: serializer.Serialize(alter), Params: params}) + } + + for _, idx := range indexAdds { + results = append(results, TranspiledStatement{SQL: buildIndexSQL(tableName, idx), Params: nil}) + } + + return results, nil +} + +// buildIndexSQL generates a SQLite `CREATE [UNIQUE] INDEX "name" ON "table" (cols)` statement +// for an extracted ADD CONSTRAINT/ADD INDEX alter option or a standalone CREATE INDEX. Names +// are double-quoted since MySQL identifiers may contain characters (e.g. hyphens) that are not +// valid in unquoted SQLite identifiers. +func buildIndexSQL(tableName string, idx *sqlparser.IndexDefinition) string { + unique := idx.Info.Type == sqlparser.IndexTypeUnique + + name := idx.Info.ConstraintName.String() + if name == "" { + name = idx.Info.Name.String() + } + if name == "" { + name = generatedIndexName(tableName, idx.Columns, unique) + } + + var sb strings.Builder + sb.WriteString("CREATE ") + if unique { + sb.WriteString("UNIQUE ") + } + sb.WriteString("INDEX ") + sb.WriteString(QuoteIdentifier(name)) + sb.WriteString(" ON ") + sb.WriteString(QuoteIdentifier(tableName)) + sb.WriteString(" (") + for i, col := range idx.Columns { + if i > 0 { + sb.WriteString(", ") + } + sb.WriteString(QuoteIdentifier(col.Column.String())) + } + sb.WriteString(")") + + return sb.String() +} + +// generatedIndexName mirrors MySQL's own convention for an unnamed index/constraint: +// derive a deterministic name from the table and column names. +func generatedIndexName(tableName string, columns []*sqlparser.IndexColumn, unique bool) string { + cols := make([]string, len(columns)) + for i, c := range columns { + cols[i] = c.Column.String() + } + suffix := "idx" + if unique { + suffix = "unique" + } + return tableName + "_" + strings.Join(cols, "_") + "_" + suffix +} + +// QuoteIdentifier double-quotes a SQLite identifier, escaping embedded quotes. Exported so +// other packages that build SQLite DDL text outside the AST/transform pipeline (e.g. the +// query package's DROP INDEX pattern-based extraction, which Vitess cannot parse at all) +// can safely quote identifiers using the same convention. +func QuoteIdentifier(name string) string { + return `"` + strings.ReplaceAll(name, `"`, `""`) + `"` +} diff --git a/protocol/query/transform/alter_table_constraint_test.go b/protocol/query/transform/alter_table_constraint_test.go new file mode 100644 index 00000000..5d0098d9 --- /dev/null +++ b/protocol/query/transform/alter_table_constraint_test.go @@ -0,0 +1,151 @@ +package transform + +import ( + "testing" + + "vitess.io/vitess/go/vt/sqlparser" +) + +func TestAlterTableConstraintRule_Name(t *testing.T) { + rule := &AlterTableConstraintRule{} + if rule.Name() != "AlterTableConstraint" { + t.Errorf("Name() = %q, want %q", rule.Name(), "AlterTableConstraint") + } +} + +func TestAlterTableConstraintRule_Priority(t *testing.T) { + rule := &AlterTableConstraintRule{} + if rule.Priority() != 10 { + t.Errorf("Priority() = %d, want %d", rule.Priority(), 10) + } +} + +// TestAlterTableConstraintRule_AddUnique verifies MySQL's "ALTER TABLE t ADD [CONSTRAINT +// name] UNIQUE (cols)" - which has no SQLite equivalent as an ALTER TABLE option - is +// rewritten into a standalone CREATE UNIQUE INDEX statement. +func TestAlterTableConstraintRule_AddUnique(t *testing.T) { + tests := []struct { + name string + input string + wantSQL string + wantStmts int + }{ + { + name: "named constraint, unquoted identifier", + input: "ALTER TABLE users ADD CONSTRAINT uq_email UNIQUE (email)", + wantSQL: `CREATE UNIQUE INDEX "uq_email" ON "users" ("email")`, + wantStmts: 1, + }, + { + name: "named constraint, backtick-quoted identifier", + input: "ALTER TABLE users ADD CONSTRAINT `uq_email` UNIQUE (email)", + wantSQL: `CREATE UNIQUE INDEX "uq_email" ON "users" ("email")`, + wantStmts: 1, + }, + { + name: "named constraint, hyphenated backtick-quoted identifier", + input: "ALTER TABLE users ADD CONSTRAINT `unique-user-email` UNIQUE (email)", + wantSQL: `CREATE UNIQUE INDEX "unique-user-email" ON "users" ("email")`, + wantStmts: 1, + }, + { + name: "named constraint, multi-column", + input: "ALTER TABLE users ADD CONSTRAINT uq_email_name UNIQUE (email, name)", + wantSQL: `CREATE UNIQUE INDEX "uq_email_name" ON "users" ("email", "name")`, + wantStmts: 1, + }, + { + name: "ADD UNIQUE INDEX with explicit name (no CONSTRAINT keyword)", + input: "ALTER TABLE users ADD UNIQUE INDEX uq_email (email)", + wantSQL: `CREATE UNIQUE INDEX "uq_email" ON "users" ("email")`, + wantStmts: 1, + }, + { + name: "ADD UNIQUE with no name at all", + input: "ALTER TABLE users ADD UNIQUE (email)", + wantSQL: `CREATE UNIQUE INDEX "users_email_unique" ON "users" ("email")`, + wantStmts: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stmt, err := sqlparser.NewTestParser().Parse(tt.input) + if err != nil { + t.Fatalf("failed to parse SQL: %v", err) + } + + rule := &AlterTableConstraintRule{} + results, err := rule.Transform(stmt, nil, nil, "", &SQLiteSerializer{}) + if err != nil { + t.Fatalf("Transform failed: %v", err) + } + + if len(results) != tt.wantStmts { + t.Fatalf("statement count = %d, want %d (statements: %+v)", len(results), tt.wantStmts, results) + } + if results[0].SQL != tt.wantSQL { + t.Errorf("SQL = %q, want %q", results[0].SQL, tt.wantSQL) + } + }) + } +} + +// TestAlterTableConstraintRule_MixedOptions verifies that when ADD CONSTRAINT UNIQUE is +// combined with other alter options (e.g. ADD COLUMN) in the same statement, the +// non-constraint options remain in a (still valid) ALTER TABLE statement and the unique +// constraint is emitted as an additional CREATE UNIQUE INDEX statement. +func TestAlterTableConstraintRule_MixedOptions(t *testing.T) { + input := "ALTER TABLE users ADD COLUMN age INT, ADD CONSTRAINT uq_email UNIQUE (email)" + stmt, err := sqlparser.NewTestParser().Parse(input) + if err != nil { + t.Fatalf("failed to parse SQL: %v", err) + } + + rule := &AlterTableConstraintRule{} + results, err := rule.Transform(stmt, nil, nil, "", &SQLiteSerializer{}) + if err != nil { + t.Fatalf("Transform failed: %v", err) + } + + if len(results) != 2 { + t.Fatalf("expected 2 statements, got %d: %+v", len(results), results) + } + + wantAlter := "alter table users add column age INT" + if results[0].SQL != wantAlter { + t.Errorf("first statement = %q, want %q", results[0].SQL, wantAlter) + } + + wantIndex := `CREATE UNIQUE INDEX "uq_email" ON "users" ("email")` + if results[1].SQL != wantIndex { + t.Errorf("second statement = %q, want %q", results[1].SQL, wantIndex) + } +} + +// TestAlterTableConstraintRule_NotApplicable verifies the rule is a no-op for +// statements without a UNIQUE constraint addition, so plain column changes keep +// going through the default serializer path unchanged. +func TestAlterTableConstraintRule_NotApplicable(t *testing.T) { + tests := []string{ + "ALTER TABLE users ADD COLUMN age INT", + "ALTER TABLE users DROP COLUMN age", + "ALTER TABLE users RENAME COLUMN age TO years", + "SELECT * FROM users", + } + + for _, sql := range tests { + t.Run(sql, func(t *testing.T) { + stmt, err := sqlparser.NewTestParser().Parse(sql) + if err != nil { + t.Fatalf("failed to parse SQL: %v", err) + } + + rule := &AlterTableConstraintRule{} + _, err = rule.Transform(stmt, nil, nil, "", &SQLiteSerializer{}) + if err != ErrRuleNotApplicable { + t.Errorf("Transform() err = %v, want ErrRuleNotApplicable", err) + } + }) + } +} diff --git a/protocol/query/transform/create_table.go b/protocol/query/transform/create_table.go index f290aedc..66776a4a 100644 --- a/protocol/query/transform/create_table.go +++ b/protocol/query/transform/create_table.go @@ -54,21 +54,7 @@ func (r *CreateTableRule) Transform(stmt sqlparser.Statement, params []interface // Process columns: strip MySQL-specific options for _, col := range create.TableSpec.Columns { - if col.Type != nil { - // Strip display widths from integer types: INTEGER(20) → INTEGER - if isIntegerType(col.Type.Type) { - col.Type.Length = nil - } - // Strip MySQL-specific column options - if col.Type.Options != nil { - // Strip MySQL-specific COLLATE (SQLite only supports NOCASE, BINARY, RTRIM) - col.Type.Options.Collate = "" - // Strip MySQL-specific COMMENT (not supported in SQLite column definitions) - col.Type.Options.Comment = nil - } - // Also strip charset (SQLite doesn't use MySQL charsets) - col.Type.Charset = sqlparser.ColumnCharset{} - } + stripMySQLColumnType(col.Type) } // Build results diff --git a/protocol/query/transform/literal_extractor.go b/protocol/query/transform/literal_extractor.go index 6a616b7f..0ceef970 100644 --- a/protocol/query/transform/literal_extractor.go +++ b/protocol/query/transform/literal_extractor.go @@ -17,19 +17,40 @@ import ( // Input: INSERT INTO t VALUES ('text', 123, 'binary\x00data') // Output: INSERT INTO t VALUES (:v1, :v2, :v3) // Params: [string("text"), int64(123), string("binary\x00data")] -func ExtractLiterals(stmt sqlparser.Statement) []interface{} { +// +// The AST can also already contain placeholders of its own - e.g. a client's +// own `?` bind marks in a prepared statement, still present as +// *sqlparser.Argument nodes because they were never literal values to begin +// with. order records, for EVERY placeholder in the final statement (both +// those pre-existing ones and the newly-extracted ones) in the same +// left-to-right order they will serialize in, whether that slot's value at +// execution time comes from the caller's own bind values (true) or from +// params (false, consumed in order) - see protocol.MergeExecParams, which +// uses this to interleave the two sources correctly instead of assuming one +// always comes before the other (a literal can appear before, after, or +// between a statement's own placeholders in the source SQL). order is nil +// when the statement has no pre-existing placeholders, matching today's +// wire-params-XOR-extracted-params callers. +func ExtractLiterals(stmt sqlparser.Statement) (params []interface{}, order []bool) { if stmt == nil { - return nil + return nil, nil } - var params []interface{} + var hasArgument bool counter := 0 sqlparser.Rewrite(stmt, func(cursor *sqlparser.Cursor) bool { + if _, ok := cursor.Node().(*sqlparser.Argument); ok { + hasArgument = true + order = append(order, true) + return true + } + lit, ok := cursor.Node().(*sqlparser.Literal) if !ok { return true } + order = append(order, false) // Extract value based on literal type var value interface{} @@ -103,8 +124,15 @@ func ExtractLiterals(stmt sqlparser.Statement) []interface{} { // Return nil if no literals were found if len(params) == 0 { - return nil + return nil, nil + } + + // order is only useful to callers when there is something to interleave: + // a statement made only of literals (the common non-prepared case) needs + // no positional merge, so keep returning nil there too. + if !hasArgument { + return params, nil } - return params + return params, order } diff --git a/protocol/query/transform/literal_extractor_test.go b/protocol/query/transform/literal_extractor_test.go index c9023e9b..320a8261 100644 --- a/protocol/query/transform/literal_extractor_test.go +++ b/protocol/query/transform/literal_extractor_test.go @@ -14,7 +14,7 @@ func TestExtractLiterals_Insert(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -62,7 +62,7 @@ func TestExtractLiterals_BinaryData(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -92,7 +92,7 @@ func TestExtractLiterals_Update(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -132,7 +132,7 @@ func TestExtractLiterals_Select(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -160,7 +160,7 @@ func TestExtractLiterals_HexLiteral(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -196,7 +196,7 @@ func TestExtractLiterals_NoLiterals(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params != nil { t.Errorf("Expected nil for statement with no literals, got %d params", len(params)) @@ -212,7 +212,7 @@ func TestExtractLiterals_MixedTypes(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -248,7 +248,7 @@ func TestExtractLiterals_NestedSubquery(t *testing.T) { t.Fatalf("Failed to parse SQL: %v", err) } - params := ExtractLiterals(stmt) + params, _ := ExtractLiterals(stmt) if params == nil { t.Fatal("Expected params, got nil") @@ -270,7 +270,7 @@ func TestExtractLiterals_NestedSubquery(t *testing.T) { } func TestExtractLiterals_NilStatement(t *testing.T) { - params := ExtractLiterals(nil) + params, _ := ExtractLiterals(nil) if params != nil { t.Errorf("Expected nil for nil statement, got %v", params) } diff --git a/protocol/query/transform/sqlite_serializer.go b/protocol/query/transform/sqlite_serializer.go index 6d78f8a1..0da493ef 100644 --- a/protocol/query/transform/sqlite_serializer.go +++ b/protocol/query/transform/sqlite_serializer.go @@ -67,8 +67,18 @@ func (s *SQLiteSerializer) nodeFormatter(buf *sqlparser.TrackedBuffer, node sqlp // Skip MySQL-specific index hints (FORCE INDEX, USE INDEX, IGNORE INDEX) // SQLite doesn't support them case *sqlparser.Where: - // Format WHERE with uppercase keyword - buf.WriteString(" WHERE ") + // Format WHERE/HAVING with uppercase keyword. Vitess represents both + // clauses with the same *Where struct, distinguished by n.Type - the + // previous unconditional " WHERE " here silently turned HAVING clauses + // (including ones inside subqueries) into WHERE clauses. + if n == nil || n.Expr == nil { + return + } + if n.Type == sqlparser.HavingClause { + buf.WriteString(" HAVING ") + } else { + buf.WriteString(" WHERE ") + } buf.Myprintf("%v", n.Expr) case sqlparser.OrderBy: // Format ORDER BY with uppercase keywords diff --git a/protocol/query/transform/sqlite_serializer_test.go b/protocol/query/transform/sqlite_serializer_test.go index 056eba84..15e9b080 100644 --- a/protocol/query/transform/sqlite_serializer_test.go +++ b/protocol/query/transform/sqlite_serializer_test.go @@ -186,3 +186,58 @@ func TestSQLiteSerializer_SerializeBasic(t *testing.T) { }) } } + +// TestSQLiteSerializer_Having pins a regression where HAVING clauses were +// serialized as WHERE. Vitess represents both WHERE and HAVING with the same +// *sqlparser.Where struct, distinguished only by its Type field; the +// nodeFormatter's *sqlparser.Where case used to ignore that field and always +// write " WHERE ", which silently corrupted any HAVING clause - including +// ones nested inside a subquery, where the result was outright invalid SQL. +func TestSQLiteSerializer_Having(t *testing.T) { + tests := []struct { + name string + input string + shouldContain []string // case-insensitive + mustNotMatch string // substring that must NOT appear (case-insensitive) + }{ + { + name: "top-level GROUP BY HAVING", + input: "SELECT category, COUNT(*) FROM products GROUP BY category HAVING COUNT(*) > 5", + shouldContain: []string{"group by category", "having count(*) > 5"}, + }, + { + name: "HAVING inside IN-subquery", + input: "SELECT email, user_id FROM users WHERE email IN (SELECT email FROM users GROUP BY email HAVING COUNT(email) > ?) ORDER BY email", + shouldContain: []string{"where email in (select email from users group by email having count(email) > ?)"}, + }, + { + name: "WHERE and HAVING together", + input: "SELECT category, COUNT(*) FROM products WHERE price > 10 GROUP BY category HAVING COUNT(*) > 5", + shouldContain: []string{"where price > 10", "group by category", "having count(*) > 5"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stmt, err := sqlparser.NewTestParser().Parse(tt.input) + if err != nil { + t.Fatalf("failed to parse SQL: %v", err) + } + + serializer := &SQLiteSerializer{} + result := serializer.Serialize(stmt) + lowerResult := strings.ToLower(result) + + for _, expected := range tt.shouldContain { + if !strings.Contains(lowerResult, expected) { + t.Errorf("expected output to contain %q (case-insensitive), got: %s", expected, result) + } + } + + // The subquery's HAVING must never degrade into a second WHERE. + if strings.Count(lowerResult, " where ") > 1 { + t.Errorf("HAVING clause was serialized as WHERE, got: %s", result) + } + }) + } +} diff --git a/protocol/query/transform/table_utils.go b/protocol/query/transform/table_utils.go index c0e3570f..9f80ad7b 100644 --- a/protocol/query/transform/table_utils.go +++ b/protocol/query/transform/table_utils.go @@ -6,6 +6,31 @@ import ( "vitess.io/vitess/go/vt/sqlparser" ) +// stripMySQLColumnType removes MySQL-specific column type attributes that have no SQLite +// equivalent, in place: display widths on integer types, the COLLATE/COMMENT column +// options, and the CHARACTER SET/COLLATE charset clause. Shared by CreateTableRule (CREATE +// TABLE columns) and AlterTableColumnTypeRule (ALTER TABLE ADD/MODIFY/CHANGE COLUMN). +func stripMySQLColumnType(colType *sqlparser.ColumnType) { + if colType == nil { + return + } + + // Strip display widths from integer types: INTEGER(20) → INTEGER + if isIntegerType(colType.Type) { + colType.Length = nil + } + + if colType.Options != nil { + // Strip MySQL-specific COLLATE (SQLite only supports NOCASE, BINARY, RTRIM) + colType.Options.Collate = "" + // Strip MySQL-specific COMMENT (not supported in SQLite column definitions) + colType.Options.Comment = nil + } + + // Strip charset (SQLite doesn't use MySQL charsets) + colType.Charset = sqlparser.ColumnCharset{} +} + // HasJoin checks if any TableExpr in the slice is a JoinTableExpr. func HasJoin(tableExprs sqlparser.TableExprs) bool { for _, expr := range tableExprs { diff --git a/protocol/query/transpiler.go b/protocol/query/transpiler.go index ebd16962..71ebb342 100644 --- a/protocol/query/transpiler.go +++ b/protocol/query/transpiler.go @@ -41,7 +41,9 @@ func NewTranspiler(cacheSize int, idGen id.Generator) (*Transpiler, error) { &transform.DualTableRule{}, // Priority 1: Strip FROM dual &transform.LastInsertIDRule{}, // Priority 5: LAST_INSERT_ID() → last_insert_rowid() &transform.IntTypeRule{}, // Priority 5: Strip UNSIGNED, normalize int types + &transform.AlterTableColumnTypeRule{}, // Priority 8: Strip CHARACTER SET/COLLATE from ADD/MODIFY/CHANGE COLUMN &transform.CreateTableRule{}, // Priority 10: Extract KEY → CREATE INDEX + &transform.AlterTableConstraintRule{}, // Priority 10: ADD CONSTRAINT UNIQUE / ADD INDEX / CREATE INDEX → CREATE [UNIQUE] INDEX &transform.InsertOnDuplicateKeyRule{}, // Priority 20: ON DUPLICATE KEY → ON CONFLICT &transform.DeleteJoinRule{}, // Priority 30: DELETE+JOIN → subquery &transform.UpdateJoinRule{}, // Priority 40: UPDATE+JOIN → subquery @@ -167,8 +169,9 @@ func (t *Transpiler) Transpile(ctx *QueryContext) error { // Extract literals BEFORE serialization (AST mutation only) var extractedParams []interface{} + var paramOrder []bool if needsLiteralExtraction { - extractedParams = transform.ExtractLiterals(ast) + extractedParams, paramOrder = transform.ExtractLiterals(ast) } // Single serialization with all state @@ -182,7 +185,7 @@ func (t *Transpiler) Transpile(ctx *QueryContext) error { params = extractedParams } - ctx.Output.Statements = []TranspiledStatement{{SQL: sql, Params: params}} + ctx.Output.Statements = []TranspiledStatement{{SQL: sql, Params: params, ParamOrder: paramOrder}} ctx.MySQLState.AST = ast ctx.MySQLState.Transformations = transformations diff --git a/protocol/server.go b/protocol/server.go index 7297524d..19459c25 100644 --- a/protocol/server.go +++ b/protocol/server.go @@ -3,6 +3,7 @@ package protocol import ( "bytes" "encoding/binary" + "errors" "fmt" "io" "math" @@ -62,6 +63,13 @@ type SessionTransaction struct { StartTS hlc.Timestamp Statements []Statement Database string + + // HadPinnedState records that eager DML pinned a real SQLite transaction + // for this txn at some point, so COMMIT can tell a genuinely empty + // transaction apart from one whose pinned state went missing before + // COMMIT read it (e.g. a concurrent session eviction released it). See + // ConnectionSession.MarkPinnedStateActive. + HadPinnedState bool } // ConnectionSession represents per-connection state @@ -154,6 +162,17 @@ func (s *ConnectionSession) EndTransaction() { s.activeTxn = nil } +// MarkPinnedStateActive records that the active transaction has pinned a +// real SQLite transaction for eager DML execution. A no-op if there is no +// active transaction. See SessionTransaction.HadPinnedState. +func (s *ConnectionSession) MarkPinnedStateActive() { + s.activeTxnMu.Lock() + defer s.activeTxnMu.Unlock() + if s.activeTxn != nil { + s.activeTxn.HadPinnedState = true + } +} + // NextForwardRequestID allocates the next idempotency key for write forwarding. func (s *ConnectionSession) NextForwardRequestID() uint64 { return s.ForwardRequestSeq.Add(1) @@ -167,6 +186,12 @@ type PreparedStatement struct { ParamTypes []byte // Cached parameter types for subsequent executions OriginalType StatementCode Context *query.QueryContext + + // LongData holds parameter data accumulated via COM_STMT_SEND_LONG_DATA, + // keyed by param_id. Per the MySQL protocol spec it persists across + // COM_STMT_EXECUTE calls and is only cleared by COM_STMT_RESET or + // COM_STMT_CLOSE (the latter simply drops the whole statement). + LongData map[uint16][]byte } // ConnectionHandler defines the interface for handling MySQL commands @@ -192,6 +217,17 @@ type ResultColumnDescriber interface { DescribeResultColumns(session *ConnectionSession, sql string) ([]ColumnDef, error) } +// SessionCloser is an optional extension for handlers that need to release +// per-session resources when a connection ends - notably an interactive +// transaction's pinned execution state, which must be rolled back if the +// client disconnects without sending COMMIT or ROLLBACK, so the SQLite +// writer is not left locked forever. Called exactly once per connection, +// after the command loop exits, regardless of how it exited (client EOF, +// read error, or the connection being force-closed during shutdown). +type SessionCloser interface { + CloseSession(session *ConnectionSession) +} + // ResultSet represents a MySQL result set type ResultSet struct { Columns []ColumnDef @@ -387,6 +423,16 @@ func (s *MySQLServer) handleConnection(conn net.Conn) { VecVars: vecindex.DefaultVecSessionVars(), } + // Notify the handler (if it opts in) that this session is going away, + // on every exit path from this function. Registered first so it is the + // last deferred call to run, after everything else about this + // connection has already been torn down. + defer func() { + if closer, ok := s.handler.(SessionCloser); ok { + closer.CloseSession(session) + } + }() + telemetry.MySQLConnections.Inc() defer telemetry.MySQLConnections.Dec() @@ -473,6 +519,10 @@ func (s *MySQLServer) handleConnection(conn net.Conn) { case 0x17: // COM_STMT_EXECUTE log.Debug().Uint64("conn_id", session.ConnID).Msg("COM_STMT_EXECUTE received") s.handleStmtExecute(conn, session, payload[1:]) + case 0x18: // COM_STMT_SEND_LONG_DATA + s.handleStmtSendLongData(session, payload[1:]) + // Per protocol spec, COM_STMT_SEND_LONG_DATA gets no response at all, + // success or failure - writing one here would desync the packet stream. case 0x19: // COM_STMT_CLOSE if len(payload) >= 5 { stmtID := binary.LittleEndian.Uint32(payload[1:5]) @@ -482,6 +532,8 @@ func (s *MySQLServer) handleConnection(conn net.Conn) { log.Debug().Uint64("conn_id", session.ConnID).Uint32("stmt_id", stmtID).Msg("Statement closed") } // COM_STMT_CLOSE doesn't send a response + case 0x1A: // COM_STMT_RESET + s.handleStmtReset(conn, session, payload[1:]) case 0x0E: // COM_PING _ = s.writeOK(conn, 1, session, 0, 0) case 0x01: // COM_QUIT @@ -1159,6 +1211,56 @@ func (s *MySQLServer) handleStmtPrepare(conn net.Conn, session *ConnectionSessio } } +// handleStmtSendLongData implements COM_STMT_SEND_LONG_DATA. Per the MySQL +// protocol spec, repeated calls for the same (statement_id, param_id) append +// to the previously accumulated data, and the server never replies - not even +// on error, since the client does not expect a packet for this command. +func (s *MySQLServer) handleStmtSendLongData(session *ConnectionSession, payload []byte) { + if len(payload) < 6 { + log.Warn().Uint64("conn_id", session.ConnID).Msg("Malformed COM_STMT_SEND_LONG_DATA packet") + return + } + stmtID := binary.LittleEndian.Uint32(payload[0:4]) + paramID := binary.LittleEndian.Uint16(payload[4:6]) + data := payload[6:] + + session.preparedStmtLock.Lock() + defer session.preparedStmtLock.Unlock() + stmt, ok := session.preparedStmts[stmtID] + if !ok { + log.Warn().Uint64("conn_id", session.ConnID).Uint32("stmt_id", stmtID). + Msg("COM_STMT_SEND_LONG_DATA for unknown statement") + return + } + if stmt.LongData == nil { + stmt.LongData = make(map[uint16][]byte) + } + stmt.LongData[paramID] = append(stmt.LongData[paramID], data...) +} + +// handleStmtReset implements COM_STMT_RESET: it clears any parameter data +// accumulated via COM_STMT_SEND_LONG_DATA for the statement and responds OK. +func (s *MySQLServer) handleStmtReset(conn net.Conn, session *ConnectionSession, payload []byte) { + if len(payload) < 4 { + _ = s.writeError(conn, 1, 1064, "Invalid COM_STMT_RESET packet") + return + } + stmtID := binary.LittleEndian.Uint32(payload[0:4]) + + session.preparedStmtLock.Lock() + stmt, ok := session.preparedStmts[stmtID] + if ok { + stmt.LongData = nil + } + session.preparedStmtLock.Unlock() + + if !ok { + _ = s.writeError(conn, 1, 1243, fmt.Sprintf("Unknown statement ID: %d", stmtID)) + return + } + _ = s.writeOK(conn, 1, session, 0, 0) +} + func (s *MySQLServer) handleStmtExecute(conn net.Conn, session *ConnectionSession, payload []byte) { if len(payload) < 9 { _ = s.writeError(conn, 1, 1064, "Invalid COM_STMT_EXECUTE packet") @@ -1234,15 +1336,41 @@ func (s *MySQLServer) handleStmtExecute(conn net.Conn, session *ConnectionSessio continue } - // Parse value based on type - if len(paramTypes) > int(i)*2 { + // Parameters that received COM_STMT_SEND_LONG_DATA carry no inline + // value in this packet - the client omits them entirely and the + // server must use the previously accumulated buffer instead. + // Guarded by preparedStmtLock for consistency with every write to + // stmt.LongData, even though the per-connection command loop is + // otherwise single-threaded. + session.preparedStmtLock.Lock() + longData, hasLongData := stmt.LongData[i] + var longDataCopy []byte + if hasLongData { + longDataCopy = make([]byte, len(longData)) + copy(longDataCopy, longData) + } + session.preparedStmtLock.Unlock() + if hasLongData { + params[i] = longDataCopy + continue + } + + // Parse value based on type. Each parameter type is 2 bytes: the + // type byte followed by a flags byte whose 0x80 bit marks UNSIGNED. + if len(paramTypes) > int(i)*2+1 { paramType := paramTypes[i*2] + unsigned := paramTypes[i*2+1]&0x80 != 0 var val interface{} var err error - offset, val, err = parseParamValue(payload, offset, paramType) + offset, val, err = parseParamValue(payload, offset, paramType, unsigned) if err != nil { - _ = s.writeError(conn, 1, 1064, fmt.Sprintf("Failed to parse parameter %d: %v", i, err)) + if errors.Is(err, errUnsignedBigintOutOfRange) { + _ = s.writeErrorWithState(conn, 1, 1264, "22003", + fmt.Sprintf("Out of range value for parameter %d", i)) + } else { + _ = s.writeError(conn, 1, 1064, fmt.Sprintf("Failed to parse parameter %d: %v", i, err)) + } return } params[i] = val @@ -1337,7 +1465,21 @@ func countPlaceholders(query string) int { return count } -func parseParamValue(payload []byte, offset int, paramType byte) (int, interface{}, error) { +// errUnsignedBigintOutOfRange indicates an UNSIGNED BIGINT parameter's value +// is >= 2^63 and therefore cannot be represented as the int64 that SQLite's +// INTEGER storage (and every SQL parameter binding path in this server) +// requires. Callers must report this distinctly from a generic parse +// failure - see handleStmtExecute's ER_WARN_DATA_OUT_OF_RANGE (1264) handling. +var errUnsignedBigintOutOfRange = errors.New("value out of range for signed BIGINT") + +// parseParamValue decodes a single COM_STMT_EXECUTE binary parameter value. +// unsigned reflects the 0x80 bit of the parameter's flags byte. Every +// integer type decodes to a signed Go type (int8/int16/int32/int64) because +// every downstream SQL parameter binding path only accepts signed integers; +// an unsigned value that fits in int64 converts explicitly, and an unsigned +// BIGINT that doesn't fit (>= 2^63) returns errUnsignedBigintOutOfRange +// rather than silently reinterpreting it as negative. +func parseParamValue(payload []byte, offset int, paramType byte, unsigned bool) (int, interface{}, error) { const ( MYSQL_TYPE_DECIMAL = 0x00 MYSQL_TYPE_TINY = 0x01 @@ -1375,25 +1517,56 @@ func parseParamValue(payload []byte, offset int, paramType byte) (int, interface if len(payload) < offset+1 { return offset, nil, fmt.Errorf("not enough data for TINY") } + if unsigned { + // An unsigned TINY (0..255) always fits in int64; decode to it + // explicitly rather than handing callers a bare uint8, since + // downstream SQL parameter binding (database/sql, go-sqlite3) + // only accepts signed integers. + return offset + 1, int64(payload[offset]), nil + } return offset + 1, int8(payload[offset]), nil case MYSQL_TYPE_SHORT, MYSQL_TYPE_YEAR: if len(payload) < offset+2 { return offset, nil, fmt.Errorf("not enough data for SHORT") } - return offset + 2, int16(binary.LittleEndian.Uint16(payload[offset:])), nil + v := binary.LittleEndian.Uint16(payload[offset:]) + if unsigned { + // An unsigned SHORT (0..65535) always fits in int64. + return offset + 2, int64(v), nil + } + return offset + 2, int16(v), nil case MYSQL_TYPE_LONG, MYSQL_TYPE_INT24: if len(payload) < offset+4 { return offset, nil, fmt.Errorf("not enough data for LONG") } - return offset + 4, int32(binary.LittleEndian.Uint32(payload[offset:])), nil + v := binary.LittleEndian.Uint32(payload[offset:]) + if unsigned { + // An unsigned LONG (0..2^32-1) always fits in int64. + return offset + 4, int64(v), nil + } + return offset + 4, int32(v), nil case MYSQL_TYPE_LONGLONG: if len(payload) < offset+8 { return offset, nil, fmt.Errorf("not enough data for LONGLONG") } - return offset + 8, int64(binary.LittleEndian.Uint64(payload[offset:])), nil + v := binary.LittleEndian.Uint64(payload[offset:]) + if unsigned { + // SQLite's INTEGER storage is signed 64-bit, and downstream SQL + // parameter binding only accepts int64 (database/sql's default + // converter rejects a uint64 with the high bit set, and + // go-sqlite3 has no NamedValueChecker to work around that). A + // value that fits decodes to int64 cleanly; one that doesn't + // genuinely cannot be represented, so report it rather than + // silently reinterpreting it as a negative number. + if v > math.MaxInt64 { + return offset, nil, errUnsignedBigintOutOfRange + } + return offset + 8, int64(v), nil + } + return offset + 8, int64(v), nil case MYSQL_TYPE_FLOAT: if len(payload) < offset+4 { diff --git a/protocol/session_closer_test.go b/protocol/session_closer_test.go new file mode 100644 index 00000000..b10a1eb7 --- /dev/null +++ b/protocol/session_closer_test.go @@ -0,0 +1,129 @@ +package protocol + +import ( + "net" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// sessionCloserHandler is a ConnectionHandler that also implements +// SessionCloser, recording every session it is asked to close as well as +// every session it saw a query from. +type sessionCloserHandler struct { + mockHandler + + mu sync.Mutex + queried []*ConnectionSession + closed []*ConnectionSession +} + +func (h *sessionCloserHandler) HandleQuery(session *ConnectionSession, sql string, params []interface{}) (*ResultSet, error) { + h.mu.Lock() + h.queried = append(h.queried, session) + h.mu.Unlock() + return h.mockHandler.HandleQuery(session, sql, params) +} + +func (h *sessionCloserHandler) CloseSession(session *ConnectionSession) { + h.mu.Lock() + defer h.mu.Unlock() + h.closed = append(h.closed, session) +} + +func (h *sessionCloserHandler) closedSessions() []*ConnectionSession { + h.mu.Lock() + defer h.mu.Unlock() + out := make([]*ConnectionSession, len(h.closed)) + copy(out, h.closed) + return out +} + +func (h *sessionCloserHandler) queriedSessions() []*ConnectionSession { + h.mu.Lock() + defer h.mu.Unlock() + out := make([]*ConnectionSession, len(h.queried)) + copy(out, h.queried) + return out +} + +// TestSessionCloser_CalledOnceOnDisconnect verifies CloseSession is invoked +// exactly once, with the same session used for the connection, when the +// client simply closes the socket after handshaking. +func TestSessionCloser_CalledOnceOnDisconnect(t *testing.T) { + t.Parallel() + + handler := &sessionCloserHandler{} + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + require.NoError(t, server.Start()) + defer server.Stop() + + addr := server.listeners[0].Addr().String() + + conn, err := net.Dial("tcp", addr) + require.NoError(t, err) + resp := completeHandshake(t, conn) + require.Equal(t, byte(0x00), resp[0], "expected OK after handshake") + + require.Eventually(t, func() bool { + return server.ActiveConnectionCount() == 1 + }, time.Second, 10*time.Millisecond, "expected 1 active connection") + + // Send a query so we know which session this connection was assigned. + sendComQuery(t, conn, "SELECT 1") + _ = readMySQLPacket(t, conn) // response to the query + + // Disconnect without COMMIT/ROLLBACK or COM_QUIT. + require.NoError(t, conn.Close()) + + require.Eventually(t, func() bool { + return len(handler.closedSessions()) == 1 + }, time.Second, 10*time.Millisecond, "expected CloseSession to be called exactly once") + + closed := handler.closedSessions() + require.Len(t, closed, 1) + require.NotNil(t, closed[0]) + + queried := handler.queriedSessions() + require.Len(t, queried, 1) + require.Equal(t, queried[0].ConnID, closed[0].ConnID, + "CloseSession must receive the same session (ConnID) used for the connection's queries") +} + +// TestSessionCloser_HandlerWithoutSessionCloser verifies that a handler +// which only implements ConnectionHandler (not SessionCloser) causes no +// panic and no behavior change when a connection closes. +func TestSessionCloser_HandlerWithoutSessionCloser(t *testing.T) { + t.Parallel() + + handler := &mockHandler{} + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + require.NoError(t, server.Start()) + defer server.Stop() + + addr := server.listeners[0].Addr().String() + + conn, err := net.Dial("tcp", addr) + require.NoError(t, err) + resp := completeHandshake(t, conn) + require.Equal(t, byte(0x00), resp[0], "expected OK after handshake") + + require.Eventually(t, func() bool { + return server.ActiveConnectionCount() == 1 + }, time.Second, 10*time.Millisecond, "expected 1 active connection") + + require.NoError(t, conn.Close()) + + require.Eventually(t, func() bool { + return server.ActiveConnectionCount() == 0 + }, time.Second, 10*time.Millisecond, "connection should be deregistered without panicking") +} + +// sendComQuery writes a COM_QUERY packet for sql on conn. +func sendComQuery(t *testing.T, conn net.Conn, sql string) { + t.Helper() + payload := append([]byte{0x03}, []byte(sql)...) // COM_QUERY = 0x03 + writeMySQLPacket(t, conn, 0, payload) +} diff --git a/protocol/stmt_long_data_test.go b/protocol/stmt_long_data_test.go new file mode 100644 index 00000000..f18ed539 --- /dev/null +++ b/protocol/stmt_long_data_test.go @@ -0,0 +1,339 @@ +package protocol + +import ( + "database/sql" + "encoding/binary" + "math" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// sqliteExecHandler routes HandleQuery through a real *sql.DB backed by the +// project's actual SQLite driver, so tests using it exercise the real +// database/sql parameter-binding path (including its rejection of uint64 +// values with the high bit set) instead of a mock that merely records +// whatever value it was handed. +type sqliteExecHandler struct { + db *sql.DB + queries []string +} + +func newSQLiteExecHandler(t *testing.T, schema string) *sqliteExecHandler { + t.Helper() + db := openTestDB(t, ":memory:") + _, err := db.Exec(schema) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + return &sqliteExecHandler{db: db} +} + +func (h *sqliteExecHandler) HandleQuery(session *ConnectionSession, sqlText string, params []interface{}) (*ResultSet, error) { + h.queries = append(h.queries, sqlText) + res, err := h.db.Exec(sqlText, params...) + if err != nil { + return nil, err + } + rowsAffected, _ := res.RowsAffected() + lastInsertID, _ := res.LastInsertId() + return &ResultSet{RowsAffected: rowsAffected, LastInsertId: lastInsertID}, nil +} + +// buildSendLongDataPayload constructs a COM_STMT_SEND_LONG_DATA payload +// (without the leading 0x18 command byte, matching how the command +// dispatcher in handleConnection slices it before calling the handler). +func buildSendLongDataPayload(stmtID uint32, paramID uint16, data string) []byte { + payload := make([]byte, 6+len(data)) + binary.LittleEndian.PutUint32(payload[0:4], stmtID) + binary.LittleEndian.PutUint16(payload[4:6], paramID) + copy(payload[6:], data) + return payload +} + +// TestStmtSendLongData_NoResponseAccumulatesAndExecuteUsesIt exercises the +// full COM_STMT_SEND_LONG_DATA -> COM_STMT_EXECUTE lifecycle at the packet +// level: repeated SEND_LONG_DATA calls append to the same parameter buffer, +// produce zero response bytes on the wire, and the following EXECUTE (which +// per spec omits the inline value for a long-data parameter) picks up the +// accumulated blob. +func TestStmtSendLongData_NoResponseAccumulatesAndExecuteUsesIt(t *testing.T) { + handler := &captureHandler{} + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + + stmt := &PreparedStatement{ + ID: 7, + Query: "INSERT INTO blobs (id, data) VALUES (?, ?)", + ParamCount: 1, + OriginalType: StatementInsert, + } + session := &ConnectionSession{ + ConnID: 1, + preparedStmts: map[uint32]*PreparedStatement{stmt.ID: stmt}, + } + + clientConn, serverConn := net.Pipe() + defer clientConn.Close() + defer serverConn.Close() + + // Two chunks for the same (statement_id, param_id) must accumulate. + server.handleStmtSendLongData(session, buildSendLongDataPayload(stmt.ID, 0, "hello ")) + server.handleStmtSendLongData(session, buildSendLongDataPayload(stmt.ID, 0, "world")) + require.Equal(t, []byte("hello world"), stmt.LongData[0]) + + // Per spec, COM_STMT_SEND_LONG_DATA never gets a response. Prove zero + // bytes crossed the wire before EXECUTE runs. + require.NoError(t, clientConn.SetReadDeadline(time.Now().Add(20*time.Millisecond))) + _, err := clientConn.Read(make([]byte, 1)) + require.Error(t, err, "expected no bytes written for COM_STMT_SEND_LONG_DATA") + require.NoError(t, clientConn.SetReadDeadline(time.Time{})) + + // EXECUTE with newParamsBoundFlag=1: per spec the client omits the inline + // value for a parameter that received long data, so no value bytes + // follow the 2-byte type for param 0. + execPayload := make([]byte, 13) + binary.LittleEndian.PutUint32(execPayload[0:4], stmt.ID) // statement_id + execPayload[4] = 0 // flags + binary.LittleEndian.PutUint32(execPayload[5:9], 1) // iteration_count + execPayload[9] = 0x00 // NULL bitmap: param 0 not null + execPayload[10] = 0x01 // new_params_bound_flag + execPayload[11] = 0xFC // MYSQL_TYPE_BLOB + execPayload[12] = 0x00 // param flags (unsigned bit unset) + + done := make(chan struct{}) + go func() { + server.handleStmtExecute(serverConn, session, execPayload) + close(done) + }() + + resp := mustReadPacket(t, clientConn) + <-done + require.NotEmpty(t, resp) + require.Equal(t, byte(0x00), resp[0], "expected OK packet for INSERT") + + require.Len(t, handler.params, 1) + require.Equal(t, []byte("hello world"), handler.params[0][0]) +} + +// TestHandleStmtReset_ClearsLongDataAndReturnsOK verifies COM_STMT_RESET +// clears any accumulated long data and responds with an OK packet. +func TestHandleStmtReset_ClearsLongDataAndReturnsOK(t *testing.T) { + handler := &captureHandler{} + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + + stmt := &PreparedStatement{ + ID: 9, + Query: "INSERT INTO blobs (id, data) VALUES (?, ?)", + ParamCount: 1, + LongData: map[uint16][]byte{0: []byte("stale data")}, + } + session := &ConnectionSession{ + ConnID: 1, + preparedStmts: map[uint32]*PreparedStatement{stmt.ID: stmt}, + } + + clientConn, serverConn := net.Pipe() + defer clientConn.Close() + defer serverConn.Close() + + payload := make([]byte, 4) + binary.LittleEndian.PutUint32(payload, stmt.ID) + + done := make(chan struct{}) + go func() { + server.handleStmtReset(serverConn, session, payload) + close(done) + }() + + resp := mustReadPacket(t, clientConn) + <-done + require.NotEmpty(t, resp) + require.Equal(t, byte(0x00), resp[0], "expected OK packet") + require.Nil(t, stmt.LongData) +} + +// TestHandleStmtReset_UnknownStatement verifies an ERR packet (1243) is sent +// for an unrecognized statement ID, matching the existing COM_STMT_EXECUTE +// behavior for unknown statements. +func TestHandleStmtReset_UnknownStatement(t *testing.T) { + handler := &captureHandler{} + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + session := &ConnectionSession{ + ConnID: 1, + preparedStmts: map[uint32]*PreparedStatement{}, + } + + clientConn, serverConn := net.Pipe() + defer clientConn.Close() + defer serverConn.Close() + + payload := make([]byte, 4) + binary.LittleEndian.PutUint32(payload, 999) + + done := make(chan struct{}) + go func() { + server.handleStmtReset(serverConn, session, payload) + close(done) + }() + + resp := mustReadPacket(t, clientConn) + <-done + require.NotEmpty(t, resp) + require.Equal(t, byte(0xFF), resp[0], "expected ERR packet for unknown statement") +} + +// TestParseParamValue_LONGLONG_UnsignedFitsDecodesToInt64 verifies that when +// the UNSIGNED flag is set, a LONGLONG parameter within the int64 range +// (0..math.MaxInt64) decodes to an explicit int64, not a bare uint64 - every +// downstream SQL binding path only accepts signed integers. +func TestParseParamValue_LONGLONG_UnsignedFitsDecodesToInt64(t *testing.T) { + payload := make([]byte, 8) + binary.LittleEndian.PutUint64(payload, uint64(math.MaxInt64)) + + offset, val, err := parseParamValue(payload, 0, 0x08, true) + require.NoError(t, err) + require.Equal(t, 8, offset) + + got, ok := val.(int64) + require.True(t, ok, "expected int64, got %T", val) + require.Equal(t, int64(math.MaxInt64), got) +} + +// TestParseParamValue_LONGLONG_UnsignedOutOfRange verifies that an UNSIGNED +// BIGINT value >= 2^63 - which cannot be represented as SQLite's signed +// 64-bit INTEGER - is reported as errUnsignedBigintOutOfRange rather than +// silently reinterpreted as a negative int64. +func TestParseParamValue_LONGLONG_UnsignedOutOfRange(t *testing.T) { + payload := make([]byte, 8) + binary.LittleEndian.PutUint64(payload, uint64(math.MaxInt64)+1) // 2^63 + + offset, val, err := parseParamValue(payload, 0, 0x08, true) + require.ErrorIs(t, err, errUnsignedBigintOutOfRange) + require.Nil(t, val) + require.Equal(t, 0, offset, "offset should not advance on error") + + // The true maximum (2^64-1) must also be rejected, not wrapped to -1. + binary.LittleEndian.PutUint64(payload, math.MaxUint64) + _, val, err = parseParamValue(payload, 0, 0x08, true) + require.ErrorIs(t, err, errUnsignedBigintOutOfRange) + require.Nil(t, val) +} + +// TestParseParamValue_LONGLONG_SignedUnaffected confirms the signed decode +// path is unchanged by the unsigned-flag plumbing. +func TestParseParamValue_LONGLONG_SignedUnaffected(t *testing.T) { + payload := make([]byte, 8) + binary.LittleEndian.PutUint64(payload, math.MaxUint64) + + _, val, err := parseParamValue(payload, 0, 0x08, false) + require.NoError(t, err) + require.Equal(t, int64(-1), val) +} + +// TestStmtExecute_UnsignedBigintFitsInt64_RealSQLiteDriver is a packet-level +// test through a real *sql.DB (the project's actual SQLite driver, not a +// mock): an UNSIGNED BIGINT parameter equal to math.MaxInt64 - the largest +// value representable as int64 - must decode, bind, and persist correctly. +func TestStmtExecute_UnsignedBigintFitsInt64_RealSQLiteDriver(t *testing.T) { + handler := newSQLiteExecHandler(t, "CREATE TABLE counters (id INTEGER PRIMARY KEY, big INTEGER)") + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + + stmt := &PreparedStatement{ + ID: 3, + Query: "INSERT INTO counters (id, big) VALUES (1, ?)", + ParamCount: 1, + OriginalType: StatementInsert, + } + session := &ConnectionSession{ + ConnID: 1, + preparedStmts: map[uint32]*PreparedStatement{stmt.ID: stmt}, + } + + clientConn, serverConn := net.Pipe() + defer clientConn.Close() + defer serverConn.Close() + + const want = uint64(math.MaxInt64) // 9223372036854775807 + + execPayload := make([]byte, 21) + binary.LittleEndian.PutUint32(execPayload[0:4], stmt.ID) // statement_id + execPayload[4] = 0 // flags + binary.LittleEndian.PutUint32(execPayload[5:9], 1) // iteration_count + execPayload[9] = 0x00 // NULL bitmap + execPayload[10] = 0x01 // new_params_bound_flag + execPayload[11] = 0x08 // MYSQL_TYPE_LONGLONG + execPayload[12] = 0x80 // UNSIGNED flag set + binary.LittleEndian.PutUint64(execPayload[13:21], want) // inline value + + done := make(chan struct{}) + go func() { + server.handleStmtExecute(serverConn, session, execPayload) + close(done) + }() + + resp := mustReadPacket(t, clientConn) + <-done + require.NotEmpty(t, resp) + require.Equal(t, byte(0x00), resp[0], "expected OK packet for INSERT via real driver") + + var got int64 + require.NoError(t, handler.db.QueryRow("SELECT big FROM counters WHERE id = 1").Scan(&got)) + require.Equal(t, int64(want), got) +} + +// TestStmtExecute_UnsignedBigintOutOfRange_ReturnsER1264 verifies that an +// UNSIGNED BIGINT parameter >= 2^63 produces a clean ER_WARN_DATA_OUT_OF_RANGE +// (1264) protocol error during parameter parsing, and never reaches the +// handler/driver at all - avoiding the confusing internal error +// database/sql's default converter would otherwise raise. +func TestStmtExecute_UnsignedBigintOutOfRange_ReturnsER1264(t *testing.T) { + handler := newSQLiteExecHandler(t, "CREATE TABLE counters (id INTEGER PRIMARY KEY, big INTEGER)") + server := NewMySQLServer("127.0.0.1:0", "", 0, handler) + + stmt := &PreparedStatement{ + ID: 4, + Query: "INSERT INTO counters (id, big) VALUES (2, ?)", + ParamCount: 1, + OriginalType: StatementInsert, + } + session := &ConnectionSession{ + ConnID: 1, + preparedStmts: map[uint32]*PreparedStatement{stmt.ID: stmt}, + } + + clientConn, serverConn := net.Pipe() + defer clientConn.Close() + defer serverConn.Close() + + const tooBig = uint64(math.MaxInt64) + 1 // 2^63, first value out of int64 range + + execPayload := make([]byte, 21) + binary.LittleEndian.PutUint32(execPayload[0:4], stmt.ID) + execPayload[4] = 0 + binary.LittleEndian.PutUint32(execPayload[5:9], 1) + execPayload[9] = 0x00 + execPayload[10] = 0x01 + execPayload[11] = 0x08 // MYSQL_TYPE_LONGLONG + execPayload[12] = 0x80 // UNSIGNED flag set + binary.LittleEndian.PutUint64(execPayload[13:21], tooBig) + + done := make(chan struct{}) + go func() { + server.handleStmtExecute(serverConn, session, execPayload) + close(done) + }() + + resp := mustReadPacket(t, clientConn) + <-done + require.NotEmpty(t, resp) + require.Equal(t, byte(0xFF), resp[0], "expected ERR packet") + code := binary.LittleEndian.Uint16(resp[1:3]) + require.Equal(t, uint16(1264), code, "expected ER_WARN_DATA_OUT_OF_RANGE") + + require.Empty(t, handler.queries, "handler/driver must never be reached for an unrepresentable value") + + var count int + require.NoError(t, handler.db.QueryRow("SELECT COUNT(*) FROM counters").Scan(&count)) + require.Equal(t, 0, count) +} diff --git a/protocol/transaction.go b/protocol/transaction.go index 6d00a8f7..a26882b2 100644 --- a/protocol/transaction.go +++ b/protocol/transaction.go @@ -168,6 +168,14 @@ type Statement struct { // DML ships OldValues/NewValues via CDC, not SQL+params. ExtractedParams []interface{} `msgpack:"-"` // Exclude from msgpack serialization + // ParamOrder is set only when SQL mixes caller-supplied bind placeholders + // with ExtractedParams (e.g. an auto-increment id injected alongside a + // prepared statement's own `?` marks): for every placeholder in SQL, in + // left-to-right order, true means "take the next wire-supplied value" and + // false means "take the next value from ExtractedParams". nil means only + // one of the two sources is in play - see MergeExecParams. + ParamOrder []bool `msgpack:"-"` + // ParsedAST carries the Vitess AST produced during MySQL-dialect parse. // Non-nil only when the pipeline parsed the statement via Vitess (SELECT, // DML, and most DDL). Downstream components that need the AST — notably @@ -184,6 +192,58 @@ type Statement struct { LoadDataPayload []byte `msgpack:"LoadDataPayload,omitempty"` } +// MergeExecParams produces the final positional argument list for executing +// s.SQL against SQLite, combining the caller's wire-supplied params with +// s.ExtractedParams (literals the pipeline pulled out of the SQL text, e.g. a +// server-injected auto-increment id). +// +// When s.ParamOrder is nil, exactly one of the two sources is in play - the +// common case - so whichever is non-empty is used as-is. When s.ParamOrder is +// set, SQL contains a mix of the caller's own `?` placeholders and +// pipeline-extracted ones interleaved in serialization order; ParamOrder +// records that order (true = next wireParams value, false = next +// ExtractedParams value) so the two sources are threaded back together +// positionally instead of one being silently dropped. +func (s Statement) MergeExecParams(wireParams []interface{}) []interface{} { + if len(s.ParamOrder) == 0 { + if len(wireParams) == 0 && len(s.ExtractedParams) > 0 { + return s.ExtractedParams + } + return wireParams + } + + merged := make([]interface{}, 0, len(s.ParamOrder)) + wireIdx, extractedIdx := 0, 0 + for _, fromWire := range s.ParamOrder { + if fromWire { + if wireIdx < len(wireParams) { + merged = append(merged, wireParams[wireIdx]) + } + wireIdx++ + continue + } + if extractedIdx < len(s.ExtractedParams) { + merged = append(merged, s.ExtractedParams[extractedIdx]) + } + extractedIdx++ + } + return merged +} + +// WithResolvedParams returns a copy of s for executing different SQL whose +// params are already fully resolved and positional - e.g. a vector-search +// rewrite's primary or fallback query, where params has already been +// computed from the original statement's bound values. It clears +// ParamOrder: the copy's params are complete on their own, and s.ParamOrder +// (if any) describes sql's placeholder layout, not the new one, so +// MergeExecParams must not try to interleave them against a second source. +func (s Statement) WithResolvedParams(sql string, params []interface{}) Statement { + s.SQL = sql + s.ExtractedParams = params + s.ParamOrder = nil + return s +} + // Transaction represents a buffered transaction type Transaction struct { ID uint64 diff --git a/publisher/doc.go b/publisher/doc.go index 70657364..15be6eb3 100644 --- a/publisher/doc.go +++ b/publisher/doc.go @@ -1,5 +1,5 @@ // Package publisher provides the CDC (Change Data Capture) Publishing System -// foundation for Marmot v2.9.15-beta. +// foundation for Marmot v2.9.16-beta. // // This package implements a durable, ordered event log backed by Pebble that // captures CDC events and tracks per-sink consumption cursors for reliable diff --git a/test/crash_recovery_test.go b/test/crash_recovery_test.go index 32034d29..e0a5b3c5 100644 --- a/test/crash_recovery_test.go +++ b/test/crash_recovery_test.go @@ -180,7 +180,7 @@ func (h *ClusterHarness) createNodeConfig(node *ClusterNode) { seedNodes = append(seedNodes, fmt.Sprintf("\"localhost:%d\"", baseGRPCPort+1)) } - config := fmt.Sprintf(`# Marmot v2.9.15-beta Test Node %d + config := fmt.Sprintf(`# Marmot v2.9.16-beta Test Node %d node_id = %d data_dir = "%s"