Optimize audit log filter-attribute query (6 scans → 1)#3443
Open
priyamkarn wants to merge 2 commits into
Open
Optimize audit log filter-attribute query (6 scans → 1)#3443priyamkarn wants to merge 2 commits into
priyamkarn wants to merge 2 commits into
Conversation
Author
|
@oschwartz10612 @miloschwartz can you please review and merge this |
Member
|
Haha thanks for taking a look at that comment. I am thinking longer term, the actual fix for the underlying TODO is probably adding an index (or narrowing what needs to be indexed) rather than moving aggregation into the app tier. Or a single query using array_agg(DISTINCT ...) (or similar) per column, or a UNION ALL of six small LIMIT-ed subqueries, so distinctness is still computed in Postgres and only small result sets cross the wire - genuinely 1 round trip without the memory blowup |
Author
|
@oschwartz10612 can you please review now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Optimizes
queryUniqueFilterAttributesin:server/routers/auditLogs/queryRequestAuditLog.tsPreviously, the function executed six separate
SELECT DISTINCTqueries in parallel to populate the request audit log filter options:This resolves the existing:
// TODO: SOMEONE PLEASE OPTIMIZE THIS!!!!!Problem
All six queries used the same
orgIdand time-range filters.However, the table only has indexes on:
timestamp(orgId, timestamp)There are no indexes on
actor,location,host,path,resourceId, orsiteResourceId.As a result, each
DISTINCTquery had to independently scan the same matched row set. Every request to this endpoint therefore performed six database scans and six round trips over effectively the same underlying dataset.Fix
Replaced the six separate queries with a single
UNION ALLquery.The query contains six subqueries—one per filter field—with each subquery:
GROUP BY <field>LIMITresourceIdandsiteResourceIdare cast to text inside their respective subqueries so that all sixUNION ALLbranches return compatible column types. They are parsed back into numbers after the query returns.This keeps distinct-value computation inside the database rather than pulling every matched audit-log row into application memory.
That is important because audit logs typically have a high row count but low-to-moderate cardinality for individual filter fields. For example, a time range may contain millions of requests but only a small number of distinct paths. Deduplicating in application memory would require transferring the entire matched dataset across the wire just to derive a limited number of filter values.
The new approach reduces the number of database round trips from six to one while keeping every field's result set bounded by
DISTINCT_LIMIT, matching the original per-query limits.Credit to
@oschwartz10612for suggesting this approach during review. An earlier version of the PR used a single scan followed by in-memorySetdeduplication, which reduced database scans but introduced an unbounded network and application-memory footprint for large time ranges. This version avoids that trade-off.Behavior
Existing behavior is preserved:
DISTINCT_LIMITentries—currently500—matching the previous per-query limit.resourceIdandsiteResourceIdmutual-exclusivity rule is unchanged.siteResourceIdis only included whenresourceIdisnull, matching the originalisNull(resourceId)condition.Cleanup
Adjusted the
isNullimport usage to support thesiteResourceIdsubquery's null condition.Testing
.toSQL().20,000rows, including rows outside the target organization and time range.I was unable to run the full test suite in my local environment, so a CI run would be appreciated before merging.
Scope / Risk