From 382bd024245797c84496ee1ae9a032b4faee8e17 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 13 Aug 2026 13:03:47 +0300 Subject: [PATCH 1/8] Stamp JDBC tables with their tree name and refresh statistics after import Table names are opaque SHA-224 hashes of the tree name, so on the database side there was no way to tell which tree a table holds - identifying dn2id in discussion #859 required recomputing hashes by hand. openTree() now stores the tree name as the table comment (COMMENT ON TABLE for postgres/oracle/h2, ALTER TABLE ... COMMENT for mysql, the MS_Description extended property for mssql), so "\dt+" and the information schema show it directly; existing deployments get stamped on the next backend open. The same investigation found a bulk-imported dn2id table that was never analyzed, leaving the planner a 13x-off row estimate for the "where k>? order by k" cursor batches. Importer.close() now refreshes optimizer statistics per dialect (ANALYZE / ANALYZE TABLE / dbms_stats.gather_table_stats / UPDATE STATISTICS), covering both import-ldif and rebuild-index. Both operations are best-effort - a failure is logged and never fails the backend or the import - and the statistics path reports success so tests catch rejected SQL on every supported database. --- .../server/backends/jdbc/JDBCStorage.java | 79 ++++++++++++ .../opends/server/backends/jdbc/TestCase.java | 113 ++++++++++++++++++ 2 files changed, 192 insertions(+) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 09878862b3..b88a263d1e 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -88,6 +88,14 @@ int execute(PreparedStatement statement) throws SQLException { return statement.executeUpdate(); } + // unlike execute(), tolerates statements that return a result set ("analyze table" on mysql) + void executeAny(PreparedStatement statement) throws SQLException { + if (logger.isTraceEnabled()) { + logger.trace(LocalizableMessage.raw("jdbc: %s",statement)); + } + statement.execute(); + } + Connection getConnection() throws Exception { return CachedConnection.getConnection(config.getDBDirectory()); } @@ -151,6 +159,75 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ return name; } + // Table names are opaque SHA-224 hashes, so on the database side there is no way to tell + // which tree a table holds. Stamp each table with its tree name (visible in "\dt+" and the + // information schema) so database-level troubleshooting does not require recomputing hashes. + // The comment is a diagnostic aid: failing to store it must not fail the backend. + void commentTable(Connection con, TreeName treeName) { + final String tableName=getTableName(treeName); + final String comment=treeName.toString().replace("'","''"); + final String driverName=((CachedConnection) con).parent.getClass().getName(); + final String sql; + if (driverName.contains("mysql")) { + sql="alter table "+tableName+" comment '"+comment+"'"; + }else if (driverName.contains("microsoft")) { // no COMMENT ON in t-sql: MS_Description extended property + sql="declare @s sysname = schema_name()" + +" if exists (select 1 from sys.extended_properties where major_id=object_id('"+tableName+"') and minor_id=0 and name='MS_Description')" + +" exec sys.sp_updateextendedproperty N'MS_Description', N'"+comment+"', N'SCHEMA', @s, N'TABLE', N'"+tableName+"'" + +" else" + +" exec sys.sp_addextendedproperty N'MS_Description', N'"+comment+"', N'SCHEMA', @s, N'TABLE', N'"+tableName+"'"; + }else { // postgres, oracle and h2 all accept COMMENT ON TABLE + sql="comment on table "+tableName+" is '"+comment+"'"; + } + try (final PreparedStatement statement=con.prepareStatement(sql)) { + executeAny(statement); + con.commit(); + }catch (SQLException e) { + try { + con.rollback(); + } catch (SQLException e2) {} + logger.trace(LocalizableMessage.raw("jdbc: unable to comment table %s with tree name %s: %s", + tableName, treeName, stackTraceToSingleLineString(e))); + } + } + + // A bulk load leaves the optimizer statistics of freshly created tables stale (a table that + // was never analyzed can make the planner badly misestimate the "where k>? order by k" cursor + // batches - see OpenIdentityPlatform/OpenDJ#859), so refresh them once the data is in place. + // Statistics upkeep is best-effort: a failure must not fail the import that produced the data, + // so failures are only logged - the return value makes them observable to tests. + boolean updateTableStatistics(Connection con) { + final String driverName=((CachedConnection) con).parent.getClass().getName(); + boolean allRefreshed=true; + for (final TreeName treeName : listTrees()) { + final String tableName=getTableName(treeName); + final String sql; + if (driverName.contains("postgres")) { + sql="analyze "+tableName; + }else if (driverName.contains("mysql")) { + sql="analyze table "+tableName; + }else if (driverName.contains("oracle")) { + sql="begin dbms_stats.gather_table_stats(user, '"+tableName.toUpperCase()+"'); end;"; + }else if (driverName.contains("microsoft")) { + sql="update statistics "+tableName; + }else { // no portable statistics refresh for other engines + return allRefreshed; + } + try (final PreparedStatement statement=con.prepareStatement(sql)) { + executeAny(statement); + con.commit(); + }catch (SQLException e) { + try { + con.rollback(); + } catch (SQLException e2) {} + allRefreshed=false; + logger.warn(LocalizableMessage.raw("jdbc: unable to refresh statistics of table %s (tree %s): %s", + tableName, treeName, stackTraceToSingleLineString(e))); + } + } + return allRefreshed; + } + @Override public void removeStorageFiles() throws StorageRuntimeException { final boolean isOpen=getStorageStatus().isWorking(); @@ -363,6 +440,7 @@ public void openTree(TreeName treeName, boolean createOnDemand) { } } // mssql: k is varbinary(max), which cannot be an index key column - cursor batches stay unindexed there + commentTable(con, treeName); } } @@ -723,6 +801,7 @@ public ImporterImpl() { public void close() { try { con.commit(); + updateTableStatistics(con); con.close(); } catch (SQLException e) { throw new StorageRuntimeException(e); diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index f7ad4ec14d..22e10ecd11 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -21,6 +21,7 @@ import org.opends.server.backends.pluggable.PluggableBackendImplTestCase; import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.Importer; import org.opends.server.backends.pluggable.spi.ReadOperation; import org.opends.server.backends.pluggable.spi.ReadableTransaction; import org.opends.server.backends.pluggable.spi.TreeName; @@ -320,6 +321,118 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * Each table must be stamped with the tree name it stores: table names are opaque SHA-224 + * hashes, so without the comment there is no way to tell the trees apart on the database + * side (#859). The single quote in the base DN exercises the comment escaping. + */ + @Test + public void testTreeNameStoredAsTableComment() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("o=comment'test", "dn2id"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + String readTableComment(String tableName) throws Exception { + final String url = getJdbcUrl(); + final String sql; + if (url.startsWith("jdbc:postgresql")) { + sql = "select obj_description('" + tableName + "'::regclass, 'pg_class')"; + } else if (url.startsWith("jdbc:mysql")) { + sql = "select table_comment from information_schema.tables where table_schema=database() and table_name='" + tableName + "'"; + } else if (url.startsWith("jdbc:oracle")) { + sql = "select comments from user_tab_comments where table_name='" + tableName.toUpperCase() + "'"; + } else if (url.startsWith("jdbc:sqlserver")) { + sql = "select cast(value as nvarchar(4000)) from sys.extended_properties where major_id=object_id('" + tableName + "') and minor_id=0 and name='MS_Description'"; + } else { + throw new SkipException("no table comment query for " + url); + } + try (final Connection con = DriverManager.getConnection(url); + final Statement st = con.createStatement(); + final ResultSet rs = st.executeQuery(sql)) { + return rs.next() ? rs.getString(1) : null; + } + } + + /** A bulk import must refresh optimizer statistics: fresh tables were never analyzed (#859). */ + @Test + public void testImportRefreshesTableStatistics() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("testImportAnalyze", "tree"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + try (final Importer importer = storage.startImport()) { + for (int i = 0; i < 40; i++) { + importer.put(tree, key(i), value(i)); + } + } + assertTableStatisticsFresh(storage.getTableName(tree)); + // import swallows statistics failures by design: assert directly that the + // dialect-specific refresh statement is accepted by this database + try (final Connection con = CachedConnection.getConnection(getJdbcUrl())) { + assertTrue(storage.updateTableStatistics(con), "statistics refresh reported failures"); + } + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + void assertTableStatisticsFresh(String tableName) throws Exception { + final String url = getJdbcUrl(); + final String sql; + if (url.startsWith("jdbc:postgresql")) { + // reltuples stays -1/0 until the first ANALYZE + sql = "select reltuples::bigint from pg_class where relname='" + tableName + "'"; + } else if (url.startsWith("jdbc:oracle")) { + // num_rows stays null until dbms_stats gathers statistics + sql = "select num_rows from user_tables where table_name='" + tableName.toUpperCase() + "'"; + } else { + // mysql/mssql maintain their estimates on their own: nothing distinguishable to assert + return; + } + try (final Connection con = DriverManager.getConnection(url); + final Statement st = con.createStatement(); + final ResultSet rs = st.executeQuery(sql)) { + assertTrue(rs.next(), "table " + tableName + " not found"); + final long rows = rs.getLong(1); + assertFalse(rs.wasNull(), "statistics were never gathered for " + tableName); + assertTrue(rows > 0, "statistics of " + tableName + " look stale: " + rows); + } + } + /** Cursor operations must keep working when the tree spans several "fetchsize" batches. */ @Test public void testCursorCrossesFetchSizeBatches() throws Exception { From 46ee6e26ec4b60f419edbd06c381f9fce42f3e57 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 13 Aug 2026 16:13:49 +0300 Subject: [PATCH 2/8] Bind SQL values where dialects allow, stamp comments only when stale Review follow-up for the table-comment/statistics change: - The MySQL comment literal escaped only single quotes, but backslash is an escape character in MySQL literals, so a tree name containing one (DN escapes, or a crafted VLV index name) could corrupt the comment or break out of the literal. MySQL now escapes backslashes too; MS SQL passes the value and table name as bind parameters to the extended-property procedures and object_id(); Oracle binds the table name in dbms_stats.gather_table_stats; the remaining COMMENT ON / ALTER TABLE splices (DDL takes no binds) are verified by a paired-quotes guard. Clears the CodeQL java/concatenated-sql-query alerts 1267 and 1268. - Comment statements are DDL - a metadata lock on MySQL, a DDL lock on Oracle - and ran on every backend open. The stored comment is now read back from the catalog first and the DDL is issued only when it is absent or stale, so repeated opens cost one SELECT per tree. - deleteTree() invalidates the tree2table mapping so a later statistics refresh does not analyze dropped tables and spam warnings about them. - MySQL ANALYZE TABLE reports problems as a result row rather than an SQLException: Msg_type=error now surfaces as a failure. The sys.extended_properties probes constrain class=1 so a non-table property cannot misroute the add/update choice. Comment failures log at debug instead of trace. The statistics dialect switch is hoisted out of the per-tree loop. - Tests: the comment tree name carries a backslash next to the quote (fails on the old MySQL escaping), and statistics freshness is now asserted on all four databases - mysql.innodb_table_stats.n_rows and sys.dm_db_stats_properties(...).last_updated join the existing pg_class.reltuples and user_tables.num_rows checks. --- .../server/backends/jdbc/JDBCStorage.java | 146 ++++++++++++++---- .../opends/server/backends/jdbc/TestCase.java | 13 +- 2 files changed, 128 insertions(+), 31 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index b88a263d1e..07301575ed 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -159,62 +159,150 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ return name; } + private static final String[] NO_ARGS=new String[0]; + + static String driverNameOf(Connection con) { + return ((CachedConnection) con).parent.getClass().getName(); + } + + // The comment DDL takes no bind parameters, so the value is spliced into a single-quoted SQL + // literal: verify every quote in the escaped value is paired and thus cannot terminate the literal. + private static void requireQuotesPaired(String escaped) { + for (int i=0;i=escaped.length() || escaped.charAt(i+1)!='\'') { + throw new IllegalArgumentException("unpaired quote in SQL literal: "+escaped); + } + i++; + } + } + } + // Table names are opaque SHA-224 hashes, so on the database side there is no way to tell // which tree a table holds. Stamp each table with its tree name (visible in "\dt+" and the // information schema) so database-level troubleshooting does not require recomputing hashes. // The comment is a diagnostic aid: failing to store it must not fail the backend. void commentTable(Connection con, TreeName treeName) { final String tableName=getTableName(treeName); - final String comment=treeName.toString().replace("'","''"); - final String driverName=((CachedConnection) con).parent.getClass().getName(); - final String sql; - if (driverName.contains("mysql")) { - sql="alter table "+tableName+" comment '"+comment+"'"; - }else if (driverName.contains("microsoft")) { // no COMMENT ON in t-sql: MS_Description extended property - sql="declare @s sysname = schema_name()" - +" if exists (select 1 from sys.extended_properties where major_id=object_id('"+tableName+"') and minor_id=0 and name='MS_Description')" - +" exec sys.sp_updateextendedproperty N'MS_Description', N'"+comment+"', N'SCHEMA', @s, N'TABLE', N'"+tableName+"'" - +" else" - +" exec sys.sp_addextendedproperty N'MS_Description', N'"+comment+"', N'SCHEMA', @s, N'TABLE', N'"+tableName+"'"; - }else { // postgres, oracle and h2 all accept COMMENT ON TABLE - sql="comment on table "+tableName+" is '"+comment+"'"; - } - try (final PreparedStatement statement=con.prepareStatement(sql)) { - executeAny(statement); - con.commit(); - }catch (SQLException e) { + try { + final String treeComment=treeName.toString(); + // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() + // runs on every backend open: only stamp when the stored comment is absent or stale + if (treeComment.equals(readStoredComment(con, tableName))) { + return; + } + final String sql; + final String[] args; + if (driverNameOf(con).contains("mysql")) { // ALTER TABLE takes no binds; backslash is an escape character in mysql literals + final String comment=treeComment.replace("\\","\\\\").replace("'","''"); + requireQuotesPaired(comment); + sql="alter table "+tableName+" comment '"+comment+"'"; + args=NO_ARGS; + }else if (driverNameOf(con).contains("microsoft")) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) + sql="declare @s sysname = schema_name()" + +" if exists (select 1 from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description')" + +" exec sys.sp_updateextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?" + +" else" + +" exec sys.sp_addextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?"; + args=new String[]{tableName, treeComment, tableName, treeComment, tableName}; + }else { // postgres and oracle accept COMMENT ON TABLE (no binds in ddl); untested default for other engines + final String comment=treeComment.replace("'","''"); + requireQuotesPaired(comment); + sql="comment on table "+tableName+" is '"+comment+"'"; + args=NO_ARGS; + } + try (final PreparedStatement statement=con.prepareStatement(sql)) { + for (int i=0;i? order by k" cursor // batches - see OpenIdentityPlatform/OpenDJ#859), so refresh them once the data is in place. // Statistics upkeep is best-effort: a failure must not fail the import that produced the data, // so failures are only logged - the return value makes them observable to tests. boolean updateTableStatistics(Connection con) { - final String driverName=((CachedConnection) con).parent.getClass().getName(); + final String driverName=driverNameOf(con); + final boolean postgres=driverName.contains("postgres"); + final boolean mysql=driverName.contains("mysql"); + final boolean oracle=driverName.contains("oracle"); + final boolean microsoft=driverName.contains("microsoft"); + if (!postgres && !mysql && !oracle && !microsoft) { // no portable statistics refresh for other engines + return true; + } boolean allRefreshed=true; for (final TreeName treeName : listTrees()) { final String tableName=getTableName(treeName); final String sql; - if (driverName.contains("postgres")) { + final String[] args; + if (postgres) { sql="analyze "+tableName; - }else if (driverName.contains("mysql")) { + args=NO_ARGS; + }else if (mysql) { sql="analyze table "+tableName; - }else if (driverName.contains("oracle")) { - sql="begin dbms_stats.gather_table_stats(user, '"+tableName.toUpperCase()+"'); end;"; - }else if (driverName.contains("microsoft")) { + args=NO_ARGS; + }else if (oracle) { + sql="begin dbms_stats.gather_table_stats(user, ?); end;"; + args=new String[]{tableName.toUpperCase()}; + }else { sql="update statistics "+tableName; - }else { // no portable statistics refresh for other engines - return allRefreshed; + args=NO_ARGS; } try (final PreparedStatement statement=con.prepareStatement(sql)) { - executeAny(statement); + for (int i=0;i Date: Fri, 14 Aug 2026 09:20:25 +0300 Subject: [PATCH 3/8] Stamp table comments on a dedicated connection, analyze only written trees The comment stamp ran on the transaction that opened the tree: comment DDL implicitly commits on mysql/oracle, and a failing sp_addextendedproperty rolls the whole transaction back on sql server, committing or discarding work pending on the caller's connection (such as the trusted flag DefaultIndex.afterOpen() writes between openTree() calls). commentTable() now runs on a dedicated pooled connection and skips dialects it does not recognize before doing anything. The importer tracks the trees it wrote and close() refreshes statistics for those trees only, so rebuild-index no longer re-analyzes the whole backend. removeStorageFiles() invalidates the tree-to-table cache, and the importer returns its pooled connection in a finally. sqlLiteral() escapes and verifies in one place - quotes, plus backslashes where they are escape characters - and postgres comments use the E'' form so escaping does not depend on standard_conforming_strings. --- .../server/backends/jdbc/JDBCStorage.java | 114 +++++++++------ .../opends/server/backends/jdbc/TestCase.java | 133 +++++++++++++++++- 2 files changed, 205 insertions(+), 42 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 07301575ed..e6aff1e46a 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -39,6 +39,7 @@ import java.security.NoSuchAlgorithmException; import java.sql.*; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import static org.opends.server.backends.pluggable.spi.StorageUtils.addErrorMessage; import static org.opends.server.util.StaticUtils.stackTraceToSingleLineString; @@ -165,50 +166,68 @@ static String driverNameOf(Connection con) { return ((CachedConnection) con).parent.getClass().getName(); } - // The comment DDL takes no bind parameters, so the value is spliced into a single-quoted SQL - // literal: verify every quote in the escaped value is paired and thus cannot terminate the literal. - private static void requireQuotesPaired(String escaped) { + // Splices a value into a single-quoted SQL literal for the comment DDL, which takes no bind + // parameters: doubles every quote, and every backslash on dialects where backslash is an + // escape character inside literals. The scan of the escaped result is defence in depth: it + // re-verifies that no quote (or live backslash) is left unpaired and able to terminate the + // literal, so a regression in the escaping throws instead of reaching the database. + private static String sqlLiteral(String value, boolean backslashIsEscape) { + final String escaped=(backslashIsEscape?value.replace("\\","\\\\"):value).replace("'","''"); for (int i=0;i=escaped.length() || escaped.charAt(i+1)!='\'') { - throw new IllegalArgumentException("unpaired quote in SQL literal: "+escaped); + final char c=escaped.charAt(i); + if (c=='\'' || (backslashIsEscape && c=='\\')) { + if (i+1>=escaped.length() || escaped.charAt(i+1)!=c) { + throw new IllegalArgumentException("unpaired "+c+" in SQL literal: "+escaped); } i++; } } + return "'"+escaped+"'"; } // Table names are opaque SHA-224 hashes, so on the database side there is no way to tell // which tree a table holds. Stamp each table with its tree name (visible in "\dt+" and the // information schema) so database-level troubleshooting does not require recomputing hashes. - // The comment is a diagnostic aid: failing to store it must not fail the backend. - void commentTable(Connection con, TreeName treeName) { + // Runs on a dedicated pooled connection, never on the transaction that opened the tree: + // comment statements are DDL (an implicit commit on mysql and oracle), and a failing + // sp_addextendedproperty rolls the whole transaction back on sql server - either would + // corrupt work pending on the caller's connection (e.g. the trusted flag written by + // DefaultIndex.afterOpen()). Returns true when the comment was stamped; the comment is a + // diagnostic aid, so a failed attempt only returns false and must not fail the backend. + boolean commentTable(TreeName treeName) { final String tableName=getTableName(treeName); - try { + try (final Connection con=getConnection()) { + final String driverName=driverNameOf(con); + final boolean postgres=driverName.contains("postgres"); + final boolean mysql=driverName.contains("mysql"); + final boolean oracle=driverName.contains("oracle"); + final boolean microsoft=driverName.contains("microsoft"); + if (!postgres && !mysql && !oracle && !microsoft) { // no comment syntax and readback known for other engines: leave the table unstamped + return false; + } final String treeComment=treeName.toString(); // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() // runs on every backend open: only stamp when the stored comment is absent or stale if (treeComment.equals(readStoredComment(con, tableName))) { - return; + return false; } final String sql; final String[] args; - if (driverNameOf(con).contains("mysql")) { // ALTER TABLE takes no binds; backslash is an escape character in mysql literals - final String comment=treeComment.replace("\\","\\\\").replace("'","''"); - requireQuotesPaired(comment); - sql="alter table "+tableName+" comment '"+comment+"'"; + if (mysql) { // ALTER TABLE takes no binds; backslash is an escape character in mysql literals + sql="alter table "+tableName+" comment "+sqlLiteral(treeComment,true); args=NO_ARGS; - }else if (driverNameOf(con).contains("microsoft")) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) + }else if (microsoft) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) sql="declare @s sysname = schema_name()" +" if exists (select 1 from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description')" +" exec sys.sp_updateextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?" +" else" +" exec sys.sp_addextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?"; args=new String[]{tableName, treeComment, tableName, treeComment, tableName}; - }else { // postgres and oracle accept COMMENT ON TABLE (no binds in ddl); untested default for other engines - final String comment=treeComment.replace("'","''"); - requireQuotesPaired(comment); - sql="comment on table "+tableName+" is '"+comment+"'"; + }else if (postgres) { // no binds in ddl; the E'' form keeps backslash an escape character regardless of standard_conforming_strings + sql="comment on table "+tableName+" is E"+sqlLiteral(treeComment,true); + args=NO_ARGS; + }else { // oracle: no binds in ddl; backslash is never an escape character in oracle literals + sql="comment on table "+tableName+" is "+sqlLiteral(treeComment,false); args=NO_ARGS; } try (final PreparedStatement statement=con.prepareStatement(sql)) { @@ -218,18 +237,17 @@ void commentTable(Connection con, TreeName treeName) { executeAny(statement); con.commit(); } - }catch (SQLException|RuntimeException e) { - try { - con.rollback(); - } catch (SQLException e2) {} + return true; + }catch (Exception e) { // CachedConnection.close() has rolled back whatever the failed attempt left pending logger.debug(LocalizableMessage.raw("jdbc: unable to comment table %s with tree name %s: %s", tableName, treeName, stackTraceToSingleLineString(e))); + return false; } } - // Returns the comment currently stored on the table, or null when there is none - // (or the dialect has no known readback: the caller then stamps unconditionally). - private String readStoredComment(Connection con, String tableName) throws SQLException { + // Returns the comment currently stored on the table, or null when there is none. + // Only called for the four dialects commentTable() recognizes. + String readStoredComment(Connection con, String tableName) throws SQLException { final String driverName=driverNameOf(con); final String sql; final String arg; @@ -246,7 +264,7 @@ private String readStoredComment(Connection con, String tableName) throws SQLExc sql="select cast(value as nvarchar(4000)) from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description'"; arg=tableName; }else { - return null; + throw new SQLException("no table comment readback for "+driverName); } try (final PreparedStatement statement=con.prepareStatement(sql)) { statement.setString(1,arg); @@ -259,9 +277,11 @@ private String readStoredComment(Connection con, String tableName) throws SQLExc // A bulk load leaves the optimizer statistics of freshly created tables stale (a table that // was never analyzed can make the planner badly misestimate the "where k>? order by k" cursor // batches - see OpenIdentityPlatform/OpenDJ#859), so refresh them once the data is in place. - // Statistics upkeep is best-effort: a failure must not fail the import that produced the data, - // so failures are only logged - the return value makes them observable to tests. - boolean updateTableStatistics(Connection con) { + // Only the trees the import actually wrote are refreshed: rebuild-index imports a few index + // trees, and gathering statistics of the whole backend on its behalf is a full scan per + // table on oracle. Statistics upkeep is best-effort: a failure must not fail the import that + // produced the data, so failures are only logged - the return value makes them observable to tests. + boolean updateTableStatistics(Connection con, Collection trees) { final String driverName=driverNameOf(con); final boolean postgres=driverName.contains("postgres"); final boolean mysql=driverName.contains("mysql"); @@ -271,7 +291,7 @@ boolean updateTableStatistics(Connection con) { return true; } boolean allRefreshed=true; - for (final TreeName treeName : listTrees()) { + for (final TreeName treeName : trees) { final String tableName=getTableName(treeName); final String sql; final String[] args; @@ -345,6 +365,10 @@ public void removeStorageFiles() throws StorageRuntimeException { } catch (Exception e) { throw new StorageRuntimeException(e); } + // all tables are gone: forget the mappings so listTrees() consumers skip the dropped trees + for (final TreeName treeName : trees) { + tree2table.invalidate(treeName); + } } if (!isOpen) { close(); @@ -528,7 +552,7 @@ public void openTree(TreeName treeName, boolean createOnDemand) { } } // mssql: k is varbinary(max), which cannot be an index key column - cursor batches stay unindexed there - commentTable(con, treeName); + commentTable(treeName); } } @@ -866,9 +890,11 @@ private final class ImporterImpl implements Importer { final Connection con; final ReadableTransactionImpl txr; final WriteableTransactionTransactionImpl txw; + // the trees this import wrote: close() refreshes the statistics of these and only these + final Set writtenTrees = ConcurrentHashMap.newKeySet(); final Boolean isOpen; - + public ImporterImpl() { isOpen=getStorageStatus().isWorking(); if (!isOpen) { @@ -890,25 +916,31 @@ public ImporterImpl() { @Override public void close() { try { - con.commit(); - updateTableStatistics(con); - con.close(); + try { + con.commit(); + updateTableStatistics(con, writtenTrees); + } finally { // the pooled connection must be returned even when the commit or a statistics statement throws + con.close(); + } } catch (SQLException e) { throw new StorageRuntimeException(e); - } - if (!isOpen) { - JDBCStorage.this.close(); + } finally { + if (!isOpen) { + JDBCStorage.this.close(); + } } } - + @Override public void clearTree(TreeName name) { txw.clearTree(name); + writtenTrees.add(name); } - + @Override public void put(TreeName treeName, ByteSequence key, ByteSequence value) { txw.put(treeName, key, value); + writtenTrees.add(treeName); } @Override diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index 5591dcc40c..f413cea0c4 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -40,6 +40,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.NoSuchElementException; @@ -374,6 +375,136 @@ String readTableComment(String tableName) throws Exception { } } + void writeTableComment(String tableName, String comment) throws Exception { + final String url = getJdbcUrl(); + final String sql; + if (url.startsWith("jdbc:postgresql") || url.startsWith("jdbc:oracle")) { + sql = "comment on table " + tableName + " is '" + comment + "'"; + } else if (url.startsWith("jdbc:mysql")) { + sql = "alter table " + tableName + " comment '" + comment + "'"; + } else if (url.startsWith("jdbc:sqlserver")) { + // exec arguments must be constants or variables: schema_name() cannot be passed inline + sql = "declare @s sysname = schema_name()" + + " exec sys.sp_updateextendedproperty N'MS_Description', N'" + comment + "', N'SCHEMA', @s, N'TABLE', N'" + tableName + "'"; + } else { + throw new SkipException("no table comment statement for " + url); + } + try (final Connection con = DriverManager.getConnection(url); + final Statement st = con.createStatement()) { + st.execute(sql); + } + } + + /** + * Comment statements are DDL (a metadata lock on mysql, a ddl lock on oracle), so a table + * whose stored comment already matches its tree name must not be re-stamped on subsequent + * opens - while a stale comment must be refreshed. + */ + @Test + public void testCommentStampSkippedWhenAlreadyStored() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("o=commentSkip", "dn2id"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); // stamps the freshly created table + } + }); + assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); + assertFalse(storage.commentTable(tree), "an up-to-date comment was re-stamped"); + writeTableComment(storage.getTableName(tree), "stale"); + assertTrue(storage.commentTable(tree), "a stale comment was not re-stamped"); + assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * A failing comment stamp must never disturb the transaction that opened the tree: it used + * to roll back the caller's connection, silently discarding writes pending in the same + * transaction (the way DefaultIndex.afterOpen() writes the trusted flag between openTree() calls). + */ + @Test + public void testCommentFailureLeavesTransactionIntact() throws Exception { + final TreeName tree = new TreeName("o=commentFailure", "dn2id"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + String readStoredComment(Connection con, String tableName) throws SQLException { + throw new SQLException("injected comment readback failure"); + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); // create the table up front + } + }); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.put(tree, key(1), value(1)); // pending in this transaction... + txn.openTree(tree, true); // ...while the comment machinery fails + } + }); + storage.read(new ReadOperation() { + @Override + public Void run(ReadableTransaction txn) throws Exception { + assertEquals(txn.read(tree, key(1)), value(1), "failing comment stamp discarded a pending write"); + return null; + } + }); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** deleteTree() must forget the tree: statistics refresh iterates known trees and must skip dropped tables. */ + @Test + public void testDeleteTreeForgetsTree() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("o=deleteTree", "dn2id"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + assertTrue(storage.listTrees().contains(tree)); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + assertFalse(storage.listTrees().contains(tree), "deleteTree() left the tree in the tree-to-table cache"); + } finally { + storage.close(); + } + } + /** A bulk import must refresh optimizer statistics: fresh tables were never analyzed (#859). */ @Test public void testImportRefreshesTableStatistics() throws Exception { @@ -396,7 +527,7 @@ public void run(WriteableTransaction txn) throws Exception { // import swallows statistics failures by design: assert directly that the // dialect-specific refresh statement is accepted by this database try (final Connection con = CachedConnection.getConnection(getJdbcUrl())) { - assertTrue(storage.updateTableStatistics(con), "statistics refresh reported failures"); + assertTrue(storage.updateTableStatistics(con, Collections.singleton(tree)), "statistics refresh reported failures"); } } finally { try { From 3b66d2560ff3731ba0aa7fcee5e95b8c2e60af6f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 18 Aug 2026 13:02:05 +0300 Subject: [PATCH 4/8] Bound the comment statement's lock wait, remember stamps that failed Review follow-up for the table-comment/statistics change: - Comment statements take a lock (metadata lock on MySQL, schema modification lock on SQL Server, DDL lock on Oracle), and MySQL waits a year while SQL Server waits forever by default, so a stamp could queue behind an unrelated transaction of another session. The stamp now runs on a connection of its own, outside the pool, and bounds its wait first (lock_wait_timeout, LOCK_TIMEOUT, lock_timeout, ddl_lock_timeout). It is outside the pool because a session setting would otherwise be carried over to whoever borrows the connection next: CachedConnection.close() only rolls back. The setting is sent as a plain batch - the SQL Server driver would revert one made inside sp_executesql before the statement it protects. - A failed stamp was logged at debug and reissued on every open, so an account that may not comment its tables retried all trees forever, invisibly. The failure is now logged at warning, as its sibling updateTableStatistics() does, and remembered until the next start; deleteTree() and removeStorageFiles() forget it again. An interrupt taken while waiting for a pooled connection re-asserts the flag instead of being swallowed. - The MySQL comment literal doubled backslashes unconditionally, which stores the wrong comment - and never matches the readback, so the table is re-stamped on every open - under the NO_BACKSLASH_ESCAPES sql mode. The escaping now follows the connection's @@sql_mode. - commentTable() returns an outcome (STAMPED / UP_TO_DATE / UNSUPPORTED / FAILED) instead of a boolean that could not tell "skipped" from "rejected", and updateTableStatistics() reports failure for an engine outside the dialect switch, where it refreshes nothing. Tests: a stamp issued while another session holds an uncommitted row of the table must return promptly (it is what caught an invalid PostgreSQL SET lock_timeout form); the transaction-integrity test keeps its pending write in another tree, injects the failure at the statement rather than at the readback, and checks the failure is not reissued; the MySQL statistics assertion runs with STATS_AUTO_RECALC=0, without which InnoDB's background recalculation satisfied it on its own; the SQL Server readback in the test matches class=1 as production does. PgSql, MySql, MsSql and Oracle container suites: 43/43 each, no skips. --- .../server/backends/jdbc/JDBCStorage.java | 181 +++++++++++++----- .../opends/server/backends/jdbc/TestCase.java | 150 +++++++++++++-- 2 files changed, 266 insertions(+), 65 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index e6aff1e46a..9dca769979 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -162,10 +162,35 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ private static final String[] NO_ARGS=new String[0]; + // Comment statements take a lock (a metadata lock on mysql, a schema modification lock on sql + // server, a ddl lock on oracle), and mysql and sql server wait for it without limit by + // default (lock_wait_timeout is a year, lock_timeout is infinite): a stamp could queue behind + // an unrelated transaction of another session on the same database and - on mysql - park + // every other query on the table behind itself. The stamp is a diagnostic aid, so every + // dialect is told to give up after this many seconds instead of waiting. + private static final int COMMENT_LOCK_TIMEOUT_SECONDS=5; + + // Trees whose stamp failed. A backend whose account may not comment its tables (no ALTER + // privilege, for instance) would otherwise reissue the statement for every tree on every + // open; the attempt is made again when the server restarts, so nothing is lost for good. + private final Set unstampableTrees=ConcurrentHashMap.newKeySet(); + static String driverNameOf(Connection con) { return ((CachedConnection) con).parent.getClass().getName(); } + /** Outcome of a comment stamp: openTree() ignores it, tests tell the cases apart. */ + enum CommentResult { + /** the table now carries its tree name */ + STAMPED, + /** the stored comment already matched: no statement was issued */ + UP_TO_DATE, + /** neither comment syntax nor readback is known for this engine */ + UNSUPPORTED, + /** the comment could not be read back or stored */ + FAILED + } + // Splices a value into a single-quoted SQL literal for the comment DDL, which takes no bind // parameters: doubles every quote, and every backslash on dialects where backslash is an // escape character inside literals. The scan of the escaped result is defence in depth: it @@ -185,63 +210,115 @@ private static String sqlLiteral(String value, boolean backslashIsEscape) { return "'"+escaped+"'"; } + // Whether backslash is an escape character inside string literals on this mysql connection: + // under the NO_BACKSLASH_ESCAPES sql mode it is an ordinary character, and doubling it there + // would store a comment that never matches its tree name - re-stamping the table forever. + boolean isMysqlBackslashEscape(Connection con) throws SQLException { + try (final PreparedStatement statement=con.prepareStatement("select @@sql_mode"); + final ResultSet rs=executeResultSet(statement)) { + final String sqlMode=rs.next() ? rs.getString(1) : null; + return sqlMode==null || !sqlMode.toUpperCase().contains("NO_BACKSLASH_ESCAPES"); + } + } + + // A connection of its own for the comment statement, outside the pool: the statement needs a + // session setting (the lock timeout above) that a pooled connection would carry over to + // whoever borrows it next, since CachedConnection.close() only rolls back. Opened only when + // a table really has to be stamped, which is once per tree per deployment. + Connection newStampConnection() throws SQLException { + final Connection con=DriverManager.getConnection(config.getDBDirectory()); + con.setAutoCommit(false); + return con; + } + + // A session setting must reach the server as a plain batch: the sql server driver runs a + // prepared statement through sp_executesql, and a setting made there is reverted when that + // call returns - before the statement it is meant to protect ever runs. + private void executeSessionStatement(Connection con, String sql) throws SQLException { + try (final Statement statement=con.createStatement()) { + if (logger.isTraceEnabled()) { + logger.trace(LocalizableMessage.raw("jdbc: %s",sql)); + } + statement.execute(sql); + } + } + // Table names are opaque SHA-224 hashes, so on the database side there is no way to tell // which tree a table holds. Stamp each table with its tree name (visible in "\dt+" and the // information schema) so database-level troubleshooting does not require recomputing hashes. - // Runs on a dedicated pooled connection, never on the transaction that opened the tree: - // comment statements are DDL (an implicit commit on mysql and oracle), and a failing + // Runs on a dedicated connection, never on the transaction that opened the tree: comment + // statements are DDL (an implicit commit on mysql and oracle), and a failing // sp_addextendedproperty rolls the whole transaction back on sql server - either would // corrupt work pending on the caller's connection (e.g. the trusted flag written by - // DefaultIndex.afterOpen()). Returns true when the comment was stamped; the comment is a - // diagnostic aid, so a failed attempt only returns false and must not fail the backend. - boolean commentTable(TreeName treeName) { + // DefaultIndex.afterOpen()). The comment is a diagnostic aid: a failed attempt only logs and + // must not fail the backend. + CommentResult commentTable(TreeName treeName) { final String tableName=getTableName(treeName); - try (final Connection con=getConnection()) { - final String driverName=driverNameOf(con); - final boolean postgres=driverName.contains("postgres"); - final boolean mysql=driverName.contains("mysql"); - final boolean oracle=driverName.contains("oracle"); - final boolean microsoft=driverName.contains("microsoft"); - if (!postgres && !mysql && !oracle && !microsoft) { // no comment syntax and readback known for other engines: leave the table unstamped - return false; - } - final String treeComment=treeName.toString(); - // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() - // runs on every backend open: only stamp when the stored comment is absent or stale - if (treeComment.equals(readStoredComment(con, tableName))) { - return false; - } - final String sql; - final String[] args; - if (mysql) { // ALTER TABLE takes no binds; backslash is an escape character in mysql literals - sql="alter table "+tableName+" comment "+sqlLiteral(treeComment,true); - args=NO_ARGS; - }else if (microsoft) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) - sql="declare @s sysname = schema_name()" - +" if exists (select 1 from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description')" - +" exec sys.sp_updateextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?" - +" else" - +" exec sys.sp_addextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?"; - args=new String[]{tableName, treeComment, tableName, treeComment, tableName}; - }else if (postgres) { // no binds in ddl; the E'' form keeps backslash an escape character regardless of standard_conforming_strings - sql="comment on table "+tableName+" is E"+sqlLiteral(treeComment,true); - args=NO_ARGS; - }else { // oracle: no binds in ddl; backslash is never an escape character in oracle literals - sql="comment on table "+tableName+" is "+sqlLiteral(treeComment,false); - args=NO_ARGS; + if (unstampableTrees.contains(treeName)) { // this tree already failed in this JVM: do not ask again + return CommentResult.FAILED; + } + final String treeComment=treeName.toString(); + final String sql; + final String[] args; + final String lockTimeoutSql; + try { + try (final Connection con=getConnection()) { // pooled: dialect detection and readback take no lock + final String driverName=driverNameOf(con); + final boolean postgres=driverName.contains("postgres"); + final boolean mysql=driverName.contains("mysql"); + final boolean oracle=driverName.contains("oracle"); + final boolean microsoft=driverName.contains("microsoft"); + if (!postgres && !mysql && !oracle && !microsoft) { // no comment syntax and readback known for other engines: leave the table unstamped + return CommentResult.UNSUPPORTED; + } + // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() + // runs on every backend open: only stamp when the stored comment is absent or stale + if (treeComment.equals(readStoredComment(con, tableName))) { + return CommentResult.UP_TO_DATE; + } + if (mysql) { // ALTER TABLE takes no binds; whether backslash escapes inside the literal depends on the sql mode + sql="alter table "+tableName+" comment "+sqlLiteral(treeComment,isMysqlBackslashEscape(con)); + args=NO_ARGS; + lockTimeoutSql="set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS; + }else if (microsoft) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) + sql="declare @s sysname = schema_name()" + +" if exists (select 1 from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description')" + +" exec sys.sp_updateextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?" + +" else" + +" exec sys.sp_addextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?"; + args=new String[]{tableName, treeComment, tableName, treeComment, tableName}; + lockTimeoutSql="set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000); + }else if (postgres) { // no binds in ddl; the E'' form keeps backslash an escape character regardless of standard_conforming_strings + sql="comment on table "+tableName+" is E"+sqlLiteral(treeComment,true); + args=NO_ARGS; + lockTimeoutSql="set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000); // milliseconds + }else { // oracle: no binds in ddl; backslash is never an escape character in oracle literals + sql="comment on table "+tableName+" is "+sqlLiteral(treeComment,false); + args=NO_ARGS; + // ddl_lock_timeout defaults to 0 (give up at once), but it can be raised globally + lockTimeoutSql="alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS; + } } - try (final PreparedStatement statement=con.prepareStatement(sql)) { - for (int i=0;i trees) { final boolean oracle=driverName.contains("oracle"); final boolean microsoft=driverName.contains("microsoft"); if (!postgres && !mysql && !oracle && !microsoft) { // no portable statistics refresh for other engines - return true; + return false; // nothing was refreshed: reporting success here would make the assertion of the tests vacuous } boolean allRefreshed=true; for (final TreeName treeName : trees) { @@ -368,6 +445,7 @@ public void removeStorageFiles() throws StorageRuntimeException { // all tables are gone: forget the mappings so listTrees() consumers skip the dropped trees for (final TreeName treeName : trees) { tree2table.invalidate(treeName); + unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt } } if (!isOpen) { @@ -589,6 +667,7 @@ public void deleteTree(TreeName treeName) { } // forget the mapping so listTrees() consumers (updateTableStatistics) skip the dropped table tree2table.invalidate(treeName); + unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt } @Override @@ -890,7 +969,11 @@ private final class ImporterImpl implements Importer { final Connection con; final ReadableTransactionImpl txr; final WriteableTransactionTransactionImpl txw; - // the trees this import wrote: close() refreshes the statistics of these and only these + // The trees this import wrote: close() refreshes the statistics of these and only these, + // so rebuilding a single index does not gather statistics for the whole backend. A full + // import legitimately covers every tree - AbstractTwoPhaseImportStrategy.beforePhaseOne + // clears them all before the first record is written - including when the import is + // aborted, since close() runs from the try-with-resources of OnDiskMergeImporter. final Set writtenTrees = ConcurrentHashMap.newKeySet(); final Boolean isOpen; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index f413cea0c4..ec5ab4ae4d 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -36,6 +36,7 @@ import java.sql.Connection; import java.sql.DriverManager; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; @@ -43,6 +44,7 @@ import java.util.Collections; import java.util.List; import java.util.NoSuchElementException; +import java.util.concurrent.atomic.AtomicInteger; import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; import static org.mockito.Mockito.when; @@ -364,7 +366,8 @@ String readTableComment(String tableName) throws Exception { } else if (url.startsWith("jdbc:oracle")) { sql = "select comments from user_tab_comments where table_name='" + tableName.toUpperCase() + "'"; } else if (url.startsWith("jdbc:sqlserver")) { - sql = "select cast(value as nvarchar(4000)) from sys.extended_properties where major_id=object_id('" + tableName + "') and minor_id=0 and name='MS_Description'"; + // class=1 is the table itself: major_id is only unique within a class + sql = "select cast(value as nvarchar(4000)) from sys.extended_properties where class=1 and major_id=object_id('" + tableName + "') and minor_id=0 and name='MS_Description'"; } else { throw new SkipException("no table comment query for " + url); } @@ -395,6 +398,24 @@ void writeTableComment(String tableName, String comment) throws Exception { } } + /** + * Removes the stored comment, so that the next stamp has to create one rather than replace + * it: on sql server that is sp_addextendedproperty, which is the statement reported to wait + * for an uncommitted row of another session. + */ + void clearTableComment(String tableName) throws Exception { + final String url = getJdbcUrl(); + if (!url.startsWith("jdbc:sqlserver")) { + writeTableComment(tableName, "stale"); // the other engines have one statement for both cases + return; + } + try (final Connection con = DriverManager.getConnection(url); + final Statement st = con.createStatement()) { + st.execute("declare @s sysname = schema_name()" + + " exec sys.sp_dropextendedproperty N'MS_Description', N'SCHEMA', @s, N'TABLE', N'" + tableName + "'"); + } + } + /** * Comment statements are DDL (a metadata lock on mysql, a ddl lock on oracle), so a table * whose stored comment already matches its tree name must not be re-stamped on subsequent @@ -413,9 +434,10 @@ public void run(WriteableTransaction txn) throws Exception { } }); assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); - assertFalse(storage.commentTable(tree), "an up-to-date comment was re-stamped"); + // UP_TO_DATE and not FAILED: the statement was skipped, not rejected + assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.UP_TO_DATE, "an up-to-date comment was re-stamped"); writeTableComment(storage.getTableName(tree), "stale"); - assertTrue(storage.commentTable(tree), "a stale comment was not re-stamped"); + assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.STAMPED, "a stale comment was not re-stamped"); assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); } finally { try { @@ -430,18 +452,97 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * A stamp must never queue behind another session's transaction. Comment statements take a + * lock (a metadata lock on mysql, a schema modification lock on sql server) and both engines + * wait for it without limit by default - lock_wait_timeout is a year, lock_timeout is + * infinite - so an unbounded stamp could hang the backend open and, on mysql, park every + * other query on that table behind itself. Whether an uncommitted row of another session + * conflicts with the statement at all differs between engines and versions, so the assertion + * is on the timing: the call comes back rather than waiting for that transaction to end. + */ + @Test(timeOut = 180000) + public void testCommentStampGivesUpOnLock() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("o=commentLock", "dn2id"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + final String tableName = storage.getTableName(tree); + clearTableComment(tableName); // no comment stored: the next attempt must issue a statement + try (final Connection blocker = DriverManager.getConnection(getJdbcUrl())) { + blocker.setAutoCommit(false); + try (final PreparedStatement st = blocker.prepareStatement("insert into " + tableName + " (h,k) values (?,?)")) { + st.setString(1, String.format("%1$-128s", "blocker").replace(' ', 'x')); + st.setBytes(2, new byte[]{1}); + st.executeUpdate(); + } + // the row is left uncommitted, so the lock it holds is still there + final long start = System.currentTimeMillis(); + final JDBCStorage.CommentResult result = storage.commentTable(tree); + final long elapsedMs = System.currentTimeMillis() - start; + blocker.rollback(); + // giving up and stamping anyway are both fine here - the engines differ in whether an + // uncommitted row of another session conflicts with the comment statement at all. + // Waiting for that session to finish is what must never happen. + assertTrue(elapsedMs < 60000, "the comment statement waited " + elapsedMs + " ms for a lock, result " + result); + if (result == JDBCStorage.CommentResult.STAMPED) { // it reported success: the comment must be there + assertEquals(readTableComment(tableName), tree.toString()); + } + } + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + /** * A failing comment stamp must never disturb the transaction that opened the tree: it used * to roll back the caller's connection, silently discarding writes pending in the same * transaction (the way DefaultIndex.afterOpen() writes the trusted flag between openTree() calls). + * The write pending during the failing stamp deliberately targets another tree: a write left + * pending on the very table being stamped would make the comment statement wait for the + * caller's own lock, which is a shape no production path has. */ @Test public void testCommentFailureLeavesTransactionIntact() throws Exception { - final TreeName tree = new TreeName("o=commentFailure", "dn2id"); + final TreeName stamped = new TreeName("o=commentFailure", "dn2id"); + final TreeName written = new TreeName("o=commentFailure", "id2entry"); + final JDBCStorage setUp = new JDBCStorage(createBackendCfg(), null); + try { // create both tables up front, with a storage that stamps them normally + setUp.open(AccessMode.READ_WRITE); + setUp.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(stamped, true); + txn.openTree(written, true); + } + }); + } finally { + setUp.close(); + } + final AtomicInteger stampAttempts = new AtomicInteger(); final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { @Override String readStoredComment(Connection con, String tableName) throws SQLException { - throw new SQLException("injected comment readback failure"); + return null; // pretend the table carries no comment, so a stamp is attempted + } + @Override + Connection newStampConnection() throws SQLException { + stampAttempts.incrementAndGet(); + throw new SQLException("injected comment failure"); } }; try { @@ -449,29 +550,27 @@ String readStoredComment(Connection con, String tableName) throws SQLException { storage.write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { - txn.openTree(tree, true); // create the table up front - } - }); - storage.write(new WriteOperation() { - @Override - public void run(WriteableTransaction txn) throws Exception { - txn.put(tree, key(1), value(1)); // pending in this transaction... - txn.openTree(tree, true); // ...while the comment machinery fails + txn.put(written, key(1), value(1)); // pending in this transaction... + txn.openTree(stamped, true); // ...while the comment machinery fails } }); storage.read(new ReadOperation() { @Override public Void run(ReadableTransaction txn) throws Exception { - assertEquals(txn.read(tree, key(1)), value(1), "failing comment stamp discarded a pending write"); + assertEquals(txn.read(written, key(1)), value(1), "failing comment stamp discarded a pending write"); return null; } }); + // the failure is remembered: an unstampable table is not asked again in this JVM + assertEquals(storage.commentTable(stamped), JDBCStorage.CommentResult.FAILED); + assertEquals(stampAttempts.get(), 1, "a failed stamp was reissued"); } finally { try { storage.write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { - txn.deleteTree(tree); + txn.deleteTree(stamped); + txn.deleteTree(written); } }); } catch (Exception ignored) {} @@ -518,6 +617,7 @@ public void run(WriteableTransaction txn) throws Exception { txn.openTree(tree, true); } }); + suspendAutomaticStatistics(storage.getTableName(tree)); try (final Importer importer = storage.startImport()) { for (int i = 0; i < 40; i++) { importer.put(tree, key(i), value(i)); @@ -542,6 +642,23 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * Suspends the automatic statistics upkeep of the engines that have it, so that what the + * assertion below sees was produced by the refresh of the import and by nothing else: InnoDB + * recalculates innodb_table_stats.n_rows on its own (innodb_stats_auto_recalc is on by + * default), which would let the assertion pass with no "analyze table" ever issued. + */ + void suspendAutomaticStatistics(String tableName) throws Exception { + final String url = getJdbcUrl(); + if (!url.startsWith("jdbc:mysql")) { + return; // nothing refreshes what is asserted below on the other engines within a test run + } + try (final Connection con = DriverManager.getConnection(url); + final Statement st = con.createStatement()) { + st.execute("alter table " + tableName + " stats_auto_recalc=0"); + } + } + void assertTableStatisticsFresh(String tableName) throws Exception { final String url = getJdbcUrl(); final String sql; @@ -552,7 +669,8 @@ void assertTableStatisticsFresh(String tableName) throws Exception { // num_rows stays null until dbms_stats gathers statistics sql = "select num_rows from user_tables where table_name='" + tableName.toUpperCase() + "'"; } else if (url.startsWith("jdbc:mysql")) { - // n_rows in the persistent stats table is refreshed by ANALYZE TABLE + // n_rows in the persistent stats table is refreshed by ANALYZE TABLE, and - with the + // automatic recalculation suspended above - by nothing else: it stays 0 without it sql = "select n_rows from mysql.innodb_table_stats where database_name=database() and table_name='" + tableName + "'"; } else if (url.startsWith("jdbc:sqlserver")) { // last_updated stays null until the first UPDATE STATISTICS From 19bae194b5408386c22cecf02649c889720e6ffc Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 18 Aug 2026 21:41:05 +0300 Subject: [PATCH 5/8] [#866] Bound the statistics refresh and share one connection for table comments Round 4 review of #866: - an import that failed or was cancelled no longer refreshes optimizer statistics: Importer gained a default aborted() that OnDiskMergeImporter reports from the catch around the whole import, and the JDBC importer skips the refresh. The refresh is bounded as well - setQueryTimeout, 600 s per table by default (org.openidentityplatform.opendj.jdbc.statistics.timeout, 0 for no limit) - and can be turned off entirely with org.openidentityplatform.opendj.jdbc.statistics=false; - the connection the comment statement runs on bounds its connect attempt through the driver property of its dialect (10 s), so a database that keeps its established connections but accepts no new ones leaves a table unstamped instead of hanging the open of a tree; - the comment statements of one open share one connection instead of connecting per tree, which was ~25 physical connects on the first open after an upgrade; - the mysql sql_mode probe moved onto the stamping session, so the session that answers it is the session that parses the literal; - a stamp that failed for a reason of the moment (connection exception, the dialect's lock timeout) is attempted again; only a rejected statement is remembered; - newStampConnection() and CachedConnection.getConnection() no longer leak a connection when setting it up throws. Tests: PgSql 47/47, MySql 47/47, Oracle 47/47, MsSql 47/47, zero skips. --- .../backends/jdbc/CachedConnection.java | 8 +- .../server/backends/jdbc/JDBCStorage.java | 363 ++++++++++++++---- .../pluggable/OnDiskMergeImporter.java | 21 + .../backends/pluggable/TracedStorage.java | 9 + .../backends/pluggable/spi/Importer.java | 12 + .../opends/server/backends/jdbc/TestCase.java | 198 +++++++++- 6 files changed, 528 insertions(+), 83 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java index f752ca518e..188f7ba2fb 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java @@ -95,12 +95,18 @@ static Connection getConnection(String connectionString, final int waitTime) thr return con; } } + Connection conNew = null; try { - final Connection conNew = DriverManager.getConnection(connectionString); + conNew = DriverManager.getConnection(connectionString); conNew.setAutoCommit(false); conNew.setTransactionIsolation(TRANSACTION_READ_COMMITTED); return new CachedConnection(connectionString, conNew); } catch (SQLException e) { // max_connection server error: try recursion for reuse connection + if (conNew != null) { // the connection was established but not set up: nothing else would close it + try { + conNew.close(); + } catch (SQLException e2) {} + } return getConnection(connectionString, (waitTime == 0) ? 1 : waitTime * 2); } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 9dca769979..3353f554a1 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -34,6 +34,7 @@ import org.opends.server.types.RestoreConfig; import org.opends.server.util.BackupManager; +import java.io.Closeable; import java.nio.ByteBuffer; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -170,15 +171,71 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ // dialect is told to give up after this many seconds instead of waiting. private static final int COMMENT_LOCK_TIMEOUT_SECONDS=5; - // Trees whose stamp failed. A backend whose account may not comment its tables (no ALTER - // privilege, for instance) would otherwise reissue the statement for every tree on every - // open; the attempt is made again when the server restarts, so nothing is lost for good. + // The comment statement runs on a connection of its own (newStampConnection() below), and a + // driver waits for a connect attempt without limit unless it is told otherwise: a database + // that keeps its established connections alive but accepts no new ones (a moved vip, a proxy + // at its connection limit) would otherwise hang the open of a tree - dsconfig + // create-backend-index opens one on a running server - instead of leaving a table unstamped. + // Every dialect gets the same bound, in the unit its own driver property takes. + private static final int STAMP_CONNECT_TIMEOUT_SECONDS=10; + + // Trees whose stamp failed for a reason that is not going to change by itself: an account that + // may not comment its tables (no ALTER privilege, for instance) would otherwise reissue the + // statement for every tree on every open. A failure that says nothing about the table - a lock + // timeout, a connection that broke - is not remembered (isTransientFailure() below), so a + // contended moment does not leave the backend unstamped until it is restarted. private final Set unstampableTrees=ConcurrentHashMap.newKeySet(); + /** + * The engines whose comment statement, comment readback and statistics refresh this backend + * knows, with the two session settings a comment statement needs: the driver property that + * bounds its connect attempt, and the statement that bounds its wait for the table lock. + */ + enum Dialect { + /** postgresql: lock_timeout takes milliseconds, connectTimeout seconds. */ + POSTGRES("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, + "set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000)), + /** mysql: lock_wait_timeout takes seconds, connectTimeout milliseconds. */ + MYSQL("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS*1000, + "set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS), + /** oracle: ddl_lock_timeout takes seconds and defaults to 0 (give up at once), but it can be raised globally. */ + ORACLE("oracle.net.CONNECT_TIMEOUT", STAMP_CONNECT_TIMEOUT_SECONDS*1000, + "alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS), + /** ms sql server: lock_timeout takes milliseconds, loginTimeout seconds. */ + MICROSOFT("loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, + "set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000)); + + final String connectTimeoutProperty; + final String connectTimeoutValue; + final String lockTimeoutSql; + + Dialect(String connectTimeoutProperty, int connectTimeoutValue, String lockTimeoutSql) { + this.connectTimeoutProperty=connectTimeoutProperty; + this.connectTimeoutValue=String.valueOf(connectTimeoutValue); + this.lockTimeoutSql=lockTimeoutSql; + } + } + static String driverNameOf(Connection con) { return ((CachedConnection) con).parent.getClass().getName(); } + // The dialect behind a pooled connection, or null for an engine none of the statements of this + // class fit: it is left unstamped and its statistics untouched rather than fed untested SQL. + static Dialect dialectOf(Connection con) { + final String driverName=driverNameOf(con); + if (driverName.contains("postgres")) { + return Dialect.POSTGRES; + }else if (driverName.contains("mysql")) { + return Dialect.MYSQL; + }else if (driverName.contains("oracle")) { + return Dialect.ORACLE; + }else if (driverName.contains("microsoft")) { + return Dialect.MICROSOFT; + } + return null; + } + /** Outcome of a comment stamp: openTree() ignores it, tests tell the cases apart. */ enum CommentResult { /** the table now carries its tree name */ @@ -213,6 +270,8 @@ private static String sqlLiteral(String value, boolean backslashIsEscape) { // Whether backslash is an escape character inside string literals on this mysql connection: // under the NO_BACKSLASH_ESCAPES sql mode it is an ordinary character, and doubling it there // would store a comment that never matches its tree name - re-stamping the table forever. + // Asked of the very session that parses the literal: sql_mode is a session setting, and a + // session opened at another moment can have been given another value of it. boolean isMysqlBackslashEscape(Connection con) throws SQLException { try (final PreparedStatement statement=con.prepareStatement("select @@sql_mode"); final ResultSet rs=executeResultSet(statement)) { @@ -221,16 +280,66 @@ boolean isMysqlBackslashEscape(Connection con) throws SQLException { } } - // A connection of its own for the comment statement, outside the pool: the statement needs a - // session setting (the lock timeout above) that a pooled connection would carry over to - // whoever borrows it next, since CachedConnection.close() only rolls back. Opened only when - // a table really has to be stamped, which is once per tree per deployment. - Connection newStampConnection() throws SQLException { - final Connection con=DriverManager.getConnection(config.getDBDirectory()); - con.setAutoCommit(false); + // A connection of its own for the comment statements, outside the pool: they need session + // settings (the lock timeout above) that a pooled connection would carry over to whoever + // borrows it next, since CachedConnection.close() only rolls back. + Connection newStampConnection(Dialect dialect) throws SQLException { + final Properties properties=new Properties(); + properties.setProperty(dialect.connectTimeoutProperty, dialect.connectTimeoutValue); + final Connection con=DriverManager.getConnection(config.getDBDirectory(), properties); + try { + con.setAutoCommit(false); + executeSessionStatement(con, dialect.lockTimeoutSql); // give up instead of waiting for another session + }catch (SQLException e) { // nothing else holds this connection yet: it would leak + try { + con.close(); + }catch (SQLException e2) {} + throw e; + } return con; } + // The connection the comment statements of one sweep of openTree() calls share. Opening a + // backend opens every tree it holds (about 25 for a stock suffix), so a connection per stamp + // would mean that many physical connects on the first open after an upgrade - the one open + // that stamps them all. Opened lazily: a sweep that finds every comment up to date, which is + // every open after the first, opens nothing at all. + final class StampSession implements Closeable { + private Connection con; + + Connection connection(Dialect dialect) throws SQLException { + if (con==null) { + con=newStampConnection(dialect); + } + return con; + } + + // A statement that failed can leave the session unusable (postgres refuses every further + // statement of the transaction with 25P02 until it is rolled back), so the stamp of the + // next tree gets a clean one: rolled back, or replaced when even the rollback fails. + void reset() { + if (con!=null) { + try { + con.rollback(); + }catch (SQLException e) { + close(); + } + } + } + + @Override + public void close() { + if (con!=null) { + try { + con.close(); + }catch (SQLException e) { + logger.trace(LocalizableMessage.raw("jdbc: unable to close the comment connection: %s", stackTraceToSingleLineString(e))); + } + con=null; + } + } + } + // A session setting must reach the server as a plain batch: the sql server driver runs a // prepared statement through sp_executesql, and a setting made there is reverted when that // call returns - before the statement it is meant to protect ever runs. @@ -253,22 +362,22 @@ private void executeSessionStatement(Connection con, String sql) throws SQLExcep // DefaultIndex.afterOpen()). The comment is a diagnostic aid: a failed attempt only logs and // must not fail the backend. CommentResult commentTable(TreeName treeName) { + try (final StampSession session=new StampSession()) { // a stamp of its own: no sweep to share a connection with + return commentTable(treeName, session); + } + } + + CommentResult commentTable(TreeName treeName, StampSession session) { final String tableName=getTableName(treeName); if (unstampableTrees.contains(treeName)) { // this tree already failed in this JVM: do not ask again return CommentResult.FAILED; } final String treeComment=treeName.toString(); - final String sql; - final String[] args; - final String lockTimeoutSql; + Dialect dialect=null; try { try (final Connection con=getConnection()) { // pooled: dialect detection and readback take no lock - final String driverName=driverNameOf(con); - final boolean postgres=driverName.contains("postgres"); - final boolean mysql=driverName.contains("mysql"); - final boolean oracle=driverName.contains("oracle"); - final boolean microsoft=driverName.contains("microsoft"); - if (!postgres && !mysql && !oracle && !microsoft) { // no comment syntax and readback known for other engines: leave the table unstamped + dialect=dialectOf(con); + if (dialect==null) { // no comment syntax and readback known for other engines: leave the table unstamped return CommentResult.UNSUPPORTED; } // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() @@ -276,72 +385,130 @@ CommentResult commentTable(TreeName treeName) { if (treeComment.equals(readStoredComment(con, tableName))) { return CommentResult.UP_TO_DATE; } - if (mysql) { // ALTER TABLE takes no binds; whether backslash escapes inside the literal depends on the sql mode - sql="alter table "+tableName+" comment "+sqlLiteral(treeComment,isMysqlBackslashEscape(con)); - args=NO_ARGS; - lockTimeoutSql="set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS; - }else if (microsoft) { // no COMMENT ON in t-sql: MS_Description extended property (procedure arguments take binds) - sql="declare @s sysname = schema_name()" - +" if exists (select 1 from sys.extended_properties where class=1 and major_id=object_id(?) and minor_id=0 and name='MS_Description')" - +" exec sys.sp_updateextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?" - +" else" - +" exec sys.sp_addextendedproperty N'MS_Description', ?, N'SCHEMA', @s, N'TABLE', ?"; - args=new String[]{tableName, treeComment, tableName, treeComment, tableName}; - lockTimeoutSql="set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000); - }else if (postgres) { // no binds in ddl; the E'' form keeps backslash an escape character regardless of standard_conforming_strings - sql="comment on table "+tableName+" is E"+sqlLiteral(treeComment,true); - args=NO_ARGS; - lockTimeoutSql="set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000); // milliseconds - }else { // oracle: no binds in ddl; backslash is never an escape character in oracle literals - sql="comment on table "+tableName+" is "+sqlLiteral(treeComment,false); - args=NO_ARGS; - // ddl_lock_timeout defaults to 0 (give up at once), but it can be raised globally - lockTimeoutSql="alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS; - } } - try (final Connection con=newStampConnection()) { - executeSessionStatement(con, lockTimeoutSql); // give up instead of waiting for another session - try (final PreparedStatement statement=con.prepareStatement(sql)) { - for (int i=0;i? order by k" cursor // batches - see OpenIdentityPlatform/OpenDJ#859), so refresh them once the data is in place. @@ -359,37 +535,44 @@ String readStoredComment(Connection con, String tableName) throws SQLException { // table on oracle. Statistics upkeep is best-effort: a failure must not fail the import that // produced the data, so failures are only logged - the return value makes them observable to tests. boolean updateTableStatistics(Connection con, Collection trees) { - final String driverName=driverNameOf(con); - final boolean postgres=driverName.contains("postgres"); - final boolean mysql=driverName.contains("mysql"); - final boolean oracle=driverName.contains("oracle"); - final boolean microsoft=driverName.contains("microsoft"); - if (!postgres && !mysql && !oracle && !microsoft) { // no portable statistics refresh for other engines + if (!Boolean.parseBoolean(System.getProperty(STATISTICS_PROPERTY,"true"))) { + logger.debug(LocalizableMessage.raw("jdbc: statistics refresh turned off by %s", STATISTICS_PROPERTY)); + return false; // nothing was refreshed + } + final Dialect dialect=dialectOf(con); + if (dialect==null) { // no portable statistics refresh for other engines return false; // nothing was refreshed: reporting success here would make the assertion of the tests vacuous } + final int timeoutSeconds=Math.max(0,Integer.getInteger(STATISTICS_TIMEOUT_PROPERTY,STATISTICS_TIMEOUT_SECONDS_DEFAULT)); boolean allRefreshed=true; for (final TreeName treeName : trees) { final String tableName=getTableName(treeName); final String sql; final String[] args; - if (postgres) { + switch (dialect) { + case POSTGRES: sql="analyze "+tableName; args=NO_ARGS; - }else if (mysql) { + break; + case MYSQL: sql="analyze table "+tableName; args=NO_ARGS; - }else if (oracle) { + break; + case ORACLE: sql="begin dbms_stats.gather_table_stats(user, ?); end;"; args=new String[]{tableName.toUpperCase()}; - }else { + break; + default: sql="update statistics "+tableName; args=NO_ARGS; + break; } try (final PreparedStatement statement=con.prepareStatement(sql)) { + statement.setQueryTimeout(timeoutSeconds); // 0: wait without limit for (int i=0;i T read(ReadOperation readOperation) throws Exception { @Override public void write(WriteOperation writeOperation) throws Exception { try (final Connection con=getConnection()) { + final WriteableTransactionTransactionImpl txn=new WriteableTransactionTransactionImpl(con); try { - writeOperation.run(new WriteableTransactionTransactionImpl(con)); + writeOperation.run(txn); con.commit(); } catch (Exception e) { try { con.rollback(); } catch (SQLException ex) {} throw e; + } finally { // the comment connection lives no longer than the trees it stamped + txn.stampSession.close(); } } } @@ -541,6 +727,11 @@ public long getRecordCount(TreeName treeName) { } private final class WriteableTransactionTransactionImpl extends ReadableTransactionImpl implements WriteableTransaction { + // Shared by every table this transaction stamps: opening a backend opens all its trees, + // and each stamp of its own connection would be a physical connect of its own. Closed by + // write() (and by ImporterImpl.close()) when the transaction is done with. + final StampSession stampSession=new StampSession(); + public WriteableTransactionTransactionImpl(Connection con) { super(con); if (!accessMode.isWriteable()) { @@ -630,7 +821,7 @@ public void openTree(TreeName treeName, boolean createOnDemand) { } } // mssql: k is varbinary(max), which cannot be an index key column - cursor batches stay unindexed there - commentTable(treeName); + commentTable(treeName, stampSession); } } @@ -976,6 +1167,13 @@ private final class ImporterImpl implements Importer { // aborted, since close() runs from the try-with-resources of OnDiskMergeImporter. final Set writtenTrees = ConcurrentHashMap.newKeySet(); + // Set when the import failed or was cancelled. Its trees hold whatever the import got + // through before it stopped - beforePhaseOne cleared them all, so that can be nothing at + // all - and the operator is going to run it again, so there is nothing worth describing + // to the optimizer here: on oracle gathering those statistics is a full scan per table + // that would delay the report of a failure, or of a cancellation, by all of its duration. + volatile boolean aborted = false; + final Boolean isOpen; public ImporterImpl() { @@ -996,14 +1194,27 @@ public ImporterImpl() { txw =new WriteableTransactionTransactionImpl(con); } + @Override + public void aborted() { + aborted = true; + } + @Override public void close() { try { try { con.commit(); - updateTableStatistics(con, writtenTrees); + if (aborted) { + logger.debug(LocalizableMessage.raw("jdbc: import aborted: statistics of the trees it wrote are left alone")); + }else { + updateTableStatistics(con, writtenTrees); + } } finally { // the pooled connection must be returned even when the commit or a statistics statement throws - con.close(); + try { + con.close(); + } finally { + txw.stampSession.close(); + } } } catch (SQLException e) { throw new StorageRuntimeException(e); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java index 9adafb7c27..c00494b80e 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java @@ -1194,6 +1194,21 @@ private OnDiskMergeImporter(String phase2ThreadNameTemplate, AbstractTwoPhaseImp } private void doImport(final Source source) throws InterruptedException, ExecutionException + { + try + { + importAllEntries(source); + } + catch (Exception e) + { + // Cancellation lands here as well (see the InterruptedException below). The trees hold an + // incomplete import: the storage must not treat what is in them as the final data. + importStrategy.aborted(); + throw e; + } + } + + private void importAllEntries(final Source source) throws InterruptedException, ExecutionException { final long phaseOneStartTime = System.currentTimeMillis(); final PhaseOneWriteableTransaction transaction = new PhaseOneWriteableTransaction(importStrategy); @@ -1327,6 +1342,12 @@ void afterPhaseOne() closeSilently(bufferPool); } + /** Tells the storage that the import stopped before it was through, so that its data is not final. */ + void aborted() + { + importer.aborted(); + } + abstract Callable newPhaseTwoTask(TreeName treeName, Chunk source, PhaseTwoProgressReporter progressReporter); void afterPhaseTwo(EntryContainer entryContainer) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java index e01eb94b17..172388ef62 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java @@ -12,6 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2015 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -226,6 +227,14 @@ public ByteString read(TreeName name, ByteSequence key) return value; } + @Override + public void aborted() + { + traceEnter("aborted"); + importer.aborted(); + traceLeave("aborted"); + } + @Override public void close() { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Importer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Importer.java index 1012c443cc..3309faca28 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Importer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Importer.java @@ -12,6 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2014-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable.spi; @@ -74,6 +75,17 @@ public interface Importer extends Closeable */ SequentialCursor openCursor(TreeName treeName); + /** + * Notifies this importer that the import failed or was cancelled, before {@link #close()} runs: what the trees hold + * is an incomplete import that is going to be run again. Implementations doing work of their own in {@link #close()} + * on the assumption that the data is final - refreshing the optimizer statistics of a database, for instance - can + * skip it. The default implementation does nothing. + */ + default void aborted() + { + // nothing by default: an importer that treats a partial import like a complete one is not wrong, only wasteful + } + @Override void close(); } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index ec5ab4ae4d..7b79249080 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -41,6 +41,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.NoSuchElementException; @@ -490,7 +491,9 @@ public void run(WriteableTransaction txn) throws Exception { // giving up and stamping anyway are both fine here - the engines differ in whether an // uncommitted row of another session conflicts with the comment statement at all. // Waiting for that session to finish is what must never happen. - assertTrue(elapsedMs < 60000, "the comment statement waited " + elapsedMs + " ms for a lock, result " + result); + // the bound is 5 s (COMMENT_LOCK_TIMEOUT_SECONDS): the slack is for the connect and the + // statement around it, not for a regression of the bound itself + assertTrue(elapsedMs < 20000, "the comment statement waited " + elapsedMs + " ms for a lock, result " + result); if (result == JDBCStorage.CommentResult.STAMPED) { // it reported success: the comment must be there assertEquals(readTableComment(tableName), tree.toString()); } @@ -512,9 +515,10 @@ public void run(WriteableTransaction txn) throws Exception { * A failing comment stamp must never disturb the transaction that opened the tree: it used * to roll back the caller's connection, silently discarding writes pending in the same * transaction (the way DefaultIndex.afterOpen() writes the trusted flag between openTree() calls). - * The write pending during the failing stamp deliberately targets another tree: a write left - * pending on the very table being stamped would make the comment statement wait for the - * caller's own lock, which is a shape no production path has. + * The write pending during the failing stamp deliberately targets another tree: a statement + * left pending on the very table being stamped - a write, or on mysql any statement, since a + * transaction holds a shared metadata lock on every table it touched - would make the comment + * statement wait for the caller's own lock, which is a shape no production path has. */ @Test public void testCommentFailureLeavesTransactionIntact() throws Exception { @@ -540,9 +544,9 @@ String readStoredComment(Connection con, String tableName) throws SQLException { return null; // pretend the table carries no comment, so a stamp is attempted } @Override - Connection newStampConnection() throws SQLException { + Connection newStampConnection(Dialect dialect) throws SQLException { stampAttempts.incrementAndGet(); - throw new SQLException("injected comment failure"); + throw new SQLException("injected comment failure"); // no sql state, no vendor code: not a failure of the moment } }; try { @@ -578,6 +582,188 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * A stamp that failed for a reason of the moment - the lock timeout the statement is given, + * a connection that broke - must be attempted again: only a failure saying that this table + * cannot be commented at all is remembered, or one contended moment would leave a backend + * unstamped until it is restarted. + */ + @Test + public void testTransientStampFailureIsRetried() throws Exception { + final TreeName tree = new TreeName("o=transientStamp", "dn2id"); + final AtomicInteger stampAttempts = new AtomicInteger(); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + String readStoredComment(Connection con, String tableName) throws SQLException { + return null; // pretend the table carries no comment, so a stamp is attempted + } + @Override + Connection newStampConnection(Dialect dialect) throws SQLException { + stampAttempts.incrementAndGet(); + throw new SQLException("injected connection failure", "08006"); // connection exception: a failure of the moment + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); // stamp #1, fails + } + }); + assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.FAILED); + assertEquals(stampAttempts.get(), 2, "a stamp that failed for a reason of the moment was not attempted again"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * Opening a backend opens every tree it holds - about 25 for a stock suffix - and the first + * open after an upgrade stamps them all: the stamps must share one connection rather than + * make a physical connect each. An open that finds the comments in place must make none. + */ + @Test + public void testCommentStampsShareOneConnection() throws Exception { + final TreeName[] trees = { + new TreeName("o=commentSweep", "dn2id"), + new TreeName("o=commentSweep", "id2entry"), + new TreeName("o=commentSweep", "state") }; + final AtomicInteger connects = new AtomicInteger(); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + Connection newStampConnection(Dialect dialect) throws SQLException { + connects.incrementAndGet(); + return super.newStampConnection(dialect); + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); // freshly created: every one of them is stamped + } + } + }); + for (final TreeName tree : trees) { + assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); + } + assertEquals(connects.get(), 1, "the stamps of one open did not share a connection"); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); + } + } + }); + assertEquals(connects.get(), 1, "an open that found every comment in place still connected"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.deleteTree(tree); + } + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * An import that failed or was cancelled leaves trees holding an incomplete import that is + * going to be run again: refreshing statistics of it describes data nobody will query, and on + * oracle it is a full scan per table between the failure and its report. + */ + @Test + public void testAbortedImportSkipsStatistics() throws Exception { + final TreeName tree = new TreeName("o=abortedImport", "dn2id"); + final AtomicInteger refreshes = new AtomicInteger(); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + boolean updateTableStatistics(Connection con, Collection trees) { + refreshes.incrementAndGet(); + return super.updateTableStatistics(con, trees); + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + try (final Importer importer = storage.startImport()) { + importer.put(tree, key(1), value(1)); + importer.aborted(); // what OnDiskMergeImporter reports when the import throws or is cancelled + } + assertEquals(refreshes.get(), 0, "statistics were refreshed for an import that was aborted"); + try (final Importer importer = storage.startImport()) { + importer.put(tree, key(2), value(2)); + } + assertEquals(refreshes.get(), 1, "statistics were not refreshed for an import that finished"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * The statistics refresh must be possible to turn off: on oracle it gathers with + * AUTO_SAMPLE_SIZE, a full scan of every table the import wrote. + */ + @Test + public void testStatisticsRefreshCanBeTurnedOff() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName tree = new TreeName("o=statisticsOff", "dn2id"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + System.setProperty(JDBCStorage.STATISTICS_PROPERTY, "false"); + try (final Connection con = CachedConnection.getConnection(getJdbcUrl())) { + assertFalse(storage.updateTableStatistics(con, Collections.singleton(tree)), + "the refresh ran with " + JDBCStorage.STATISTICS_PROPERTY + "=false"); + } + } finally { + System.clearProperty(JDBCStorage.STATISTICS_PROPERTY); + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + /** deleteTree() must forget the tree: statistics refresh iterates known trees and must skip dropped tables. */ @Test public void testDeleteTreeForgetsTree() throws Exception { From b4a2af43149951b69ee7f6edc3985830a1746655 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 19 Aug 2026 12:02:16 +0300 Subject: [PATCH 6/8] [#866] Keep the comment connection's lock bound after a failed stamp, and bound its whole login The lock bound was set inside an uncommitted transaction, and postgres undoes a plain SET when that transaction is rolled back - which is what the session does after a failed stamp while keeping the connection for the trees that follow. Commit the setting, so the first failure of a sweep no longer leaves the rest of it stamping unbounded. Bound the whole login attempt of that connection, not only the socket connect: the connect property of postgres and mysql covers socket.connect() alone and oracle's CONNECT_TIMEOUT excludes authentication, so those three carry a read bound as well - loginTimeout on postgres, socketTimeout on mysql, oracle.jdbc.ReadTimeout on oracle. The sql server loginTimeout already covers the phase by itself. End the sweep when a stamp fails for a reason of the moment: every tree left in the same open would pay that same lock bound, or that same connect attempt, again - about 25 of them for a stock suffix. Ask @@sql_mode once per sweep instead of once per tree, report an aborted import for an Error as well, and name every dialect in the four switches that select per-engine SQL, so a fifth one cannot silently inherit another dialect's statements. --- .../server/backends/jdbc/JDBCStorage.java | 121 ++++++++--- .../pluggable/OnDiskMergeImporter.java | 8 +- .../opends/server/backends/jdbc/TestCase.java | 201 ++++++++++++++++++ 3 files changed, 299 insertions(+), 31 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 3353f554a1..57c08ef63a 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -179,6 +179,16 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ // Every dialect gets the same bound, in the unit its own driver property takes. private static final int STAMP_CONNECT_TIMEOUT_SECONDS=10; + // Only the sql server driver has one property for the whole login phase: the connect property + // of postgres and mysql bounds socket.connect() alone, and oracle's own reference says + // CONNECT_TIMEOUT "doesn't include user authentication" - so the reads of tls and + // authentication, which is exactly where a proxy that accepts a connection and then goes + // quiet leaves the driver, need a read bound of their own. That bound outlives the login + // phase and covers the comment statement too, so it is kept well clear of the lock bound + // above: the statement gives up on a contended lock long before the socket gives up on the + // server. + private static final int STAMP_READ_TIMEOUT_SECONDS=30; + // Trees whose stamp failed for a reason that is not going to change by itself: an account that // may not comment its tables (no ALTER privilege, for instance) would otherwise reissue the // statement for every tree on every open. A failure that says nothing about the table - a lock @@ -188,31 +198,36 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ /** * The engines whose comment statement, comment readback and statistics refresh this backend - * knows, with the two session settings a comment statement needs: the driver property that - * bounds its connect attempt, and the statement that bounds its wait for the table lock. + * knows, with the session settings a comment statement needs: the driver properties that + * bound the connect attempt of the connection it runs on, and the statement that bounds its + * wait for the table lock. */ enum Dialect { - /** postgresql: lock_timeout takes milliseconds, connectTimeout seconds. */ - POSTGRES("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, - "set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000)), - /** mysql: lock_wait_timeout takes seconds, connectTimeout milliseconds. */ - MYSQL("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS*1000, - "set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS), - /** oracle: ddl_lock_timeout takes seconds and defaults to 0 (give up at once), but it can be raised globally. */ - ORACLE("oracle.net.CONNECT_TIMEOUT", STAMP_CONNECT_TIMEOUT_SECONDS*1000, - "alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS), - /** ms sql server: lock_timeout takes milliseconds, loginTimeout seconds. */ - MICROSOFT("loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, - "set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000)); - - final String connectTimeoutProperty; - final String connectTimeoutValue; + /** postgresql: lock_timeout takes milliseconds; loginTimeout bounds the whole login - the driver runs it on a thread of its own - and connectTimeout the socket connect inside it, both in seconds. */ + POSTGRES("set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000), + "connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS), + /** mysql: lock_wait_timeout takes seconds; connectTimeout bounds the socket connect and socketTimeout every read after it, both in milliseconds. */ + MYSQL("set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS, + "connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS*1000, "socketTimeout", STAMP_READ_TIMEOUT_SECONDS*1000), + /** oracle: ddl_lock_timeout takes seconds and defaults to 0 (give up at once), but it can be raised globally; the connect and read bounds take milliseconds. */ + ORACLE("alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS, + "oracle.net.CONNECT_TIMEOUT", STAMP_CONNECT_TIMEOUT_SECONDS*1000, "oracle.jdbc.ReadTimeout", STAMP_READ_TIMEOUT_SECONDS*1000), + /** ms sql server: lock_timeout takes milliseconds; loginTimeout covers the whole login phase, in seconds, so no read bound of its own. */ + MICROSOFT("set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000), + "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS); + final String lockTimeoutSql; + // The driver properties bounding the connect attempt of a stamp connection, as the + // name/value pairs the constructor was given. newStampConnection() hands the driver a + // copy of them: a driver is free to write into the map it is passed, and the sql server + // one gives a supplied property precedence over the same property of the url. + final Properties connectProperties=new Properties(); - Dialect(String connectTimeoutProperty, int connectTimeoutValue, String lockTimeoutSql) { - this.connectTimeoutProperty=connectTimeoutProperty; - this.connectTimeoutValue=String.valueOf(connectTimeoutValue); + Dialect(String lockTimeoutSql, Object... connectPropertyPairs) { this.lockTimeoutSql=lockTimeoutSql; + for (int i=0;i trees) { sql="begin dbms_stats.gather_table_stats(user, ?); end;"; args=new String[]{tableName.toUpperCase()}; break; - default: + case MICROSOFT: sql="update statistics "+tableName; args=NO_ARGS; break; + default: // a dialect this switch was never told about must not inherit another one's statement + throw new IllegalStateException("no statistics refresh for dialect "+dialect); } try (final PreparedStatement statement=con.prepareStatement(sql)) { statement.setQueryTimeout(timeoutSeconds); // 0: wait without limit diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java index c00494b80e..77d779be3a 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java @@ -1199,12 +1199,14 @@ private void doImport(final Source source) throws InterruptedException, Executio { importAllEntries(source); } - catch (Exception e) + catch (Throwable t) { // Cancellation lands here as well (see the InterruptedException below). The trees hold an - // incomplete import: the storage must not treat what is in them as the final data. + // incomplete import: the storage must not treat what is in them as the final data. Errors + // are caught too - an import killed by an OutOfMemoryError leaves the trees just as partial + // as one killed by an exception. importStrategy.aborted(); - throw e; + throw t; } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index 7b79249080..a13f47582f 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -417,6 +417,44 @@ void clearTableComment(String tableName) throws Exception { } } + /** The dialect of the database this suite runs against, as the backend detects it from the driver. */ + JDBCStorage.Dialect dialect() { + final String url = getJdbcUrl(); + if (url.startsWith("jdbc:postgresql")) { + return JDBCStorage.Dialect.POSTGRES; + } else if (url.startsWith("jdbc:mysql")) { + return JDBCStorage.Dialect.MYSQL; + } else if (url.startsWith("jdbc:oracle")) { + return JDBCStorage.Dialect.ORACLE; + } else if (url.startsWith("jdbc:sqlserver")) { + return JDBCStorage.Dialect.MICROSOFT; + } + throw new SkipException("no dialect for " + url); + } + + /** + * What this connection reports as its lock bound, in the unit and the rendering of its own + * engine, or null where reading it needs a privilege the test user does not have: oracle + * keeps ddl_lock_timeout in v$parameter, which an application user cannot select from. + */ + String sessionLockBound(Connection con) throws Exception { + final String url = getJdbcUrl(); + final String sql; + if (url.startsWith("jdbc:postgresql")) { + sql = "show lock_timeout"; + } else if (url.startsWith("jdbc:mysql")) { + sql = "select @@session.lock_wait_timeout"; + } else if (url.startsWith("jdbc:sqlserver")) { + sql = "select @@lock_timeout"; + } else { + return null; + } + try (final Statement st = con.createStatement(); + final ResultSet rs = st.executeQuery(sql)) { + return rs.next() ? rs.getString(1) : null; + } + } + /** * Comment statements are DDL (a metadata lock on mysql, a ddl lock on oracle), so a table * whose stored comment already matches its tree name must not be re-stamped on subsequent @@ -683,6 +721,169 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * The bound a stamp connection is given must survive a stamp that failed. Postgres undoes a + * plain SET when the transaction that ran it is rolled back, and a failed stamp is rolled + * back with the connection kept and reused - one connection serves every tree of a backend + * open - so every tree stamped after the first failure used to run with no bound at all, + * which is what the bound exists to prevent. + */ + @Test + public void testLockBoundSurvivesAFailedStamp() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + // no table was ever created for this tree, so its comment statement fails - on a connection + // that stays usable, which is the case the session rolls back rather than replaces + final TreeName missing = new TreeName("o=lockBound", "neverCreated"); + try { + storage.open(AccessMode.READ_WRITE); + final JDBCStorage.Dialect dialect = dialect(); + final String bound; + try (final Connection fresh = storage.newStampConnection(dialect)) { + bound = sessionLockBound(fresh); // what a connection carrying the bound reports + } + try (final JDBCStorage.StampSession session = storage.new StampSession()) { + assertEquals(storage.commentTable(missing, session), JDBCStorage.CommentResult.FAILED, + "stamping a table that does not exist was reported as done"); + if (bound != null) { // oracle: ddl_lock_timeout is only in v$parameter, which the test user cannot read + assertEquals(sessionLockBound(session.connection(dialect)), bound, + "the lock bound was lost when the failed stamp was rolled back"); + } + } + } finally { + storage.close(); + } + } + + /** + * A stamp failure of the moment - a connect that did not go through, a lock the statement + * gave up on - says nothing about one table: every remaining tree of the same open would pay + * the same bound, or the same connect attempt, again. That is about 25 of them for a stock + * suffix, all for a diagnostic aid, so the sweep gives up after the first - and since nothing + * is remembered, the next open tries again. + */ + @Test + public void testTransientStampFailureEndsTheSweep() throws Exception { + final TreeName[] trees = { + new TreeName("o=sweepGiveUp", "dn2id"), + new TreeName("o=sweepGiveUp", "id2entry"), + new TreeName("o=sweepGiveUp", "state") }; + final AtomicInteger stampAttempts = new AtomicInteger(); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + String readStoredComment(Connection con, String tableName) throws SQLException { + return null; // pretend no table carries a comment, so every one of them is stamped + } + @Override + Connection newStampConnection(Dialect dialect) throws SQLException { + stampAttempts.incrementAndGet(); + throw new SQLException("injected connection failure", "08006"); // connection exception: a failure of the moment + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); + } + } + }); + assertEquals(stampAttempts.get(), 1, "a failure of the moment was paid once per tree of the same open"); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); + } + } + }); + assertEquals(stampAttempts.get(), 2, "the open after a failure of the moment did not try again"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.deleteTree(tree); + } + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * @@sql_mode decides whether a backslash escapes inside the comment literal. It belongs to + * the session, and the stamps of one open share a connection, so it is asked once for the + * whole sweep rather than once per tree - and only on mysql, the one engine whose literal + * depends on it. + */ + @Test + public void testSqlModeProbedOncePerSweep() throws Exception { + final TreeName[] trees = { + new TreeName("o=sqlModeProbe", "dn2id"), + new TreeName("o=sqlModeProbe", "id2entry"), + new TreeName("o=sqlModeProbe", "state") }; + final AtomicInteger probes = new AtomicInteger(); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + boolean isMysqlBackslashEscape(Connection con) throws SQLException { + probes.incrementAndGet(); + return super.isMysqlBackslashEscape(con); + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); // freshly created: every one of them is stamped + } + } + }); + for (final TreeName tree : trees) { + assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); + } + assertEquals(probes.get(), dialect() == JDBCStorage.Dialect.MYSQL ? 1 : 0, + "the sql mode of one sweep was not asked exactly once"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.deleteTree(tree); + } + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * Every dialect must bound the whole login attempt of a stamp connection, not only its socket + * connect: postgres and mysql apply their connect property to socket.connect() alone, and + * oracle's CONNECT_TIMEOUT excludes authentication, so those three carry a second property + * for the reads of tls and authentication, while the sql server loginTimeout covers the phase + * on its own. A server that accepts a connection and then goes quiet cannot be staged in a + * container, so this guards the declaration rather than the behaviour. + */ + @Test + public void testEveryDialectBoundsItsLoginAttempt() { + for (final JDBCStorage.Dialect dialect : JDBCStorage.Dialect.values()) { + assertEquals(dialect.connectProperties.size(), dialect == JDBCStorage.Dialect.MICROSOFT ? 1 : 2, + "connect bounds of " + dialect + ": " + dialect.connectProperties); + for (final String name : dialect.connectProperties.stringPropertyNames()) { + assertTrue(Integer.parseInt(dialect.connectProperties.getProperty(name)) > 0, + name + " of " + dialect + " bounds nothing: " + dialect.connectProperties.getProperty(name)); + } + } + } + /** * An import that failed or was cancelled leaves trees holding an incomplete import that is * going to be run again: refreshing statistics of it describes data nobody will query, and on From 60a17f71669a09a181431fd010170923e86d5565 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 19 Aug 2026 12:35:52 +0300 Subject: [PATCH 7/8] [#866] Declare the connect bounds of a dialect without indexing an array CodeQL alert 1269 (java/index-out-of-bounds): the varargs constructor read connectPropertyPairs[i+1] without anything guaranteeing the array holds whole pairs. Two constructors take the properties by name instead - one for the driver whose single property covers the whole login phase, one for those needing a read bound as well - so there is no index to get wrong, and the declarations at the constants read the same. --- .../server/backends/jdbc/JDBCStorage.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 57c08ef63a..6bebec5bb3 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -217,17 +217,22 @@ enum Dialect { "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS); final String lockTimeoutSql; - // The driver properties bounding the connect attempt of a stamp connection, as the - // name/value pairs the constructor was given. newStampConnection() hands the driver a - // copy of them: a driver is free to write into the map it is passed, and the sql server - // one gives a supplied property precedence over the same property of the url. + // The driver properties bounding the connect attempt of a stamp connection. + // newStampConnection() hands the driver a copy of them: a driver is free to write into the + // map it is passed, and the sql server one gives a supplied property precedence over the + // same property of the url. final Properties connectProperties=new Properties(); - Dialect(String lockTimeoutSql, Object... connectPropertyPairs) { + /** For the one driver whose single property covers the whole login phase. */ + Dialect(String lockTimeoutSql, String connectProperty, int connectValue) { this.lockTimeoutSql=lockTimeoutSql; - for (int i=0;i Date: Wed, 19 Aug 2026 18:47:58 +0300 Subject: [PATCH 8/8] [#866] Bound the sql server stamp connection, take its readback off the pool Round 6 review of #866: - the sql server stamp connection carried loginTimeout alone, and mssql-jdbc discards it for the reads of a login: TDSChannel.open() hands the socket min(what is left of loginTimeout, socketTimeout), and socketTimeout defaults to 0, so the read of the prelogin answer was left unbounded - which is the very state the bound exists for, a proxy that accepts a connection and then goes quiet. Every dialect now declares the property bounding its connect and the property bounding the reads behind it, postgres included, where socketTimeout takes seconds; - the comment readback borrowed a second connection from the pool, while the thread doing the open was inside a transaction holding one already. Where no further connection can be had, the pool waits for a peer to return one - which there is the very thread it is blocking. The readback runs on the stamp connection now, and the dialect is taken off the caller's own connection, so the comment path takes no borrow at all. An open costs one connection for the whole sweep, readback included; - a stamp that gave up on a lock ended the sweep it was part of, so one permanently contended table meant a backend was never stamped at all, on any open. The two-way isTransientFailure() is a three-way FailureScope: a rejected statement is remembered, a lock is neither remembered nor fatal to the sweep, and only a connection that is gone ends it. Both chains of the failure are walked, getCause() and getNextException(): a driver reports a vendor code as the next exception at least as often as it reports it as the cause, and reading one of the two classified a lock timeout as a rejection - leaving the tree unstamped until the next start over a moment of contention; - the dialect guard of updateTableStatistics() threw out of ImporterImpl.close(), against the best-effort contract stated twenty lines above it. It degrades to "this table was not refreshed", like every other failure of that loop; - OnDiskMergeImporter lost the Throwable that had aborted an import if aborted() threw on the way out; it is suppressed into it instead; - a rejection is forgotten when the storage closes, so re-enabling the backend is enough to try again once the privilege has been granted. Tests: StampConnectionTestCase is new and needs no database. The bounds are the one thing the container suites cannot guard - they skip themselves whole where no docker is reachable, so a bound only they exercise is a bound that a silent skip deletes. Four real drivers against a ServerSocket that accepts and answers nothing, attempted concurrently (31 s); a recording java.sql.Driver for what actually reaches it; the failure classification, including a vendor code only getNextException() reaches and a self-referring chain. Removing the line that hands the bounds to the driver turns it red - all four drivers then sit past 240 s - which is what its predecessor could not do: it mirrored the enum literal and passed. In the container suites, testContendedTableDoesNotEndTheSweep is new, testTransientStampFailureEndsTheSweep is now testConnectionFailureEndsTheSweep, and testCommentStampsShareOneConnection asserts one connection per open rather than none for an open that finds every comment in place. --- .../server/backends/jdbc/JDBCStorage.java | 326 +++++++++------- .../pluggable/OnDiskMergeImporter.java | 12 +- .../jdbc/StampConnectionTestCase.java | 352 ++++++++++++++++++ .../opends/server/backends/jdbc/TestCase.java | 162 +++++--- 4 files changed, 668 insertions(+), 184 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/StampConnectionTestCase.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 6bebec5bb3..f0e6557d60 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -121,6 +121,10 @@ public StorageStatus getStorageStatus() { @Override public void close() { storageStatus = StorageStatus.lockedDown(LocalizableMessage.raw("closed")); + // a stamp that the database rejected is remembered for as long as the storage is open, so + // that it is not reissued for every tree on every open; disabling and re-enabling the + // backend is the way to try again once the privilege has been granted + unstampableTrees.clear(); } final LoadingCache tree2table = Caffeine.newBuilder() @@ -179,21 +183,28 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ // Every dialect gets the same bound, in the unit its own driver property takes. private static final int STAMP_CONNECT_TIMEOUT_SECONDS=10; - // Only the sql server driver has one property for the whole login phase: the connect property - // of postgres and mysql bounds socket.connect() alone, and oracle's own reference says - // CONNECT_TIMEOUT "doesn't include user authentication" - so the reads of tls and - // authentication, which is exactly where a proxy that accepts a connection and then goes - // quiet leaves the driver, need a read bound of their own. That bound outlives the login - // phase and covers the comment statement too, so it is kept well clear of the lock bound - // above: the statement gives up on a contended lock long before the socket gives up on the - // server. + // Not one of the four drivers bounds the whole login attempt with its connect property alone: + // postgres and mysql apply theirs to socket.connect(), oracle's own reference says + // CONNECT_TIMEOUT "doesn't include user authentication", and the sql server driver leaves the + // read of the prelogin answer unbounded - TDSChannel.open() gives the socket + // min(what is left of loginTimeout, socketTimeout) and socketTimeout defaults to 0, which is + // "wait forever". Those reads - of the prelogin handshake, of tls, of authentication - are + // exactly where a proxy that accepts a connection and then goes quiet leaves the driver, so + // every dialect carries a read bound as well. All four are socket read timeouts, so the bound + // outlives the login phase and covers the comment statement too, which is why it is kept well + // clear of the lock bound above: the statement gives up on a contended lock long before the + // socket gives up on the server. On a mysql connection with tls (the sslMode=PREFERRED default + // of connector/j) the wall clock of a dead peer is twice this, since closing an SSLSocket + // drains input waiting for close_notify and pays the read bound a second time. private static final int STAMP_READ_TIMEOUT_SECONDS=30; // Trees whose stamp failed for a reason that is not going to change by itself: an account that // may not comment its tables (no ALTER privilege, for instance) would otherwise reissue the // statement for every tree on every open. A failure that says nothing about the table - a lock - // timeout, a connection that broke - is not remembered (isTransientFailure() below), so a - // contended moment does not leave the backend unstamped until it is restarted. + // timeout, a connection that broke - is not remembered (failureScope() below), so a contended + // moment does not leave the backend unstamped until it is restarted. Forgotten when the + // storage is closed, so re-enabling the backend is enough to try again once the privilege has + // been granted, without a restart of the server. private final Set unstampableTrees=ConcurrentHashMap.newKeySet(); /** @@ -203,36 +214,40 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ * wait for the table lock. */ enum Dialect { - /** postgresql: lock_timeout takes milliseconds; loginTimeout bounds the whole login - the driver runs it on a thread of its own - and connectTimeout the socket connect inside it, both in seconds. */ + /** postgresql: lock_timeout takes milliseconds; connectTimeout bounds socket.connect(), loginTimeout the whole login the driver runs on a thread of its own, socketTimeout every read after it - all three in seconds. */ POSTGRES("set lock_timeout = "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000), - "connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS), + "connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, + "socketTimeout", STAMP_READ_TIMEOUT_SECONDS), /** mysql: lock_wait_timeout takes seconds; connectTimeout bounds the socket connect and socketTimeout every read after it, both in milliseconds. */ MYSQL("set session lock_wait_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS, "connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS*1000, "socketTimeout", STAMP_READ_TIMEOUT_SECONDS*1000), /** oracle: ddl_lock_timeout takes seconds and defaults to 0 (give up at once), but it can be raised globally; the connect and read bounds take milliseconds. */ ORACLE("alter session set ddl_lock_timeout="+COMMENT_LOCK_TIMEOUT_SECONDS, "oracle.net.CONNECT_TIMEOUT", STAMP_CONNECT_TIMEOUT_SECONDS*1000, "oracle.jdbc.ReadTimeout", STAMP_READ_TIMEOUT_SECONDS*1000), - /** ms sql server: lock_timeout takes milliseconds; loginTimeout covers the whole login phase, in seconds, so no read bound of its own. */ + /** ms sql server: lock_timeout takes milliseconds; loginTimeout bounds the socket connect, in seconds, and socketTimeout the prelogin read it leaves open - and every read after it - in milliseconds. */ MICROSOFT("set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000), - "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS); + "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, "socketTimeout", STAMP_READ_TIMEOUT_SECONDS*1000); final String lockTimeoutSql; - // The driver properties bounding the connect attempt of a stamp connection. - // newStampConnection() hands the driver a copy of them: a driver is free to write into the - // map it is passed, and the sql server one gives a supplied property precedence over the - // same property of the url. + // The driver properties bounding the login attempt of a stamp connection: the one bounding + // the socket connect and the one bounding the reads behind it, each in the unit its own + // driver takes. newStampConnection() hands the driver a copy of them: a driver is free to + // write into the map it is passed, and the sql server one gives a supplied property + // precedence over the same property of the url. final Properties connectProperties=new Properties(); - /** For the one driver whose single property covers the whole login phase. */ - Dialect(String lockTimeoutSql, String connectProperty, int connectValue) { + /** For the three drivers whose connect property and read property bound the login between them. */ + Dialect(String lockTimeoutSql, String connectProperty, int connectValue, String readProperty, int readValue) { this.lockTimeoutSql=lockTimeoutSql; connectProperties.setProperty(connectProperty, String.valueOf(connectValue)); + connectProperties.setProperty(readProperty, String.valueOf(readValue)); } - /** For the drivers needing a read bound on top of the one bounding the socket connect. */ - Dialect(String lockTimeoutSql, String connectProperty, int connectValue, String readProperty, int readValue) { - this(lockTimeoutSql, connectProperty, connectValue); - connectProperties.setProperty(readProperty, String.valueOf(readValue)); + /** For the one driver bounding the login itself, on top of the socket connect and the reads behind it. */ + Dialect(String lockTimeoutSql, String connectProperty, int connectValue, String loginProperty, int loginValue, + String readProperty, int readValue) { + this(lockTimeoutSql, connectProperty, connectValue, readProperty, readValue); + connectProperties.setProperty(loginProperty, String.valueOf(loginValue)); } } @@ -417,36 +432,41 @@ private void executeSessionStatement(Connection con, String sql) throws SQLExcep // corrupt work pending on the caller's connection (e.g. the trusted flag written by // DefaultIndex.afterOpen()). The comment is a diagnostic aid: a failed attempt only logs and // must not fail the backend. - CommentResult commentTable(TreeName treeName) { + CommentResult commentTable(TreeName treeName, Dialect dialect) { try (final StampSession session=new StampSession()) { // a stamp of its own: no sweep to share a connection with - return commentTable(treeName, session); + return commentTable(treeName, dialect, session); } } - CommentResult commentTable(TreeName treeName, StampSession session) { + CommentResult commentTable(TreeName treeName, Dialect dialect, StampSession session) { final String tableName=getTableName(treeName); - if (unstampableTrees.contains(treeName)) { // this tree already failed in this JVM: do not ask again + if (dialect==null) { // no comment syntax and readback known for other engines: leave the table unstamped + return CommentResult.UNSUPPORTED; + } + if (unstampableTrees.contains(treeName)) { // the database already rejected this one: do not ask again return CommentResult.FAILED; } - if (session.hasGivenUp()) { // an earlier tree of this sweep hit what every tree of it would hit - logger.debug(LocalizableMessage.raw("jdbc: table %s is left unstamped: the stamp of an earlier table of this open failed for a reason of the moment", tableName)); + if (session.hasGivenUp()) { // an earlier tree of this sweep lost the session every tree of it needs + logger.debug(LocalizableMessage.raw("jdbc: table %s is left unstamped: the stamp of an earlier table of this open lost its connection", tableName)); return CommentResult.FAILED; } final String treeComment=treeName.toString(); - Dialect dialect=null; try { - try (final Connection con=getConnection()) { // pooled: dialect detection and readback take no lock - dialect=dialectOf(con); - if (dialect==null) { // no comment syntax and readback known for other engines: leave the table unstamped - return CommentResult.UNSUPPORTED; - } - // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() - // runs on every backend open: only stamp when the stored comment is absent or stale - if (treeComment.equals(readStoredComment(con, tableName))) { - return CommentResult.UP_TO_DATE; - } - } + // The readback runs on the stamp connection, not on one borrowed from the pool: the + // caller of openTree() is inside a transaction and holding a pooled connection already, + // and a pool that cannot open a second one waits for a peer to return one - which here + // is the very thread that is waiting. The dialect comes from the caller's connection + // for the same reason: finding it out must not cost a borrow either. final Connection con=session.connection(dialect); + // comment statements are DDL (metadata lock on mysql, ddl lock on oracle) and openTree() + // runs on every backend open: only stamp when the stored comment is absent or stale + final String storedComment=readStoredComment(con, dialect, tableName); + // end the read: this connection is shared by every tree of the sweep and must not hold + // a transaction open across all of them + con.commit(); + if (treeComment.equals(storedComment)) { + return CommentResult.UP_TO_DATE; + } final String sql; final String[] args; switch (dialect) { @@ -481,81 +501,115 @@ CommentResult commentTable(TreeName treeName, StampSession session) { con.commit(); } return CommentResult.STAMPED; - }catch (InterruptedException e) { // the connection pool hands out connections through a blocking queue - Thread.currentThread().interrupt(); - session.reset(); - logger.debug(LocalizableMessage.raw("jdbc: interrupted while commenting table %s with tree name %s", tableName, treeName)); - return CommentResult.FAILED; // not remembered: an interrupt says nothing about this table }catch (Exception e) { session.reset(); // what the failed statement left behind must not poison the stamp of the next tree - final boolean transientFailure=isTransientFailure(e, dialect); - if (transientFailure) { - session.giveUp(); // this one is not about the table: the trees left in this sweep would pay it again, one bound each - }else { + final FailureScope scope=failureScope(e, dialect); + if (scope==FailureScope.SESSION) { + // the connection itself is gone, and every tree left in this sweep needs one: each + // would pay the same connect attempt again, ~25 of them for a stock suffix + session.giveUp(); + }else if (scope==FailureScope.TREE) { unstampableTrees.add(treeName); } logger.warn(LocalizableMessage.raw("jdbc: unable to comment table %s with tree name %s, it stays unstamped %s (the comment is a diagnostic aid: the backend is unaffected): %s", - tableName, treeName, transientFailure?"for now":"until the next start", stackTraceToSingleLineString(e))); + tableName, treeName, scope==FailureScope.TREE?"until this backend is closed":"for now", stackTraceToSingleLineString(e))); return CommentResult.FAILED; } } - // Whether a failed stamp says anything about the table it was meant to stamp: a lock timeout - // (the bound above), a connection that broke or any other transient driver failure is a - // property of the moment and worth trying again, while a statement the database rejected - an - // account that may not comment its tables, say - would be rejected again for every tree on - // every open. Only the latter is remembered. - static boolean isTransientFailure(Throwable failure, Dialect dialect) { - for (Throwable e=failure;e!=null;e=e.getCause()) { - if (e instanceof SQLTransientException) { - return true; + /** What a failed stamp says about stamping again - this tree, and the trees behind it in the same sweep. */ + enum FailureScope { + /** + * The database rejected the statement: an account that may not comment its tables, say. It + * would be rejected again for this tree on every open, so the tree is remembered and not + * asked again while this backend is open. Says nothing about the other trees of the sweep, + * which are stamped as usual - the privilege may well be missing for this one table alone. + */ + TREE, + /** + * Another session held the table locked and the stamp gave up on the bound above. Nothing + * is remembered - the next open tries again - and the sweep goes on: the lock belongs to + * this table, and the trees behind it are no more likely to be contended than usual. + */ + MOMENT, + /** + * The connection the sweep runs on is gone, or was never established. Every tree left in + * the sweep would run into the same thing, one connect attempt each, so the sweep ends; + * nothing is remembered, since this says nothing about any of the tables. + */ + SESSION + } + + // What a failed stamp says about trying again. Both chains of the failure are walked: a driver + // reports the vendor error of a rejected statement as the next exception of a generic one at + // least as often as it reports it as the cause, and reading only one of the two would classify + // a lock timeout as a rejection, which leaves the tree unstamped for the life of the backend + // over a moment of contention. + static FailureScope failureScope(Throwable failure, Dialect dialect) { + FailureScope scope=FailureScope.TREE; + final Deque pending=new ArrayDeque<>(); + final Set seen=Collections.newSetFromMap(new IdentityHashMap()); + if (failure!=null) { + pending.push(failure); + } + while (!pending.isEmpty()) { + final Throwable e=pending.pop(); + if (!seen.add(e)) { // a driver that chains an exception back to itself must not loop this walk + continue; + } + if (e.getCause()!=null) { + pending.push(e.getCause()); } if (!(e instanceof SQLException)) { continue; } final SQLException sqlException=(SQLException) e; - final String sqlState=sqlException.getSQLState(); - if (sqlState!=null && sqlState.startsWith("08")) { // connection exception - return true; + if (sqlException.getNextException()!=null) { + pending.push(sqlException.getNextException()); } - if (dialect==null) { // the failure came before the engine was known - continue; + final FailureScope found=scopeOf(sqlException, dialect); + if (found==FailureScope.SESSION) { // nothing further down either chain can weaken this one + return FailureScope.SESSION; } - switch (dialect) { - case POSTGRES: - if ("55P03".equals(sqlState)) { // lock not available: lock_timeout expired - return true; - } - break; - case MYSQL: - if (sqlException.getErrorCode()==1205) { // lock wait timeout exceeded - return true; - } - break; - case ORACLE: // ORA-00054 resource busy and acquire with NOWAIT specified or timeout expired, - if (sqlException.getErrorCode()==54 || sqlException.getErrorCode()==4021) { // ORA-04021 timeout occurred while waiting to lock object - return true; - } - break; - case MICROSOFT: - if (sqlException.getErrorCode()==1222) { // lock request time out period exceeded - return true; - } - break; - default: // a dialect with no lock timeout code of its own here: its failures are not treated as ones of the moment - break; + if (found==FailureScope.MOMENT) { + scope=FailureScope.MOMENT; } } - return false; + return scope; } - // Returns the comment currently stored on the table, or null when there is none. - // Only called for the dialects commentTable() recognizes. - String readStoredComment(Connection con, String tableName) throws SQLException { - final Dialect dialect=dialectOf(con); - if (dialect==null) { - throw new SQLException("no table comment readback for "+driverNameOf(con)); + // What one exception of the chain says on its own. + private static FailureScope scopeOf(SQLException e, Dialect dialect) { + final String sqlState=e.getSQLState(); + if (e instanceof SQLTransientConnectionException || e instanceof SQLNonTransientConnectionException + || e instanceof SQLRecoverableException // what oracle throws for a connection that has gone + || (sqlState!=null && sqlState.startsWith("08"))) { // connection exception + return FailureScope.SESSION; } + if (e instanceof SQLTimeoutException || e instanceof SQLTransientException) { + return FailureScope.MOMENT; + } + if (dialect==null) { // the failure came before the engine was known + return FailureScope.TREE; + } + switch (dialect) { + case POSTGRES: // 55P03 lock not available: lock_timeout expired + return "55P03".equals(sqlState) ? FailureScope.MOMENT : FailureScope.TREE; + case MYSQL: // 1205 lock wait timeout exceeded + return e.getErrorCode()==1205 ? FailureScope.MOMENT : FailureScope.TREE; + case ORACLE: // ORA-00054 resource busy, ORA-04021 timeout occurred while waiting to lock object + return e.getErrorCode()==54 || e.getErrorCode()==4021 ? FailureScope.MOMENT : FailureScope.TREE; + case MICROSOFT: // 1222 lock request time out period exceeded + return e.getErrorCode()==1222 ? FailureScope.MOMENT : FailureScope.TREE; + default: // a dialect with no lock timeout code of its own here: its failures are not treated as ones of the moment + return FailureScope.TREE; + } + } + + // Returns the comment currently stored on the table, or null when there is none. The dialect is + // passed in rather than read off the connection: this runs on the stamp connection, which is + // not a pooled one, and only for the dialects commentTable() recognizes. + String readStoredComment(Connection con, Dialect dialect, String tableName) throws SQLException { final String sql; final String arg; switch (dialect) { @@ -615,46 +669,52 @@ boolean updateTableStatistics(Connection con, Collection trees) { boolean allRefreshed=true; for (final TreeName treeName : trees) { final String tableName=getTableName(treeName); - final String sql; - final String[] args; - switch (dialect) { - case POSTGRES: - sql="analyze "+tableName; - args=NO_ARGS; - break; - case MYSQL: - sql="analyze table "+tableName; - args=NO_ARGS; - break; - case ORACLE: - sql="begin dbms_stats.gather_table_stats(user, ?); end;"; - args=new String[]{tableName.toUpperCase()}; - break; - case MICROSOFT: - sql="update statistics "+tableName; - args=NO_ARGS; - break; - default: // a dialect this switch was never told about must not inherit another one's statement - throw new IllegalStateException("no statistics refresh for dialect "+dialect); - } - try (final PreparedStatement statement=con.prepareStatement(sql)) { - statement.setQueryTimeout(timeoutSeconds); // 0: wait without limit - for (int i=0;i= 2, + "the login of " + dialect + " is not bounded in both phases: " + dialect.connectProperties); + for (final String name : dialect.connectProperties.stringPropertyNames()) { + assertTrue(Integer.parseInt(dialect.connectProperties.getProperty(name)) > 0, + name + " of " + dialect + " bounds nothing: " + dialect.connectProperties.getProperty(name)); + } + } + } + + /** + * The declaration above is worth nothing unless it reaches the driver, and nothing else in the + * suite notices if it stops doing so: dropping the properties from the connect call leaves + * every stamp unbounded again, which no round-trip test can see against a database that + * answers. + */ + @Test + public void testStampConnectionHandsItsBoundsToTheDriver() throws Exception { + final JDBCStorage storage = storageFor(ProbeDriver.URL); + for (final JDBCStorage.Dialect dialect : JDBCStorage.Dialect.values()) { + probeDriver.lastProperties = null; + storage.newStampConnection(dialect).close(); + final Properties handed = probeDriver.lastProperties; + assertNotNull(handed, "no properties were handed to the driver for " + dialect); + for (final Map.Entry bound : dialect.connectProperties.entrySet()) { + assertEquals(handed.getProperty((String) bound.getKey()), bound.getValue(), + bound.getKey() + " of " + dialect + " did not reach the driver"); + } + // a driver is free to write into the map it is passed: the declaration must not be it + assertNotSame(handed, dialect.connectProperties, + "the driver was handed the declaration of " + dialect + " rather than a copy of it"); + } + } + + /** + * What the bounds are for: a database that keeps its established connections alive but accepts + * no new ones - a moved vip, a proxy at its connection limit - usually completes the tcp + * connect and then goes quiet, which leaves the driver in a read. Unbounded, that hangs the + * open of a tree; dsconfig create-backend-index opens one on a running server. + *

