Delete messages from device_inbox table when deleting device

[dklimpel]

Fixes: #9346
Replaces: #10098

Related:

• to_device messages aimed at a local device are not removed after that device is deleted #9346
• Synapse accepts to_device messages to hidden devices #9348
• Stop synapse from saving messages in device_inbox for hidden devices. #10097
• Streams tables are never cleared out #5888
• device_inbox never gets emptied #3599

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)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[DMRobertson]

Fixes #3599

I think this actually fixes #9346 (?)

[dklimpel]

Fixes #3599

I think this actually fixes #9346 (?)

I have changed it.

[DMRobertson]

Looks good to me. I'm not too familiar with this kind of db wrangling in synapse though, so I'd appreciate another pair of eyes from @matrix-org/synapse-core.

[dklimpel]

Looks good to me. I'm not too familiar with this kind of db wrangling in synapse though, so I'd appreciate another pair of eyes from @matrix-org/synapse-core.

This is my first bg task. It was not very easy for me.

If this PR is merged, I can work on

• Stop synapse from saving messages in device_inbox for hidden devices. #10097

[DMRobertson]

This is my first bg task. It was not very easy for me.

It's much appreciated! I forgot to say earlier: thank you for picking up where the previous PR left off!

Is there anything we could have done to make it easier for you? E.g. documentation that was lacking?

[dklimpel]

At the moment there is no doc for big tasks. But it is an very special topic.

There are missing unit tests for device_inbox. That is the reason why I am poke in database table in unit tests. This is not my favorite style.
Most time is write unit tests. But it is necessary.

[clokep]

Looks like a good start! Thanks for tackling something new to you, background updates are definitely a bit under documented at the moment.

[dklimpel]

Is it important which class contains the bg task remove_deleted_devices_from_device_inbox?
DeviceBackgroundUpdateStore or DeviceInboxBackgroundUpdateStore

And which one is the correct one?

[clokep]

