Invalidate the global cache after a redaction #296

DMRobertson · 2023-09-07T19:31:53Z

Part of #294.

TODO:

Didn't have to clean up anything in the user cache. That's surprising. Is there anything to do here? Suggest leaving it for a future patch.
Address TODOs in the diff
Unit test for the new storage method?

It is not the creation content (i.e. the content of the m.room.create event).

state/accumulator.go

sync2/handler2/handler.go

kegsay

Looks very sane.

state/accumulator.go

kegsay · 2023-09-12T11:39:04Z

state/storage.go

+	WHERE (event_type IN ('m.room.name', 'm.room.avatar', 'm.room.canonical_alias') AND state_key = '')
+	   OR (event_type = 'm.room.member' AND membership IN ('join', '_join', 'invite', '_invite'))
+	ORDER BY event_nid ASC
+	;`, metadata.RoomID)


How long does this take on m.org?

I did some testing on Matrix HQ---let me rerun and get you some numbers.

According to notes this was ~200ms for Matrix HQ. Running it now is ~270ms.

postgres=> \set room_id !OGEhHVWSdvArJzumhm:matrix.org \set snapshot_id 682264 \\ postgres=> postgres=> EXPLAIN ANALYZE postgres-> WITH snapshot(events, membership_events) AS ( postgres(> SELECT events, membership_events postgres(> FROM syncv3_snapshots postgres(> JOIN syncv3_rooms ON snapshot_id = current_snapshot_id postgres(> WHERE syncv3_rooms.room_id = :'room_id' postgres(> ) postgres-> SELECT event_id, event_type, state_key, event, membership postgres-> FROM syncv3_events JOIN snapshot ON ( postgres(> event_nid = ANY (ARRAY_CAT(events, membership_events)) postgres(> ) postgres-> WHERE (event_type IN ('m.room.name', 'm.room.avatar', 'm.room.canonical_alias') AND state_key = '') postgres-> OR (event_type = 'm.room.member' AND membership IN ('join', '_join', 'invite', '_invite')) postgres-> ORDER BY event_nid ASC postgres-> ; QUERY PLAN ---------------------------------------------------------------------------------------------------------------------------------------------- --------------------------------------------------------------------------------------------------- Sort (cost=24.28..24.28 rows=2 width=501) (actual time=246.204..258.618 rows=48046 loops=1) Sort Key: syncv3_events.event_nid Sort Method: external merge Disk: 19976kB -> Nested Loop (cost=1.15..24.27 rows=2 width=501) (actual time=17.091..200.713 rows=48046 loops=1) -> Nested Loop (cost=0.71..16.75 rows=1 width=271) (actual time=0.032..0.034 rows=1 loops=1) -> Index Scan using syncv3_rooms_pkey on syncv3_rooms (cost=0.29..8.30 rows=1 width=8) (actual time=0.020..0.021 rows=1 loops =1) Index Cond: (room_id = '!OGEhHVWSdvArJzumhm:matrix.org'::text) -> Index Scan using syncv3_snapshots_snapshot_id_room_id_key on syncv3_snapshots (cost=0.42..8.44 rows=1 width=279) (actual t ime=0.010..0.010 rows=1 loops=1) Index Cond: (snapshot_id = syncv3_rooms.current_snapshot_id) -> Index Scan using syncv3_events_pkey on syncv3_events (cost=0.44..7.50 rows=2 width=501) (actual time=15.542..192.561 rows=48046 loops=1) Index Cond: (event_nid = ANY (array_cat(syncv3_snapshots.events, syncv3_snapshots.membership_events))) Filter: (((event_type = ANY ('{m.room.name,m.room.avatar,m.room.canonical_alias}'::text[])) AND (state_key = ''::text)) OR ((ev ent_type = 'm.room.member'::text) AND (membership = ANY ('{join,_join,invite,_invite}'::text[])))) Rows Removed by Filter: 53701 Planning Time: 2.699 ms Execution Time: 268.729 ms (15 rows)

Eyeballing it, the ORDER BY adds ~70ms, which surprised me! (I want to process the Nids in ascending order to pick out the heroes.)

I wonder if it might be quicker to do the sorting in Go? Suggest this is good enough to leave as-is for now though.

(Also I'm surprised that we use the 6 newest heroes, not the 6 oldest)

sync3/handler/handler.go

kegsay · 2023-09-12T11:44:21Z

sync3/caches/global.go

+	}
+
+	logger.Warn().Any("before", metadata).Msgf("DMR: invalidate GLOBALLLLLL room %s", roomID)
+	err := c.store.ResetMetadataState(metadata)


Presumably needs to be written back to c.roomIDToMetadata?

Here metadata is a pointer and I'm updating *metadata in-place. We have a lock on roomIDToMetadataMu in the caller so this might be safe. I wasn't completely sure though, see e.g.

sliding-sync/state/storage.go

Lines 311 to 312 in 1b14ac3

// TODO: could be brittle. Might be better to just create a new RoomMetadata from scratch

// and copy over the fields we don't want to reload.

From discussion elsewhere:

it should be safe so long as you're doing that from the subscriber goroutine, and not per-conn goroutines

DMRobertson · 2023-09-12T14:56:57Z

state/storage.go

+		return fmt.Errorf("ResetMetadataState[%s]: %w", metadata.RoomID, err)
+	}
+
+	heroMemberships := circularSlice[*Event]{max: 6}


Note: when populating the heroes I only care about the 6 most recent heroes. However to get accurate join and invite counts we effectively need to pull all the members out.

If we're only worrying about redactions then there aren't gonna be any count changes. But as #294 says there's the more general problem of "our state is completely out of date".

Happens a lot of the time, e.g. if we know that a non-proxy user has joined the room.

DMRobertson · 2023-09-12T16:56:09Z

state/accumulator.go

+			SELECT COUNT(*)
+			FROM syncv3_events
+			    JOIN syncv3_snapshots ON event_nid = ANY (ARRAY_CAT(events, membership_events))
+			WHERE snapshot_id = $1 AND event_id = ANY($2)


Sanity checked the query plan as follows:

\set room_id !OGEhHVWSdvArJzumhm:matrix.org \\ \set snapshotID 756886 \\ \set event1 $liLBpCuSUVPqPVsff3jCFT9VN9dW551TiDFx5QE8MmI \\ \set event2 $ARQ3jrwiMsX1pLeDIjvOW_ga4CucgAnghrzHWwlULS0 \\ EXPLAIN ANALYZE SELECT COUNT(*) FROM syncv3_events JOIN syncv3_snapshots ON event_nid = ANY (ARRAY_CAT(events, membership_events)) WHERE snapshot_id = :snapshotID AND event_id = ANY (ARRAY[:'event1', :'event2']) ; QUERY PLAN ---------------------------------------------------------------------------------------------------------------------------------------------- --------------------------- Aggregate (cost=25.64..25.65 rows=1 width=8) (actual time=3.044..3.045 rows=1 loops=1) -> Nested Loop (cost=0.99..25.64 rows=1 width=0) (actual time=1.495..3.040 rows=2 loops=1) Join Filter: (syncv3_events.event_nid = ANY (array_cat(syncv3_snapshots.events, syncv3_snapshots.membership_events))) -> Index Scan using syncv3_snapshots_snapshot_id_room_id_key on syncv3_snapshots (cost=0.42..8.44 rows=1 width=271) (actual time=0. 024..0.025 rows=1 loops=1) Index Cond: (snapshot_id = 756886) -> Index Scan using syncv3_events_event_id_key on syncv3_events (cost=0.56..17.15 rows=2 width=8) (actual time=0.024..0.038 rows=2 loops=1) Index Cond: (event_id = ANY ('{$liLBpCuSUVPqPVsff3jCFT9VN9dW551TiDFx5QE8MmI,$ARQ3jrwiMsX1pLeDIjvOW_ga4CucgAnghrzHWwlULS0}'::tex t[])) Planning Time: 0.577 ms Execution Time: 3.073 ms (9 rows)

kegsay

Excellent

David Robertson added 3 commits September 7, 2023 18:34

creationContent -> reqBody

956ef54

It is not the creation content (i.e. the content of the m.room.create event).

Redaction E2E test case

7ccc488

Define V2InvalidateRoom

be78e6f

DMRobertson commented Sep 7, 2023

View reviewed changes

state/accumulator.go Outdated Show resolved Hide resolved

DMRobertson commented Sep 7, 2023

View reviewed changes

sync2/handler2/handler.go Outdated Show resolved Hide resolved

David Robertson added 3 commits September 7, 2023 20:41

Factor out AccumulateResult struct

777cb35

Emit Invalidation payloads

f4555f3

Tell global and user caches to invalidate

c1cc0eb

DMRobertson force-pushed the dmr/cache-invalidation branch from 63c56da to c1cc0eb Compare September 7, 2023 19:41

David Robertson added 9 commits September 8, 2023 18:17

Invalidation: don't bother propagating a snapshot

afe5899

Make circularSlice generic

773a28c

Fix test: correctly set alias

6df13a1

Reload global metadata

6628cd3

Fix bad fmt strings

636d85b

Expose Domain on CSAPI

57d8c1d

WIP more tests

f47267b

Fix heroes pointer nonsense

1b14ac3

Fix the displayname test handling

0bc33f3

DMRobertson changed the title ~~WIP invalidate cached room data after a redaction~~ Invalidate the global cache after a redaction Sep 12, 2023

DMRobertson marked this pull request as ready for review September 12, 2023 11:32

kegsay reviewed Sep 12, 2023

View reviewed changes

DMRobertson commented Sep 12, 2023

View reviewed changes

David Robertson added 4 commits September 12, 2023 16:25

Remove debug and improve comments

0d4e8c6

Let the dispatcher dispatch to the global cache

6f37ac0

Check to see if we redacted a current state event

897410e

Don't warn about missing receivers

115ae1d

Happens a lot of the time, e.g. if we know that a non-proxy user has joined the room.

DMRobertson commented Sep 12, 2023

View reviewed changes

David Robertson added 2 commits September 12, 2023 18:11

Use the txn's connection!!

1cc2c35

Unit test the cache reload emission logic

e83a9d6

DMRobertson requested a review from kegsay September 12, 2023 18:11

DMRobertson mentioned this pull request Sep 13, 2023

Don't load events when there's a gap between known events #300

Merged

kegsay approved these changes Sep 14, 2023

View reviewed changes

Update comment

ea9cfc3

DMRobertson merged commit 5b32cc4 into main Sep 14, 2023
6 checks passed

DMRobertson deleted the dmr/cache-invalidation branch September 14, 2023 10:08

DMRobertson mentioned this pull request Oct 4, 2023

Cache invalidation #294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalidate the global cache after a redaction #296

Invalidate the global cache after a redaction #296

DMRobertson commented Sep 7, 2023 •

edited

Loading

kegsay left a comment

kegsay Sep 12, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 12, 2023

kegsay Sep 12, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 14, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 12, 2023

DMRobertson Sep 12, 2023

kegsay left a comment

	// TODO: could be brittle. Might be better to just create a new RoomMetadata from scratch
	// and copy over the fields we don't want to reload.

Invalidate the global cache after a redaction #296

Invalidate the global cache after a redaction #296

Conversation

DMRobertson commented Sep 7, 2023 • edited Loading

kegsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

DMRobertson commented Sep 7, 2023 •

edited

Loading