From 10cb232980bba22c5edb056f4ffdcceea5c63b63 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 13 Aug 2026 13:03:47 +0300 Subject: [PATCH 1/3] 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 fcf3737df4..0fa3f68386 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()); } @@ -134,6 +142,75 @@ String getTableName(TreeName treeName) { return tree2table.get(treeName); } + // 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(); @@ -335,6 +412,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); } } @@ -695,6 +773,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 472f62fc9a..2379e0fe96 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; @@ -269,6 +270,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 90b93abd7bd7797cef5b8c2a998b562abaa1ce1a Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 13 Aug 2026 16:13:49 +0300 Subject: [PATCH 2/3] 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 0fa3f68386..86006ba8bb 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 @@ -142,62 +142,150 @@ String getTableName(TreeName treeName) { return tree2table.get(treeName); } + 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/3] 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 86006ba8bb..95a9112e6b 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; @@ -148,50 +149,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)) { @@ -201,18 +220,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; @@ -229,7 +247,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); @@ -242,9 +260,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"); @@ -254,7 +274,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; @@ -328,6 +348,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(); @@ -500,7 +524,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); } } @@ -838,9 +862,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) { @@ -862,25 +888,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 292e2c2bf0..65a1ed0af5 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; @@ -323,6 +324,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 { @@ -345,7 +476,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 {