Warn when caching is disabled / document

This patch adds new warnings for all elements that
don't indicate their caching behavior, including user-defined
ClauseElement subclasses and third party dialects.
it additionally adds new documentation to discuss an apparent
performance degradation in 1.4 when caching is disabled as a
result in the significant expense incurred by ORM
lazy loaders, which in 1.3 used BakedQuery so were actually
cached.

As a result of adding the warnings, a fair degree of
lesser used SQL expression objects identified that they did not
define caching behavior so would have been producing
``[no key]``, including PostgreSQL constructs ``hstore``
and ``array``.  These have been amended to use inherit
cache where appropriate.  "on conflict" constructs in
PostgreSQL, MySQL, SQLite still explicitly don't generate
a cache key at this time.

The change also adds a test for all constructs via
assert_compile() to assert they will not generate cache
warnings.

Fixes: #7394
Change-Id: I85958affbb99bfad0f5efa21bc8f2a95e7e46981

8 files changed, 147 insertions, 11 deletions

diff --git a/lib/sqlalchemy/sql/base.py b/lib/sqlalchemy/sql/base.py
index 6b0182751..4165751ca 100644
--- a/lib/sqlalchemy/sql/base.py
+++ b/lib/sqlalchemy/sql/base.py
@@ -769,11 +769,13 @@ class CacheableOptions(Options, HasCacheKey):
         return HasCacheKey._generate_cache_key_for_object(self)
 
 
-class ExecutableOption(HasCopyInternals, HasCacheKey):
+class ExecutableOption(HasCopyInternals):
     _annotations = util.EMPTY_DICT
 
     __visit_name__ = "executable_option"
 
+    _is_has_cache_key = False
+
     def _clone(self, **kw):
         """Create a shallow copy of this ExecutableOption."""
         c = self.__class__.__new__(self.__class__)
@@ -847,7 +849,8 @@ class Executable(roles.StatementRole, Generative):
 
         """
         self._with_options += tuple(
-            coercions.expect(roles.HasCacheKeyRole, opt) for opt in options
+            coercions.expect(roles.ExecutableOptionRole, opt)
+            for opt in options
         )
 
     @_generative
diff --git a/lib/sqlalchemy/sql/coercions.py b/lib/sqlalchemy/sql/coercions.py
index 480d2c680..07da49c4e 100644
--- a/lib/sqlalchemy/sql/coercions.py
+++ b/lib/sqlalchemy/sql/coercions.py
@@ -12,6 +12,7 @@ import re
 from . import operators
 from . import roles
 from . import visitors
+from .base import ExecutableOption
 from .base import Options
 from .traversals import HasCacheKey
 from .visitors import Visitable
@@ -458,6 +459,21 @@ class HasCacheKeyImpl(RoleImpl):
         return element
 
 
+class ExecutableOptionImpl(RoleImpl):
+    __slots__ = ()
+
+    def _implicit_coercions(
+        self, original_element, resolved, argname=None, **kw
+    ):
+        if isinstance(original_element, ExecutableOption):
+            return original_element
+        else:
+            self._raise_for_expected(original_element, argname, resolved)
+
+    def _literal_coercion(self, element, **kw):
+        return element
+
+
 class ExpressionElementImpl(_ColumnCoercions, RoleImpl):
     __slots__ = ()
 
diff --git a/lib/sqlalchemy/sql/ddl.py b/lib/sqlalchemy/sql/ddl.py
index c700271e9..ef0906328 100644
--- a/lib/sqlalchemy/sql/ddl.py
+++ b/lib/sqlalchemy/sql/ddl.py
@@ -21,6 +21,9 @@ from ..util import topological
 
 
 class _DDLCompiles(ClauseElement):
+    _hierarchy_supports_caching = False
+    """disable cache warnings for all _DDLCompiles subclasses. """
+
     def _compiler(self, dialect, **kw):
         """Return a compiler appropriate for this ClauseElement, given a
         Dialect."""
diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index a7b86d3ec..00270c9b5 100644
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -3646,6 +3646,8 @@ class CollectionAggregate(UnaryExpression):
 
     """
 
