perf: make hot-path adapter catalog queries sargable - #686
Conversation
Optimizes the four highest-frequency catalog queries in the SQL Server adapter so they seek instead of scan: - list_relations_without_caching: filter by schema_id = SCHEMA_ID(@Schema) before computing names, instead of scanning every table+view in the database and post-filtering with LIKE on SCHEMA_NAME(...). - get_relation_without_caching: same predicate-pushdown; also switch identifier match from LIKE (no wildcards) to equality. - get_columns_in_relation: drop the wrapping CTE and the no-op ROW_NUMBER() OVER (PARTITION BY object_name(c.object_id) ...); the WHERE already pins one object_id. Order directly by c.column_id. - get_view_definition_sql: replace UPPER()-on-both-sides join across sys.views + sys.schemas with a single OBJECT_DEFINITION(OBJECT_ID( '[schema].[name]', 'V')) lookup. Preserves zero-row behavior when the view doesn't exist so the view materialization's diff-skip logic is unchanged.
axellpadilla
left a comment
axellpadilla
left a comment
There was a problem hiding this comment.
The performance direction looks good, but I think get_view_definition_sql needs one more pass before merge.
The new object_id('[{{ relation.schema }}].[{{ relation.identifier }}]', 'V') path changes semantics in two ways:
-
It manually bracket-quotes identifiers but does not escape
], so legal identifiers containing]can produce an invalid object name. Consider building the object name withquotename(schema) + '.' + quotename(identifier). -
The previous query used
upper(s.name) = upper(...)andupper(v.name) = upper(...), so it intentionally did case-insensitive lookup even on CS_AS databases.OBJECT_ID(...)may return NULL when relation casing differs from catalog casing under case-sensitive collations. Since CI runs CS_AS variants, please add a focused test or preserve the old case-insensitive semantics, and check whether we have enough coverage for these changes.
The column-query simplification and schema_id predicate pushdown otherwise look sound.
Address PR review feedback on get_view_definition_sql: - Build the object name with quotename(schema) + '.' + quotename(identifier) instead of manual [..].[..] bracketing. quotename() doubles embedded ] characters, so legal identifiers containing ] no longer produce a malformed object name (which OBJECT_ID would resolve to NULL). - Add a focused functional test (runs under both CI collations, CS_AS and CI_AS) covering an identifier that contains ] and the zero-row behavior for a missing view that the view materialization's diff-skip relies on. OBJECT_ID resolution still follows database collation rules. In the view materialization, get_view_definition_sql is only ever called on the cached existing_relation, whose casing comes straight from the catalog, so the lookup casing always matches the stored object even under CS_AS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
We lost the information_schema_hints() in the get view definition, was that planned? We are swapping from SCHEMA_NAME(t.schema_id) to '{{ schema_relation.schema }}'. I'm not sure this really matters, the catalog should have the same casing as is returned by SCHEMA_NAME as I assume the catalog is populated via that method, but I've not checked. I think the CI would have picked up any issues. |
|
|
Hi guys, please let me know if you conclude the same. Short version: I don’t think removing The old query was not truly “non-locking”. SQL Server docs say that all queries, including Also, for this specific flow, So the previous From a locking perspective, this looks reasonable and reducing time. The main things worth validating are semantic compatibility: identifier quoting, case-sensitive collation behavior, permissions/metadata visibility, and missing-object behavior, which look mostly covered. |
I'd argue the literal is the better choice because it's cheaper. |
Quoting: Collation: The old Permissions: OBJECT_ID/SCHEMA_ID/OBJECT_DEFINITION and the Missing-object: Let me know if there's anything else I can do to 1) get this merged 2) facilitate cutting a new release with the latest changes :) |
I do not think removing the no lock is an issue, but if anyone does update what goes in information_schema_hints() expecting it to be applied to all information schema queries we just have one that it does not. Can't think of a use case right now, just noting it. |
|
Is it possible to cut a new release after this merges? TIA |
Included on v1.10, and good news I benchmarked it, 1% to 10% improvement on different scenarios without increasing any locking issue or timing. Improvements increases with objects, just tried with 2000 to 20000 simulated on the schemas, people with more or complex pipelines could get noticeable differences. |
3 participants
Optimizes the four highest-frequency catalog queries in the SQL Server adapter so they seek instead of scan:
list_relations_without_caching: filter by schema_id = SCHEMA_ID(@Schema) before computing names, instead of scanning every table+view in the database and post-filtering with LIKE on SCHEMA_NAME(...).get_relation_without_caching: same predicate-pushdown; also switch identifier match from LIKE (no wildcards) to equality.get_columns_in_relation: drop the wrapping CTE and the no-op ROW_NUMBER() OVER (PARTITION BY object_name(c.object_id) ...); the WHERE already pins one object_id. Order directly by c.column_id.get_view_definition_sql: replace UPPER()-on-both-sides join across sys.views + sys.schemas with a single OBJECT_DEFINITION(OBJECT_ID( '[schema].[name]', 'V')) lookup. Preserves zero-row behavior when the view doesn't exist so the view materialization's diff-skip logic is unchanged.