Filter added to Admin-API GET /rooms

[afechler]

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.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct
  (run the linters)

Signed-off-by: Alexander Fechler alexander.fechler@bwi.de

[dklimpel]

It is not clear to me what the exception is supposed to do here and where the output should be. Wouldn't a SynapseError and a log output make more sense?

[afechler]

My fault. I had only used the code fragment during development. I remove the exception handling.

[dklimpel]

As a user, it is not really clear to me what is meant here. The documentation in the code is more meaningful.
Precisely because the bools have three states, since None is apparently not equal to false or true.

[afechler]

I have adjusted the parameter names and descriptions.

[dklimpel]

Is it possible to estimate whether two more joins will have a significant impact on performance?

[anoadragon453]

We can EXPLAIN ANALYZE the query on a sizeable Matrix deployment, such as matrix.org.

Running the following query:

EXPLAIN ANALYZE SELECT count(*) FROM (
    SELECT room_id FROM room_stats_state state
    INNER JOIN room_stats_current curr USING (room_id)
    INNER JOIN rooms USING (room_id)
    WHERE LOWER(state.name) LIKE '%hello%'
        OR LOWER(state.canonical_alias) LIKE '%hello%'
        OR state.room_id = '!xyz:domain.com'
) AS get_room_ids;

which gives us:

                                                                                    QUERY PLAN                              
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Finalize Aggregate  (cost=248961.11..248961.12 rows=1 width=8) (actual time=708.339..714.750 rows=1 loops=1)
   ->  Gather  (cost=248960.90..248961.11 rows=2 width=8) (actual time=708.028..714.726 rows=3 loops=1)
         Workers Planned: 2
         Workers Launched: 2
         ->  Partial Aggregate  (cost=247960.90..247960.91 rows=1 width=8) (actual time=687.989..687.991 rows=1 loops=3)
               ->  Nested Loop  (cost=1.12..247930.10 rows=12320 width=0) (actual time=8.294..687.832 rows=658 loops=3)
                     Join Filter: (state.room_id = curr.room_id)
                     ->  Nested Loop  (cost=0.56..240275.86 rows=12320 width=62) (actual time=8.262..682.422 rows=658 loops=3)
                           ->  Parallel Seq Scan on room_stats_state state  (cost=0.00..212549.39 rows=12320 width=31) (actual time=8.210..676.798 rows=658 loops=3)
                                 Filter: ((lower(name) ~~ '%hello%'::text) OR (lower(canonical_alias) ~~ '%hello%'::text) OR (room_id = '!xyz:domain.com'::text))
                                 Rows Removed by Filter: 3097391
                           ->  Index Only Scan using rooms_pkey on rooms  (cost=0.56..2.25 rows=1 width=31) (actual time=0.008..0.008 rows=1 loops=1975)
                                 Index Cond: (room_id = state.room_id)
                                 Heap Fetches: 212
                     ->  Index Only Scan using room_stats_current_pkey on room_stats_current curr  (cost=0.56..0.61 rows=1 width=31) (actual time=0.007..0.007 rows=1 loops=1975)
                           Index Cond: (room_id = rooms.room_id)
                           Heap Fetches: 1144
 Planning Time: 0.830 ms
 JIT:
   Functions: 38
   Options: Inlining false, Optimization false, Expressions true, Deforming true
   Timing: Generation 2.227 ms, Inlining 0.000 ms, Optimization 0.961 ms, Emission 22.621 ms, Total 25.808 ms
 Execution Time: 715.623 ms
(23 rows)

(Note that I only analyzed after ensuring that a bare EXPLAIN would not be too costly.)

This is indeed fairly slow, and takes up a connection for nearly a whole second. As these filters are only using data from room_stats_state_current, do we really need the second INNER JOIN rooms USING (room_id)?

[afechler]

I based the implementation on existing code (e.g. get_users_paginate_txn) The second join is used to determine the number of public or non-public rooms in the case of filtering. Perhaps it is also possible to determine the state of the flag via the events, but I would assume that this is not faster.

[anoadragon453]

I see. In that case rooms is required. I've created a separate issue to track the slow performance of these queries: #17330

Otherwise this codepath is only hit for the /rooms admin API. Thus I think we can live with a slower, but more useful query.

[anoadragon453]

Overall this looks useful, thank you!

I agree with @dklimpel's review and have a couple more comments to add.

[anoadragon453]

We can EXPLAIN ANALYZE the query on a sizeable Matrix deployment, such as matrix.org.

Running the following query:

EXPLAIN ANALYZE SELECT count(*) FROM (
    SELECT room_id FROM room_stats_state state
    INNER JOIN room_stats_current curr USING (room_id)
    INNER JOIN rooms USING (room_id)
    WHERE LOWER(state.name) LIKE '%hello%'
        OR LOWER(state.canonical_alias) LIKE '%hello%'
        OR state.room_id = '!xyz:domain.com'
) AS get_room_ids;

which gives us:

                                                                                    QUERY PLAN                              
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Finalize Aggregate  (cost=248961.11..248961.12 rows=1 width=8) (actual time=708.339..714.750 rows=1 loops=1)
   ->  Gather  (cost=248960.90..248961.11 rows=2 width=8) (actual time=708.028..714.726 rows=3 loops=1)
         Workers Planned: 2
         Workers Launched: 2
         ->  Partial Aggregate  (cost=247960.90..247960.91 rows=1 width=8) (actual time=687.989..687.991 rows=1 loops=3)
               ->  Nested Loop  (cost=1.12..247930.10 rows=12320 width=0) (actual time=8.294..687.832 rows=658 loops=3)
                     Join Filter: (state.room_id = curr.room_id)
                     ->  Nested Loop  (cost=0.56..240275.86 rows=12320 width=62) (actual time=8.262..682.422 rows=658 loops=3)
                           ->  Parallel Seq Scan on room_stats_state state  (cost=0.00..212549.39 rows=12320 width=31) (actual time=8.210..676.798 rows=658 loops=3)
                                 Filter: ((lower(name) ~~ '%hello%'::text) OR (lower(canonical_alias) ~~ '%hello%'::text) OR (room_id = '!xyz:domain.com'::text))
                                 Rows Removed by Filter: 3097391
                           ->  Index Only Scan using rooms_pkey on rooms  (cost=0.56..2.25 rows=1 width=31) (actual time=0.008..0.008 rows=1 loops=1975)
                                 Index Cond: (room_id = state.room_id)
                                 Heap Fetches: 212
                     ->  Index Only Scan using room_stats_current_pkey on room_stats_current curr  (cost=0.56..0.61 rows=1 width=31) (actual time=0.007..0.007 rows=1 loops=1975)
                           Index Cond: (room_id = rooms.room_id)
                           Heap Fetches: 1144
 Planning Time: 0.830 ms
 JIT:
   Functions: 38
   Options: Inlining false, Optimization false, Expressions true, Deforming true
   Timing: Generation 2.227 ms, Inlining 0.000 ms, Optimization 0.961 ms, Emission 22.621 ms, Total 25.808 ms
 Execution Time: 715.623 ms
(23 rows)

(Note that I only analyzed after ensuring that a bare EXPLAIN would not be too costly.)

This is indeed fairly slow, and takes up a connection for nearly a whole second. As these filters are only using data from room_stats_state_current, do we really need the second INNER JOIN rooms USING (room_id)?

[anoadragon453]

Thanks for the changes. This now LGTM!

[anoadragon453]

I see. In that case rooms is required. I've created a separate issue to track the slow performance of these queries: #17330

Otherwise this codepath is only hit for the /rooms admin API. Thus I think we can live with a slower, but more useful query.