Skip to content

Reset PreparedStatement.maxRows on pooled statements - #736

Closed
toaditi wants to merge 1 commit into
moqui:masterfrom
hotwax:maxrows-reset-upstream
Closed

Reset PreparedStatement.maxRows on pooled statements#736
toaditi wants to merge 1 commit into
moqui:masterfrom
hotwax:maxrows-reset-upstream

Conversation

@toaditi

@toaditi toaditi commented Aug 11, 2026

Copy link
Copy Markdown

Problem

EntityFindBuilder.makePreparedStatement() sets maxRows only when a positive value is requested, and never resets it otherwise:

if (maxRows != null && maxRows > 0) ps.setMaxRows(maxRows);   // never reset to 0

The default transaction manager caches prepared statements — TransactionInternalBitronix calls setPreparedStatementCacheSize(100). JDBC's maxRows is statement-level state that survives statement caching, so a PreparedStatement returned from the pool keeps the maxRows set by a prior find. A later find with the same SQL but no maxRows then inherits the stale limit:

  • Silent truncation on most databases (e.g. a paginated find with maxRows=20 leaves 20 on the pooled statement; the next unbounded find with the same SQL returns at most 20 rows).
  • Hard failure on H2, which requires fetchSize <= maxRows: the next find's default setFetchSize(100) throws Invalid value "100" for parameter "rows" because the stale maxRows is smaller.

Fix

Always call setMaxRows, using 0 (JDBC "no limit") when no positive value is requested — mirroring the always-set setFetchSize handling on the very next line, which already avoids this exact trap:

if (maxRows != null && maxRows > 0) { ps.setMaxRows(maxRows); } else { ps.setMaxRows(0); }

This is the only site that calls setMaxRows, and it is shared by the list/iterator and count paths, so the reset covers all finds.

Testing

Adds EntityFindTests > "maxRows does not leak onto pooled PreparedStatement", using the non-cached TestEntity so the finds actually build and execute a pooled statement: a first find with .maxRows(2) followed by an identical-SQL find with no maxRows must return the full result set. The test fails before the change (on H2, throws Invalid value "100" for parameter "rows" on the second find) and passes after. A full :framework:test run showed no regressions.

EntityFindBuilder.makePreparedStatement only set maxRows when a positive
value was requested, and never reset it otherwise. With Bitronix prepared
statement caching (setPreparedStatementCacheSize(100)) a pooled statement
retained the maxRows from a prior find, so a later find with the same SQL
and no maxRows inherited the stale limit -- silently truncating results
(and on H2 throwing, because the default fetchSize=100 then exceeds maxRows).

Always call setMaxRows (0 = no limit), mirroring the always-set fetchSize
handling on the following line. Adds an EntityFindTests regression test that
reproduces the pooled-statement leak.
@toaditi

toaditi commented Aug 11, 2026

Copy link
Copy Markdown
Author

Superseded by #737 (same change, opened from a personal fork).

@toaditi toaditi closed this Aug 11, 2026
@toaditi
toaditi deleted the maxrows-reset-upstream branch August 11, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant