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

[vharseko]

Fixes #1012.

The issue as filed is not a defect

The four-step order in #1012 is the order of ImporterImpl.close(), and the statistics are not one step of it: updateTableStatistics() issues one statement per tree and commits each of them on the spot (JDBCStorage.java:2163), rolling back only on the failure path and logging what failed. By the time release() reaches con.close() — which does roll back, CachedConnection.java:2211 — every statistics statement of the import is already durable. The commit has been on that line since the feature's first commit, 3800973 (#866); #940 moves the call to describing.con under that connection's monitor and does not touch the method, so the same holds there.

The full analysis, including the per-engine measurements that answer the question #1012 asks first, is in a comment below.

What changed

Nothing of the behaviour. What the issue did turn up is that the invariant is pinned nowhere, on any engine, and that the read-back of the suite could not have caught it on the one engine where it bites:

• JDBCStatementBoundTestCase.testEveryStatisticsStatementIsCommittedWhereItRuns — two trees against a mock connection of the postgres dialect, InOrder: execute(), commit(), execute(), commit(), with times(2) on the commit and never() on the rollback. Behind each statement rather than 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. Needs no database.
• TestCase.assertTableStatisticsFresh() reads the per-column rows of pg_statistic on postgres instead of pg_class.reltuples. reltuples is overwritten in place — vac_update_relstats(): "We violate transaction semantics here" — so it survives the rollback of the connection's return and can pin nothing; the pg_statistic rows are ordinary catalog rows and do not survive it. Strictly stronger than what it replaces: the count is 0 both for a table that was never analyzed and for one whose refresh was rolled back, and the row still comes from pg_class, so a table that is not there is still reported as "not found" rather than as stale statistics.
• A comment at JDBCStorage.java:2163 saying why the commit is there, the way readStoredComment() already says it for the stamp path. Reading the order of close() without it is what produced JDBC: post-import table statistics may be rolled back by the connection's return #1012.

Verification

Mutation = con.commit() taken out of JDBCStorage.java:2163.

run	result
JDBCStatementBoundTestCase	45 tests, 0 failures
the same, mutated	exactly the new case fails — VerificationInOrderFailure: Wanted but not invoked: postgresConnection.commit() — and the other 44 stay green
PgSqlTestCase	79 tests, 0 failures, 0 skipped
the same, mutated	exactly testImportRefreshesTableStatistics fails — statistics of opendj_8e0159e8... look stale: 0 — where the reltuples read it replaces passes

MySqlTestCase, MsSqlTestCase and OracleTestCase are untouched by the change — the read-back it edits is the postgres branch of that helper — and were not run here: this machine has 8 GB and three other worktrees were building on it. The sql server behaviour the analysis reports was measured against the suite's own image with a hand probe instead.

[vharseko]

Not a defect as described — the refresh commits where it runs. Line numbers below are master at 36d4af9.

The four-step order in #1012 is the order of ImporterImpl.close(), and the statistics are not one step of it: updateTableStatistics() issues one statement per tree and commits each of them on the spot.