+ * The four dialects are attempted at once, so the suite pays the bound of the slowest of them + * rather than the sum of all four. + */ + @Test + public void testEveryDriverGivesUpOnASilentServer() throws Exception { + try (final SilentServer silent = new SilentServer()) { + final List> attempts = new ArrayList<>(); + for (final JDBCStorage.Dialect dialect : JDBCStorage.Dialect.values()) { + final JDBCStorage storage = storageFor(silent.urlFor(dialect)); + attempts.add(new Callable() { + @Override + public String call() { + final long startedAt = System.nanoTime(); + try (final Connection con = storage.newStampConnection(dialect)) { + return dialect + " connected to a server that never answered"; + } catch (Exception expected) { + // the failure itself is the point: which one it is belongs to the driver + } + final long tookSeconds = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() - startedAt); + return tookSeconds <= GIVE_UP_CEILING_SECONDS ? null + : dialect + " took " + tookSeconds + "s to give up on a silent server"; + } + }); + } + final ExecutorService attempted = Executors.newFixedThreadPool(attempts.size()); + try { + final StringBuilder failures = new StringBuilder(); + for (final Future attempt : attempted.invokeAll(attempts, + GIVE_UP_CEILING_SECONDS * 2, TimeUnit.SECONDS)) { + final String failure = attempt.isCancelled() // the driver never came back at all + ? "a driver did not give up on a silent server within " + + (GIVE_UP_CEILING_SECONDS * 2) + "s" + : attempt.get(); + if (failure != null) { + failures.append(failure).append('\n'); + } + } + if (failures.length() > 0) { + fail(failures.toString()); + } + } finally { + attempted.shutdownNow(); + } + } + } + + /** + * A stamp that the database rejected is remembered, so that an account which may not comment + * its tables does not reissue the statement for every tree on every open. A stamp that lost + * its connection ends the sweep, since every tree behind it needs that same connection. A + * stamp that gave up on a lock is neither: the lock belongs to that one table. + */ + @Test + public void testFailureScopeTellsTheThreeApart() { + assertEquals(JDBCStorage.failureScope( + new SQLException("permission denied for table", "42501"), JDBCStorage.Dialect.POSTGRES), + JDBCStorage.FailureScope.TREE, "a rejected statement was not remembered"); + assertEquals(JDBCStorage.failureScope( + new SQLException("lock not available", "55P03"), JDBCStorage.Dialect.POSTGRES), + JDBCStorage.FailureScope.MOMENT, "a lock timeout was not read as one of the moment"); + assertEquals(JDBCStorage.failureScope( + new SQLException("connection closed", "08006"), JDBCStorage.Dialect.POSTGRES), + JDBCStorage.FailureScope.SESSION, "a connection exception did not end the sweep"); + assertEquals(JDBCStorage.failureScope( + new SQLNonTransientConnectionException("socket closed"), JDBCStorage.Dialect.MICROSOFT), + JDBCStorage.FailureScope.SESSION, "a connection exception of the driver did not end the sweep"); + assertEquals(JDBCStorage.failureScope( + new SQLTimeoutException("query timed out"), JDBCStorage.Dialect.MYSQL), + JDBCStorage.FailureScope.MOMENT, "a timeout was not read as a failure of the moment"); + } + + /** + * A driver reports the vendor error of a failed statement as the next exception of a generic + * one at least as often as it reports it as the cause. Reading only the cause chain classifies + * a lock timeout as a rejection, which leaves the tree unstamped until the next start over a + * moment of contention. + */ + @Test + public void testFailureScopeWalksBothChains() { + final SQLException reportedAsTheCause = new SQLException("statement failed", + new SQLException("lock wait timeout exceeded", "HY000", 1205)); + assertEquals(JDBCStorage.failureScope(reportedAsTheCause, JDBCStorage.Dialect.MYSQL), + JDBCStorage.FailureScope.MOMENT, "a lock timeout on the cause chain was missed"); + + final SQLException reportedAsTheNext = new SQLException("statement failed"); + reportedAsTheNext.setNextException(new SQLException("lock wait timeout exceeded", "HY000", 1205)); + assertEquals(JDBCStorage.failureScope(reportedAsTheNext, JDBCStorage.Dialect.MYSQL), + JDBCStorage.FailureScope.MOMENT, "a lock timeout on the next-exception chain was missed"); + + // a connection exception anywhere in either chain outweighs the rest: the session is gone + final SQLException connectionGone = new SQLException("statement failed"); + connectionGone.setNextException(new SQLException("communications link failure", "08S01")); + assertEquals(JDBCStorage.failureScope(connectionGone, JDBCStorage.Dialect.MYSQL), + JDBCStorage.FailureScope.SESSION, "a connection exception on the next-exception chain was missed"); + + // a driver that chains an exception back to itself must not make the walk loop + final SQLException selfReferring = new SQLException("statement failed"); + selfReferring.setNextException(selfReferring); + assertEquals(JDBCStorage.failureScope(selfReferring, JDBCStorage.Dialect.MYSQL), + JDBCStorage.FailureScope.TREE, "a self-referring chain was not walked to an end"); + } + + /** Accepts connections and answers nothing at all, the shape of a proxy at its connection limit. */ + private static final class SilentServer implements AutoCloseable { + private final ServerSocket listening; + private final List accepted = new ArrayList<>(); + private final Thread acceptor; + + SilentServer() throws IOException { + listening = new ServerSocket(0, 16, InetAddress.getLoopbackAddress()); + acceptor = new Thread(new Runnable() { + @Override + public void run() { + while (!Thread.currentThread().isInterrupted()) { + try { + final Socket socket = listening.accept(); + synchronized (accepted) { // held open, and never written to + accepted.add(socket); + } + } catch (IOException closed) { + return; + } + } + } + }, "silent-server"); + acceptor.setDaemon(true); + acceptor.start(); + } + + String urlFor(JDBCStorage.Dialect dialect) { + final String host = listening.getInetAddress().getHostAddress(); + final int port = listening.getLocalPort(); + switch (dialect) { + case POSTGRES: + return "jdbc:postgresql://" + host + ":" + port + "/probe?user=probe&password=probe"; + case MYSQL: + return "jdbc:mysql://" + host + ":" + port + "/probe?user=probe&password=probe"; + case ORACLE: + return "jdbc:oracle:thin:probe/probe@//" + host + ":" + port + "/probe"; + case MICROSOFT: + return "jdbc:sqlserver://" + host + ":" + port + ";databaseName=probe;user=probe;password=probe"; + default: + throw new IllegalStateException("no probe url for dialect " + dialect); + } + } + + @Override + public void close() throws IOException { + acceptor.interrupt(); + listening.close(); + synchronized (accepted) { + for (final Socket socket : accepted) { + try { + socket.close(); + } catch (IOException ignored) { + } + } + } + } + } + + /** Records the properties a stamp connection hands its driver, and connects to nothing. */ + private static final class ProbeDriver implements Driver { + static final String URL = "jdbc:stampprobe:"; + + volatile Properties lastProperties; + + @Override + public Connection connect(String url, Properties info) throws SQLException { + if (!acceptsURL(url)) { + return null; // not ours: DriverManager goes on to the next driver + } + lastProperties = info; + final Connection con = mock(Connection.class); + when(con.createStatement()).thenReturn(mock(Statement.class)); + return con; + } + + @Override + public boolean acceptsURL(String url) { + return url != null && url.startsWith(URL); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) { + return new DriverPropertyInfo[0]; + } + + @Override + public int getMajorVersion() { + return 1; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public boolean jdbcCompliant() { + return false; + } + + @Override + public java.util.logging.Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } +} diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index a13f47582f..76760c1882 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java @@ -51,6 +51,7 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; @@ -474,9 +475,9 @@ public void run(WriteableTransaction txn) throws Exception { }); assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); // UP_TO_DATE and not FAILED: the statement was skipped, not rejected - assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.UP_TO_DATE, "an up-to-date comment was re-stamped"); + assertEquals(storage.commentTable(tree, dialect()), JDBCStorage.CommentResult.UP_TO_DATE, "an up-to-date comment was re-stamped"); writeTableComment(storage.getTableName(tree), "stale"); - assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.STAMPED, "a stale comment was not re-stamped"); + assertEquals(storage.commentTable(tree, dialect()), JDBCStorage.CommentResult.STAMPED, "a stale comment was not re-stamped"); assertEquals(readTableComment(storage.getTableName(tree)), tree.toString()); } finally { try { @@ -523,7 +524,7 @@ public void run(WriteableTransaction txn) throws Exception { } // the row is left uncommitted, so the lock it holds is still there final long start = System.currentTimeMillis(); - final JDBCStorage.CommentResult result = storage.commentTable(tree); + final JDBCStorage.CommentResult result = storage.commentTable(tree, dialect()); final long elapsedMs = System.currentTimeMillis() - start; blocker.rollback(); // giving up and stamping anyway are both fine here - the engines differ in whether an @@ -577,14 +578,10 @@ public void run(WriteableTransaction txn) throws Exception { } final AtomicInteger stampAttempts = new AtomicInteger(); final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { - @Override - String readStoredComment(Connection con, String tableName) throws SQLException { - return null; // pretend the table carries no comment, so a stamp is attempted - } @Override Connection newStampConnection(Dialect dialect) throws SQLException { stampAttempts.incrementAndGet(); - throw new SQLException("injected comment failure"); // no sql state, no vendor code: not a failure of the moment + throw new SQLException("injected comment failure"); // no sql state, no vendor code: a rejection, not a failure of the moment } }; try { @@ -603,8 +600,8 @@ public Void run(ReadableTransaction txn) throws Exception { return null; } }); - // the failure is remembered: an unstampable table is not asked again in this JVM - assertEquals(storage.commentTable(stamped), JDBCStorage.CommentResult.FAILED); + // the failure is remembered: an unstampable table is not asked again while this backend is open + assertEquals(storage.commentTable(stamped, dialect()), JDBCStorage.CommentResult.FAILED); assertEquals(stampAttempts.get(), 1, "a failed stamp was reissued"); } finally { try { @@ -631,10 +628,6 @@ public void testTransientStampFailureIsRetried() throws Exception { final TreeName tree = new TreeName("o=transientStamp", "dn2id"); final AtomicInteger stampAttempts = new AtomicInteger(); final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { - @Override - String readStoredComment(Connection con, String tableName) throws SQLException { - return null; // pretend the table carries no comment, so a stamp is attempted - } @Override Connection newStampConnection(Dialect dialect) throws SQLException { stampAttempts.incrementAndGet(); @@ -649,7 +642,7 @@ public void run(WriteableTransaction txn) throws Exception { txn.openTree(tree, true); // stamp #1, fails } }); - assertEquals(storage.commentTable(tree), JDBCStorage.CommentResult.FAILED); + assertEquals(storage.commentTable(tree, dialect()), JDBCStorage.CommentResult.FAILED); assertEquals(stampAttempts.get(), 2, "a stamp that failed for a reason of the moment was not attempted again"); } finally { try { @@ -666,8 +659,10 @@ public void run(WriteableTransaction txn) throws Exception { /** * Opening a backend opens every tree it holds - about 25 for a stock suffix - and the first - * open after an upgrade stamps them all: the stamps must share one connection rather than - * make a physical connect each. An open that finds the comments in place must make none. + * open after an upgrade stamps them all: the trees of one open must share one connection + * rather than make a physical connect each. One per open is what the comment machinery costs, + * readback included - the readback runs on that same connection, because the thread doing the + * open is inside a transaction and holding a pooled connection already. */ @Test public void testCommentStampsShareOneConnection() throws Exception { @@ -705,7 +700,9 @@ public void run(WriteableTransaction txn) throws Exception { } } }); - assertEquals(connects.get(), 1, "an open that found every comment in place still connected"); + // three trees, one more connect: the open that finds every comment in place issues no + // statement and takes no lock, and pays one connection for the whole sweep either way + assertEquals(connects.get(), 2, "the trees of an open that found every comment in place did not share a connection"); } finally { try { storage.write(new WriteOperation() { @@ -742,7 +739,7 @@ public void testLockBoundSurvivesAFailedStamp() throws Exception { bound = sessionLockBound(fresh); // what a connection carrying the bound reports } try (final JDBCStorage.StampSession session = storage.new StampSession()) { - assertEquals(storage.commentTable(missing, session), JDBCStorage.CommentResult.FAILED, + assertEquals(storage.commentTable(missing, dialect, session), JDBCStorage.CommentResult.FAILED, "stamping a table that does not exist was reported as done"); if (bound != null) { // oracle: ddl_lock_timeout is only in v$parameter, which the test user cannot read assertEquals(sessionLockBound(session.connection(dialect)), bound, @@ -755,28 +752,23 @@ public void testLockBoundSurvivesAFailedStamp() throws Exception { } /** - * A stamp failure of the moment - a connect that did not go through, a lock the statement - * gave up on - says nothing about one table: every remaining tree of the same open would pay - * the same bound, or the same connect attempt, again. That is about 25 of them for a stock - * suffix, all for a diagnostic aid, so the sweep gives up after the first - and since nothing - * is remembered, the next open tries again. + * A stamp that lost its connection ends the sweep it happened in: every tree behind it needs + * that same connection, so each would pay the same connect attempt again. That is about 25 of + * them for a stock suffix, all for a diagnostic aid. Nothing is remembered, so the next open + * tries again. */ @Test - public void testTransientStampFailureEndsTheSweep() throws Exception { + public void testConnectionFailureEndsTheSweep() throws Exception { final TreeName[] trees = { new TreeName("o=sweepGiveUp", "dn2id"), new TreeName("o=sweepGiveUp", "id2entry"), new TreeName("o=sweepGiveUp", "state") }; final AtomicInteger stampAttempts = new AtomicInteger(); final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { - @Override - String readStoredComment(Connection con, String tableName) throws SQLException { - return null; // pretend no table carries a comment, so every one of them is stamped - } @Override Connection newStampConnection(Dialect dialect) throws SQLException { stampAttempts.incrementAndGet(); - throw new SQLException("injected connection failure", "08006"); // connection exception: a failure of the moment + throw new SQLException("injected connection failure", "08006"); // connection exception: the session is gone } }; try { @@ -789,7 +781,7 @@ public void run(WriteableTransaction txn) throws Exception { } } }); - assertEquals(stampAttempts.get(), 1, "a failure of the moment was paid once per tree of the same open"); + assertEquals(stampAttempts.get(), 1, "a connection that was gone was paid once per tree of the same open"); storage.write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { @@ -798,7 +790,7 @@ public void run(WriteableTransaction txn) throws Exception { } } }); - assertEquals(stampAttempts.get(), 2, "the open after a failure of the moment did not try again"); + assertEquals(stampAttempts.get(), 2, "the open after a lost connection did not try again"); } finally { try { storage.write(new WriteOperation() { @@ -814,6 +806,90 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * A lock belongs to the table it is held on, so a stamp that gave up on one must not cost the + * trees behind it their comments: the trees of an open are stamped in a fixed order, and a + * table left permanently contended by another session would otherwise mean nothing is ever + * stamped, on any open. Nothing is remembered either - the open that follows stamps the table + * whose moment has passed. + *

+ * The failure is injected at the readback rather than at the comment statement, which is built + * inline; what is under test is the classification of the failure and what the sweep does with + * it, and those do not depend on which of the two statements produced it. + */ + @Test + public void testContendedTableDoesNotEndTheSweep() throws Exception { + final TreeName[] trees = { + new TreeName("o=sweepContended", "dn2id"), + new TreeName("o=sweepContended", "id2entry"), + new TreeName("o=sweepContended", "state") }; + final AtomicInteger contended = new AtomicInteger(1); // the first tree, for one sweep only + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null) { + @Override + String readStoredComment(Connection con, Dialect dialect, String tableName) throws SQLException { + if (tableName.equals(getTableName(trees[0])) && contended.getAndDecrement() > 0) { + throw lockTimeoutOf(dialect); // as if another session held this one table + } + return super.readStoredComment(con, dialect, tableName); + } + }; + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); + } + } + }); + assertNotEquals(readTableComment(storage.getTableName(trees[0])), trees[0].toString(), + "the contended table was stamped anyway"); + for (int i = 1; i < trees.length; i++) { + assertEquals(readTableComment(storage.getTableName(trees[i])), trees[i].toString(), + "one contended table cost the trees behind it their comments"); + } + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.openTree(tree, true); + } + } + }); + assertEquals(readTableComment(storage.getTableName(trees[0])), trees[0].toString(), + "a table left unstamped by a contended moment was not stamped by the open that followed"); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + for (final TreeName tree : trees) { + txn.deleteTree(tree); + } + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** The failure a dialect reports when a statement gave up on the lock bound it was given. */ + static SQLException lockTimeoutOf(JDBCStorage.Dialect dialect) { + switch (dialect) { + case POSTGRES: + return new SQLException("canceling statement due to lock timeout", "55P03"); + case MYSQL: + return new SQLException("Lock wait timeout exceeded; try restarting transaction", "HY000", 1205); + case ORACLE: + return new SQLException("ORA-00054: resource busy and acquire with NOWAIT specified", "61000", 54); + case MICROSOFT: + return new SQLException("Lock request time out period exceeded", "HY000", 1222); + default: + throw new IllegalStateException("no lock timeout failure for dialect " + dialect); + } + } + /** * @@sql_mode decides whether a backslash escapes inside the comment literal. It belongs to * the session, and the stamps of one open share a connection, so it is asked once for the @@ -864,25 +940,9 @@ public void run(WriteableTransaction txn) throws Exception { } } - /** - * Every dialect must bound the whole login attempt of a stamp connection, not only its socket - * connect: postgres and mysql apply their connect property to socket.connect() alone, and - * oracle's CONNECT_TIMEOUT excludes authentication, so those three carry a second property - * for the reads of tls and authentication, while the sql server loginTimeout covers the phase - * on its own. A server that accepts a connection and then goes quiet cannot be staged in a - * container, so this guards the declaration rather than the behaviour. - */ - @Test - public void testEveryDialectBoundsItsLoginAttempt() { - for (final JDBCStorage.Dialect dialect : JDBCStorage.Dialect.values()) { - assertEquals(dialect.connectProperties.size(), dialect == JDBCStorage.Dialect.MICROSOFT ? 1 : 2, - "connect bounds of " + dialect + ": " + dialect.connectProperties); - for (final String name : dialect.connectProperties.stringPropertyNames()) { - assertTrue(Integer.parseInt(dialect.connectProperties.getProperty(name)) > 0, - name + " of " + dialect + " bounds nothing: " + dialect.connectProperties.getProperty(name)); - } - } - } + // The bounds of a stamp connection are covered by StampConnectionTestCase: what they are worth + // is whether they reach the driver and whether the driver then gives up on a server that never + // answers, and neither needs - nor can be staged by - a database container. /** * An import that failed or was cancelled leaves trees holding an incomplete import that is