fix(compat): close 21 defects blocking real MySQL clients

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 \

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{

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")
+	}
+}

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
 # ==============================================================================

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
 # ==============================================================================

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())
+}

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, &notNull, &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])
+}