:( I forgot that device IDs are not globally unique, so I think we need to ensure that these queries handle (user_id, device_Id) as the unique data.

Unfortunately I think that complicates the deletion clause quite a bit.

[dklimpel]

I think about the best query and did some tests.

---

left join and distinct

explain SELECT DISTINCT i.device_id, i.user_id
FROM   device_inbox AS i
LEFT   JOIN devices as d
on d.device_id = i.device_id and d.user_id = i.user_id
WHERE  d.device_id IS NULL and d.user_id IS NULL;
                                               QUERY PLAN
--------------------------------------------------------------------------------------------------------
 Unique  (cost=32671.54..32671.55 rows=1 width=61)
   ->  Sort  (cost=32671.54..32671.55 rows=1 width=61)
         Sort Key: i.device_id, i.user_id
         ->  Gather  (cost=1292.03..32671.53 rows=1 width=61)
               Workers Planned: 2
               ->  Hash Anti Join  (cost=292.02..31671.43 rows=1 width=61)
                     Hash Cond: ((i.device_id = d.device_id) AND (i.user_id = d.user_id))
                     ->  Parallel Seq Scan on device_inbox i  (cost=0.00..30665.66 rows=95166 width=61)
                     ->  Hash  (cost=220.01..220.01 rows=4801 width=62)
                           ->  Seq Scan on devices d  (cost=0.00..220.01 rows=4801 width=62)

Results has 515 rows
Same cost if I use group by instead distinct.

---

left join

explain SELECT i.device_id, i.user_id
FROM   device_inbox AS i
LEFT   JOIN devices as d
on d.device_id = i.device_id and d.user_id = i.user_id
WHERE  d.device_id IS NULL and d.user_id IS NULL;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Gather  (cost=1292.03..32671.53 rows=1 width=61)
   Workers Planned: 2
   ->  Hash Anti Join  (cost=292.02..31671.43 rows=1 width=61)
         Hash Cond: ((i.device_id = d.device_id) AND (i.user_id = d.user_id))
         ->  Parallel Seq Scan on device_inbox i  (cost=0.00..30665.66 rows=95166 width=61)
         ->  Hash  (cost=220.01..220.01 rows=4801 width=62)
               ->  Seq Scan on devices d  (cost=0.00..220.01 rows=4801 width=62)

Result has 51353 rows

---

left join in sub query and distinct

explain with dev as (
SELECT i.device_id, i.user_id
FROM   device_inbox AS i
LEFT   JOIN devices as d
on d.device_id = i.device_id and d.user_id = i.user_id
WHERE  d.device_id IS NULL and d.user_id IS NULL
limit 100
)
select distinct * from dev;
                                                  QUERY PLAN
--------------------------------------------------------------------------------------------------------------
 Unique  (cost=32671.55..32671.56 rows=1 width=61)
   ->  Sort  (cost=32671.55..32671.56 rows=1 width=61)
         Sort Key: i.device_id, i.user_id
         ->  Limit  (cost=1292.03..32671.53 rows=1 width=61)
               ->  Gather  (cost=1292.03..32671.53 rows=1 width=61)
                     Workers Planned: 2
                     ->  Hash Anti Join  (cost=292.02..31671.43 rows=1 width=61)
                           Hash Cond: ((i.device_id = d.device_id) AND (i.user_id = d.user_id))
                           ->  Parallel Seq Scan on device_inbox i  (cost=0.00..30665.66 rows=95166 width=61)
                           ->  Hash  (cost=220.01..220.01 rows=4801 width=62)
                                 ->  Seq Scan on devices d  (cost=0.00..220.01 rows=4801 width=62)

Result has 6 rows

---

not in

explain SELECT i.device_id, i.user_id
FROM   device_inbox AS i
WHERE  ( i.device_id, i.user_id )
         NOT IN (SELECT device_id,
                        user_id
                 FROM   devices);
                                QUERY PLAN
--------------------------------------------------------------------------
 Seq Scan on device_inbox i  (cost=232.01..33372.00 rows=114200 width=61)
   Filter: (NOT (hashed SubPlan 1))
   SubPlan 1
     ->  Seq Scan on devices  (cost=0.00..220.01 rows=4801 width=62)

Result has 51353 rows

---

not in and distinct

explain SELECT Distinct i.device_id, i.user_id
FROM   device_inbox AS i
WHERE  ( i.device_id, i.user_id )
         NOT IN (SELECT device_id,
                        user_id
                 FROM   devices);
                                   QUERY PLAN
--------------------------------------------------------------------------------
 HashAggregate  (cost=33943.00..34171.18 rows=22818 width=61)
   Group Key: i.device_id, i.user_id
   ->  Seq Scan on device_inbox i  (cost=232.01..33372.00 rows=114200 width=61)
         Filter: (NOT (hashed SubPlan 1))
         SubPlan 1
           ->  Seq Scan on devices  (cost=0.00..220.01 rows=4801 width=62)

Result has 515 rows

---

not in in sub query with and distinct with limit

explain
with dev as (
SELECT i.device_id, i.user_id
FROM   device_inbox AS i
WHERE  ( i.device_id, i.user_id )
         NOT IN (SELECT device_id,
                        user_id
                 FROM   devices)
)
select distinct * from dev limit 100;
                                      QUERY PLAN
--------------------------------------------------------------------------------------
 Limit  (cost=33983.86..33984.86 rows=100 width=61)
   ->  HashAggregate  (cost=33983.86..34212.31 rows=22845 width=61)
         Group Key: i.device_id, i.user_id
         ->  Seq Scan on device_inbox i  (cost=232.03..33412.17 rows=114338 width=61)
               Filter: (NOT (hashed SubPlan 1))
               SubPlan 1
                 ->  Seq Scan on devices  (cost=0.00..220.02 rows=4802 width=62)

---

not in in sub query with limit and distinct

explain
with dev as (
SELECT i.device_id, i.user_id
FROM   device_inbox AS i
WHERE  ( i.device_id, i.user_id )
         NOT IN (SELECT device_id,
                        user_id
                 FROM   devices)
limit 100)
select distinct * from dev;
                                      QUERY PLAN
--------------------------------------------------------------------------------------
 HashAggregate  (cost=262.53..263.53 rows=100 width=61)
   Group Key: i.device_id, i.user_id
   ->  Limit  (cost=232.01..261.03 rows=100 width=61)
         ->  Seq Scan on device_inbox i  (cost=232.01..33372.00 rows=114200 width=61)
               Filter: (NOT (hashed SubPlan 1))
               SubPlan 1
                 ->  Seq Scan on devices  (cost=0.00..220.01 rows=4801 width=62)

Result has 6 rows

---

IMO the last one is the best solution. The disadvantage is that the cleanup of the database can take a lot of time, but with small costs.

@clokep Do you have better suggestions or an other opinion?

[dklimpel]

not exists

explain SELECT i.device_id, i.user_id
FROM   device_inbox AS i
WHERE  NOT EXISTS (
   SELECT  device_id, user_id
   FROM   devices as d
   WHERE  d.device_id=i.device_id and d.user_id=i.user_id
   );
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Gather  (cost=1292.03..32671.53 rows=1 width=61)
   Workers Planned: 2
   ->  Hash Anti Join  (cost=292.02..31671.43 rows=1 width=61)
         Hash Cond: ((i.device_id = d.device_id) AND (i.user_id = d.user_id))
         ->  Parallel Seq Scan on device_inbox i  (cost=0.00..30665.66 rows=95166 width=61)
         ->  Hash  (cost=220.01..220.01 rows=4801 width=62)
               ->  Seq Scan on devices d  (cost=0.00..220.01 rows=4801 width=62)

Result has 51353 rows

[JohannesKleine]

explain delete from device_inbox where device_id not in (select device_id from devices) AND stream_id >= 230020 AND stream_id <230030;
                                                     QUERY PLAN                                                     
--------------------------------------------------------------------------------------------------------------------
 Delete on device_inbox  (cost=0.99..4982976.37 rows=266 width=6)
   ->  Index Scan using device_inbox_stream_id_user_id on device_inbox  (cost=0.99..4982976.37 rows=266 width=6)
         Index Cond: ((stream_id >= 230020) AND (stream_id < 230030))
         Filter: (NOT (SubPlan 1))
         SubPlan 1
           ->  Materialize  (cost=0.42..18196.19 rows=200267 width=20)
                 ->  Index Only Scan using device_uniqueness on devices  (cost=0.42..16020.85 rows=200267 width=20)
 JIT:
   Functions: 8
   Options: Inlining true, Optimization true, Expressions true, Deforming true
(10 rows)

This is the way it works for me. As you can see it uses the already existing index which speeds up the process. Otherwise it will easily run over days / weeks and blocking the database when writing the result for a long time.

[JohannesKleine]

The trick is, to get the highest stream_id, generate small chunks (1000 is a good value) of stream_id ranges and execute the query as background update. Hope this helps, i tested it with a bash script manually to find good values, but was unable to write it in python, so i am very happy to see someone is continuing. I dropped in my pull request the background job related things.

[dklimpel]

The device_id is not unique. Multiple users can have the same device_id. Unique is the tuple of device_id and user_id.

[dklimpel]

explain select device_id, user_id from device_inbox where device_id not in (select device_id from devices);
                               QUERY PLAN
------------------------------------------------------------------------
 Seq Scan on device_inbox  (cost=232.03..32840.47 rows=114338 width=61)
   Filter: (NOT (hashed SubPlan 1))
   SubPlan 1
     ->  Seq Scan on devices  (cost=0.00..220.02 rows=4802 width=11)

[JohannesKleine]

The device_id is not unique. Multiple users can have the same device_id. Unique is the tuple of device_id and user_id.

ok, my posted delete query is wrong

[dklimpel]

Cost of the delete

explain DELETE FROM device_inbox WHERE device_id = 'dummy' AND user_id = 'dummy';
                                              QUERY PLAN
------------------------------------------------------------------------------------------------------
 Delete on device_inbox  (cost=0.42..8.44 rows=1 width=6)
   ->  Index Scan using device_inbox_user_stream_id on device_inbox  (cost=0.42..8.44 rows=1 width=6)
         Index Cond: ((user_id = 'dummy'::text) AND (device_id = 'dummy'::text))

[dklimpel]

I have no streaam_id 230000.

explain delete from device_inbox where device_id not in (select device_id from devices) AND stream_id >= 230020 AND stream_id <230030;
                                                 QUERY PLAN
-------------------------------------------------------------------------------------------------------------
 Delete on device_inbox  (cost=232.44..240.47 rows=1 width=6)
   ->  Index Scan using device_inbox_stream_id_user_id on device_inbox  (cost=232.44..240.47 rows=1 width=6)
         Index Cond: ((stream_id >= 230020) AND (stream_id < 230030))
         Filter: (NOT (hashed SubPlan 1))
         SubPlan 1
           ->  Seq Scan on devices  (cost=0.00..220.02 rows=4802 width=11)

---

explain delete from device_inbox where device_id not in (select device_id from devices);
                                 QUERY PLAN
-----------------------------------------------------------------------------
 Delete on device_inbox  (cost=232.03..32840.47 rows=114338 width=6)
   ->  Seq Scan on device_inbox  (cost=232.03..32840.47 rows=114338 width=6)
         Filter: (NOT (hashed SubPlan 1))
         SubPlan 1
           ->  Seq Scan on devices  (cost=0.00..220.02 rows=4802 width=11)

---

explain delete from device_inbox where device_id not in (select device_id from devices) AND stream_id >= 20 AND stream_id <23030;
                                        QUERY PLAN
------------------------------------------------------------------------------------------
 Delete on device_inbox  (cost=232.03..33983.86 rows=45320 width=6)
   ->  Seq Scan on device_inbox  (cost=232.03..33983.86 rows=45320 width=6)
         Filter: ((NOT (hashed SubPlan 1)) AND (stream_id >= 20) AND (stream_id < 23030))
         SubPlan 1
           ->  Seq Scan on devices  (cost=0.00..220.02 rows=4802 width=11)

[dklimpel]

I am not sure if it necessary to call the update pogress.
No intermediate status is required for processing this task.
If I am add something like WHERE uiser_id > to the SQL query, the costs rise immensely.

[JohannesKleine]

Could you run my explain query on your database to get a better cost comparison?

[dklimpel]

See: #10969 (comment)

[erikjohnston]

(Sorry for taking so long to get to this)

[erikjohnston]

My concern with this approach is that it starts off fast (as it will quickly find rows to delete), but will become increasingly heavyweight until at the end it will do a full sequential scan of device_inbox.

I think a simple fix for that is to order by stream_id and then track the latest stream_id that was reached (via the progress json):

SELECT device_id, user_id, stream_id
FROM device_inbox
WHERE
    stream_id >= ?
    AND (device_id, user_id) NOT IN (
        SELECT device_id, user_id FROM devices
    )
ORDER BY stream_id
LIMIT ?

(Note that since stream_id is not unique we need to use an inclusive stream_id >= ? clause, since we might not have deleted all dead device messages for the stream_id returned from the previous query)

I would also then delete only rows matching the (user_id, device_id, stream_id) tuple, to avoid problems of deleting a large number of rows all at once due to a single device having lots of device messages (which I think is common).

[dklimpel]

Ok. I work on it. This is similar to my first approach in a543a27#diff-3811c5dc2d8444a4922451939dbd64b55056168b6055094c27207cd7ff809552
Also: comment

But we have more informations now.

[dklimpel]

Or should it be easier like:

Suggested change

	{
	"device_id": rows[-1][0],
	"user_id": rows[-1][1],
	"stream_id": rows[-1][2],
	},
	row[-1],

[erikjohnston]

I think the current code is a bit clearer tbh

[erikjohnston]

Thanks!