Don't remember enabled of deleted push rules and properly return 404 for missing push rules in .../actions and .../enabled

[reivilibre]

Fixes #1475. Fixes #1531. Requires MSC2663.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[reivilibre]

Going to open for review even though it depends on MSC2663

[richvdh]

I think you win an award for ingenuity in finding different ways to misspell "non-existent".

generally looks good, a few bits and bobs.

[richvdh]

still a few things here

[reivilibre]

So the write skew stuff is an interesting problem.

I posed a question: will this execution flow cause us a dangling enabled row on REPEATABLE READ isolation level as used in Synapse?

1: BEGIN -- set_enabled
2: BEGIN -- delete_push_rule
1: SELECT push_rule WHERE id=a -- found!
2: DELETE push_rule WHERE id=a
2: DELETE push_rule_enabled WHERE id=a -- (0 deleted)
1: INSERT INTO push_rule_enabled (id=a, ...)
1: COMMIT
2: COMMIT

To set up an experiment, I got out psql and tried:

CREATE TABLE push_rules1(id TEXT PRIMARY KEY);
CREATE TABLE push_rules_enabled1(id TEXT PRIMARY KEY, enabled BOOL);
INSERT INTO push_rules1 VALUES ('a');

then I got two psqls up and did this test:

1 $ BEGIN ISOLATION LEVEL REPEATABLE READ;
2 $ BEGIN ISOLATION LEVEL REPEATABLE READ;
1 $ SELECT * FROM push_rules1 WHERE id='a';
 id 
----
 a
(1 row)
2 $ DELETE FROM push_rules1 WHERE id='a';
DELETE 1
2 $ DELETE FROM push_rules_enabled1 WHERE id='a';
DELETE 0
1 $ INSERT INTO push_rules_enabled1 VALUES ('a', TRUE);
INSERT 0 1
1 $ COMMIT;
COMMIT
2 $ COMMIT;
COMMIT

$ SELECT * FROM push_rules1;
 id 
----
(0 rows)

$ SELECT * FROM push_rules_enabled1;
 id | enabled 
----+---------
 a  | t
(1 row)

oups!

OK, so no real surprise, we get a serialisation anomaly here (a write skew, I believe)

now with SELECT FOR KEY SHARE, which will lock the row such that it cannot be deleted until we unlock it.

1 $ BEGIN ISOLATION LEVEL REPEATABLE READ;
2 $ BEGIN ISOLATION LEVEL REPEATABLE READ;
1 $ SELECT * FROM push_rules1 WHERE id='a' FOR KEY SHARE;
 id 
----
 a
(1 row)
2 $ DELETE FROM push_rules1 WHERE id='a';
<blocks>
-- 2 is blocked so can't run other query yet.
1 $ INSERT INTO push_rules_enabled1 VALUES ('a', TRUE);
INSERT 0 1
1 $ COMMIT;
-- now 2 unblocks
2 $ DELETE FROM push_rules_enabled1 WHERE id='a';
DELETE 0 — urr
2 $ COMMIT;

-- and then:

$ SELECT * FROM push_rules1;
 id 
----
(0 rows)

$ SELECT * FROM push_rules_enabled1;
 id | enabled 
----+---------
 a  | t
(1 row)

So, grumble grumble, FOR KEY SHARE isn't a silver bullet!

I can't think of a way to re-order these to make it race-safe…

I believe you'd have to go SERIALISABLE instead of REPEATABLE READ if you cared about no dangling enabled rows… Is there something I'm missing?

[richvdh]

generally looks good!

Top marks for the comprehensive set of tests.

[richvdh]

can you expand on this comment? why do we need it? (also: was there a conclusion after #7796 (comment)? sounds like it might not help very much?)

[reivilibre]

Thanks!

Yes, I did some experiments and got Erik's advice.

By making all push_rules rows need a push_rules_enable row, we can make the database properly lock and cause conflicts on the delete transaction.

It seems impossible to lock a row that doesn't exist, so without that invariant, it is possible to be left with a dangling push_rules_enable row.

For key share will lock on the push_rule itself, which seems helpful too.

[richvdh]

lgtm, thanks!

sytests would have been nice, but hohum.