matrei commented on PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5716508025
## Review round 4
**Head:** `d2f76c9e3b` (5 commits) · **Base:** `8.1.x` · Merge-base
`0980623`, `git diff --check` clean.
**What I ran locally** (`cleanTest` + `--no-build-cache`, XML report
timestamps verified):
| Run | Result |
|---|---|
| `grails-data-hibernate5-core:test` (full, incl.
`Hibernate5RefreshLockSpec` 73/73) | 911 tests, 0 failures, 39 skipped |
| `grails-data-hibernate7-core:test` (full, incl.
`Hibernate7RefreshLockSpec` 72/72) | 3151 tests, 0 failures, 29 skipped |
| `codeStyle` on both modules | clean |
| GitHub checks at the time of writing | 31 passed, 44 still pending |
The round-3 blocker is fixed: `lockRow` now locks the hierarchy root's row,
and the new *"waits for a competing commit to the root row"* feature has
exactly the shape of my probe (raw JDBC competitor updating the root table,
subclass load, polymorphic root load, and both static forms). Thanks for the
quick turnaround.
As in the earlier rounds I ran throwaway specs against both versions for the
shapes the new tests do not reach. This round I probed the third GORM
inheritance mapping, `tablePerConcreteClass true`, since the fix keys the lock
query off the hierarchy root. That mapping goes the other way.
### Findings
#### 1. Hibernate 7: with `tablePerConcreteClass true` the root lock query
locks nothing, and this commit regresses the case that worked before (medium)
For a union-subclass hierarchy Hibernate 7 renders `select 1 from
<RootEntity> e where e = :instance` as a query over a derived table, `select 1
from (select ... from probe_union_sub) pur1_0 where pur1_0.id = ? for update`,
or a `UNION ALL` of every concrete table when the root is concrete too. H2 does
not lock rows through the derived table, so the competing `FOR UPDATE` is
granted immediately and the reload reads whatever is committed at that moment.
Before this commit `lockRow` queried the loaded class, which for a leaf class
renders the plain concrete table (`select 1 from probe_union_sub pus1_0 where
pus1_0.id = ? for update`) and does lock the row.
Probe: `@Entity abstract class Root { Long version; String title; static
mapping = { tablePerConcreteClass true; id generator: 'table' } }`, `@Entity
class Sub extends Root { String extra }`, plus a variant with a concrete root.
After each call a raw JDBC `select id from <concrete table> where id = ? for
update` contender with a 300 ms lock timeout reports whether the row is
actually locked:
| Hibernate 7, H2 | `d2f76c9e3b` (root query) | `4198a9cb84` (loaded-class
query) | with the fix below |
|---|---|---|---|
| `Sub.get(id).refresh(lock: true)` | contender **granted**, no lock |
refused, locked | refused, locked |
| `Root.get(id).refresh(lock: true)` (concrete class `Sub`) | contender
**granted** | granted (root query) | refused, locked |
| `Sub.lock(id, refresh: true)`, managed | contender **granted** | refused |
refused, locked |
| same three on the concrete-root hierarchy | contender **granted** | as
above | refused, locked |
| `Sub.get(id).lock()` / `Sub.lock(id)` on a managed instance | contender
granted | granted | granted (see note) |
| `Sub.lock(id)`, not loaded | refused | refused | refused |
The competing-commit shape from your new feature, with the competitor
updating the concrete table, then does the user-visible thing: on `d2f76c9e3b`
every `refresh(lock: true)` variant returns without waiting and still holds
`title == 'original'` and the old version while `getCurrentLockMode` reports
`PESSIMISTIC_WRITE`; the trailing `session.lock` does not fail because it also
renders the derived table and reads the committed snapshot. With the fix below
all five competing-commit variants wait for the commit and reload `version +
1`, as the joined-table feature does. Hibernate 5 is fine for every form: its
`refresh(entity, lockMode)` and `lock` statements both go against the concrete
table (`select ... from probe5union_sub ... for update`), so the 16 probe
features pass there unchanged.
Note on the last row: plain `lock()` / managed `lock(id)` on a union
subclass also does not lock on Hibernate 7 + H2, because `session.lock` renders
`select id from (select ... from probe_union_sub) ... where id = ? and version
= ? for update`. That is pre-existing Hibernate 7 behaviour, not something this
PR introduced, and it is worth its own issue. PostgreSQL additionally documents
that `FOR UPDATE` cannot be applied to a `UNION`, so the concrete-root
hierarchy would presumably fail outright there rather than silently not lock
(not verified on PostgreSQL). It does mean the comment *"is the row lock()
contends on"* is only true for single-table and joined mappings.
**Fix (verified locally):** lock the instance's concrete entity when the
hierarchy is union-mapped, the root otherwise:
```groovy
EntityPersister descriptor =
session.unwrap(SessionImplementor).factory.mappingMetamodel
.getEntityDescriptor(persistentClass)
String lockEntityName = descriptor instanceof UnionSubclassEntityPersister ?
session.getEntityName(instance) : descriptor.rootEntityName
String hql = "select 1 from ${lockEntityName} e where e =
:instance".toString()
```
(`org.hibernate.persister.entity.UnionSubclassEntityPersister`;
`Hibernate.getClass(instance).name` works equally.) With that patch the 5
competing-commit and all 8 `refresh` / `lock(id, refresh: true)` probe features
pass on Hibernate 7, `Hibernate7RefreshLockSpec` is unaffected, and the only
remaining probe failures are the plain `lock()` rows, which are the
pre-existing behaviour. An instance whose *own* class has union subclasses (an
instance of a concrete root) still renders the union, which is the same limit
`lock()` has; I would accept that and say so in the `lockRow` javadoc. Please
add a `tablePerConcreteClass true` pair to both specs with the competing-commit
feature pointed at the concrete table, mirroring the joined-table one.
#### 2. Round-3 item 2 (three statements per locked refresh on Hibernate 7)
is unchanged (low, optional)
Still at your discretion; it becomes cheaper to do once finding 1 settles
what `lockRow` targets.
### Verified as correct
- **Joined-table fix:** `lockRow` targets `rootEntityName`, so the row that
holds the version is locked before the reload. The new feature passes 4/4 on
both versions in the fresh full-suite runs, and the four data rows cover the
subclass load, the polymorphic root load, and both static forms.
- **New test shape:** the competitor updates the root table under an open
transaction, the refresh is asserted to still be blocked after 200 ms, and
after the commit the reload shows `title == 'competing commit'`, `version ==
1`, the subclass column intact, `PESSIMISTIC_WRITE` recorded, and a follow-up
save at `version == 2`. This is the deterministic version of my round-3 probe.
One small nit: if the worker fails before `loaded.countDown()` the feature dies
on `loaded.await` with no cause shown; `loaded.await(10, SECONDS) ||
refreshing.get(1, SECONDS)` would surface the exception. Optional.
- **Helper refactor:** `openCompetingConnection` / `tableName` are shared by
the lock-contender and competing-commit features on both versions; behaviour of
the pre-existing `parentRowLockGranted` is unchanged (300 ms timeout, rollback,
close).
- **Single-table hierarchies and root-class instances:** unaffected by the
root query, as established in round 3.
- **No doc changes needed for this commit** as pushed; if finding 1 is fixed
as suggested, the union-subclass limit of `lock()` on Hibernate 7 is worth one
sentence in the Hibernate 7 guide next to the lock-mode table.
**Verdict:** request changes for finding 1. It is a small regression
introduced by the round-3 fix for a supported GORM mapping, with a verified
two-line correction and a clear test to add. Everything else from rounds 1 to 3
holds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
