feat(perf): skip redundant DDL checks in SQLSampleStore.__init__#670
feat(perf): skip redundant DDL checks in SQLSampleStore.__init__#670michael-johnston wants to merge 6 commits intomainfrom
Conversation
Previously, SQLSampleStore.__init__ called _create_source_table() unconditionally on every construction. _create_source_table() calls meta.create_all(checkfirst=True), which issues a separate table-existence query for each of the four backing tables even when the tables already exist — which they always do on read-only CLI commands. Locally this cost was ~1.45s. The changes reduce this to 0.24s. The main fix is to probe for table existence instead of blinding issuing _create_source_tables(). This removes the 1.45 and adds the probe. We use a direct SQL probe (taking 240ms locally measured) instead of sqlalchemy.inspect() which took 800ms locally. The PR also introduces a process-level _source_tables_verified that records tablename prefixes whose backing tables have already been confirmed to exist. On the second and subsequent SQLSampleStore constructions for the same store within the same process the probe is skipped entirely (0 round-trips). The main impact of this is in the tests.
AlessandroPomponio
left a comment
AlessandroPomponio
left a comment
There was a problem hiding this comment.
I'd be careful with this: the use of inspect is dialect independent and is the suggested way to check for table existence. Where do you see the 15 round trips? It should be a DESCRIBE of the 4 tables releated to the sqlsource
I'm all for caching the existence of the tables (like we added in SQLResourceStore) but I'd keep what we have in term of inspection.
Signed-off-by: Michael Johnston <66301584+michael-johnston@users.noreply.github.com>
That was a typo. Fixed
This is the typical optimisation trade off. If we keep the generic method, inspect, we have to pay the ~1.2 secs for it. |
By not checking what the table structure is, though, we will not be able to perform migrations down the line |
Could we not add it back if necessary? |
|
This should be updated now that #672 has been merged |
|
No vulnerabilities found. |
| if self.engine.dialect.name == "sqlite": | ||
| existence_query = sqlalchemy.text( | ||
| "SELECT 1 FROM sqlite_master WHERE type='table' AND name=:name" | ||
| ).bindparams(name=self._tablename) | ||
| else: | ||
| existence_query = sqlalchemy.text( | ||
| "SELECT 1 FROM information_schema.tables" | ||
| " WHERE table_schema = DATABASE() AND table_name = :name LIMIT 1" | ||
| ).bindparams(name=self._tablename) |
There was a problem hiding this comment.
this is the same as what we added for the resources table (which is why I was asking to update from main, because at first glance I thought the code was the same).
At this point it's worth having a parametrized statement in orchestrator/metastore/sql/statements.py for this
|
Checks Summary Last run: 2026-03-24T20:48:15.306Z Mend Unified Agent vulnerability scan found 1 vulnerabilities:
|
3 participants
Previously, SQLSampleStore.init called _create_source_table() unconditionally on every construction. _create_source_table() calls meta.create_all(checkfirst=True), which issues a separate table-existence query for each of the four backing tables even when the tables already exist — which they always do on read-only CLI commands. Locally this cost was ~1.45s. The changes reduce this to 0.24s.
The main fix is to probe for table existence instead of blinding issuing _create_source_tables(). This removes the 1.45 and adds the probe. We use a direct SQL probe (taking 240ms locally measured) instead of sqlalchemy.inspect() which took 800ms locally.
The PR also introduces a process-level _source_tables_verified that records tablename prefixes whose backing tables have already been confirmed to exist.
On the second and subsequent SQLSampleStore constructions for the same store within the same process the probe is skipped entirely (0 round-trips).
The main impact of this is in the tests.