From 164366db8444d3050ef2800be2304693eb99e54b Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 9 Jul 2026 17:46:11 -0700 Subject: [PATCH 1/8] GitHub Issue 1187: Auto-disable PostgreSQL JDBC result set caching --- .../labkey/api/data/SqlExecutingSelector.java | 67 +++++++++++++++++-- .../labkey/api/data/SqlSelectorTestCase.java | 27 +++++++- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index 5349c107320..622e6031f33 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -33,6 +33,7 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; @@ -46,10 +47,14 @@ public abstract class SqlExecutingSelector _namedParameters = null; - private ConnectionFactory _connectionFactory = super::getConnection; + private @Nullable ConnectionFactory _connectionFactory = null; // null means "no explicit choice"; see getEffectiveConnectionFactory() + private boolean _jdbcCachingExplicitlySet = false; private Integer _fetchSize = null; // By default, use the standard fetch size private @Nullable AsyncQueryRequest _asyncRequest = null; @@ -79,21 +84,51 @@ public interface ConnectionFactory @Override public Connection getConnection() throws SQLException { - return _connectionFactory.get(); + return getEffectiveConnectionFactory().get(); + } + + /** + * Determines which {@link ConnectionFactory} to use for this query. When a caller has explicitly chosen a caching + * behavior via {@link #setJdbcCaching(boolean)} or supplied a Connection at construction time, that choice is + * honored. Otherwise, JDBC caching is disabled by default: we ask the dialect for a streaming ConnectionFactory so + * the driver won't buffer the entire ResultSet in memory. The dialect returns null (meaning "use the shared + * Connection with the driver's default caching") when a transaction is active, the dialect is not PostgreSQL, or the + * statement is not a SELECT, so this default is safe by construction. Resolving lazily here (rather than at + * construction) ensures the transaction check reflects the state at execution time. + */ + private ConnectionFactory getEffectiveConnectionFactory() + { + // Honor an explicit setJdbcCaching() call (which populated _connectionFactory)... + if (_jdbcCachingExplicitlySet) + return _connectionFactory; + + // ...or a Connection supplied at construction time (super::getConnection returns the stashed _conn) + if (null != _conn) + return super::getConnection; + + ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(false, getScope(), + new SQLFragment("SELECT FakeColumn FROM FakeTable") /* SqlExecutingSelector always generates SELECT statements */); + + return null != factory ? factory : super::getConnection; } /** *

Calling this method with cache=false ensures that the JDBC driver will not cache the produced ResultSet in * memory, which is useful when potentially working with very large (e.g., > 100MB) ResultSets. Calling it with - * cache=true (the default setting) ensures the JDBC driver's default caching behavior.

+ * cache=true ensures the JDBC driver's default caching behavior.

* *

By default, the PostgreSQL JDBC driver caches every ResultSet in its entirety. This can lead to * OutOfMemoryErrors when working with very large ResultSets. When the underlying database is PostgreSQL, calling * this method with false instructs this SqlExecutingSelector to use an unshared Connection and configure it with * special settings that disable the driver caching. The trade-off is that the underlying database query will not * use the shared Connection that other code on the thread (up or down the call stack) may be using, making - * Connection exhaustion more likely; that's why JDBC caching is on by default. Calling this method is not - * compatible with passing in an explicit Connection to the constructor.

+ * Connection exhaustion more likely. Calling this method is not compatible with passing in an explicit Connection to + * the constructor.

+ * + *

