Optimize space supporter filter for v2 calls

[will-gant]

UPDATE: I've completely changed this optimization since first opening the PR so this description is out of date (I'm leaving it as a record of how this has progressed) - see comments below for latest details

A short explanation of the proposed change:

Optimizes the checks that prevent users with only a space supporter role from accessing the v2 API, cutting around 7% off the overall querytime when there are large tables.

This PR uses Sequel's any? method instead of the existing check, which retrieves .all of a given user's roles and then, in the CC's ruby code, counts them to see whether the user has exactly 1 space supporter role and zero of all other role types.

The method will also now return false right away, without querying the space supporters table, if the user has any non-supporter role.

An explanation of the use cases your change solves

The existing check currently adds a pretty significant delay to every v2 call on large foundations, and this change slightly mitigates that.

I've run the old and the new queries in postgres with EXPLAIN ANALYZE 100 times each on a foundation with about 75k users and 646k roles. Averages from the results are presented in the table below (times in ms):

	Old	New
Planning time	1.50904	1.06917
Execution time	36.42587	34.286
Overall time	37.93491	35.35517

If the user lacks any non-supporter role, we then have to separately check the supporters table. That little query has a planning time of 0.186ms, and execution 1.366ms. So we're still a smidgen faster even for the minority of requests that also need to execute that.

So it's a pretty modest improvement, but given that this is middleware that gets executed for all v2 calls, I think cumulatively it should be a useful saving.

Links to any other associated PRs

n/a

• I have reviewed the contributing guide

• I have viewed, signed, and submitted the Contributor License Agreement

• I have made this pull request to the main branch

• I have run all the unit tests using bundle exec rake

• I have run CF Acceptance Tests

[philippthun]

The main difference between your query and the original one is the from_self: false parameter to the union method invocations. That's why the original query is nested (e.g. SELECT * FROM ( SELECT * FROM ...) and yours is not.

I don't think that .all vs. .any? makes a huge difference here, as the returned list from the old query consists of 0 to 7 type strings. That's why I would be in favor of keeping the original single query instead of having (up to) two queries.

Of course, adding the from_self: false to the original query would still make sense.

[will-gant]

I don't think that .all vs. .any? makes a huge difference here, as the returned list from the old query consists of 0 to 7 type strings. That's why I would be in favor of keeping the original single query instead of having (up to) two queries.

So I think the difference is that with .any? you get a SELECT 1 - i.e. check whether the row exists, don't retrieve any column values - which is cheaper than the SELECT * of .all. I do agree it's a teeny tiny thing, and admittedly only at the top level, though it still seems we might as well take it. But I could improve it by getting rid of those unnecessary selects at the lower level.

But the main thing is that while space supporter roles can apparently be quite common (almost 10% of all roles on this foundation), users who only have a space supporter role seem to be very rare. In fact, this foundation has just one space supporter role (out of 60k) that belongs to a user who has no other role (other than organization user, since any user with a space role has that too). That means:

1. Like 99% of the time we'll avoid that separate scan of the space supporters table, because the user will have one of the other roles. So in this foundation cut down the potential number of rows that need to be scanned by 10%.
2. In the very small number of requests where the user lacks some other role, and we actually do need to do the separate scan on space supporters, my numbers suggest the combined querytime is about the same as it is for the existing single query. So we don't seem to lose out even then.

[will-gant]

@philippthun Your feedback made me ponder this a bit more deeply, and I think I've come up with a much better solution in 2db8b34, which I hope will turn your frown upside down. Although it still retains the separate query for space supporters 😉

My latest commit produces a SQL query that looks like this:

SELECT 1 AS "one"
FROM   "users"
WHERE  ( ( ( EXISTS (SELECT 1
                     FROM   "spaces_developers"
                     WHERE  ( "user_id" = 2023961 )) ) IS TRUE )
          OR ( ( EXISTS (SELECT 1
                         FROM   "organizations_managers"
                         WHERE  ( "user_id" = 2023961 )) ) IS TRUE )
          OR ( ( EXISTS (SELECT 1
                         FROM   "spaces_managers"
                         WHERE  ( "user_id" = 2023961 )) ) IS TRUE )
          OR ( ( EXISTS (SELECT 1
                         FROM   "spaces_auditors"
                         WHERE  ( "user_id" = 2023961 )) ) IS TRUE )
          OR ( ( EXISTS (SELECT 1
                         FROM   "organizations_auditors"
                         WHERE  ( "user_id" = 2023961 )) ) IS TRUE )
          OR ( ( EXISTS (SELECT 1
                         FROM   "organizations_billing_managers"
                         WHERE  ( "user_id" = 2023961 )) ) IS TRUE ) )