// JDBCStorage.java:2146
bounded(con, STATISTICS_TIMEOUT_PROPERTY, timeoutSeconds, cancelArmed, () -> { ... });
con.commit();                       // :2163 - unconditional, one per tree
} catch (Exception e) {
    try { con.rollback(); } ...     // :2167 - the failure path only
    allRefreshed=false;
    logger.warn(...);               // :2170 - and the operator is told

So step 4 has nothing to discard: by the time release() reaches con.close() — which does roll back, CachedConnection.java:2211 — every statistics statement of the import is already durable. This is not new either: the commit has been on that line since the feature's first commit, 3800973 (#866), and #882/#885/#888 left it alone. In #940 the call moves to describing.con under that connection's monitor and the method is unchanged, so the same per-statement commit applies there.

Worth adding that the order is the opposite of a hazard: the data of the import is committed first (:4733), then the statistics run (:4737). On sql server a failing statement of this kind rolls the whole transaction back — the reason commentTable() runs on a connection of its own — and here there is nothing of the import left in the transaction for it to take.

The open question, answered — measured on each engine

postgres (postgres:latest, 18.6, the image PgSqlTestCase uses), one session, a table of the shape JDBCStorage.getTableDialect() creates (h char(128),k bytea,v bytea), autovacuum_enabled=false so that nothing but the analyze of the probe writes anything, 40 rows as in the test:

	pg_statistic rows	pg_class.reltuples
before analyze	0	-1
inside the transaction, after analyze	3	40
after rollback	0	40 (survives)
after commit	3	40

analyze is therefore partly transactional, and the halves are documented in the source: update_attstats() writes the per-column rows of pg_statistic through CatalogTupleInsertWithInfo/UpdateWithInfo, ordinary MVCC catalog rows, while vac_update_relstats() overwrites the pg_class row in place — "We violate transaction semantics here by overwriting the rel's existing pg_class tuple with the new values".

sql server (mcr.microsoft.com/mssql/server:2019-CU30-ubuntu-20.04, 15.0.4415.2, the image MsSqlTestCase uses). The product documentation says nothing about an enclosing transaction, so measured: in one transaction, an insert of a marker row, then update statistics, then rollback.

@@trancount after update statistics	1 — the statement does not commit the caller's transaction
rows after the rollback	40 — the marker row inside that transaction is gone
statistics with last_updated after the rollback	1 — the refresh survived

So update statistics commits independently of the enclosing transaction and cannot be rolled back, while the transaction itself goes on; nothing of the import's refresh is at risk here.

mysql — ANALYZE TABLE is on the implicit-commit list (8.4 manual).
oracle — the DBMS_STATS gather procedures commit before and after the work, for anything but a transaction-specific temporary table (21c ARPLS).

Postgres alone would have lost anything had the commit been missing — and precisely the part the feature exists for.

What is broken: the commit is pinned by nothing

Walking the same close() sequence by hand — commit of the data, analyze, commit, then the rollback of the connection's return — and reading it back from a second session, the way TestCase.assertTableStatisticsFresh() does:

sequence	pg_statistic	pg_stats	reltuples	assertTableStatisticsFresh
as the code runs it (commit at :2163)	3	3	40	passes
with that commit taken out (the defect as filed)	0	0	40	passes

TestCase.java:1738 asserts pg_class.reltuples — the half that is written in place and survives the rollback. Delete con.commit() from :2163 and PgSqlTestCase.testImportRefreshesTableStatistics stays green; on mysql, oracle and sql server it stays green because the database commits for us. The invariant is pinned on no engine at all.

What the green suite would be hiding, same 40 rows in both tables, the cursor-batch query of #859:

with the committed statistics:  Seq Scan on probe_with_commit     rows=36  width=202
with them rolled back:          Seq Scan on probe_without_commit  rows=13  width=580

35 rows actually match. rows=13 is 40 * 0.3333, the default selectivity of an inequality with no histogram, and width drifts too because the average column width lives in pg_statistic — a 2.8x underestimate of the batch, which is the shape of #859.

What this PR does with that

Nothing of the behaviour — the three items above the fold are all that was left to do, and the diff is the first two plus a comment:

1. Pin the order with no database at all. JDBCStatementBoundTestCase already drove updateTableStatistics() against a mock connection with an InOrder (:1202), so "one execute(), then one commit(), per tree" belongs there.
2. Read the postgres statistics back where a rollback would show — pg_statistic instead of reltuples.
3. Say why the commit is there, the way readStoredComment() (:2033) already says it for the stamp path: "CachedConnection.close() rolls back before the connection is handed on". One line at :2163, and the order in close() no longer reads like the defect this issue describes.

Both tests were watched failing against the commit taken out — the runs are in the description.

If the preference is to close #1012 as not-a-defect and leave the hardening out, say so and I will drop this; the analysis above stands either way.