hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
67 stars 44 forks source link

Optimize get active communities query #8887

Closed timolegros closed 1 month ago

timolegros commented 1 month ago

Description

The following query is the worst-performing query in the database:

    SELECT
        c.id,
        COUNT(DISTINCT t.id) AS topic_count,
        c.profile_count,
        COUNT(DISTINCT cs.community_id) AS community_stake_count
    FROM
        "Communities" c
        LEFT JOIN "Topics" t ON c.id = t.community_id
        LEFT JOIN "Addresses" a ON c.id = a.community_id
        LEFT JOIN "CommunityStakes" cs ON c.id = cs.community_id
        LEFT JOIN "CommunityTags" ct ON c.id = ct.community_id
    WHERE
      c.active = true
    GROUP BY
        c.id
    HAVING
      (
        COUNT(DISTINCT t.id) > 0
        AND
        COUNT(DISTINCT a.id) >= 10
      ) OR
      COUNT(DISTINCT cs.community_id) > 0;

The average duration is 819ms which is almost 5 times worse than the next slowest query.

This query is found in packages/commonwealth/server/controllers/server_communities_methods/get_active_communities.ts

Additional context

Datadog dashboard query

We may be able to replace all usage of this route with a modified version of trpc.getCommunities.

jnaviask commented 1 month ago

@timolegros we should replace calls to this with trpc.getCommunities when possible cc @mzparacha for use case required

timolegros commented 1 month ago

@timolegros we should replace calls to this with trpc.getCommunities when possible cc @mzparacha for use case required

Yes, may be able to eliminate this query entirely. The worst part of it is COUNT(DISTINCT a.id) >= 10 which I think we could replace with Community.profile_count as a filter in the getCommunities query. This would boost performance but it would also be a product improvement since the current query selects communities with a single user that has 10 addresses vs 10 individual users.

mzparacha commented 1 month ago

@timolegros we should replace calls to this with trpc.getCommunities when possible cc @mzparacha for use case required

In https://github.com/hicommonwealth/commonwealth/pull/8877, we already replaced the server active communities route with trpc.getCommunities.

cc: @jnaviask @timolegros

timolegros commented 1 month ago

@timolegros we should replace calls to this with trpc.getCommunities when possible cc @mzparacha for use case required

In #8877, we already replaced the server active communities route with trpc.getCommunities.

cc: @jnaviask @timolegros

@mzparacha are you sure? I'm still seeing usage of useFetchActiveCommunitiesQuery which doesn't use trpc.getCommunities:

mzparacha commented 1 month ago

the pr isn't merged yet. But yeah it removed the active communities query from the communities page.

Thanks for identifying https://github.com/hicommonwealth/commonwealth/blob/master/packages/commonwealth/client/scripts/views/pages/user_dashboard/TrendingCommunitiesPreview/TrendingCommunitiesPreview.tsx#L14, that isn't removed in my pr, but I have a ticket https://github.com/hicommonwealth/commonwealth/issues/8760 to remove that.

timolegros commented 1 month ago

the pr isn't merged yet. But yeah it removed the active communities query from the communities page.

Thanks for identifying master/packages/commonwealth/client/scripts/views/pages/user_dashboard/TrendingCommunitiesPreview/TrendingCommunitiesPreview.tsx#L14, that isn't removed in my pr, but I have a ticket #8760 to remove that.

Ah oops my bad didn't notice it was still in draft lol :smile:. Does your PR remove this unused route or should I ticket that up as a follow-up to your PR?

mzparacha commented 1 month ago

Nw, there is not a ticket for that yet, so u can prob create one. But that would be blocked by https://github.com/hicommonwealth/commonwealth/issues/8760