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

DMRobertson · 2023-09-13T11:28:16Z

Fixes #283
Part of #294.
Is going to conflict with #296.

Recommend commitwise review. I was going to write an E2E test, but it's fiddley to get a gappy sync deterministically, so opted for an integration test.

state/event_table.go

state/migrations/20230913120537_events_missing_previous.sql

kegsay

Overall methodology looks great.

state/event_table.go

state/migrations/20230913120537_events_missing_previous.sql

internal/types.go

kegsay · 2023-09-14T08:47:20Z

state/accumulator.go

+	// filled in by other pollers.
+	//
+	//  A:  E1 persisted, E2 omitted and unknown, E3 unknown       (missing previous)
+	//  B:  E1 persisted, E2 omitted and   known, E3 unknown   (not missing previous)


Terminology is too confusing. Hard to grok what:

omitted

unknown

missing previous

are all supposed to mean. The proxy has either seen the event or not, why do we have 3^2 states?

There are two edge cases I have in mind here. Starting with the simpler of the two:

Case B. Suppose Alice's poller sees E1, E2, and E3 all in the same timeline. All is well; nothing is MissingPrevious. Then suppose Bob's poller sees E1 in a first sync, followed by a limited (gappy) sync beginning with E3. In this situation:

The Accumulate logic doesn't know that E2 directly preceeds E3. (For all it knows, there could be an E2.5 between them.) Therefore it has to consider E3 as MissingPrevious.

However the proxy already knows that E2 comes before E3. Therefore E3 should not be marked as MissingPrevious in the database. (This should be fine because the event upsert logic says ON CONFLICT DO NOTHING.)

Case A. Suppose that Alice's poller sees E1 and E2 in the same timeline. Neither is MissingPrevious. Suppose Bob's sees the same two responses as before: E1 in a first sync, followed by a limited (gappy) sync beginning with E3. This time:

The Accumulate logic still doesn't know that E2 directly preceeds E3, and has to consider E3 as MissingPrevious.

Nor does the proxy already knows that E2 comes before E3. Therefore E3 should be marked as MissingPrevious in the database.

A follow-up: if some future poller (for Chris, say) saw E2 and E3 adacent in a timeline it could upsert the row for E3 to mark it as not MissingPrevious. However I'm not sure that's worth the effort.

a5c02b2 tries to explain this better.

state/accumulator_test.go

state/accumulator.go

state/event_table.go

state/event_table_test.go

tests-integration/poller_test.go

DMRobertson · 2023-09-20T11:04:57Z

Upgrade test is failing. After the upgrade:

Sync v3 [0.99.10] (262f8cb)
Debug=true LogLevel= MaxConns=0
panic: pq: column "missing_previous" of relation "syncv3_events" does not exist

goroutine 1 [running]:
github.com/jmoiron/sqlx.MustExec(...)
	/home/runner/go/pkg/mod/github.com/jmoiron/[email protected]/sqlx.go:722
github.com/jmoiron/sqlx.(*DB).MustExec(0x30?, {0xdbf993?, 0xc0003c4c60?}, {0x0?, 0xc0002ef900?, 0x40fd47?})
	/home/runner/go/pkg/mod/github.com/jmoiron/[email protected]/sqlx.go:366 +0x46
github.com/matrix-org/sliding-sync/state.NewEventTable(...)
	/home/runner/work/sliding-sync/sliding-sync/state/event_table.go:116
github.com/matrix-org/sliding-sync/state.NewStorageWithDB(0xc0003c4c90, 0x0)
	/home/runner/work/sliding-sync/sliding-sync/state/storage.go:85 +0x70
github.com/matrix-org/sliding-sync.Setup({0xc00004248e?, 0xd7fc8b?}, {0xc00004806a, 0x4c}, {0xc00003c00e, 0xa}, {0x0, 0x0, 0x0, 0x3b9aca00, ...})
	/home/runner/work/sliding-sync/sliding-sync/v3.go:106 +0x189
main.main()
	/home/runner/work/sliding-sync/sliding-sync/cmd/syncv3/main.go:213 +0x175b

which is

sliding-sync/state/event_table.go

