Prevent sqlalchemy Transparent SQL Compilation Caching from filling up during purge #71015
Conversation
…p during purge Since each purge cycle and can result in a different amount unioned queries to find unused attribute ids, sqlalchemys Transparent SQL Compilation Caching would cache each one. We now generate a single query and fill unused values with NULLs to ensure that only one query is cached and we only have to compile the query once.
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Well it doesn't fill up the cache but now we don't cache that statement at all which is a cpu problem |
|
this is taking forever to test since I have to generate a database that takes long enough to purge and watch the memory, cpu, and profile during the purge |
|
Latest push is performing well |
|
Memory is now stable during purge |
| @@ -112,6 +114,118 @@ def _select_event_state_and_attributes_ids_to_purge( | |||
| return event_ids, state_ids, attributes_ids | |||
|
|
|||
|
|
|||
| def _generate_find_attr_lambda(attribute_ids: list[int]) -> StatementLambdaElement: | |||
There was a problem hiding this comment.
This works and its fast, but I need to figure out how to avoid writing it all out
balloob
changed the title
|
This works really well and doesn't have the memory problem |
|
I haven't been able to come up with a solution that doesn't involve writing out the whole query. I tried generating it an using |
|
This might be one of those cases where is better to bypass the ORM and generate the query ourselves but I guess it better to have a written out query than try to deal with the complexity of all the supported DBMS ourselves |
|
|
||
| https://docs.sqlalchemy.org/en/14/core/connections.html#quick-guidelines-for-lambdas | ||
| """ | ||
| return lambda_stmt( |
There was a problem hiding this comment.
The lambda_stmt is actually looking at the code inside the lambda to generate the cache key so it seems like we are stuck writing this out if we want a single cache key with bindparams
| return select(func.min(States.attributes_id)).where(States.attributes_id == attr) | ||
|
|
||
|
|
||
| def _generate_find_attr_lambda( |
There was a problem hiding this comment.
The docs for sqlalchemy say
Avoid referring to non-SQL constructs inside of lambdas as they are not cacheable by default
....
The best way to resolve the above situation is to not refer to foo inside of the lambda, and refer to it outside instead:
>>> def my_stmt(foo):
... x_param, y_param = foo.x, foo.y
... stmt = lambda_stmt(lambda: select(func.max(x_param, y_param)))
... return stmt
So even if I pass this as a list I'm stuck unpacking them as attr1, attr2, attr3, attr4, .... = attrs outside the lambda
|
Memory testing looks solid. Actually saw memory going down from gc during the purge |
|
Tested with timescale db / postgres addon and purge is still fast + memory looks good |
|
cpu during purge also looks good |
|
I don't love the syntax here, but I can't find a better way that doesn't involve using sqlalchemy internals which seems risky. I just don't like how the code looks even if though it functions well. |
| to_remove = attributes_ids - {state[0] for state in id_query.all()} | ||
| # We used to generate a query based on how many attribute_ids to find but | ||
| # that meant sqlalchemy Transparent SQL Compilation Caching was working against | ||
| # us by cached up to MAX_ROWS_TO_PURGE different statements which could be |
There was a problem hiding this comment.
Not familiar with SQL Alchemy, but would it be possible to execute a query without it being cached ? We don't purge that often, so having slightly more expensive query generation is fine.
There was a problem hiding this comment.
Ah, you wrote that above already, dealing with the dialects. That's fair, we can keep it as this. People will have questions when they see it 😅
There was a problem hiding this comment.
My first thought was to disable the cache.
Unfortunately the query generates so many ORM objects (also the same reason the cache gets so big), generating it every time was more expensive run-time wise than all the other python code run in a purge cycle combined.
Sqlalchemy clearly isn't optimized for this use case.
I also tried caching a query with many bindparms but the code that generates the actual query from the cache clones the cached version and visits every part to fill in the bind values so that turned out to be even more expensive.
The only way I found that resulted in a single cache key (the lambda) was how it's currently implemented.
Proposed change
Since each purge cycle and can result in a many different sized
unioned queries to find unused attribute ids, sqlalchemys
Transparent SQL Compilation Caching would be a bit too helpful
and cache each one which results in an extra 500MB of memory used.
Fixes the memory jump reported here and analyzed here
We now generate a single query using sqlalchemy's
lambda_stmtand fill unused values with NULLs to ensure that only one query is
cached and we only have to compile the query once.
Because of how
lambda_stmtworks, I had to write out the entirestatement since it relies on getting the bind values being passed
to the function as python arguments:
https://docs.sqlalchemy.org/en/14/core/connections.html#quick-guidelines-for-lambdas
If there is any way to improve this without having to write it out,
I haven't been able to find it after beating my head against the wall
for hours ( I have 12+ hours into this PR ). If anyone has a better way, please help!
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: