Remove JOIN from GetAuthorizations2 and filter in code #4512
Conversation
| var authz2IDs []int64 | ||
| _, err = ssa.dbMap.Select( | ||
| &authz2IDs, | ||
| fmt.Sprintf(`SELECT DISTINCT(authzID) FROM orderToAuthz2 WHERE authzID IN (%s)`, strings.Join(qmarks, ",")), |
There was a problem hiding this comment.
Looking at this, it may turn out to be similarly expensive in the corner case we found, since it's potentially reading many orderToAuthz2 rows for a single authorization that's included in many orders. Presumably it's still somewhat cheaper since at least it's not doing the join, so I think it's worthwhile, but not the big win we'd hoped for.
The only alternative I see would be to split this into N independent queries, each with a LIMIT 1, but that definitely increases the complexity and might actually make performance work. So on balance I think this approach is fine.
sa/sa.go
Outdated
| // Instead of using a JOIN on the orderToAuthz2 table in order to filter out | ||
| // authorizations that don't have associated orders we do a second query | ||
| // and do the filtering in code. This allows us to bypass the large number | ||
| // of duplicate rows that are generated by using a JOIN. |
There was a problem hiding this comment.
I'd like to see a little more description here of the conditions under which we see a large number of rows in the orderToAuthz2 table for a single authorization, since it's slightly harder to grok, and is the motivating reason for this change. I'd also like to see a similar description in the PR description.
There was a problem hiding this comment.
@rolandshoemaker Are you confident this doesn't need to be feature flagged?
LGTM other than that thought.
Hm, it's probably a good idea to flag it, good call. |
@rolandshoemaker 👍 In that case should we wait on merging this until you've had a chance to push another commit? |
|
Pushed flag change. |
Seem to have forgotten to do this in #4512.
Previously we used a JOIN on the orderToAuthz2 table in order to make sure
we only returned authorizations created using the ACME v2 API. Each time an
order is created a pivot row (order ID + authz ID) is added to the
orderToAuthz2 table. If a large number of orders are created that all contain
the same authorization, due to reuse, then the JOINd query would return a full
authorization row for each entry in the orderToAuthz2 table with the authorization
ID.
Instead we now filter out these authorizations by doing a second query against
the orderToAuthz2 table. Using this query still requires examining a large number
of rows, but because we don't need to construct a temporary table for the JOIN
and fill it with all the full authorization rows we should save resources.
Fixes #4500.