Allow use of the /filter Client-Server APIs on workers.

[reivilibre]

Since clients (Element Web at least) check and set /filters before doing basic operations like /sync, this endpoint being married to the master process is a single point of failure.

By my reading, the endpoint itself would Just Work™ on a worker today, as long as we enabled it. That's what I'm proposing in this PR.

Base: develop

Original commit schedule, with full messages:

1. Collapse FilteringStore into FilteringWorkerStore
   This is safe because all transactions do not write to any streams

   or otherwise do anything racy.

2. Register /filter servlets on workers too

[clokep]

I think we also need to update:

• docker/configure_workers_and_start.py so complement uses this
• https://github.com/matrix-org/sytest/blob/d1c931af1179848526d323f3955a9f21f81d600f/lib/SyTest/Homeserver/Synapse.pm#L1257 so sytest uses this.

[clokep]

And yes this is annoying, see #12139.

[reivilibre]

→ matrix-org/sytest#1331

[clokep]

(Just starting a thread somewhere).

The REST servlet to add a filter:

1. Checks the filter JSON against what's in the database and if an exact match is found it returns the same ID (i.e. you cannot have duplicate filters w/ different IDs).
2. Otherwise it, finds the maximum current filter ID and increments by 1.
3. Inserts the new filter.

I think this change might break step 1 -- if we have two requests to insert a filter, they can both race and return no known filter in step 1, then both insert the filter with different IDs?

[clokep]

(Note the only index is UNIQUE across (user_id, filter_id).)

[reivilibre]

You were almost right (I think) that there was a phantom write here. But funnily enough, step 1 won't be broken because step 2 will fix it. The reason I say this is:

• we run transactions at the REPEATABLE READ isolation level by default and do not override the default for this txn
• For your race condition to hold, Step 1 for both transactions needs to happen concurrently so neither sees the other's filter.
• Both transactions will therefore see the same maximum filter ID in step 2 — because of REPEATABLE READ and step (1)
• Both transactions will attempt to insert a filter with the same ID in step 3. One of them won't be able to.

When trying in psql, the second INSERT blocks until the first has committed, but then gives a unique key constraint violation. I don't think this is one of the errors we retry a transaction for, so maybe there is a problem here after all.

syn7=*> INSERT INTO user_filters (user_id, filter_id, filter_json) VALUES ('@user:syn7', 1, '{}');
ERROR:  duplicate key value violates unique constraint "user_filters_unique"
DETAIL:  Key (user_id, filter_id)=(@user:syn7, 1) already exists.

[clokep]

Ah, yes good call with the isolation level we're using. 👍

I think we don't retry that, but I'm not sure. I guess using a sequence here would have been better? (Would that have solved our issues?)

[reivilibre]

(to be fair, this exact same issue already existed if the user made two requests at once! A single worker can still run two transactions concurrently)

[reivilibre]

Yes using a SEQUENCE or a SERIAL field would have saved this effort. I guess the original author really liked the idea of a user's filters starting at 1 and then incrementing by 1, rather than having the same space of IDs shared by all users.

[clokep]

Another way of thinking of this would have been randomly generating them, like we do in a few other spots. (Incrementing IDs are usually a API smell.)

[reivilibre]

(what's wrong with incrementing IDs?)

[clokep]

(what's wrong with incrementing IDs?)

If they're scoped by user it is likely fine. If they're not then it can (potentially) leak information about other users (or your service as a whole). It also makes it easier to guess IDs.