Lines 114 to 132 in 4aa0667

    
           func NewEventTable(db *sqlx.DB) *EventTable { 
        
           	// make sure tables are made 
        
           	db.MustExec(` 
        
           	CREATE SEQUENCE IF NOT EXISTS syncv3_event_nids_seq; 
        
           	CREATE TABLE IF NOT EXISTS syncv3_events ( 
        
           		event_nid BIGINT PRIMARY KEY NOT NULL DEFAULT NEXTVAL('syncv3_event_nids_seq'), 
        
           		event_id TEXT NOT NULL UNIQUE, 
        
           		before_state_snapshot_id BIGINT NOT NULL DEFAULT 0, 
        
           		-- which nid gets replaced in the snapshot with event_nid 
        
           		event_replaces_nid BIGINT NOT NULL DEFAULT 0, 
        
           		room_id TEXT NOT NULL, 
        
           		event_type TEXT NOT NULL, 
        
           		state_key TEXT NOT NULL, 
        
           		prev_batch TEXT, 
        
           		membership TEXT, 
        
           		is_state BOOLEAN NOT NULL, -- is this event part of the v2 state response? 
        
           		event BYTEA NOT NULL, 
        
           		missing_previous BOOLEAN NOT NULL DEFAULT FALSE 
        
           	);

That's a surprise to me because I thought CREATE TABLE IF NOT EXISTS would be a no-op if the table already exists!

DMRobertson · 2023-09-20T11:24:44Z

That's a surprise to me because I thought CREATE TABLE IF NOT EXISTS would be a no-op if the table already exists!

Oh this will be the COMMENT ON..., won't it?

kegsay

Let's give it a go.

kegsay · 2023-09-20T12:04:51Z

sync2/client.go

@@ -4,7 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/matrix-org/sliding-sync/internal"
+	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"


Why has this appeared?

Some kind of merge faffing with #301 I think?

state/event_table.go

kegsay · 2023-09-20T12:12:49Z

state/event_table_test.go

 		},
 	}

 	for _, tc := range testcases {
+		// We're using the notation (X, Y] for a half-open interval excluding X but including Y.


I personally prefer rust's notation of X..=Y

X..=Y includes X, but I want to exclude X here.

It could be worse---the French style of this notation is ]X, Y] (!)

tests-integration/poller_test.go

kegsay · 2023-09-20T12:21:55Z

state/event_table_test.go

-		{ID: "G", MissingPrevious: true},
-		{ID: "H", MissingPrevious: true},
+		{ID: "G-gap", MissingPrevious: true},
+		{ID: "H-gap", MissingPrevious: true},


I think I'm just having a lot of mental block here because I see A B C D and instinctively think there is no gap. When you then put H-gap I think "uhh before or after"? Can we +2 letters to hint at the gap maybe? E.g A B C F-gap?

Hmm. I think rename these to e.g. chunkX-eventY , so that gaps only appear when we change chunks.

state/event_table_test.go

I'm sympathetic, but let's not stress about it now.

DMRobertson · 2023-09-20T13:29:52Z

Only EW failed, but it suceeded on the prior commit. The only change I made was in unit tests, so I assume this is some Cypressy flake.

DMRobertson force-pushed the dmr/invalidate-timelines branch from d2ac630 to c10afdf Compare September 13, 2023 11:31

DMRobertson commented Sep 13, 2023

View reviewed changes

state/event_table.go Outdated Show resolved Hide resolved

DMRobertson commented Sep 13, 2023

View reviewed changes

state/migrations/20230913120537_events_missing_previous.sql Show resolved Hide resolved

DMRobertson force-pushed the dmr/invalidate-timelines branch 4 times, most recently from 1a654bf to f6783af Compare September 13, 2023 18:16

DMRobertson changed the title ~~WIP: don't load events when there's a gap between known events~~ WIP: Don't load events when there's a gap between known events Sep 13, 2023

David Robertson added 5 commits September 13, 2023 19:17

Add syncv3_events.missing_previous

16796db

Pass TimelineResponse struct around

df01e50

Set missing_parents field in the DB

a65a69b

Test helper driver-by comment

3150c17

Stop loading timelines if you hit missing_previous

fa227b7

DMRobertson force-pushed the dmr/invalidate-timelines branch from f6783af to fa227b7 Compare September 13, 2023 18:17

DMRobertson marked this pull request as ready for review September 13, 2023 18:24

kegsay reviewed Sep 14, 2023

View reviewed changes

David Robertson added 9 commits September 19, 2023 12:40

Merge branch 'main' into dmr/invalidate-timelines

957bdee

Merge remote-tracking branch 'origin/main' into dmr/invalidate-timelines

d9ecbbb

Move TimelineResponse back to sync2

d3ba1f1

Fixup merges

a4c90fb

Accumulator.Accumulate: tweak variable names

ed83ab9

Tweak test prose

41ed56e

Review comment

4aa0667

Improve comment

a5c02b2

Integration test

4da0813

DMRobertson commented Sep 19, 2023

View reviewed changes

tests-integration/poller_test.go Outdated Show resolved Hide resolved

DMRobertson changed the title ~~WIP: Don't load events when there's a gap between known events~~ Don't load events when there's a gap between known events Sep 19, 2023

David Robertson added 3 commits September 20, 2023 11:13

run-tests.sh: timeout after a minute

afdb0d7

Faff with CI config

921ed6e

Maybe this is how you increment an integer?

bc5aa54

Ditch COMMENT ON

e06a543

kegsay approved these changes Sep 20, 2023

View reviewed changes

David Robertson added 4 commits September 20, 2023 13:36

Undo GoLand's SQL string formatting

3e02510

I'm sympathetic, but let's not stress about it now.

Assert we fetch the correct prev batch

507b262

A bunch of comments from review

ed7f052

Ditch the alphabet

f264c4f

DMRobertson merged commit e75a462 into main Sep 20, 2023
6 of 7 checks passed

DMRobertson deleted the dmr/invalidate-timelines branch September 20, 2023 13:29

DMRobertson mentioned this pull request Sep 13, 2023

Cache invalidation #294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DMRobertson commented Sep 13, 2023 •

edited

Loading

kegsay left a comment

kegsay Sep 14, 2023

DMRobertson Sep 19, 2023

DMRobertson Sep 19, 2023

DMRobertson commented Sep 20, 2023 •

edited

Loading

DMRobertson commented Sep 20, 2023

kegsay left a comment

kegsay Sep 20, 2023

DMRobertson Sep 20, 2023

kegsay Sep 20, 2023

DMRobertson Sep 20, 2023

DMRobertson Sep 20, 2023

kegsay Sep 20, 2023

DMRobertson Sep 20, 2023

DMRobertson commented Sep 20, 2023

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

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

Conversation

DMRobertson commented Sep 13, 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

DMRobertson commented Sep 20, 2023 • edited Loading

DMRobertson commented Sep 20, 2023

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

DMRobertson commented Sep 20, 2023

DMRobertson commented Sep 13, 2023 •

edited

Loading

DMRobertson commented Sep 20, 2023 •

edited

Loading