fix: Improve performance on TestStatusHistory endpoint#1863
fix: Improve performance on TestStatusHistory endpoint#1863alanpeixinho wants to merge 1 commit intokernelci:mainfrom
Conversation
alanpeixinho
force-pushed
the
fix/improve-test-status-history-performance
branch
from
33c96f7 to
18d172a
Compare
gustavobtflores
added
Backend
MarceloRobert
left a comment
MarceloRobert
left a comment
There was a problem hiding this comment.
Looks good, only some small comments
alanpeixinho
force-pushed
the
fix/improve-test-status-history-performance
branch
from
18d172a to
b513e72
Compare
| start_time: Optional[datetime] | ||
| id: str | ||
| status: Optional[str] | ||
| git_commit_hash: Optional[str] |
There was a problem hiding this comment.
if reverting to optional[type] then we can keep using the value that was here before. We are just using these types (such as Test_StartTime) to signal that these values come directly from the database
There was a problem hiding this comment.
I thought you were going to change the status field though, to default to "NULL" when it is None
There was a problem hiding this comment.
I have changed back to use the database types. But in there, Test__Status is an Optional[Literal["FAIL", "PASS", "SKIP", "ERROR", "MISS", "DONE"]], so the null value is represented as None.
Changing that could impact all uses of this type.
alanpeixinho
force-pushed
the
fix/improve-test-status-history-performance
branch
7 times, most recently
from
14e9d0f to
2861582
Compare
MarceloRobert
left a comment
MarceloRobert
left a comment
There was a problem hiding this comment.
LGTM, tested and it seems all good
| if test_start_time is None: | ||
| if field_timestamp is None: | ||
| query = query.filter( | ||
| start_time__isnull=True, | ||
| field_timestamp__isnull=True, | ||
| ) | ||
| time_clause = "AND T.START_TIME IS NULL AND T.FIELD_TIMESTAMP IS NULL" | ||
| order_clause = "ORDER BY T.FIELD_TIMESTAMP DESC" | ||
| else: | ||
| query = query.filter(field_timestamp__lte=field_timestamp) | ||
| return query.order_by("-field_timestamp")[:group_size] | ||
| params["field_timestamp"] = field_timestamp | ||
| time_clause = "AND T.FIELD_TIMESTAMP <= %(field_timestamp)s" | ||
| order_clause = "ORDER BY T.FIELD_TIMESTAMP DESC" |
There was a problem hiding this comment.
It shouldn't be T._TIMESTAMP instead of T.FIELD_TIMESTAMP here?
There was a problem hiding this comment.
Good catch, I think both the frontend and tests do dot give us this case.
I will fix and include some tests to catch it.
| cursor.execute("SET LOCAL enable_nestloop = off") | ||
| cursor.execute(query, params) | ||
| rows = dict_fetchall(cursor) | ||
| set_query_cache(key=cache_key, params=params, rows=rows) |
There was a problem hiding this comment.
I would like to see some EXPLAIN ANALYZE with this turned on and off attached in the PR, just to ensure we aren't missing anything here
There was a problem hiding this comment.
For the regular queries (which are already fast) the benefit, if it exists should be minimal (with some chance of even being slightly worse).
I am including one of the slowest queries, where the benefit can be seen. The main problem is that the regular query offered by the planner is bad in its worst case scenarios.
Example obtained with this query
| Before | analyze-nested.md |
| After | analyze-no-nested.md |
There was a problem hiding this comment.
More regular case from this query
| Before | analyze-nested-small.md |
| After | analyze-no-nested-small.md |
* Moving to a raw SQL query, instead of ORM.
* Force the query to use a hash based comparison, instead of a slower
nested loop
Closes kernelci#1131
alanpeixinho
force-pushed
the
fix/improve-test-status-history-performance
branch
from
2861582 to
8887737
Compare
3 participants
Moving to a raw SQL query, instead of ORM.
Force the query to use a hash based comparison, instead of a slower nested loop
Closes Improve test status history query #1131