Note that when neither this method nor an explicit Connection is supplied, JDBC caching is disabled by default + * whenever it's safe to do so (PostgreSQL, no active transaction, SELECT statement) — see + * {@link #getEffectiveConnectionFactory()}. Callers that require the driver's default caching behavior (e.g., to + * share the thread's Connection) must therefore opt in explicitly by calling this method with cache=true.

* *

When the underlying database is not PostgreSQL, calling this method has no effect, other than validating that * the stashed Connection is null.

@@ -109,10 +144,32 @@ public SELECTOR setJdbcCaching(boolean cache) ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(cache, getScope(), new SQLFragment("SELECT FakeColumn FROM FakeTable") /* SqlExecutingSelector always generates SELECT statements */); _connectionFactory = null != factory ? factory : super::getConnection; + _jdbcCachingExplicitlySet = true; return getThis(); } + /** + * Overridden to warn when a large number of rows is pulled into a Java collection. Loading many rows into memory + * (here plus, potentially, in the JDBC driver's buffer) is a common source of OutOfMemoryErrors; callers should + * generally prefer a streaming method — {@link #forEach}, {@link #forEachBatch}, or {@link #uncachedStream} — that + * processes rows without materializing them all at once. {@code getArray}, {@code getCollection}, + * {@code getMapArray}, and {@code getMapCollection} all delegate here, so they're covered as well. + */ + @Override + public @NotNull ArrayList getArrayList(Class clazz) + { + ArrayList result = super.getArrayList(clazz); + + if (result.size() >= LARGE_RESULT_THRESHOLD) + { + LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to forEach(), forEachBatch(), or uncachedStream() to reduce memory usage. SQL: {}", + result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql(), new Throwable("Stack trace for large collection load")); + } + + return result; + } + /** * Set a ResultSet fetch size that differs from the default value (1,000 rows on PostgreSQL). This is normally a * fine fetch size, but not when dealing with rows containing large TEXT or BYTEA columns. diff --git a/api/src/org/labkey/api/data/SqlSelectorTestCase.java b/api/src/org/labkey/api/data/SqlSelectorTestCase.java index e77de43d433..ecf0bbb9370 100644 --- a/api/src/org/labkey/api/data/SqlSelectorTestCase.java +++ b/api/src/org/labkey/api/data/SqlSelectorTestCase.java @@ -186,13 +186,23 @@ public void testJdbcUncached() throws SQLException DbScope scope = CoreSchema.getInstance().getScope(); try (Connection conn = scope.getConnection()) { - // Default setting is to cache and share the connection + // Default (no explicit setJdbcCaching() call) now auto-disables JDBC caching when it's safe: a separate, + // uncached Connection on PostgreSQL (outside a transaction), but still the shared Connection on SQL Server. try (Connection conn2 = new SqlSelector(scope, "SELECT RowId, Body FROM comm.Announcements").getConnection()) { - assertEquals(conn, conn2); + if (scope.getSqlDialect().isPostgreSQL()) + { + assertNotEquals(conn, conn2); + assertEquals(TRANSACTION_READ_UNCOMMITTED, conn2.getTransactionIsolation()); + assertFalse(conn2.getAutoCommit()); + } + else + { + assertEquals(conn, conn2); + } } - // Same as the default setting + // Explicitly requesting caching shares the connection, even on PostgreSQL try (Connection conn2 = new SqlSelector(scope, "SELECT RowId, Body FROM comm.Announcements").setJdbcCaching(true).getConnection()) { assertEquals(conn, conn2); @@ -221,6 +231,17 @@ public void testJdbcUncached() throws SQLException } } } + + // Inside a transaction, the default must NOT grab a separate Connection, even on PostgreSQL: the caller may be + // relying on reading its own uncommitted writes, so we fall back to the shared, transactional Connection. + try (DbScope.Transaction tx = scope.ensureTransaction()) + { + try (Connection conn2 = new SqlSelector(scope, "SELECT RowId, Body FROM comm.Announcements").getConnection()) + { + assertEquals(scope.getConnection(), conn2); + } + tx.commit(); + } } // Passing in a Connection and calling setJdbcCaching() should throw From 1c6a3b9ff3a41d4a853d97af21dec7e210acde80 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 13 Jul 2026 18:06:51 -0700 Subject: [PATCH 2/8] Reuse connections when possible --- api/src/org/labkey/api/data/DbScope.java | 21 ++++++ .../labkey/api/data/SqlExecutingSelector.java | 68 +++++++++++++++---- .../labkey/api/data/SqlSelectorTestCase.java | 37 ++++++++++ .../data/dialect/BasePostgreSqlDialect.java | 28 ++++++-- .../labkey/api/data/dialect/SqlDialect.java | 45 ++++++------ 5 files changed, 162 insertions(+), 37 deletions(-) diff --git a/api/src/org/labkey/api/data/DbScope.java b/api/src/org/labkey/api/data/DbScope.java index d84f495a64d..4ec508a649c 100644 --- a/api/src/org/labkey/api/data/DbScope.java +++ b/api/src/org/labkey/api/data/DbScope.java @@ -1202,6 +1202,11 @@ public synchronized boolean release(Connection conn) return 0 == _refCount; } + + public synchronized int getRefCount() + { + return _refCount; + } } /** @@ -1259,6 +1264,22 @@ private Connection getCurrentConnection(@Nullable Logger log) throws SQLExceptio return getConnectionHolder().get(log); } + /** + * @return true if this thread already holds the shared, ref-counted thread connection — i.e., some code up the stack + * called {@link #getConnection()} and hasn't released it yet. This simply reports the existing {@link + * ConnectionHolder} reference count (the same count that already governs {@link ConnectionType#Thread} sharing); it + * is NOT a separate/new counter maintained for this purpose. + *

+ * Callers that borrow the thread connection and temporarily modify its state (e.g., disabling JDBC caching for a + * streaming read) must do so only when this returns false, so that they are the outermost borrower and can safely + * restore the original state — via the connection's runOnClose, which {@link ConnectionType#Thread} fires when the + * last holder releases it (ref count returns to 0). + */ + public boolean isThreadConnectionActive() + { + return getConnectionHolder().getRefCount() > 0; + } + /** * Get a fresh connection directly from the pool... not part of the current transaction, not shared with the thread, etc. */ diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index 622e6031f33..ed1bae67286 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -19,6 +19,8 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.labkey.api.cache.Cache; +import org.labkey.api.cache.CacheManager; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.data.dialect.SqlDialect.ExecutionPlanType; import org.labkey.api.data.dialect.StatementWrapper; @@ -34,9 +36,11 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.labkey.api.util.ExceptionUtil.CALCULATED_COLUMN_SQL_TAG; @@ -50,6 +54,10 @@ public abstract class SqlExecutingSelector LARGE_RESULT_WARNING_THROTTLE = CacheManager.getCache(1000, CacheManager.DAY, "SqlSelector large result warnings"); + int _maxRows = Table.ALL_ROWS; protected long _offset = Table.NO_OFFSET; @Nullable Map _namedParameters = null; @@ -84,19 +92,37 @@ public interface ConnectionFactory @Override public Connection getConnection() throws SQLException { - return getEffectiveConnectionFactory().get(); + // Public callers may hold the returned Connection beyond this call, so treat it as "escaping": don't borrow the + // thread's shared connection. Internal callers that fully consume and close the ResultSet within a single + // selector call use getConnection(true); see getEffectiveConnectionFactory(). + return getConnection(false); + } + + Connection getConnection(boolean selfContained) throws SQLException + { + return getEffectiveConnectionFactory(selfContained).get(); } /** * Determines which {@link ConnectionFactory} to use for this query. When a caller has explicitly chosen a caching * behavior via {@link #setJdbcCaching(boolean)} or supplied a Connection at construction time, that choice is - * honored. Otherwise, JDBC caching is disabled by default: we ask the dialect for a streaming ConnectionFactory so - * the driver won't buffer the entire ResultSet in memory. The dialect returns null (meaning "use the shared - * Connection with the driver's default caching") when a transaction is active, the dialect is not PostgreSQL, or the - * statement is not a SELECT, so this default is safe by construction. Resolving lazily here (rather than at - * construction) ensures the transaction check reflects the state at execution time. + * honored. Otherwise, JDBC caching is disabled by default: we ask the dialect for a ConnectionFactory so the driver + * won't buffer the entire ResultSet in memory. + *

+ * The {@code selfContained} flag reflects how the ResultSet is consumed. When true (e.g. {@link #getArrayList}, + * {@link #forEach}, {@link #getRowCount}), the ResultSet is fully consumed and closed within this selector call, so + * the dialect may borrow the thread's shared, ref-counted connection — nested queries then reuse it (avoiding + * connection-pool exhaustion) and connection-local state (temp tables, search_path) stays visible — because its + * state can be restored before control returns to the caller. When false (e.g. {@code getResultSet(false)}, + * {@link #uncachedStream}), a live ResultSet/Stream is handed back to the caller, so the dialect uses a dedicated, + * unshared connection whose lifetime the caller controls. + *

+ * The dialect returns null (meaning "use the shared Connection with the driver's default caching") when a + * transaction is active, the dialect is not PostgreSQL, or the statement is not a SELECT, so this default is safe by + * construction. Resolving lazily here (rather than at construction) ensures the transaction check reflects the state + * at execution time. */ - private ConnectionFactory getEffectiveConnectionFactory() + private ConnectionFactory getEffectiveConnectionFactory(boolean selfContained) { // Honor an explicit setJdbcCaching() call (which populated _connectionFactory)... if (_jdbcCachingExplicitlySet) @@ -106,7 +132,7 @@ private ConnectionFactory getEffectiveConnectionFactory() if (null != _conn) return super::getConnection; - ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(false, getScope(), + ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(false, selfContained, getScope(), new SQLFragment("SELECT FakeColumn FROM FakeTable") /* SqlExecutingSelector always generates SELECT statements */); return null != factory ? factory : super::getConnection; @@ -141,7 +167,8 @@ public SELECTOR setJdbcCaching(boolean cache) if (null != _conn) throw new IllegalStateException("Calling setJdbcCaching() is not valid when a Connection has already been provided"); - ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(cache, getScope(), + // Explicitly disabling caching is documented to use an unshared Connection, so this path is never self-contained + ConnectionFactory factory = getScope().getSqlDialect().getConnectionFactory(cache, false, getScope(), new SQLFragment("SELECT FakeColumn FROM FakeTable") /* SqlExecutingSelector always generates SELECT statements */); _connectionFactory = null != factory ? factory : super::getConnection; _jdbcCachingExplicitlySet = true; @@ -163,13 +190,28 @@ public SELECTOR setJdbcCaching(boolean cache) if (result.size() >= LARGE_RESULT_THRESHOLD) { - LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to forEach(), forEachBatch(), or uncachedStream() to reduce memory usage. SQL: {}", - result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql(), new Throwable("Stack trace for large collection load")); + Throwable stackTrace = new Throwable("Stack trace for large collection load"); + String stackKey = getStackKey(stackTrace); + + // Warn at most once per day per unique call stack to avoid flooding the log. A benign race (two threads + // logging the same stack at once) is acceptable for a throttle. + if (null == LARGE_RESULT_WARNING_THROTTLE.get(stackKey)) + { + LARGE_RESULT_WARNING_THROTTLE.put(stackKey, Boolean.TRUE); + LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to forEach(), forEachBatch(), or uncachedStream() to reduce memory usage. SQL: {}", + result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql(), stackTrace); + } } return result; } + // Builds a stable key from a Throwable's stack trace so identical call stacks map to the same throttle entry + private static String getStackKey(Throwable t) + { + return Arrays.stream(t.getStackTrace()).map(StackTraceElement::toString).collect(Collectors.joining("\n")); + } + /** * Set a ResultSet fetch size that differs from the default value (1,000 rows on PostgreSQL). This is normally a * fine fetch size, but not when dealing with rows containing large TEXT or BYTEA columns. @@ -453,7 +495,9 @@ public T handleResultSet(ResultSetHandler handler) if (null != _sql) { DbScope scope = getScope(); - conn = getConnection(); + // _closeResultSet means this factory fully consumes and closes the ResultSet within this call, so + // the connection can be borrowed from the thread and restored rather than dedicated to the caller + conn = getConnection(_closeResultSet); try { diff --git a/api/src/org/labkey/api/data/SqlSelectorTestCase.java b/api/src/org/labkey/api/data/SqlSelectorTestCase.java index ecf0bbb9370..8bbe42e0631 100644 --- a/api/src/org/labkey/api/data/SqlSelectorTestCase.java +++ b/api/src/org/labkey/api/data/SqlSelectorTestCase.java @@ -232,6 +232,43 @@ public void testJdbcUncached() throws SQLException } } + // A "self-contained" read (getArrayList(), forEach(), getRowCount(), etc., which fully consume and close the + // ResultSet within the call) borrows the thread's shared connection rather than a dedicated one, so nested + // queries reuse it and connection-local state stays visible. On PostgreSQL the outermost borrower puts it into + // no-caching mode and restores it on release; on SQL Server it's simply the shared connection. + Connection borrowed = new SqlSelector(scope, "SELECT RowId, Body FROM comm.Announcements").getConnection(true); + try + { + // A plain thread-connection acquisition returns the very same object (it was borrowed, not dedicated) + try (Connection threadConn = scope.getConnection()) + { + assertEquals(borrowed, threadConn); + } + + // A nested self-contained read reuses the same connection rather than grabbing another one + try (Connection nested = new SqlSelector(scope, "SELECT RowId, Body FROM comm.Announcements").getConnection(true)) + { + assertEquals(borrowed, nested); + } + + if (scope.getSqlDialect().isPostgreSQL()) + { + assertEquals(TRANSACTION_READ_UNCOMMITTED, borrowed.getTransactionIsolation()); + assertFalse(borrowed.getAutoCommit()); + } + } + finally + { + borrowed.close(); + } + + // Once the outermost borrower releases it, the thread connection is restored to normal caching mode + try (Connection restored = scope.getConnection()) + { + assertTrue(restored.getAutoCommit()); + assertEquals(TRANSACTION_READ_COMMITTED, restored.getTransactionIsolation()); + } + // Inside a transaction, the default must NOT grab a separate Connection, even on PostgreSQL: the caller may be // relying on reading its own uncommitted writes, so we fall back to the shared, transactional Connection. try (DbScope.Transaction tx = scope.ensureTransaction()) diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index fa53d3cf3c9..ad9fba5b8f8 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -1039,7 +1039,7 @@ public String getExtraInfo(SQLException e) } @Override - public ConnectionFactory getConnectionFactory(boolean useJdbcCaching, DbScope scope, SQLFragment sql) + public ConnectionFactory getConnectionFactory(boolean useJdbcCaching, boolean selfContained, DbScope scope, SQLFragment sql) { // Fiddle with the Connection settings only if asked to turn off JDBC caching, we're not inside a transaction, // and it's a read-only statement (a SELECT), so we won't mess up any state the caller is relying on. @@ -1047,11 +1047,31 @@ public ConnectionFactory getConnectionFactory(boolean useJdbcCaching, DbScope sc { return null; } + else if (selfContained) + { + // The ResultSet will be fully consumed and the connection released within a single selector call, so borrow + // the thread's shared, ref-counted connection (the same one scope.getConnection() hands out) instead of + // grabbing a separate one. Nested queries then reuse it (avoiding connection-pool exhaustion), and + // connection-local state (temp tables, search_path, session settings) stays visible. We piggyback on the + // existing thread-connection reference count rather than tracking our own: only the outermost borrower (when + // isThreadConnectionActive() is false) flips the connection into no-JDBC-caching mode and registers the + // restore via runOnClose, which ConnectionType.Thread fires when the last holder releases it (ref count + // returns to 0). Nested borrows find it already configured and reuse it as-is. + return () -> { + boolean alreadyHeld = scope.isThreadConnectionActive(); + Connection conn = scope.getConnection(); + + if (!alreadyHeld && conn instanceof ConnectionWrapper cw) + cw.setRunOnClose(configureToDisableJdbcCaching(cw, scope)); + + return conn; + }; + } else { - // Factory that gets a fresh, read-only connection directly from the pool (not shared with the thread) and - // configures it to not cache ResultSet data in the JDBC driver, making it suitable for streaming very large - // ResultSets. See #39753 and #39888. + // The connection escapes the selector call (a live, streaming ResultSet/Stream is handed back to the caller) + // or the caller explicitly disabled caching, so use a fresh, read-only connection directly from the pool + // (not shared with the thread) whose lifetime the caller controls. See #39753 and #39888. return () -> { ConnectionWrapper conn = scope.getPooledConnection(DbScope.ConnectionType.Pooled, null); Closer closer = configureToDisableJdbcCaching(conn, scope); diff --git a/api/src/org/labkey/api/data/dialect/SqlDialect.java b/api/src/org/labkey/api/data/dialect/SqlDialect.java index 946edfa95b2..c2fff7572bd 100644 --- a/api/src/org/labkey/api/data/dialect/SqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/SqlDialect.java @@ -361,8 +361,11 @@ public String getSqlCastTypeName(JdbcType type) return null; } - // Return a ConnectionFactory only if the default behavior needs to be overridden - public @Nullable ConnectionFactory getConnectionFactory(boolean useJdbcCaching, DbScope scope, SQLFragment sql) + // Return a ConnectionFactory only if the default behavior needs to be overridden. selfContained indicates that the + // ResultSet will be fully consumed and closed within a single selector call (so the shared, ref-counted thread + // connection can be borrowed and its state restored on release); when false, the connection escapes to the caller + // as a live ResultSet/Stream and must therefore be an unshared connection whose lifetime the caller controls. + public @Nullable ConnectionFactory getConnectionFactory(boolean useJdbcCaching, boolean selfContained, DbScope scope, SQLFragment sql) { return null; } @@ -863,25 +866,25 @@ public SQLFragment getNumericCast(SQLFragment expression) * @param arguments Arguments passed from the LK SQL * @return the dialect equivalent SQLFragrment */ - public SQLFragment getGreatestAndLeastSQL(String method, SQLFragment... arguments) - { - throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement"); - } - - public boolean supportsIsNumeric() - { - return false; - } - - public SQLFragment isNumericExpr(SQLFragment expression) - { - throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement"); - } - - public void handleCreateDatabaseException(SQLException e) throws ServletException - { - throw(new ServletException("Can't create database", e)); - } + public SQLFragment getGreatestAndLeastSQL(String method, SQLFragment... arguments) + { + throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement"); + } + + public boolean supportsIsNumeric() + { + return false; + } + + public SQLFragment isNumericExpr(SQLFragment expression) + { + throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement"); + } + + public void handleCreateDatabaseException(SQLException e) throws ServletException + { + throw(new ServletException("Can't create database", e)); + } /** * Wrap one or more INSERT statements to allow explicit specification From 102b9bac03ab68ca437a2076e535dd7c38950416 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 16 Jul 2026 20:52:52 -0700 Subject: [PATCH 3/8] Fix test --- api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index ad9fba5b8f8..c4fe0d8629f 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -1061,7 +1061,7 @@ else if (selfContained) boolean alreadyHeld = scope.isThreadConnectionActive(); Connection conn = scope.getConnection(); - if (!alreadyHeld && conn instanceof ConnectionWrapper cw) + if (!alreadyHeld && conn instanceof ConnectionWrapper cw && cw.getAutoCommit()) cw.setRunOnClose(configureToDisableJdbcCaching(cw, scope)); return conn; From 9d2edc9abdff9ca37a924f480ae4515013f07d42 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 17 Jul 2026 16:48:41 -0700 Subject: [PATCH 4/8] Trim comments --- api/src/org/labkey/api/data/DbScope.java | 12 +++---- .../labkey/api/data/SqlExecutingSelector.java | 36 +++++++------------ .../data/dialect/BasePostgreSqlDialect.java | 21 +++++------ 3 files changed, 27 insertions(+), 42 deletions(-) diff --git a/api/src/org/labkey/api/data/DbScope.java b/api/src/org/labkey/api/data/DbScope.java index 4ec508a649c..e8bfbf7b492 100644 --- a/api/src/org/labkey/api/data/DbScope.java +++ b/api/src/org/labkey/api/data/DbScope.java @@ -1266,14 +1266,12 @@ private Connection getCurrentConnection(@Nullable Logger log) throws SQLExceptio /** * @return true if this thread already holds the shared, ref-counted thread connection — i.e., some code up the stack - * called {@link #getConnection()} and hasn't released it yet. This simply reports the existing {@link - * ConnectionHolder} reference count (the same count that already governs {@link ConnectionType#Thread} sharing); it - * is NOT a separate/new counter maintained for this purpose. + * called {@link #getConnection()} and hasn't released it. Reports the existing {@link ConnectionHolder} ref count + * that governs {@link ConnectionType#Thread} sharing. *

- * Callers that borrow the thread connection and temporarily modify its state (e.g., disabling JDBC caching for a - * streaming read) must do so only when this returns false, so that they are the outermost borrower and can safely - * restore the original state — via the connection's runOnClose, which {@link ConnectionType#Thread} fires when the - * last holder releases it (ref count returns to 0). + * A caller that borrows the thread connection and temporarily changes its state (e.g. disabling JDBC caching for a + * streaming read) must do so only when this returns false, so it is the outermost borrower and can restore the + * original state via runOnClose, which {@link ConnectionType#Thread} fires when the last holder releases it. */ public boolean isThreadConnectionActive() { diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index ed1bae67286..6351f6c52a6 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -104,23 +104,14 @@ Connection getConnection(boolean selfContained) throws SQLException } /** - * Determines which {@link ConnectionFactory} to use for this query. When a caller has explicitly chosen a caching - * behavior via {@link #setJdbcCaching(boolean)} or supplied a Connection at construction time, that choice is - * honored. Otherwise, JDBC caching is disabled by default: we ask the dialect for a ConnectionFactory so the driver - * won't buffer the entire ResultSet in memory. + * Determines which {@link ConnectionFactory} to use for this query, resolved lazily so the transaction check + * reflects execution-time state. An explicit {@link #setJdbcCaching(boolean)} call or a Connection supplied at + * construction is honored; otherwise JDBC caching is disabled by default (via the dialect) so the driver won't + * buffer the whole ResultSet in memory. The dialect returns null — use the shared Connection with default caching — + * when that default is unnecessary or unsafe: in a transaction, non-PostgreSQL, or not a SELECT. *

- * The {@code selfContained} flag reflects how the ResultSet is consumed. When true (e.g. {@link #getArrayList}, - * {@link #forEach}, {@link #getRowCount}), the ResultSet is fully consumed and closed within this selector call, so - * the dialect may borrow the thread's shared, ref-counted connection — nested queries then reuse it (avoiding - * connection-pool exhaustion) and connection-local state (temp tables, search_path) stays visible — because its - * state can be restored before control returns to the caller. When false (e.g. {@code getResultSet(false)}, - * {@link #uncachedStream}), a live ResultSet/Stream is handed back to the caller, so the dialect uses a dedicated, - * unshared connection whose lifetime the caller controls. - *

- * The dialect returns null (meaning "use the shared Connection with the driver's default caching") when a - * transaction is active, the dialect is not PostgreSQL, or the statement is not a SELECT, so this default is safe by - * construction. Resolving lazily here (rather than at construction) ensures the transaction check reflects the state - * at execution time. + * {@code selfContained} is passed through to {@link SqlDialect#getConnectionFactory}, which documents how it governs + * whether the thread's shared connection may be borrowed. */ private ConnectionFactory getEffectiveConnectionFactory(boolean selfContained) { @@ -151,10 +142,10 @@ private ConnectionFactory getEffectiveConnectionFactory(boolean selfContained) * Connection exhaustion more likely. Calling this method is not compatible with passing in an explicit Connection to * the constructor.

* - *

Note that when neither this method nor an explicit Connection is supplied, JDBC caching is disabled by default - * whenever it's safe to do so (PostgreSQL, no active transaction, SELECT statement) — see - * {@link #getEffectiveConnectionFactory()}. Callers that require the driver's default caching behavior (e.g., to - * share the thread's Connection) must therefore opt in explicitly by calling this method with cache=true.

+ *

When neither this method nor an explicit Connection is supplied, JDBC caching is disabled by default whenever + * it's safe (PostgreSQL, no active transaction, SELECT) — see {@link #getEffectiveConnectionFactory(boolean)}. Callers that + * require the driver's default caching (e.g. to share the thread's Connection) must opt in by calling this with + * cache=true.

* *

When the underlying database is not PostgreSQL, calling this method has no effect, other than validating that * the stashed Connection is null.

@@ -179,7 +170,7 @@ public SELECTOR setJdbcCaching(boolean cache) /** * Overridden to warn when a large number of rows is pulled into a Java collection. Loading many rows into memory * (here plus, potentially, in the JDBC driver's buffer) is a common source of OutOfMemoryErrors; callers should - * generally prefer a streaming method — {@link #forEach}, {@link #forEachBatch}, or {@link #uncachedStream} — that + * generally prefer a streaming method — {@link #forEach(Class, Selector.ForEachBlock)}, {@link #forEachBatch}, or {@link #uncachedStream} — that * processes rows without materializing them all at once. {@code getArray}, {@code getCollection}, * {@code getMapArray}, and {@code getMapCollection} all delegate here, so they're covered as well. */ @@ -193,8 +184,7 @@ public SELECTOR setJdbcCaching(boolean cache) Throwable stackTrace = new Throwable("Stack trace for large collection load"); String stackKey = getStackKey(stackTrace); - // Warn at most once per day per unique call stack to avoid flooding the log. A benign race (two threads - // logging the same stack at once) is acceptable for a throttle. + // Warn at most once per day (tolerating a race condition) per unique call stack to avoid flooding the log. if (null == LARGE_RESULT_WARNING_THROTTLE.get(stackKey)) { LARGE_RESULT_WARNING_THROTTLE.put(stackKey, Boolean.TRUE); diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index c4fe0d8629f..749691e16e8 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -599,7 +599,7 @@ private void initializeUserDefinedTypes(DbScope scope) String maxLength = rs.getString("character_maximum_length"); // VARCHAR with no specific size has null maxLength... but character_octet_length seems okay - scale = Integer.valueOf(null != maxLength ? maxLength : rs.getString("character_octet_length")); + scale = Integer.parseInt(null != maxLength ? maxLength : rs.getString("character_octet_length")); } else { @@ -903,10 +903,10 @@ public int readOutputParameters(DbScope scope, CallableStatement stmt, Map { boolean alreadyHeld = scope.isThreadConnectionActive(); Connection conn = scope.getConnection(); From 280456bfe3cc1dd461afdeae6d432107cb1f0c07 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 20 Jul 2026 15:37:27 -0700 Subject: [PATCH 5/8] Code review suggestions --- .../labkey/api/data/SqlExecutingSelector.java | 3 +- .../labkey/api/data/SqlSelectorTestCase.java | 51 +++++++++++++++++++ .../data/dialect/BasePostgreSqlDialect.java | 20 +++++++- 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index 6351f6c52a6..000e6b60e09 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -188,8 +188,9 @@ public SELECTOR setJdbcCaching(boolean cache) if (null == LARGE_RESULT_WARNING_THROTTLE.get(stackKey)) { LARGE_RESULT_WARNING_THROTTLE.put(stackKey, Boolean.TRUE); + // Log the parameterized SQL only (getSQL(), not the SQLFragment) so bound parameter values stay out of the log LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to forEach(), forEachBatch(), or uncachedStream() to reduce memory usage. SQL: {}", - result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql(), stackTrace); + result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql().getSQL(), stackTrace); } } diff --git a/api/src/org/labkey/api/data/SqlSelectorTestCase.java b/api/src/org/labkey/api/data/SqlSelectorTestCase.java index 8bbe42e0631..6c38fefc2d0 100644 --- a/api/src/org/labkey/api/data/SqlSelectorTestCase.java +++ b/api/src/org/labkey/api/data/SqlSelectorTestCase.java @@ -22,7 +22,10 @@ import java.sql.Connection; import java.sql.SQLException; import java.util.Collection; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.Map; +import java.util.Set; import java.util.stream.Stream; import static java.sql.Connection.TRANSACTION_READ_COMMITTED; @@ -281,6 +284,54 @@ public void testJdbcUncached() throws SQLException } } + // Verify that nested DB access from a row callback reuses that same borrowed connection (rather than grabbing a + // second one), returns correct results while the outer ResultSet is still open, doesn't truncate the outer + // iteration, and leaves the thread connection fully restored once forEach() completes. + @Test + public void testNestedQueryDuringForEach() throws SQLException + { + DbScope scope = CoreSchema.getInstance().getScope(); + + // The borrow-and-disable-caching path only engages outside a transaction + assertFalse("Test assumes no active transaction on this thread", scope.isTransactionActive()); + + // core.Containers always has at least the root container + long expectedRows = new SqlSelector(scope, new SQLFragment("SELECT RowId FROM core.Containers")).getRowCount(); + assertTrue("core.Containers should never be empty", expectedRows > 0); + + MutableInt visited = new MutableInt(0); + Set callbackConnections = Collections.newSetFromMap(new IdentityHashMap<>()); + + new SqlSelector(scope, new SQLFragment("SELECT RowId FROM core.Containers ORDER BY RowId")).forEach(Integer.class, rowId -> { + visited.increment(); + + // The callback runs while the outer ResultSet is open. Acquiring the thread connection must return the same + // borrowed connection, in no-caching mode — proving nested code shares the transction/conncetion. + try (Connection nested = scope.getConnection()) + { + callbackConnections.add(nested); + + assertFalse("Nested access during forEach() should run on the uncached borrowed connection", nested.getAutoCommit()); + assertEquals(TRANSACTION_READ_UNCOMMITTED, nested.getTransactionIsolation()); + } + + // A nested self-contained query must return correct results even though the outer server-side cursor is open + // on the same connection — this is the interleaving that would fail if the nested statement clobbered it. + Integer nestedRowId = new SqlSelector(scope, new SQLFragment("SELECT RowId FROM core.Containers WHERE RowId = ?", rowId)).getObject(Integer.class); + assertEquals("Nested query should return exactly the matching row", rowId, nestedRowId); + }); + + assertEquals("forEach() should visit every row even with nested queries in the callback", expectedRows, visited.longValue()); + assertEquals("Nested access should reuse the single borrowed connection across all rows", 1, callbackConnections.size()); + + // Once the outermost borrower (forEach) releases the connection, it must be restored to normal caching mode + try (Connection restored = scope.getConnection()) + { + assertTrue(restored.getAutoCommit()); + assertEquals(TRANSACTION_READ_COMMITTED, restored.getTransactionIsolation()); + } + } + // Passing in a Connection and calling setJdbcCaching() should throw @Test(expected = IllegalStateException.class) public void testJdbcUncachedTrue() throws SQLException diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index 749691e16e8..06a5d29abbb 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -1058,8 +1058,24 @@ else if (selfContained) boolean alreadyHeld = scope.isThreadConnectionActive(); Connection conn = scope.getConnection(); - if (!alreadyHeld && conn instanceof ConnectionWrapper cw && cw.getAutoCommit()) - cw.setRunOnClose(configureToDisableJdbcCaching(cw, scope)); + try + { + if (!alreadyHeld && conn instanceof ConnectionWrapper cw && cw.getAutoCommit()) + cw.setRunOnClose(configureToDisableJdbcCaching(cw, scope)); + } + catch (SQLException | RuntimeException e) + { + // scope.getConnection() already bumped the ref count, so release it before propagating + try + { + conn.close(); + } + catch (SQLException suppressed) + { + e.addSuppressed(suppressed); + } + throw e; + } return conn; }; From 1c7b2ec163fcb173c10c762240ec4f0bed443a20 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 20 Jul 2026 16:13:38 -0700 Subject: [PATCH 6/8] Avoid complaints loading child container list --- .../org/labkey/api/data/ContainerManager.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/api/src/org/labkey/api/data/ContainerManager.java b/api/src/org/labkey/api/data/ContainerManager.java index 7d3c22a9729..28aff41db13 100644 --- a/api/src/org/labkey/api/data/ContainerManager.java +++ b/api/src/org/labkey/api/data/ContainerManager.java @@ -1090,17 +1090,15 @@ private static Map getChildrenMap(Container parent) { try (DbScope.Transaction t = ensureTransaction()) { - List children = new SqlSelector(CORE.getSchema(), + List ids = new ArrayList<>(); + new SqlSelector(CORE.getSchema(), "SELECT * FROM " + CORE.getTableInfoContainers() + " WHERE Parent = ? ORDER BY SortOrder, LOWER(Name)", - parent.getId()).getArrayList(Container.class); - - childIds = new ArrayList<>(children.size()); - for (Container c : children) - { - childIds.add(c.getEntityId()); + parent.getId()).forEach(Container.class, c -> { + ids.add(c.getEntityId()); _addToCache(c); - } - childIds = Collections.unmodifiableList(childIds); + }); + + childIds = Collections.unmodifiableList(ids); CACHE_CHILDREN.put(parent.getEntityId(), childIds); // No database changes to commit, but need to decrement the transaction counter t.commit(); From fc9f142a91739b5f1ad5c463491ce7129f6dd913 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 20 Jul 2026 17:44:48 -0700 Subject: [PATCH 7/8] Need to use ResultSet, not Map --- api/src/org/labkey/api/data/ContainerManager.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/data/ContainerManager.java b/api/src/org/labkey/api/data/ContainerManager.java index 28aff41db13..349f90b49ce 100644 --- a/api/src/org/labkey/api/data/ContainerManager.java +++ b/api/src/org/labkey/api/data/ContainerManager.java @@ -1091,9 +1091,12 @@ private static Map getChildrenMap(Container parent) try (DbScope.Transaction t = ensureTransaction()) { List ids = new ArrayList<>(); + ContainerFactory factory = new ContainerFactory(); + // ContainerFactory only builds from a ResultSet (fromMap is unsupported), so iterate rows directly new SqlSelector(CORE.getSchema(), "SELECT * FROM " + CORE.getTableInfoContainers() + " WHERE Parent = ? ORDER BY SortOrder, LOWER(Name)", - parent.getId()).forEach(Container.class, c -> { + parent.getId()).forEach(rs -> { + Container c = factory.handle(rs); ids.add(c.getEntityId()); _addToCache(c); }); From 4e41289f8472e54a68491bee1b878b0d61993ded Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 21 Jul 2026 17:58:59 -0700 Subject: [PATCH 8/8] Adam likes Throttles --- .../labkey/api/data/SqlExecutingSelector.java | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index 000e6b60e09..f8ae44e71b8 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -19,8 +19,8 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; +import org.labkey.api.cache.Throttle; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.data.dialect.SqlDialect.ExecutionPlanType; import org.labkey.api.data.dialect.StatementWrapper; @@ -56,7 +56,9 @@ public abstract class SqlExecutingSelector LARGE_RESULT_WARNING_THROTTLE = CacheManager.getCache(1000, CacheManager.DAY, "SqlSelector large result warnings"); + private static final Throttle LARGE_RESULT_WARNING_THROTTLE = new Throttle<>("SqlSelector large result warnings", 1000, CacheManager.DAY, + w -> LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to streaming variants to reduce memory usage. SQL: {}", + w.rowCount, w.selectorClass, w.sql, w.stackTrace)); int _maxRows = Table.ALL_ROWS; protected long _offset = Table.NO_OFFSET; @@ -172,7 +174,7 @@ public SELECTOR setJdbcCaching(boolean cache) * (here plus, potentially, in the JDBC driver's buffer) is a common source of OutOfMemoryErrors; callers should * generally prefer a streaming method — {@link #forEach(Class, Selector.ForEachBlock)}, {@link #forEachBatch}, or {@link #uncachedStream} — that * processes rows without materializing them all at once. {@code getArray}, {@code getCollection}, - * {@code getMapArray}, and {@code getMapCollection} all delegate here, so they're covered as well. + * {@code getMapArray}, {@code stream}, and {@code getMapCollection} all delegate here, so they're covered as well. */ @Override public @NotNull ArrayList getArrayList(Class clazz) @@ -182,16 +184,10 @@ public SELECTOR setJdbcCaching(boolean cache) if (result.size() >= LARGE_RESULT_THRESHOLD) { Throwable stackTrace = new Throwable("Stack trace for large collection load"); - String stackKey = getStackKey(stackTrace); - - // Warn at most once per day (tolerating a race condition) per unique call stack to avoid flooding the log. - if (null == LARGE_RESULT_WARNING_THROTTLE.get(stackKey)) - { - LARGE_RESULT_WARNING_THROTTLE.put(stackKey, Boolean.TRUE); - // Log the parameterized SQL only (getSQL(), not the SQLFragment) so bound parameter values stay out of the log - LOGGER.warn("{} rows loaded into a collection via {}. Consider switching to forEach(), forEachBatch(), or uncachedStream() to reduce memory usage. SQL: {}", - result.size(), getClass().getSimpleName(), getSqlFactory(false).getSql().getSQL(), stackTrace); - } + // Log the parameterized SQL only so bound parameter values stay out of the log + SQLFragment sql = getSqlFactory(false).getSql(); + LARGE_RESULT_WARNING_THROTTLE.execute(new LargeResultWarning(getStackKey(stackTrace), result.size(), + getClass().getSimpleName(), sql == null ? null : sql.getSQL(), stackTrace)); } return result; @@ -203,6 +199,24 @@ private static String getStackKey(Throwable t) return Arrays.stream(t.getStackTrace()).map(StackTraceElement::toString).collect(Collectors.joining("\n")); } + // Carries the fields needed to build the large-result warning, but hashes/compares only on the call-stack signature + // so the throttle dedupes per unique call stack rather than per (stack + row count + SQL) combination. + private record LargeResultWarning(String stackKey, int rowCount, String selectorClass, String sql, + Throwable stackTrace) + { + @Override + public boolean equals(Object o) + { + return o instanceof LargeResultWarning w && stackKey.equals(w.stackKey); + } + + @Override + public int hashCode() + { + return stackKey.hashCode(); + } + } + /** * Set a ResultSet fetch size that differs from the default value (1,000 rows on PostgreSQL). This is normally a * fine fetch size, but not when dealing with rows containing large TEXT or BYTEA columns.