[
https://issues.apache.org/jira/browse/OPENJPA-2950?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128405#comment-18128405
]
ASF subversion and git services commented on OPENJPA-2950:
----------------------------------------------------------
Commit 49bbeb84f52ab9f44071e87cd214ed2afed8fd20 in openjpa's branch
refs/heads/master from Richard Zowalla
[ https://gitbox.apache.org/repos/asf?p=openjpa.git;h=49bbeb84f ]
[OPENJPA-2950] Flush before a query only for the types it reads
Since OPENJPA-2940 a query with FlushModeType.AUTO flushed as soon as anything
in the persistence context was dirty, and overrode openjpa.IgnoreChanges
whenever the flush mode was AUTO - which is the default. Both are reverted, and
the gap the broadening was meant to close is fixed where it actually is.
Why the access path is enough. Section 3.10.8 of the specification scopes the
duty to the updates "which could potentially affect the result of the query",
and lets the provider discharge it "by flushing those entities to the database
or by some other means". A full-context flush is therefore permitted but not
required, and the access path check is exactly such other means: only a dirty
instance of a type the query reads can change its result. That holds, however,
only for as long as the access path is complete, and in two places it was not.
JPQL. AbstractExpressionBuilder.traversePath() registered the metadata of every
hop, so Order, Customer and Spouse are all in the access path of "SELECT o FROM
Order o WHERE o.customer.spouse.lastName = 'Miller'" - the case the comment
added with the broadening claimed was missed. What it did miss is a collection-
or map-valued path used without a join variable: the declared type of such a
field is Collection or Map, which has no metadata, so SIZE(x.coll) and
x.coll IS EMPTY left the element and key types out and a dirty element was not
flushed. traversePath() now registers them, which also makes the JPQL and the
Criteria side agree - the latter registers them for the same field.
Criteria. CriteriaExpressionBuilder.evalAccessPaths() looked only at the roots
and their immediate joins, so a type reached by an implicit join (a path
navigated with Path.get), by a join of a join, by a fetch join, inside a
subquery or behind a join made on a correlated join was left out - the subquery
case was a standing "TODO -- need to handle subqueries". That is now fixed at
the source: PathImpl registers each field it traverses on the query,
SubqueryImpl
hands its access path to the enclosing query, the join walk follows the whole
join tree and starts at the correlated joins of the enclosing subquery as well,
and CriteriaExpressionBuilder merges all of it into QueryExpressions.accessPath
after the expressions have been converted.
IgnoreChanges. openjpa.IgnoreChanges=true is a documented option whose purpose
is to let a query run straight against the datastore, recommended in
ref_guide_optimization.xml together with the default
openjpa.FlushBeforeQueries=true. Overriding it whenever the flush mode is
FLUSH_TRUE therefore silently removed it for every default configuration. It is
restored unconditionally, as in 4.1.x, and no distinction is drawn between a
flush mode set on the query and one inherited from the persistence context:
section 3.10.8 states the same duty for both triggers, so there is no basis for
honouring the option for one and not the other. The option is an OpenJPA
extension that opts a SELECT out of that duty, which ref_guide_conf.xml now
says where users enable it.
Making the override conditional on the flush mode would also have required
state the facade does not have. Writing through to the kernel's ignoreChanges
instead, as this was first implemented, is not sound: the write is one-way, so
setFlushMode(AUTO) followed by setFlushMode(COMMIT) on the same query left
ignoreChanges false with flush FLUSH_FALSE, and QueryImpl.isInMemory() then
entered the branch, could not flush and executed the query in memory over the
whole extent - a different result set and an unbounded scan for the same query
text and flush mode. It also fired for queries the application never set a flush
mode on, because EntityManagerFactoryImpl.addNamedQuery() captures a flush mode
that is never unset and createNamedQuery() replays it as setFlushMode(AUTO).
The prohibitions of section 3.10.8 hold. _broker.isActive() already covers "if
there is no transaction active, the persistence provider must not flush"; an
unjoined context cannot reach the flush at all, because
EntityManagerFactoryImpl rejects SynchronizationType.UNSYNCHRONIZED.
No compatibility option is added: the broadened behaviour only ever existed on
the unreleased 4.2.0 line, so there is nothing to be compatible with. The
migration note is rewritten to describe what actually changes for an
application upgrading from 4.1.x.
Tests, asserting on emitted SQL rather than on results, because a full-context
flush returns the same results:
- TestQueryFlushAccessPath: a dirty instance of a type outside the access path
must not flush; the candidate type, a dirty subclass of it, a type reached by
JPQL navigation, a collection path in SIZE() and in IS EMPTY, and - for the
Criteria API - an implicit join, a join of a join, a fetch join, a subquery
and a join made on a correlated join must all be visible; COMMIT must not
flush; no flush without an active transaction.
- TestQueryFlushIgnoreChanges: IgnoreChanges=true keeps suppressing the flush
for an inherited flush mode, for AUTO set on the query, across a
setFlushMode(AUTO)-then-COMMIT sequence and through an addNamedQuery() /
createNamedQuery() round trip.
Each hunk is discriminated by reverting it alone: the kernel access-path hunk by
the two collection-path tests, the correlated-join hunk by the correlated-join
test, openjpa-persistence/.../criteria/ by the five Criteria tests, and the
QueryImpl hunk by three of the four IgnoreChanges tests, each on an
assertNotSQL for an UPDATE that should not have been emitted.
Reverting the whole diff at once does not discriminate, and deliberately so:
seven of the eighteen tests pass against master as well, because the blanket
flush there masks the incomplete access paths - everything is flushed anyway.
Those seven are regression guards (dirty candidate type, dirty subclass, JPQL
navigation, subquery type, COMMIT, no active transaction, AUTO-then-COMMIT),
and the two navigation ones also back up the finding that traversePath already
covered o.customer.spouse.
> Broadened auto-flush before every query hurts perf
> --------------------------------------------------
>
> Key: OPENJPA-2950
> URL: https://issues.apache.org/jira/browse/OPENJPA-2950
> Project: OpenJPA
> Issue Type: Sub-task
> Components: jpa
> Affects Versions: 4.2.0
> Reporter: Maxim Solodovnik
> Priority: Major
> Fix For: 4.2.0
>
>
> Discussion link:
> https://github.com/apache/openjpa/pull/144#discussion_r3682999345
> **(medium)** With FLUSH_TRUE (default FlushModeType.AUTO) this now flushes
> before every query as soon as anything in the context is dirty, and the
> `flush == FLUSH_TRUE` addition at line 1033 also overrides the IgnoreChanges
> setting. That defeats the access-path optimization and can be a real perf
> regression in write-heavy transactions - can the broadened check be limited
> to types related to the query access path, or guarded by a compatibility flag?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
