Skip to content

Commit 9595934

Browse files
authored
fix: declare synchronized entity classes on native writes DHIS2-21963 [42] (#24847)
* fix: declare synchronized entity classes on native writes DHIS2-21963 (#24803) * fix: declare synchronized entity classes on native writes Native queries that run executeUpdate() without declaring which tables they touch make Hibernate conservatively evict EVERY L2 entity region, not just the affected one. NativeSQLQueryPlan.performExecuteUpdate calls coordinateSharedCacheCleanup with getCustomQuery().getQuerySpaces(). SQLCustomQuery leaves that set empty unless addSynchronizedEntityClass/QuerySpace was called, and BulkOperationCleanupAction.affectedEntity treats an empty set as "affects everything", so every persister that can write to cache gets an EntityCleanup. EntityCleanup.release() calls unlockRegion(), which is evictAll() -> evictData() on the whole region. Note AbstractReadWriteAccess.removeAll IS a no-op under read-write, which is why this is easy to miss; the eviction comes from unlockRegion, not removeAll. Both stores here use stateless sessions, where isEventSource() is false, so the cleanup runs immediately rather than being deferred to the ActionQueue. Draft to confirm the mechanism: measured on a Sierra Leone instance, a single data value write with a new period emptied the whole DataElement region (0 hits / 3037 logical misses on the next read), while the same write with an existing period left it fully warm. No eviction or removal counter moves and nothing is logged, so this is invisible without instrumenting Hibernate. HousekeepingJob runs every 20s by default and drives the jobconfiguration writes here, so on a stock instance every cached entity region is dropped on that cadence. * fix: declare query spaces on the remaining native writes * fix: synchronize entities not join tables so cached collections are evicted * docs: explain query spaces in nativeSynchronizedQuery javadoc * refactor: add stateless session variant of nativeSynchronizedQuery * refactor: use nativeSynchronizedQuery in complete data set registration store * refactor: run audit trigger DDL via jdbcTemplate * fix: declare element entities for native writes to collection tables A write to a collection table needs the collection's element entity declared, not the owning entity. An entity's query spaces are its own table(s) only (SingleTableEntityPersister), never the tables of the collections it owns, and BulkOperationCleanupAction resolves collection regions via getCollectionRolesByEntityParticipant, which is keyed by the collection's element entity. So synchronizing on the owner leaves its cached collection stale: * categories_categoryoptions needs CategoryOption, not Category * users_catdimensionconstraints needs Category, not User Confirmed against a running instance. With a category collection warm and in steady state (+1 hit, 0 misses per read), a merge of an unrelated pair of categories left a bystander category's cached collection reloading cold (+2 misses, +1 put), while an unrelated write left it fully warm. Also correct the comments in the category combo and data set stores, which reached the right regions but described the wrong reason, and expand the nativeSynchronizedQuery javadoc with the element-entity rule. * test: expect OptionSet region to survive housekeeping DHIS2-21963 The housekeeping job no longer wipes every L2 entity region, so the OptionSet region keeps its entries across the job and the query after it hits the cache instead of reloading cold. This is the same assertion change master and 2.43 already carry from "fix: Replicate users with JDBC" (#23154), which is not on 2.42.
1 parent 180ac3f commit 9595934

5 files changed

Lines changed: 58 additions & 18 deletions

File tree

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataValueTrimStore.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,7 @@ public int updateDeletedIfNotZeroIsSignificant() {
8787
AND de.zeroissignificant = false
8888
AND dv.dataelementid = de.dataelementid
8989
AND (dv.value IS NULL OR dv.value = '')""";
90-
return getSession()
91-
.createNativeQuery(sql)
90+
return nativeSynchronizedQuery(sql)
9291
.setLockOptions(new LockOptions(PESSIMISTIC_WRITE).setTimeOut(1000))
9392
.executeUpdate();
9493
}

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/hibernate/HibernateEventStore.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,6 @@ where attributeoptioncomboid in (%s)
155155
"""
156156
.formatted(coc, cocs.stream().map(String::valueOf).collect(Collectors.joining(",")));
157157

158-
entityManager.createNativeQuery(sql).executeUpdate();
158+
nativeSynchronizedQuery(sql).executeUpdate();
159159
}
160160
}

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HibernateJobConfigurationStore.java

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import org.hibernate.query.NativeQuery;
5151
import org.hisp.dhis.common.UID;
5252
import org.hisp.dhis.common.hibernate.HibernateIdentifiableObjectStore;
53+
import org.hisp.dhis.fileresource.FileResource;
5354
import org.hisp.dhis.hibernate.jsonb.type.JsonJobParametersType;
5455
import org.hisp.dhis.security.acl.AclService;
5556
import org.springframework.context.ApplicationEventPublisher;
@@ -385,7 +386,10 @@ public boolean tryExecuteNow(@Nonnull UID jobId) {
385386
and (schedulingtype != 'ONCE_ASAP' or lastfinished is null)
386387
""";
387388
return runWriteInStatelessSession(
388-
q -> q.createNativeQuery(sql).setParameter("id", jobId.getValue()).executeUpdate())
389+
q ->
390+
nativeSynchronizedQuery(q, sql)
391+
.setParameter("id", jobId.getValue())
392+
.executeUpdate())
389393
> 0;
390394
}
391395

@@ -413,7 +417,10 @@ and not exists (
413417
)
414418
""";
415419
return runWriteInStatelessSession(
416-
q -> q.createNativeQuery(sql).setParameter("id", jobId.getValue()).executeUpdate())
420+
q ->
421+
nativeSynchronizedQuery(q, sql)
422+
.setParameter("id", jobId.getValue())
423+
.executeUpdate())
417424
> 0;
418425
}
419426