LIMIT  1;

The cool thing about it is that, unlike with a UNION, it will short-circuit as soon as a match is found - skipping the remaining tables. That makes it almost 20x faster if the user has a role in the first table checked, about 2x faster if they're in the second, and for the others it's comparable to the old query (but slightly faster even for those). I made space developers first and org managers second because they're the first and second most numerous roles on a lot of foundations I've seen (though we've got no easy way of knowing if they also make the most requests).

For each of the tables below, I retrieved a sample user_id for a user who had that role only (ignoring OrganizationUser) and then used EXPLAIN ANALYZE to run the query for that user id on postgres (I used the same user ids for the 'old' and 'new' tests). Each of the times is an average from 100 repetitions of the same query.

Here's for the old query:

Old	Rows count	Planning (ms)	Execution (ms)	Total (ms)
Space Developer	174774	1.45321	20.2723	21.72551
Org Manager	118982	1.50626	32.64812	34.15438
Space Manager	100263	1.48238	33.85913	35.34151
Space Auditor	105297	1.54372	26.27451	27.81823
Org Auditor	53303	1.55326	35.10541	36.65867
Org Billing Manager	175	1.51813	33.70744	35.22557

And here's for the new one that I committed in 2db8b34:

New	Rows count	Planning (ms)	Execution (ms)	Total (ms)	% querytime of the old
Space Developer	174774	1.25895	0.10354	1.36249	6.27%
Org Manager	118982	1.26675	14.28919	15.55594	45.55%
Space Manager	100263	1.25602	32.60624	33.86226	95.81%
Space Auditor	105297	1.28501	23.51924	24.80425	89.17%
Org Auditor	53303	1.25375	33.34759	34.60134	94.39%
Org Billing Manager	175	1.32794	32.54151	33.86945	96.15%

I doubt I'm going to have time to do it, but it occurs to me that the same approach could also make quite a big difference in the Membership class where we check if a user has any applicable role for a particular endpoint.

[philippthun]

It looks like there is something that old MySQL servers don't like:

--- Caused by: ---
Mysql2::Error:
  You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE (((EXISTS (SELECT 1 FROM `spaces_developers` WHERE (`user_id` = 489) LIMIT' at line 1

I hope you have the correct manual at hand ;-)

[will-gant]

It looks like there is something that old MySQL servers don't like:

--- Caused by: ---
Mysql2::Error:
  You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE (((EXISTS (SELECT 1 FROM `spaces_developers` WHERE (`user_id` = 489) LIMIT' at line 1

I hope you have the correct manual at hand ;-)

Whaaaat

[will-gant]

Alright this seems to be a bug in Sequel, which generates a query that is invalid for mysql 5.7. Though the reference manuals for mysql 5.7 and 8.0 don't hint at any such change in behaviour, the former will apparently explode if you use a WHERE without a FROM:

mysql> SELECT
    ->   1
    -> WHERE
    ->   (
    ->     EXISTS (
    ->       SELECT
    ->         1
    ->       FROM
    ->         spaces_developers
    ->       WHERE
    ->         'user_id' = 1
    ->       LIMIT
    ->         1
    ->     )
    ->   );
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE

But with a useless FROM it's happy:

mysql> SELECT
    ->   1 FROM users
    -> WHERE
    ->   (
    ->     EXISTS (
    ->       SELECT
    ->         1
    ->       FROM
    ->         spaces_developers
    ->       WHERE
    ->         'user_id' = 1
    ->       LIMIT
    ->         1
    ->     )
    ->   );
Empty set, 1 warning (0.00 sec)

@philippthun I've just opened jeremyevans/sequel#1977 to report this apparent bug. Perhaps we could revert to my original SELECT 1 FROM users WHERE..., given that that's still quite a bit faster than the status quo?

[philippthun]

Perhaps we could revert to my original SELECT 1 FROM users WHERE..., given that that's still quite a bit faster than the status quo?

Yes, let's do this.