+    inherit_cache = True
+
     @classmethod
     def _create_any(cls, expr):
         """Produce an ANY expression.
@@ -3953,7 +3955,7 @@ class IndexExpression(BinaryExpression):
     """Represent the class of expressions that are like an "index"
     operation."""
 
-    pass
+    inherit_cache = True
 
 
 class GroupedElement(ClauseElement):
@@ -5040,14 +5042,17 @@ class _IdentifiedClause(Executable, ClauseElement):
 
 class SavepointClause(_IdentifiedClause):
     __visit_name__ = "savepoint"
+    inherit_cache = False
 
 
 class RollbackToSavepointClause(_IdentifiedClause):
     __visit_name__ = "rollback_to_savepoint"
+    inherit_cache = False
 
 
 class ReleaseSavepointClause(_IdentifiedClause):
     __visit_name__ = "release_savepoint"
+    inherit_cache = False
 
 
 class quoted_name(util.MemoizedSlots, str):
diff --git a/lib/sqlalchemy/sql/functions.py b/lib/sqlalchemy/sql/functions.py
index fff2defe0..5d2e78065 100644
--- a/lib/sqlalchemy/sql/functions.py
+++ b/lib/sqlalchemy/sql/functions.py
@@ -917,6 +917,7 @@ class GenericFunction(Function, metaclass=_GenericMeta):
 
         class as_utc(GenericFunction):
             type = DateTime
+            inherit_cache = True
 
         print(select(func.as_utc()))
 
@@ -931,6 +932,7 @@ class GenericFunction(Function, metaclass=_GenericMeta):
         class as_utc(GenericFunction):
             type = DateTime
             package = "time"
+            inherit_cache = True
 
     The above function would be available from :data:`.func`
     using the package name ``time``::
@@ -948,6 +950,7 @@ class GenericFunction(Function, metaclass=_GenericMeta):
             package = "geo"
             name = "ST_Buffer"
             identifier = "buffer"
+            inherit_cache = True
 
     The above function will render as follows::
 
@@ -966,6 +969,7 @@ class GenericFunction(Function, metaclass=_GenericMeta):
             package = "geo"
             name = quoted_name("ST_Buffer", True)
             identifier = "buffer"
+            inherit_cache = True
 
     The above function will render as::
 
diff --git a/lib/sqlalchemy/sql/roles.py b/lib/sqlalchemy/sql/roles.py
index c4eedd4a4..1f6a8ddf2 100644
--- a/lib/sqlalchemy/sql/roles.py
+++ b/lib/sqlalchemy/sql/roles.py
@@ -40,6 +40,11 @@ class HasCacheKeyRole(SQLRole):
     _role_name = "Cacheable Core or ORM object"
 
 
+class ExecutableOptionRole(SQLRole):
+    __slots__ = ()
+    _role_name = "ExecutionOption Core or ORM object"
+
+
 class LiteralValueRole(SQLRole):
     __slots__ = ()
     _role_name = "Literal Python value"
diff --git a/lib/sqlalchemy/sql/traversals.py b/lib/sqlalchemy/sql/traversals.py
index 914a78dae..d58b5c2bb 100644
--- a/lib/sqlalchemy/sql/traversals.py
+++ b/lib/sqlalchemy/sql/traversals.py
@@ -49,7 +49,50 @@ def _preconfigure_traversals(target_hierarchy):
 
 
 class HasCacheKey:
+    """Mixin for objects which can produce a cache key.
+
+    .. seealso::
+
+        :class:`.CacheKey`
+
+        :ref:`sql_caching`
+
+    """
+
     _cache_key_traversal = NO_CACHE
+
+    _is_has_cache_key = True
+
+    _hierarchy_supports_caching = True
+    """private attribute which may be set to False to prevent the
+    inherit_cache warning from being emitted for a hierarchy of subclasses.
+
+    Currently applies to the DDLElement hierarchy which does not implement
+    caching.
+
+    """
+
+    inherit_cache = None
+    """Indicate if this :class:`.HasCacheKey` instance should make use of the
+    cache key generation scheme used by its immediate superclass.
+
+    The attribute defaults to ``None``, which indicates that a construct has
+    not yet taken into account whether or not its appropriate for it to
+    participate in caching; this is functionally equivalent to setting the
+    value to ``False``, except that a warning is also emitted.
+
+    This flag can be set to ``True`` on a particular class, if the SQL that
+    corresponds to the object does not change based on attributes which
+    are local to this class, and not its superclass.
+
+    .. seealso::
+
+        :ref:`compilerext_caching` - General guideslines for setting the
+        :attr:`.HasCacheKey.inherit_cache` attribute for third-party or user
+        defined SQL constructs.
+
+    """
+
     __slots__ = ()
 
     @classmethod
@@ -60,7 +103,8 @@ class HasCacheKey:
         so should only be called once per class.
 
         """
