sqlite: fixes from sytest (#872)

* bugfix: fix panic on new invite events from sytest

I'm unsure why the previous code didn't work, but it's
clearer, quicker and easier to read the `LastInsertID()` way.
Previously, the code would panic as the SELECT would fail
to find the last inserted row ID.

* sqlite: Fix UNIQUE violations and close more cursors

- Add missing `defer rows.Close()`
- Do not have the state block NID as a PRIMARY KEY else it breaks for blocks
  with >1 state event in them. Instead, rejig the queries so we can still
  have monotonically increasing integers without using AUTOINCREMENT (which
  mandates PRIMARY KEY).

* sqlite: Add missing variadic function

* Use LastInsertId because empirically it works over the SELECT form (though I don't know why that is)

* sqlite: Fix invite table by using the global stream pos rather than one specific to invites

If we don't use the global, clients don't get notified about any invites
because the position is too low.

* linting: shadowing

* sqlite: do not use last rowid, we already know the stream pos!

* sqlite: Fix account data table in syncapi by commiting insert txns!

* sqlite: Fix failing federation invite

Was failing with 'database is locked' due to multiple write txns
being taken out.

* sqlite: Ensure we return exactly the number of events found in the database

Previously we would return exactly the number of *requested* events, which
meant that several zero-initialised events would bubble through the system,
failing at JSON serialisation time.

* sqlite: let's just ignore the problem for now....

* linting

roomserver/storage/sqlite3/invite_table.go

@@ -52,16 +52,18 @@ const selectInviteActiveForUserInRoomSQL = "" +
 // However the matrix protocol doesn't give us a way to reliably identify the
 // invites that were retired, so we are forced to retire all of them.
 const updateInviteRetiredSQL = `
-	UPDATE roomserver_invites SET retired = TRUE
-	  WHERE room_nid = $1 AND target_nid = $2 AND NOT retired;
-	SELECT invite_event_id FROM roomserver_invites
-		WHERE rowid = last_insert_rowid();
+	UPDATE roomserver_invites SET retired = TRUE WHERE room_nid = $1 AND target_nid = $2 AND NOT retired
+`
+
+const selectInvitesAboutToRetireSQL = `
+SELECT invite_event_id FROM roomserver_invites WHERE room_nid = $1 AND target_nid = $2 AND NOT retired
 `
 
 type inviteStatements struct {
 	insertInviteEventStmt               *sql.Stmt
 	selectInviteActiveForUserInRoomStmt *sql.Stmt
 	updateInviteRetiredStmt             *sql.Stmt
+	selectInvitesAboutToRetireStmt      *sql.Stmt
 }
 
 func (s *inviteStatements) prepare(db *sql.DB) (err error) {
@@ -74,6 +76,7 @@ func (s *inviteStatements) prepare(db *sql.DB) (err error) {
 		{&s.insertInviteEventStmt, insertInviteEventSQL},
 		{&s.selectInviteActiveForUserInRoomStmt, selectInviteActiveForUserInRoomSQL},
 		{&s.updateInviteRetiredStmt, updateInviteRetiredSQL},
+		{&s.selectInvitesAboutToRetireStmt, selectInvitesAboutToRetireSQL},
 	}.prepare(db)
 }
 
@@ -102,7 +105,8 @@ func (s *inviteStatements) updateInviteRetired(
 	ctx context.Context,
 	txn *sql.Tx, roomNID types.RoomNID, targetUserNID types.EventStateKeyNID,
 ) (eventIDs []string, err error) {
-	stmt := common.TxStmt(txn, s.updateInviteRetiredStmt)
+	// gather all the event IDs we will retire
+	stmt := txn.Stmt(s.selectInvitesAboutToRetireStmt)
 	rows, err := stmt.QueryContext(ctx, roomNID, targetUserNID)
 	if err != nil {
 		return nil, err
@@ -110,11 +114,15 @@ func (s *inviteStatements) updateInviteRetired(
 	defer (func() { err = rows.Close() })()
 	for rows.Next() {
 		var inviteEventID string
-		if err := rows.Scan(&inviteEventID); err != nil {
+		if err = rows.Scan(&inviteEventID); err != nil {
 			return nil, err
 		}
 		eventIDs = append(eventIDs, inviteEventID)
 	}
+
+	// now retire the invites
+	stmt = txn.Stmt(s.updateInviteRetiredStmt)
+	_, err = stmt.ExecContext(ctx, roomNID, targetUserNID)
 	return
 }
 

roomserver/storage/sqlite3/room_aliases_table.go

@@ -103,6 +103,8 @@ func (s *roomAliasesStatements) selectAliasesFromRoomID(
 		return
 	}
 
+	defer rows.Close() // nolint: errcheck
+
 	for rows.Next() {
 		var alias string
 		if err = rows.Scan(&alias); err != nil {

roomserver/storage/sqlite3/state_block_table.go

@@ -30,7 +30,7 @@ import (
 
 const stateDataSchema = `
   CREATE TABLE IF NOT EXISTS roomserver_state_block (
-    state_block_nid INTEGER PRIMARY KEY AUTOINCREMENT,
+    state_block_nid INTEGER NOT NULL,
     event_type_nid INTEGER NOT NULL,
     event_state_key_nid INTEGER NOT NULL,
     event_nid INTEGER NOT NULL,
@@ -43,10 +43,7 @@ const insertStateDataSQL = "" +
 	" VALUES ($1, $2, $3, $4)"
 
 const selectNextStateBlockNIDSQL = `
-	SELECT COALESCE((
-		SELECT seq+1 AS state_block_nid FROM sqlite_sequence
-		WHERE name = 'roomserver_state_block'), 1
-	) AS state_block_nid
+SELECT IFNULL(MAX(state_block_nid), 0) + 1 FROM roomserver_state_block
 `
 
 // Bulk state lookup by numeric state block ID.
@@ -98,11 +95,19 @@ func (s *stateBlockStatements) prepare(db *sql.DB) (err error) {
 
 func (s *stateBlockStatements) bulkInsertStateData(
 	ctx context.Context, txn *sql.Tx,
-	stateBlockNID types.StateBlockNID,
 	entries []types.StateEntry,
-) error {
+) (types.StateBlockNID, error) {
+	if len(entries) == 0 {
+		return 0, nil
+	}
+	var stateBlockNID types.StateBlockNID
+	err := txn.Stmt(s.selectNextStateBlockNIDStmt).QueryRowContext(ctx).Scan(&stateBlockNID)
+	if err != nil {
+		return 0, err
+	}
+
 	for _, entry := range entries {
-		_, err := common.TxStmt(txn, s.insertStateDataStmt).ExecContext(
+		_, err := txn.Stmt(s.insertStateDataStmt).ExecContext(
 			ctx,
 			int64(stateBlockNID),
 			int64(entry.EventTypeNID),
@@ -110,20 +115,10 @@ func (s *stateBlockStatements) bulkInsertStateData(
 			int64(entry.EventNID),
 		)
 		if err != nil {
-			return err
+			return 0, err
 		}
 	}
-	return nil
-}
-
-func (s *stateBlockStatements) selectNextStateBlockNID(
-	ctx context.Context,
-	txn *sql.Tx,
-) (types.StateBlockNID, error) {
-	var stateBlockNID int64
-	selectStmt := common.TxStmt(txn, s.selectNextStateBlockNIDStmt)
-	err := selectStmt.QueryRowContext(ctx).Scan(&stateBlockNID)
-	return types.StateBlockNID(stateBlockNID), err
+	return stateBlockNID, nil
 }
 
 func (s *stateBlockStatements) bulkSelectStateBlockEntries(

roomserver/storage/sqlite3/storage.go

@@ -54,7 +54,12 @@ func Open(dataSourceName string) (*Database, error) {
 	}
 	//d.db.Exec("PRAGMA journal_mode=WAL;")
 	//d.db.Exec("PRAGMA read_uncommitted = true;")
-	d.db.SetMaxOpenConns(2)
+
+	// FIXME: We are leaking connections somewhere. Setting this to 2 will eventually
+	// cause the roomserver to be unresponsive to new events because something will
+	// acquire the global mutex and never unlock it because it is waiting for a connection
+	// which it will never obtain.
+	d.db.SetMaxOpenConns(20)
 	if err = d.statements.prepare(d.db); err != nil {
 		return nil, err
 	}
@@ -253,12 +258,13 @@ func (d *Database) Events(
 ) ([]types.Event, error) {
 	var eventJSONs []eventJSONPair
 	var err error
-	results := make([]types.Event, len(eventNIDs))
+	var results []types.Event
 	err = common.WithTransaction(d.db, func(txn *sql.Tx) error {
 		eventJSONs, err = d.statements.bulkSelectEventJSON(ctx, txn, eventNIDs)
 		if err != nil || len(eventJSONs) == 0 {
 			return nil
 		}
+		results = make([]types.Event, len(eventJSONs))
 		for i, eventJSON := range eventJSONs {
 			result := &results[i]
 			result.EventNID = eventJSON.EventNID
@@ -286,13 +292,10 @@ func (d *Database) AddState(
 	err = common.WithTransaction(d.db, func(txn *sql.Tx) error {
 		if len(state) > 0 {
 			var stateBlockNID types.StateBlockNID
-			stateBlockNID, err = d.statements.selectNextStateBlockNID(ctx, txn)
+			stateBlockNID, err = d.statements.bulkInsertStateData(ctx, txn, state)
 			if err != nil {
 				return err
 			}
-			if err = d.statements.bulkInsertStateData(ctx, txn, stateBlockNID, state); err != nil {
-				return err
-			}
 			stateBlockNIDs = append(stateBlockNIDs[:len(stateBlockNIDs):len(stateBlockNIDs)], stateBlockNID)
 		}
 		stateNID, err = d.statements.insertState(ctx, txn, roomNID, stateBlockNIDs)
@@ -602,8 +605,9 @@ func (d *Database) StateEntriesForTuples(
 // MembershipUpdater implements input.RoomEventDatabase
 func (d *Database) MembershipUpdater(
 	ctx context.Context, roomID, targetUserID string,
-) (types.MembershipUpdater, error) {
-	txn, err := d.db.Begin()
+) (updater types.MembershipUpdater, err error) {
+	var txn *sql.Tx
+	txn, err = d.db.Begin()
 	if err != nil {
 		return nil, err
 	}
@@ -611,6 +615,18 @@ func (d *Database) MembershipUpdater(
 	defer func() {
 		if !succeeded {
 			txn.Rollback() // nolint: errcheck
+		} else {
+			// TODO: We should be holding open this transaction but we cannot have
+			// multiple write transactions on sqlite. The code will perform additional
+			// write transactions independent of this one which will consistently cause
+			// 'database is locked' errors. For now, we'll break up the transaction and
+			// hope we don't race too catastrophically. Long term, we should be able to
+			// thread in txn objects where appropriate (either at the interface level or
+			// bring matrix business logic into the storage layer).
+			txerr := txn.Commit()
+			if err == nil && txerr != nil {
+				err = txerr
+			}
 		}
 	}()
 
@@ -624,7 +640,7 @@ func (d *Database) MembershipUpdater(
 		return nil, err
 	}
 
-	updater, err := d.membershipUpdaterTxn(ctx, txn, roomNID, targetUserNID)
+	updater, err = d.membershipUpdaterTxn(ctx, txn, roomNID, targetUserNID)
 	if err != nil {
 		return nil, err
 	}
@@ -658,7 +674,8 @@ func (d *Database) membershipUpdaterTxn(
 	}
 
 	return &membershipUpdater{
-		transaction{ctx, txn}, d, roomNID, targetUserNID, membership,
+		// purposefully set the txn to nil so if we try to use it we panic and fail fast
+		transaction{ctx, nil}, d, roomNID, targetUserNID, membership,
 	}, nil
 }