Repository navigation
[OPENJPA-2951] Make the context flush after an in-memory bulk operation escapable - #216
Merged
Merged
Conversation
solomax
reviewed
Oct 13, 2026
solomax
approved these changes
Oct 13, 2026
cristof
approved these changes
Oct 18, 2026
Contributor
|
@rzo1 , can you check, please, if the expected order is really relevant for the test result? |
…on escapable A bulk UPDATE or DELETE that cannot be expressed as a single statement is executed in memory: OpenJPA loads the matching instances and applies the operation to them, so the result is a modified persistence context, not a modified database. The flush added for OPENJPA-2940 therefore carries the row count that executeUpdate() reports to the database - without it an em.clear() before the commit discards the operation while the count has already been reported, and a find() after that clear still sees the old row. Measured on Derby with SQL logging: with the flush an in-memory delete emits the owned-table and entity DELETE statements during executeUpdate() and a find() after em.clear() returns null; without it only the candidate SELECT is emitted and find() returns the instance again. The flush must not turn a legal bulk operation into a failure, though. Broker.flush() rejects an incremental flush when the runtime does not support OPTION_INC_FLUSH - which AbstractStoreManager.getUnsupportedOptions() does by default, so openjpa-xmlstore raised "This data store does not support incremental flushing" for every bulk delete - and when a savepoint is set that the SavepointManager cannot flush incrementally, which the default in-mem manager cannot. Both rejections happen before anything is written, but the persistence layer marks the transaction rollback-only on the way out, so the whole transaction was lost where 4.1.x simply applied the operation. The new Broker.isFlushSupported() reports both conditions, and the bulk operation now skips the flush and logs it at INFO instead of flushing blindly. The pre-flush of the candidate query has always been guarded by OPTION_INC_FLUSH in the same way. The second cost is that the kernel has no way to flush single instances. BrokerImpl.flush(int) is protected, always operates on getTransactionalStates(), fires the flush events with that whole collection, calls afterFlush() on every state and clears FLAG_FLUSH_REQUIRED, and the Broker interface exposes only flush() and preFlush(); flushing the affected instances alone would need more kernel API, which is left as a follow-up. So unrelated pending changes are written as well. Under the default FlushModeType.AUTO that is usually not a change of its own, because the query that loads the candidates flushes the context first - but that pre-flush of unrelated changes is itself new in 4.2.0 (OPENJPA-2940), and it does not happen at all when the query never reaches the datastore, i.e. when the candidates were handed over with setCandidateCollection(): isInMemory() short-circuits the pre-flush before it looks at the flush mode. The trailing flush is then the only flush even under AUTO, which the SQL log confirms - the candidate SELECT precedes the UPDATE of the unrelated instance. The other case is FlushModeType.COMMIT, or FlushBeforeQueries=false or with-connection without an open connection, where a dirty instance of a type that is not on the query's access path used to stay pending. IgnoreChanges is not involved: QueryImpl line 1032 short-circuits it for every operation other than OP_SELECT. The flush also decides when the AFTER_DELETE_PERFORMED and AFTER_UPDATE_PERFORMED callbacks of the affected instances fire, since StateManagerImpl.afterFlush() raises them. That is the route most applications take into this code: JDBCStoreQuery.executeDelete() gives up on the single statement precisely when the candidate has delete listeners, "so the callbacks are invoked". Neither behaviour is mandated. Jakarta Persistence 3.1 section 4.10 states that "the persistence context is not synchronized with the result of the bulk update or delete" and recommends that "bulk update and delete operations should only be performed within a transaction in a new persistence context or before fetching or accessing entities whose state might be affected by such operations". A reported row count that never reached the database is the worse of the two, so the flush stays the default and becomes escapable through the compatibility option FlushAfterInMemoryBulkOperation. Suppressing the flush must not and does not suppress the in-memory application of the operation: the instances are still modified or marked for deletion and are written by the next flush or at commit. The new section of the migration considerations documents the flush, the pending changes and callbacks it moves, the two cases in which it is the only flush, the option and the fallback when the context cannot be flushed, which covers the flush after the operation only - the flush before the candidate query is not guarded against a savepoint, so an operation issued with pending changes and a savepoint set still fails as it did before. No test in the tree depended on the flush - the bulk and criteria suites all pass with it removed. Six of the twelve new methods in openjpa-persistence-jdbc fail with the kernel part of this change reverted; the other six pin behaviour OPENJPA-2940 already introduced and pass on master as well. TestInMemoryBulkFlush pins the default, including the candidate-collection case under AUTO and the savepoint fallback on a clean context; four of its six methods assert the candidate SELECT so they cannot pass on a datastore-executed operation, and the other two assert the presence or absence of a single statement; TestInMemoryBulkFlushCompat pins the option; TestInMemoryBulkFlushCallbacks pins the callback timing on both paths; and TestPersistence of openjpa-xmlstore covers the AbstractStoreManager default, which had no bulk-operation test at all.
Contributor
Author
|
@cristof Good question, and it was the cause of the red build rather than a coincidence: the four legs started failing the moment #215 (OPENJPA-2950) was merged. The order matters, but the test had it the wrong way round. It asserted that the unrelated pending So the two statements are now pinned in the opposite order, and the test is renamed Rebased on master, braces added as solomax asked, and the bulk and flush packages are 58/0/0 on Derby and on H2. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A bulk
UPDATE/DELETEthat cannot be expressed as one statement is executed in memory, so the result is a modified persistence context rather than a modified database. The flush added in 4.2.0 is what carries the row countexecuteUpdate()reports to the database — measured on Derby: without it only the candidateSELECTis emitted, and anem.clear()discards the operation after the count was already reported. Section 4.10 mandates neither behaviour ("the persistence context is not synchronized with the result of the bulk update or delete"), so the flush stays the default and becomes escapable through the newFlushAfterInMemoryBulkOperationcompatibility option. Suppressing it does not suppress the in-memory application: the instances are still modified and written at commit.Review turned up something worse than the ticket.
Broker.flush()rejects an incremental flush when the store does not supportOPTION_INC_FLUSH— whichAbstractStoreManagerdoes not by default, so openjpa-xmlstore failed every bulk delete — and when a savepoint is set that theSavepointManagercannot flush incrementally. Both rejections marked the transaction rollback-only, so where 4.1.x applied the operation, 4.2.0 lost the transaction. The operation now checks first and skips the flush with an INFO log.Two things worth your attention:
Broker.isFlushSupported(), adefaultmethod returning true, overridden inBrokerImplandDelegatingBroker. A third-partyBrokerkeeps the default and would still hit the exception. I would rather have avoided new API, but discriminating the rejection after the fact is not possible —DelegatingBroker.flush()wraps theUnsupportedExceptioninto a genericPersistenceException, so catching it does not work.BrokerImpl.flush(int)is protected and always operates on the whole transactional cache, firing the flush events over it and clearing the flush flags, and theBrokerinterface exposes onlyflush()/preFlush(). Flushing the affected instances alone needs more kernel API; left as a follow-up rather than bodged, so unrelated pending changes still ride along — which the migration note now says.35 tests in the bulk package pass on both the Derby and H2 profiles; 12 are new, and 6 of those fail with the kernel part reverted (the other 6 pin behaviour 4.2.0 already introduced). The documented savepoint fallback covers the flush after the operation only — the pre-flush before the candidate query is not guarded against a savepoint, so that case still fails as before, and both the guide and the javadoc now say so.