-        inherit = cls.__dict__.get("inherit_cache", False)
+        inherit_cache = cls.__dict__.get("inherit_cache", None)
+        inherit = bool(inherit_cache)
 
         if inherit:
             _cache_key_traversal = getattr(cls, "_cache_key_traversal", None)
@@ -89,6 +133,23 @@ class HasCacheKey:
                 )
                 if _cache_key_traversal is None:
                     cls._generated_cache_key_traversal = NO_CACHE
+                    if (
+                        inherit_cache is None
+                        and cls._hierarchy_supports_caching
+                    ):
+                        util.warn(
+                            "Class %s will not make use of SQL compilation "
+                            "caching as it does not set the 'inherit_cache' "
+                            "attribute to ``True``.  This can have "
+                            "significant performance implications including "
+                            "some performance degradations in comparison to "
+                            "prior SQLAlchemy versions.  Set this attribute "
+                            "to True if this object can make use of the cache "
+                            "key generated by the superclass.  Alternatively, "
+                            "this attribute may be set to False which will "
+                            "disable this warning." % (cls.__name__),
+                            code="cprf",
+                        )
                     return NO_CACHE
 
             return _cache_key_traversal_visitor.generate_dispatch(
@@ -273,6 +334,15 @@ class MemoizedHasCacheKey(HasCacheKey, HasMemoized):
 
 
 class CacheKey(namedtuple("CacheKey", ["key", "bindparams"])):
+    """The key used to identify a SQL statement construct in the
+    SQL compilation cache.
+
+    .. seealso::
+
+        :ref:`sql_caching`
+
+    """
+
     def __hash__(self):
         """CacheKey itself is not hashable - hash the .key portion"""
 
@@ -480,7 +550,19 @@ class _CacheKey(ExtendedInternalTraversal):
             tuple(elem._gen_cache_key(anon_map, bindparams) for elem in obj),
         )
 
-    visit_executable_options = visit_has_cache_key_list
+    def visit_executable_options(
+        self, attrname, obj, parent, anon_map, bindparams
+    ):
+        if not obj:
+            return ()
+        return (
+            attrname,
+            tuple(
+                elem._gen_cache_key(anon_map, bindparams)
+                for elem in obj
+                if elem._is_has_cache_key
+            ),
+        )
 
     def visit_inspectable_list(
         self, attrname, obj, parent, anon_map, bindparams
@@ -1086,7 +1168,20 @@ class TraversalComparatorStrategy(InternalTraversal, util.MemoizedSlots):
             ):
                 return COMPARE_FAILED
 
-    visit_executable_options = visit_has_cache_key_list
+    def visit_executable_options(
+        self, attrname, left_parent, left, right_parent, right, **kw
+    ):
+        for l, r in zip_longest(left, right, fillvalue=None):
+            if (
+                l._gen_cache_key(self.anon_map[0], [])
+                if l._is_has_cache_key
+                else l
+            ) != (
+                r._gen_cache_key(self.anon_map[1], [])
+                if r._is_has_cache_key
+                else r
+            ):
+                return COMPARE_FAILED
 
     def visit_clauseelement(
         self, attrname, left_parent, left, right_parent, right, **kw
diff --git a/lib/sqlalchemy/sql/type_api.py b/lib/sqlalchemy/sql/type_api.py
index a32b80c95..cc226d7e3 100644
--- a/lib/sqlalchemy/sql/type_api.py
+++ b/lib/sqlalchemy/sql/type_api.py
@@ -979,18 +979,23 @@ class ExternalType:
 
     @property
     def _static_cache_key(self):
-        if self.cache_ok is None:
+        cache_ok = self.__class__.__dict__.get("cache_ok", None)
+
+        if cache_ok is None:
             subtype_idx = self.__class__.__mro__.index(ExternalType)
             subtype = self.__class__.__mro__[max(subtype_idx - 1, 0)]
 
             util.warn(
                 "%s %r will not produce a cache key because "
-                "the ``cache_ok`` flag is not set to True.  "
-                "Set this flag to True if this type object's "
+                "the ``cache_ok`` attribute is not set to True.  This can "
+                "have significant performance implications including some "
+                "performance degradations in comparison to prior SQLAlchemy "
+                "versions.  Set this attribute to True if this type object's "
                 "state is safe to use in a cache key, or False to "
-                "disable this warning." % (subtype.__name__, self)
+                "disable this warning." % (subtype.__name__, self),
+                code="cprf",
             )
-        elif self.cache_ok is True:
+        elif cache_ok is True:
             return super(ExternalType, self)._static_cache_key
 
         return NO_CACHE