Review comments @Kegsay

roomserver/storage/postgres/deltas/2021041615092700_state_blocks_refactor.go

@@ -66,6 +66,11 @@ func UpStateBlocksRefactor(tx *sql.Tx) error {
 	if _, err := tx.Exec(`ALTER TABLE roomserver_state_snapshots RENAME TO _roomserver_state_snapshots;`); err != nil {
 		return fmt.Errorf("tx.Exec: %w", err)
 	}
+	// We create new sequences starting with the maximum state snapshot and block NIDs.
+	// This means that all newly created snapshots and blocks by the migration will have
+	// NIDs higher than these values, so that when we come to update the references to
+	// these NIDs using UPDATE statements, we can guarantee we are only ever updating old
+	// values and not accidentally overwriting new ones.
 	if _, err := tx.Exec(fmt.Sprintf(`CREATE SEQUENCE roomserver_state_block_nid_sequence START WITH %d;`, maxblockid)); err != nil {
 		return fmt.Errorf("tx.Exec: %w", err)
 	}

roomserver/storage/postgres/events_table.go

@@ -89,6 +89,9 @@ const bulkSelectStateEventByIDSQL = "" +
 	" WHERE event_id = ANY($1)" +
 	" ORDER BY event_type_nid, event_state_key_nid ASC"
 
+// Bulk look up of events by event NID, optionally filtering by the event type
+// or event state key NIDs if provided. (The CARDINALITY check will return true
+// if the provided arrays are empty, ergo no filtering).
 const bulkSelectStateEventByNIDSQL = "" +
 	"SELECT event_type_nid, event_state_key_nid, event_nid FROM roomserver_events" +
 	" WHERE event_nid = ANY($1)" +
@@ -282,7 +285,7 @@ func (s *eventStatements) BulkSelectStateEventByNID(
 	if err = rows.Err(); err != nil {
 		return nil, err
 	}
-	return results, nil
+	return results[:i], nil
 }
 
 // bulkSelectStateAtEventByID lookups the state at a list of events by event ID.

roomserver/storage/postgres/state_block_table.go

@@ -41,12 +41,21 @@ const stateDataSchema = `
 -- which in turn makes it easier to merge state data blocks.
 CREATE SEQUENCE IF NOT EXISTS roomserver_state_block_nid_seq;
 CREATE TABLE IF NOT EXISTS roomserver_state_block (
+	-- The state snapshot NID that identifies this snapshot.
 	state_block_nid bigint PRIMARY KEY DEFAULT nextval('roomserver_state_block_nid_seq'),
+	-- The hash of the state block, which is used to enforce uniqueness. The hash is
+	-- generated in Dendrite and passed through to the database, as a btree index over 
+	-- this column is cheap and fits within the maximum index size.
 	state_block_hash BYTEA UNIQUE,
+	-- The event NIDs contained within the state block.
 	event_nids bigint[] NOT NULL
 );
 `
 
+// Insert a new state block. If we conflict on the hash column then
+// we must perform an update so that the RETURNING statement returns the
+// ID of the row that we conflicted with, so that we can then refer to
+// the original block.
 const insertStateDataSQL = "" +
 	"INSERT INTO roomserver_state_block (state_block_hash, event_nids)" +
 	" VALUES ($1, $2)" +

roomserver/storage/postgres/state_snapshot_table.go

@@ -41,13 +41,23 @@ const stateSnapshotSchema = `
 -- the full state under single state_block_nid.
 CREATE SEQUENCE IF NOT EXISTS roomserver_state_snapshot_nid_seq;
 CREATE TABLE IF NOT EXISTS roomserver_state_snapshots (
+	-- The state snapshot NID that identifies this snapshot.
 	state_snapshot_nid bigint PRIMARY KEY DEFAULT nextval('roomserver_state_snapshot_nid_seq'),
+	-- The hash of the state snapshot, which is used to enforce uniqueness. The hash is
+	-- generated in Dendrite and passed through to the database, as a btree index over 
+	-- this column is cheap and fits within the maximum index size.
 	state_snapshot_hash BYTEA UNIQUE,
+	-- The room NID that the snapshot belongs to.
 	room_nid bigint NOT NULL,
+	-- The state blocks contained within this snapshot.
 	state_block_nids bigint[] NOT NULL
 );
 `
 
+// Insert a new state snapshot. If we conflict on the hash column then
+// we must perform an update so that the RETURNING statement returns the
+// ID of the row that we conflicted with, so that we can then refer to
+// the original snapshot.
 const insertStateSQL = "" +
 	"INSERT INTO roomserver_state_snapshots (state_snapshot_hash, room_nid, state_block_nids)" +
 	" VALUES ($1, $2, $3)" +

roomserver/storage/shared/storage.go

@@ -278,7 +278,7 @@ func (d *Database) StateEntries(
 	if err != nil {
 		return nil, fmt.Errorf("d.StateBlockTable.BulkSelectStateBlockEntries: %w", err)
 	}
-	lists := []types.StateEntryList{}
+	lists := make([]types.StateEntryList, 0, len(entries))
 	for i, entry := range entries {
 		eventNIDs, err := d.EventsTable.BulkSelectStateEventByNID(ctx, entry, nil)
 		if err != nil {

roomserver/storage/sqlite3/events_table.go

@@ -291,7 +291,7 @@ func (s *eventStatements) BulkSelectStateEventByNID(
 			return nil, err
 		}
 	}
-	return results, err
+	return results[:i], err
 }
 
 // bulkSelectStateAtEventByID lookups the state at a list of events by event ID.

roomserver/storage/sqlite3/state_block_table.go

@@ -33,12 +33,21 @@ import (
 
 const stateDataSchema = `
   CREATE TABLE IF NOT EXISTS roomserver_state_block (
+	-- The state snapshot NID that identifies this snapshot.
     state_block_nid INTEGER PRIMARY KEY AUTOINCREMENT,
+	-- The hash of the state block, which is used to enforce uniqueness. The hash is
+	-- generated in Dendrite and passed through to the database, as a btree index over 
+	-- this column is cheap and fits within the maximum index size.
 	state_block_hash BLOB UNIQUE,
+	-- The event NIDs contained within the state block, encoded as JSON.
     event_nids TEXT NOT NULL DEFAULT '[]'
   );
 `
 
+// Insert a new state block. If we conflict on the hash column then
+// we must perform an update so that the RETURNING statement returns the
+// ID of the row that we conflicted with, so that we can then refer to
+// the original block.
 const insertStateDataSQL = `
 	INSERT INTO roomserver_state_block (state_block_hash, event_nids)
 		VALUES ($1, $2)

roomserver/storage/sqlite3/state_snapshot_table.go

@@ -32,13 +32,23 @@ import (
 
 const stateSnapshotSchema = `
   CREATE TABLE IF NOT EXISTS roomserver_state_snapshots (
+	-- The state snapshot NID that identifies this snapshot.
     state_snapshot_nid INTEGER PRIMARY KEY AUTOINCREMENT,
+	-- The hash of the state snapshot, which is used to enforce uniqueness. The hash is
+	-- generated in Dendrite and passed through to the database, as a btree index over 
+	-- this column is cheap and fits within the maximum index size.
 	state_snapshot_hash BLOB UNIQUE,
+	-- The room NID that the snapshot belongs to.
     room_nid INTEGER NOT NULL,
+	-- The state blocks contained within this snapshot, encoded as JSON.
     state_block_nids TEXT NOT NULL DEFAULT '[]'
   );
 `
 
+// Insert a new state snapshot. If we conflict on the hash column then
+// we must perform an update so that the RETURNING statement returns the
+// ID of the row that we conflicted with, so that we can then refer to
+// the original snapshot.
 const insertStateSQL = `
 	INSERT INTO roomserver_state_snapshots (state_snapshot_hash, room_nid, state_block_nids)
 	  VALUES ($1, $2, $3)