[#1012] Pin the commit of the post-import statistics refresh, and say why it is there

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

@@ -2160,6 +2160,15 @@ boolean updateTableStatistics(Connection con, Collection<TreeName> trees) {
 						}
 						return null;
 					});
+					// Committed here, per tree, rather than left to the caller: the connection of an import
+					// goes back to the pool as soon as this refresh is over, and CachedConnection.close()
+					// rolls back before the pool hands it on - so a refresh left inside the transaction of
+					// the borrow would be discarded by its own return, on the one engine where the database
+					// does not commit it for us. On postgres the rows ANALYZE writes to pg_statistic are
+					// ordinary catalog rows, and a rollback takes with them the histogram the "where k>?
+					// order by k" batches of #859 need, leaving only the pg_class.reltuples it writes in
+					// place; mysql (implicit commit), oracle (dbms_stats commits) and sql server commit the
+					// statement themselves, which is why no container suite pins this (issue #1012).
 					con.commit();
 				}
 			}catch (Exception e) {

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java

@@ -46,6 +46,7 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
 import static org.forgerock.opendj.config.ConfigurationMock.mockCfg;
 import static org.mockito.Mockito.any;
@@ -1215,6 +1216,43 @@ public void testTheStatisticsRefreshRunsUnderItsOwnBoundAndTheBackstop() throws
 		inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq(0));
 	}
 
+	/**
+	 * Every statistics statement is committed where it runs, one commit per tree. The connection of
+	 * an import goes back to the pool as soon as the refresh is over and
+	 * {@code CachedConnection.close()} rolls back before the pool hands it on, so a refresh left
+	 * inside the transaction of the borrow would be discarded by its own return - on the one engine
+	 * where the database does not commit it for us. On postgres the per-column rows ANALYZE writes to
+	 * {@code pg_statistic} are ordinary catalog rows and go with a rollback, taking the histogram the
+	 * {@code where k>? order by k} batches of #859 need and leaving only the {@code pg_class.reltuples}
+	 * that is written in place; mysql commits {@code ANALYZE TABLE} implicitly, {@code dbms_stats}
+	 * commits of its own, and sql server does not roll its statistics update back either - which is
+	 * why no container suite pins this, and why it is pinned here (issue #1012).
+	 */
+	@Test
+	public void testEveryStatisticsStatementIsCommittedWhereItRuns() throws Exception {
+		final PreparedStatement statement = mock(PreparedStatement.class);
+		// postgres rather than the oracle stand-in above: it is the engine whose statement this commit
+		// is the only thing making durable
+		final Connection con = mock(postgresConnection.class); // the dialect is read off the connection
+		when(con.getNetworkTimeout()).thenReturn(0);
+		when(con.prepareStatement(anyString())).thenReturn(statement);
+
+		assertTrue(storage.updateTableStatistics(con, asList(
+			new TreeName("dc=example,dc=com", "id2entry"), new TreeName("dc=example,dc=com", "dn2id"))));
+
+		// Behind each statement and not once at the end of the loop: a single commit there would leave
+		// every tree but the last of a rebuild-index import inside the transaction of the borrow.
+		final InOrder perTree = inOrder(con, statement);
+		perTree.verify(statement).execute();
+		perTree.verify(con).commit();
+		perTree.verify(statement).execute();
+		perTree.verify(con).commit();
+		verify(con, times(2)).commit();
+		// the rollback of this loop belongs to its failure path: a refresh that went through must not
+		// end by throwing away what it just committed
+		verify(con, never()).rollback();
+	}
+
 	/**
 	 * A connection whose driver refuses the call mid-flight is given back what it carried before.
 	 * The entry holding that value is dropped as soon as the last statement on the connection is
@@ -1433,4 +1471,7 @@ storage.new ReadableTransactionImpl(new CachedConnection("jdbc:mock", parent))
 	// this interface is an oracle connection as far as the storage is concerned - which is the
 	// whole reason for the lower case name here.
 	private interface oracleConnection extends Connection {}
+
+	/** The same trick for the engine whose statistics statement a rollback really would discard. */
+	private interface postgresConnection extends Connection {}
 }

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java

@@ -1734,8 +1734,17 @@ 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 + "'";
+			// The per-column rows of pg_statistic rather than pg_class.reltuples, which is the half of
+			// what ANALYZE writes that cannot pin the refresh: reltuples is overwritten in place
+			// (vac_update_relstats(): "We violate transaction semantics here") and so survives the
+			// rollback of the connection's return, while these rows are ordinary catalog rows and do
+			// not - which is what makes the commit of updateTableStatistics() load-bearing on this
+			// engine (issue #1012). Strictly stronger than reltuples was: the count is 0 both for a
+			// table that was never analyzed and for one whose refresh was rolled back, and the row of
+			// pg_class still carries the query, so a table that is not there is still reported as
+			// "not found" rather than as stale statistics.
+			sql = "select (select count(*) from pg_statistic s where s.starelid=c.oid)"
+				+ " from pg_class c where c.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() + "'";