[adityaanikam]

Fixes #2780.

selectAllRowsFromConcreteTable and selectOneRowFromConcreteTable in ExpressionQueryMechanism cache the prepared DatabaseCall per concrete subclass on the query, in concreteSubclassCalls. The cached call keeps a path back to the UnitOfWork that was active when it was first built, through its parameter expressions, the ExpressionBuilder, the SQLSelectStatement and that statement's query. The cache is never cleared, so a long-lived shared query can keep that unit of work reachable for as long as the process runs.

As discussed above, removing the cache is too broad, and fixing the retention itself is a larger change. This PR adds a way to opt out instead:

• ObjectLevelReadQuery.setShouldCacheConcreteSubclassCalls(boolean), which defaults to true, so existing behaviour is unchanged
• the query hint eclipselink.query.cache-concrete-subclass-calls (QueryHints.CACHE_CONCRETE_SUBCLASS_CALLS) to set it from JPA

When it is false, both methods neither read nor write concreteSubclassCalls and build the call on every execution.

The setting belongs to the query it is set on. A query that prepares itself from an equivalent cached query (prepareFromCachedQuery, then prepareFromQuery) keeps its own value, because prepareFromQuery only copies properties that affect the SQL. copyFromQuery, used when a query is morphed into another query type, carries it over like the other per-query settings.

ObjectLevelReadQueryConcreteSubclassCallsCacheTest covers the default, the setter, clone(), copyFromQuery(), and prepareFromQuery() in both directions (cached query enabled with the new query disabled, and the reverse) for ReadAllQuery and ReadObjectQuery.

[arjantijms]

This removes the cache entirely so both methods always rebuild the call,

Maybe that is a bit too much. The cache is there for a reason. Maybe better to look at the retention problem itself?

This removes the cache entirely so both methods always rebuild the call, which is what @ vrivon validated in production on #2780:

Did not @vrivon ran a sanitized-clone workaround, not cache removal?

[vrivon]

Both, in that order — and the order is the whole point.

I first ran the sanitized-clone workaround: cache a shallow clone with setQuery(null), additionally null ExpressionBuilder.session on the parameters' expressions, and hand out defensive clones on cache hits. It looked like it worked — pinned UoW counts dropped immediately. It did not hold. Over the following days the live set grew back at ~480 MB/day to 3.6 GB, and a second heap dump showed the anchor was still there through a third path I had missed:

SQLCall.parameters → ParameterExpression.localBase → ExpressionBuilder.statement → SQLSelectStatement.query → session = RepeatableWriteUnitOfWork

Only after that did I disable the cache entirely, and that is what held: 3.6 GB → ~1.1 GB, flat since, allocation stalls from dozens a day to zero. I've corrected the issue body to retract the earlier claim.

To be clear about what that does and doesn't argue for: I'm not proposing cache removal as the upstream fix, and I agree with you that it's too much. I disabled it in my own application because I was patching EclipseLink from the outside by reflection, and I couldn't safely mutate internal structures whose invariants I'm not in a position to verify — the failed sanitized clone is precisely what that risk looks like in practice. Upstream doesn't have that constraint and should fix the retention itself.

For a retention fix, the three anchors identified so far are: cachedCall.query (re-set on every cache hit by setCall()), ExpressionBuilder.session (captured by SQLSelectStatement.normalize() from the query's current session rather than the root session), and the statement/query back-reference reachable from the cached call's parameters. The third one is what survives the two obvious fixes.

One data point on cost, offered neutrally rather than as an argument: with the cache disabled I measured no CPU or latency difference in my workload (heavy polymorphic lazy loading over JOINED hierarchies). That may simply mean my workload isn't where the cache earns its keep.

[arjantijms]

Thanks for the extra data point.

The question remains what to do with this PR then? The problem seems real, but completely removing the cache is probably not the best option. Without fully addressing the true core issue (fixing the retention itself), simply introducing an option to disable the cache might be a path worth considering.

[adityaanikam]

Pushed a new approach: instead of removing the concrete subclass call cache entirely, this adds a query hint (eclipselink.query.cache-concrete-subclass-calls, default true) to opt out of it per query. Default behavior is unchanged. Disabling it avoids the UnitOfWork retention from #2780, at the cost of rebuilding the call on every execution instead of caching it.

[robertpatrick]

@adityaanikam In looking at this PR, I have a question about prepared-query reuse and the desired behavior. What I see is the following, which was observed for both ReadAllQuery and ReadObjectQuery. What I saw was the following:

Cached query’s setting	New query requests	Setting observed at execution
Enabled	Disabled	Enabled
Disabled	Enabled	Disabled

This appears to be happening due to the cached setting being copied into the new query before the new query's execution clone is created, which causes the new query to execute with the cached query's setting instead of the new query's requested setting.

If this is intentional, it would be very confusing (at least to me). It would be good to:

• clarify the intended behavior
• make sure that there are tests verifying the behavior you want
• edit the description of the PR to reflect the actual change instead of the old disabling the cache behavior

[adityaanikam]

@robertpatrick Thanks for checking this, it was not intentional. prepareFromQuery copied the flag from the cached query, but that method only copies properties that affect the SQL, and this is a per-query setting. I removed it from prepareFromQuery and copy it in copyFromQuery with the other per-query settings instead. The tests now cover both rows of your table for ReadAllQuery and ReadObjectQuery, and I rewrote the description to describe the current change.