@@ -444,7 +451,10 @@ public boolean tryCancel(@Nonnull UID jobId) {
444451
)
445452
""";
446453
return runWriteInStatelessSession(
447-
q -> q.createNativeQuery(sql).setParameter("id", jobId.getValue()).executeUpdate())
454+
q ->
455+
nativeSynchronizedQuery(q, sql)
456+
.setParameter("id", jobId.getValue())
457+
.executeUpdate())
448458
> 0;
449459
}
450460

@@ -475,7 +485,7 @@ public boolean tryFinish(@Nonnull UID jobId, JobStatus status) {
475485
""";
476486
return runWriteInStatelessSession(
477487
q ->
478-
q.createNativeQuery(sql)
488+
nativeSynchronizedQuery(q, sql)
479489
.setParameter("id", jobId.getValue())
480490
.setParameter("status", status.name())
481491
.executeUpdate())
@@ -503,7 +513,7 @@ public boolean trySkip(@Nonnull String queue) {
503513
or lastexecuted < (select lastexecuted from jobconfiguration where queuename = :queue and queueposition = 0 limit 1))
504514
""";
505515
return runWriteInStatelessSession(
506-
q -> q.createNativeQuery(sql).setParameter("queue", queue).executeUpdate())
516+
q -> nativeSynchronizedQuery(q, sql).setParameter("queue", queue).executeUpdate())
507517
> 0;
508518
}
509519

@@ -521,7 +531,7 @@ public void updateProgress(
521531
""";
522532
runWriteInStatelessSession(
523533
q ->
524-
q.createNativeQuery(sql)
534+
nativeSynchronizedQuery(q, sql)
525535
.setParameter("id", jobId.getValue())
526536
.setParameter("json", progressJson)
527537
.setParameter("errors", errorCodes)
@@ -539,7 +549,7 @@ public int updateDisabledJobs() {
539549
where jobstatus = 'SCHEDULED'
540550
and enabled = false
541551
""";
542-
return runWriteInStatelessSession(q -> q.createNativeQuery(sql).executeUpdate());
552+
return runWriteInStatelessSession(q -> nativeSynchronizedQuery(q, sql).executeUpdate());
543553
}
544554

545555
@Override
@@ -557,7 +567,7 @@ and now() > lastfinished + :ttl * interval '1 minute'
557567
int deletedCount =
558568
runWriteInStatelessSession(
559569
q ->
560-
q.createNativeQuery(sql)
570+
nativeSynchronizedQuery(q, sql)
561571
.setLockOptions(new LockOptions(LockMode.PESSIMISTIC_WRITE).setTimeOut(2000))
562572
.setParameter("ttl", max(1, ttlMinutes))
563573
.executeUpdate());
@@ -575,6 +585,7 @@ and uid not in (select uid from jobconfiguration where schedulingtype = 'ONCE_AS
575585
runWriteInStatelessSession(
576586
q ->
577587
q.createNativeQuery(sql2)
588+
.addSynchronizedEntityClass(FileResource.class)
578589
.setLockOptions(new LockOptions(LockMode.PESSIMISTIC_WRITE).setTimeOut(2000))
579590
.executeUpdate());
580591
return deletedCount;
@@ -606,7 +617,7 @@ and now() > lastalive + :timeout * interval '1 minute'
606617
""";
607618
return runWriteInStatelessSession(
608619
q ->
609-
q.createNativeQuery(sql)
620+
nativeSynchronizedQuery(q, sql)
610621
.setParameter("timeout", max(1, timeoutMinutes))
611622
.executeUpdate());
612623
}
@@ -637,7 +648,10 @@ public boolean tryRevertNow(@Nonnull UID jobId) {
637648
and now() > jobconfiguration.lastalive + interval '1 minute'
638649
""";
639650
return runWriteInStatelessSession(
640-
q -> q.createNativeQuery(sql).setParameter("id", jobId.getValue()).executeUpdate())
651+
q ->
652+
nativeSynchronizedQuery(q, sql)
653+
.setParameter("id", jobId.getValue())
654+
.executeUpdate())
641655
> 0;
642656
}
643657

dhis-2/dhis-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/HibernateNativeStore.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import javax.annotation.Nonnull;
3838
import lombok.extern.slf4j.Slf4j;
3939
import org.hibernate.Session;
40+
import org.hibernate.StatelessSession;
4041
import org.hibernate.metamodel.spi.MetamodelImplementor;
4142
import org.hibernate.persister.entity.EntityPersister;
4243
import org.hibernate.persister.entity.SingleTableEntityPersister;
@@ -97,8 +98,22 @@ protected final Session getSession() {
9798
* current class of the store. Use this to avoid all Hibernate second level caches from being
9899
* invalidated.
99100
*
100-
* <p>Be aware that it is only correct to use this if and only if the only table touched by the
101-
* native query is the one table belonging to the store.
101+
* <p>Every native write triggers a cache cleanup driven only by its declared query spaces (the
102+
* tables it affects). Declare none and Hibernate reads that as "unknown" and evicts
103+
* <em>every</em> entity and collection region, so always declare, even when this entity is not
104+
* cached.
105+
*
106+
* <p>Correct only when the sole table this statement <em>writes</em> is the store's own; tables
107+
* merely read by a join or subquery need no declaration. Otherwise call {@code
108+
* addSynchronizedEntityClass} directly, once per entity written.
109+
*
110+
* <p>Beware collection tables. An entity's query spaces are its own table(s) only, never the
111+
* tables of the collections it owns, so declaring the owner does <em>not</em> evict its cached
112+
* collections. Hibernate resolves collection regions through {@code
113+
* getCollectionRolesByEntityParticipant}, which is keyed by the collection's <em>element</em>
114+
* entity. So for a write against a join or child table, declare the entity on the far side of the
115+
* association: {@code categories_categoryoptions} needs {@code CategoryOption}, not {@code
116+
* Category}.
102117
*
103118
* @param sql the SQL query to execute
104119
* @return the {@link NativeQuery} instance
@@ -108,6 +123,17 @@ protected NativeQuery nativeSynchronizedQuery(@Language("SQL") String sql) {
108123
return getSession().createNativeQuery(sql).addSynchronizedEntityClass(getClazz());
109124
}
110125

126+
/**
127+
* Same as {@link #nativeSynchronizedQuery(String)} but on a {@link StatelessSession}, which is a
128+
* different session from the store's own. Pass the session that opened the surrounding
129+
* transaction, otherwise the query runs outside it.
130+
*/
131+
@SuppressWarnings("rawtypes")
132+
protected NativeQuery nativeSynchronizedQuery(
133+
StatelessSession session, @Language("SQL") String sql) {
134+
return session.createNativeQuery(sql).addSynchronizedEntityClass(getClazz());
135+
}
136+
111137
/**
112138
* Same as {@link #nativeSynchronizedQuery(String)} just with the return type being specified as
113139
* the store entity type. Use only when the result is a of the store entity type or a list of it.

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/cache/HibernateQueryCacheTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,17 @@ void testQueryCache() {
121121
void testHouseKeepingJobWithCache() {
122122
setUpData();
123123
createSelectQuery(10);
124-
assertEquals(9, sessionFactory.getStatistics().getQueryCacheHitCount());
124+
long queryCacheHitCountBefore = sessionFactory.getStatistics().getQueryCacheHitCount();
125+
assertEquals(9, queryCacheHitCountBefore);
125126
housekeepingJob.execute(null, JobProgress.noop());
126127
createSelectQuery(1);
127128
assertEquals(
128-
9,
129+
10,
129130
sessionFactory
130131
.getStatistics()
131132
.getCacheRegionStatistics(OptionSet.class.getName())
132133
.getHitCount());
133-
assertTrue(sessionFactory.getStatistics().getQueryCacheHitCount() > 10);
134+
assertTrue(sessionFactory.getStatistics().getQueryCacheHitCount() > queryCacheHitCountBefore);
134135
}
135136

136137
private void createSelectQuery(int numberOfQueries) {

0 commit comments

Comments
 (0)