From: Mike Bayer Date: Mon, 10 May 2021 17:19:14 +0000 (-0400) Subject: Correct cache key for proxy_owner, with_context_options X-Git-Tag: rel_1_4_15~3^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f0fc47c11986a0fa60b24c0fb62bd8b5a5306edd;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Correct cache key for proxy_owner, with_context_options Fixed issue in subquery loader strategy which prevented caching from working correctly. This would have been seen in the logs as a "generated" message instead of "cached" for all subqueryload SQL emitted, which by saturating the cache with new keys would degrade overall performance; it also would produce "LRU size alert" warnings. In this issue we also observe that the local LRU cache for lazyloader and selectinloader will get used for all subsequent loads as well, which makes it more likely to hit the limit of 30. However rather than trying to work this out, it would be better if we removed the loader-local LRU caches altogether once we are confident these are working well. Fixes: #6459 Change-Id: Id953e8f75536bb87f7e3315929cebcd8f84a5a50 --- diff --git a/doc/build/changelog/unreleased_14/6459.rst b/doc/build/changelog/unreleased_14/6459.rst new file mode 100644 index 0000000000..dd5e84a935 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6459.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 6459 + + Fixed issue in subquery loader strategy which prevented caching from + working correctly. This would have been seen in the logs as a "generated" + message instead of "cached" for all subqueryload SQL emitted, which by + saturating the cache with new keys would degrade overall performance; it + also would produce "LRU size alert" warnings. + diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 2f8c8f9402..05b12dda27 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -227,7 +227,7 @@ class QueryableAttribute( else: annotations = { "proxy_key": self.key, - "proxy_owner": self.class_, + "proxy_owner": self._parententity, "entity_namespace": self._entity_namespace, } @@ -532,6 +532,10 @@ def create_proxied_attribute(descriptor): and getattr(self.class_, self.key).impl.uses_objects ) + @property + def _parententity(self): + return inspection.inspect(self.class_, raiseerr=False) + @property def _entity_namespace(self): if hasattr(self._comparator, "_parententity"): diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index aeba9ed80b..ea84805b4d 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -2692,9 +2692,9 @@ class _ORMColumnEntity(_ColumnEntity): # within internal loaders. orm_key = annotations.get("proxy_key", None) - proxy_owner = annotations.get("proxy_owner", _entity.entity) + proxy_owner = annotations.get("proxy_owner", _entity) if orm_key: - self.expr = getattr(proxy_owner, orm_key) + self.expr = getattr(proxy_owner.entity, orm_key) self.translate_raw_column = False else: # if orm_key is not present, that means this is an ad-hoc diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 65fdcf9fc7..856afabb0f 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -723,7 +723,6 @@ class AliasedInsp( "parentmapper": self.mapper, "parententity": self, "entity_namespace": self, - "compile_state_plugin": "orm", } )._set_propagate_attrs( {"compile_state_plugin": "orm", "plugin_subject": self} @@ -784,7 +783,6 @@ class AliasedInsp( d = { "parententity": self, "parentmapper": self.mapper, - "compile_state_plugin": "orm", } if key: d["proxy_key"] = key diff --git a/lib/sqlalchemy/sql/base.py b/lib/sqlalchemy/sql/base.py index 6d65d9061b..8ff907ef03 100644 --- a/lib/sqlalchemy/sql/base.py +++ b/lib/sqlalchemy/sql/base.py @@ -781,7 +781,10 @@ class Executable(roles.StatementRole, Generative): _executable_traverse_internals = [ ("_with_options", InternalTraversal.dp_executable_options), - ("_with_context_options", ExtendedInternalTraversal.dp_plain_obj), + ( + "_with_context_options", + ExtendedInternalTraversal.dp_with_context_options, + ), ("_propagate_attrs", ExtendedInternalTraversal.dp_propagate_attrs), ] @@ -853,8 +856,8 @@ class Executable(roles.StatementRole, Generative): These are callable functions that will be given the CompileState object upon compilation. - A second argument cache_args is required, which will be combined - with the identity of the function itself in order to produce a + A second argument cache_args is required, which will be combined with + the ``__code__`` identity of the function itself in order to produce a cache key. """ diff --git a/lib/sqlalchemy/sql/traversals.py b/lib/sqlalchemy/sql/traversals.py index e8a8052858..e64eff6a41 100644 --- a/lib/sqlalchemy/sql/traversals.py +++ b/lib/sqlalchemy/sql/traversals.py @@ -310,6 +310,12 @@ class CacheKey(namedtuple("CacheKey", ["key", "bindparams"])): def __eq__(self, other): return self.key == other.key + @classmethod + def _diff_tuples(cls, left, right): + ck1 = CacheKey(left, []) + ck2 = CacheKey(right, []) + return ck1._diff(ck2) + def _whats_different(self, other): k1 = self.key @@ -413,6 +419,11 @@ class _CacheKey(ExtendedInternalTraversal): visit_propagate_attrs = PROPAGATE_ATTRS + def visit_with_context_options( + self, attrname, obj, parent, anon_map, bindparams + ): + return tuple((fn.__code__, c_key) for fn, c_key in obj) + def visit_inspectable(self, attrname, obj, parent, anon_map, bindparams): return (attrname, inspect(obj)._gen_cache_key(anon_map, bindparams)) @@ -1209,6 +1220,13 @@ class TraversalComparatorStrategy(InternalTraversal, util.MemoizedSlots): else: return left == right + def visit_with_context_options( + self, attrname, left_parent, left, right_parent, right, **kw + ): + return tuple((fn.__code__, c_key) for fn, c_key in left) == tuple( + (fn.__code__, c_key) for fn, c_key in right + ) + def visit_plain_obj( self, attrname, left_parent, left, right_parent, right, **kw ): diff --git a/lib/sqlalchemy/sql/visitors.py b/lib/sqlalchemy/sql/visitors.py index 5d60774aa7..793e7fc5c6 100644 --- a/lib/sqlalchemy/sql/visitors.py +++ b/lib/sqlalchemy/sql/visitors.py @@ -272,6 +272,8 @@ class InternalTraversal(util.with_metaclass(_InternalTraversalType, object)): dp_executable_options = symbol("EO") + dp_with_context_options = symbol("WC") + dp_fromclause_ordered_set = symbol("CO") """Visit an ordered set of :class:`_expression.FromClause` objects. """ diff --git a/test/orm/test_cache_key.py b/test/orm/test_cache_key.py index d120b05c05..390fc514d3 100644 --- a/test/orm/test_cache_key.py +++ b/test/orm/test_cache_key.py @@ -1,4 +1,5 @@ import random +import types from sqlalchemy import func from sqlalchemy import inspect @@ -12,6 +13,7 @@ from sqlalchemy.orm import defaultload from sqlalchemy.orm import defer from sqlalchemy.orm import join as orm_join from sqlalchemy.orm import joinedload +from sqlalchemy.orm import lazyload from sqlalchemy.orm import Load from sqlalchemy.orm import mapper from sqlalchemy.orm import Query @@ -27,6 +29,7 @@ from sqlalchemy.sql.expression import case from sqlalchemy.sql.visitors import InternalTraversal from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ +from sqlalchemy.testing import mock from sqlalchemy.testing.fixtures import fixture_session from test.orm import _fixtures from .inheritance import _poly_fixtures @@ -562,7 +565,9 @@ class RoundTripTest(QueryTest, AssertsCompiledSQL): User, users, properties={ - "addresses": relationship(Address, back_populates="user") + "addresses": relationship( + Address, back_populates="user", order_by=addresses.c.id + ) }, ) @@ -683,3 +688,60 @@ class RoundTripTest(QueryTest, AssertsCompiledSQL): go() else: go() + + @testing.combinations( + (lazyload, 2, 6), + (joinedload, 1, 0), + (selectinload, 2, 5), + (subqueryload, 2, 0), + argnames="strat,expected_stmt_cache,expected_lambda_cache", + ) + def test_cache_key_loader_strategies( + self, + plain_fixture, + strat, + expected_stmt_cache, + expected_lambda_cache, + connection, + ): + User, Address = plain_fixture + + cache = {} + + connection = connection.execution_options(compiled_cache=cache) + sess = Session(connection) + + with mock.patch( + "sqlalchemy.orm.strategies.LazyLoader._query_cache", cache + ), mock.patch( + "sqlalchemy.orm.strategies.SelectInLoader._query_cache", cache + ): + + def go(): + stmt = ( + select(User) + .where(User.id == 7) + .options(strat(User.addresses)) + ) + + u1 = sess.execute(stmt).scalars().first() + eq_(u1.addresses, [Address(id=1)]) + + go() + + lc = len(cache) + + stmt_entries = [ + k for k in cache if not isinstance(k[0], types.CodeType) + ] + lambda_entries = [ + k for k in cache if isinstance(k[0], types.CodeType) + ] + + eq_(len(stmt_entries), expected_stmt_cache) + eq_(len(lambda_entries), expected_lambda_cache) + + for i in range(3): + go() + + eq_(len(cache), lc) diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py index d5f5f98b19..96106b49df 100644 --- a/test/orm/test_subquery_relations.py +++ b/test/orm/test_subquery_relations.py @@ -89,6 +89,47 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): self.assert_sql_count(testing.db, go, 2) + def test_query_is_cached(self): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + + mapper( + User, + users, + properties={ + "addresses": relationship( + mapper(Address, addresses), + lazy="subquery", + order_by=Address.id, + ) + }, + ) + query_cache = {} + sess = fixture_session() + + def go(): + sess.close() + + stmt = select(User).filter(User.id == 7) + + sess.execute( + stmt, execution_options={"compiled_cache": query_cache} + ).one() + + for i in range(3): + go() + + qclen = len(query_cache) + + for i in range(5): + go() + + eq_(len(query_cache), qclen) + def test_params_arent_cached(self): users, Address, addresses, User = ( self.tables.users, @@ -113,14 +154,14 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): u1 = ( sess.query(User) - .execution_options(query_cache=query_cache) + .execution_options(compiled_cache=query_cache) .filter(User.id == 7) .one() ) u2 = ( sess.query(User) - .execution_options(query_cache=query_cache) + .execution_options(compiled_cache=query_cache) .filter(User.id == 8) .one() ) diff --git a/test/orm/test_utils.py b/test/orm/test_utils.py index c0829e9b3c..1a7fa636c4 100644 --- a/test/orm/test_utils.py +++ b/test/orm/test_utils.py @@ -246,7 +246,7 @@ class AliasedClassTest(fixtures.TestBase, AssertsCompiledSQL): "parententity": point_mapper, "parentmapper": point_mapper, "proxy_key": "x_alone", - "proxy_owner": Point, + "proxy_owner": point_mapper, }, ) eq_( @@ -256,7 +256,7 @@ class AliasedClassTest(fixtures.TestBase, AssertsCompiledSQL): "parententity": point_mapper, "parentmapper": point_mapper, "proxy_key": "x", - "proxy_owner": Point, + "proxy_owner": point_mapper, }, ) @@ -265,6 +265,17 @@ class AliasedClassTest(fixtures.TestBase, AssertsCompiledSQL): a2 = aliased(Point) eq_(str(a2.x_alone == alias.x), "point_1.x = point_2.x") + eq_( + a2.x._annotations, + { + "entity_namespace": inspect(a2), + "parententity": inspect(a2), + "parentmapper": point_mapper, + "proxy_key": "x", + "proxy_owner": inspect(a2), + }, + ) + sess = fixture_session() self.assert_compile(