On the 1.9.x-rep-cache-db-fixes branch: Merge r1663286, r1741071,
r1741072, r1741073, r1741078, r1741206 from trunk (without conflicts).
r1663286: Tweak error trace creation in svn_sqlite__finish_savepoint().
r1741071: Add a failing test for an issue with rep-cache.db.
r1741072: Add a failing test that exploits a problem in one of our sqlite
helpers.
r1741073: Fix a potential crash in a couple of our sqlite helpers.
r1741078: Lay the groundwork for fixing the commit_with_locked_rep_cache()
test by factoring out a helper function.
r1741206: Explicitly close the rep-cache.db connection if we fail to run
the preparatory work, e.g., after errors when creating the schema.
git-svn-id: https://svn.apache.org/repos/asf/subversion/branches/1.9.x-rep-cache-db-fixes@1743186 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/subversion/libsvn_fs_fs/rep-cache.c b/subversion/libsvn_fs_fs/rep-cache.c
index c2b4280..a20d85c 100644
--- a/subversion/libsvn_fs_fs/rep-cache.c
+++ b/subversion/libsvn_fs_fs/rep-cache.c
@@ -50,6 +50,13 @@
return svn_dirent_join(fs_path, REP_CACHE_DB_NAME, result_pool);
}
+#define SVN_ERR_CLOSE(x, db) do \
+{ \
+ svn_error_t *svn__err = (x); \
+ if (svn__err) \
+ return svn_error_compose_create(svn__err, svn_sqlite__close(db)); \
+} while (0)
+
/** Library-private API's. **/
@@ -99,12 +106,12 @@
0, NULL, 0,
fs->pool, pool));
- SVN_ERR(svn_sqlite__read_schema_version(&version, sdb, pool));
+ SVN_ERR_CLOSE(svn_sqlite__read_schema_version(&version, sdb, pool), sdb);
if (version < REP_CACHE_SCHEMA_FORMAT)
{
/* Must be 0 -- an uninitialized (no schema) database. Create
the schema. Results in schema version of 1. */
- SVN_ERR(svn_sqlite__exec_statements(sdb, STMT_CREATE_SCHEMA));
+ SVN_ERR_CLOSE(svn_sqlite__exec_statements(sdb, STMT_CREATE_SCHEMA), sdb);
}
/* This is used as a flag that the database is available so don't
diff --git a/subversion/libsvn_subr/sqlite.c b/subversion/libsvn_subr/sqlite.c
index 18d1c49..4124edd 100644
--- a/subversion/libsvn_subr/sqlite.c
+++ b/subversion/libsvn_subr/sqlite.c
@@ -1261,6 +1261,48 @@
return err;
}
+static svn_error_t *
+rollback_transaction(svn_sqlite__db_t *db,
+ svn_error_t *error_to_wrap)
+{
+ svn_sqlite__stmt_t *stmt;
+ svn_error_t *err;
+
+ err = get_internal_statement(&stmt, db, STMT_INTERNAL_ROLLBACK_TRANSACTION);
+ if (!err)
+ {
+ err = svn_error_trace(svn_sqlite__step_done(stmt));
+
+ if (err && err->apr_err == SVN_ERR_SQLITE_BUSY)
+ {
+ /* ### Houston, we have a problem!
+
+ We are trying to rollback but we can't because some
+ statements are still busy. This leaves the database
+ unusable for future transactions as the current transaction
+ is still open.
+
+ As we are returning the actual error as the most relevant
+ error in the chain, our caller might assume that it can
+ retry/compensate on this error (e.g. SVN_WC_LOCKED), while
+ in fact the SQLite database is unusable until the statements
+ started within this transaction are reset and the transaction
+ aborted.
+
+ We try to compensate by resetting all prepared but unreset
+ statements; but we leave the busy error in the chain anyway to
+ help diagnosing the original error and help in finding where
+ a reset statement is missing. */
+ err = svn_error_trace(reset_all_statements(db, err));
+ err = svn_error_compose_create(
+ svn_error_trace(svn_sqlite__step_done(stmt)),
+ err);
+ }
+ }
+
+ return svn_error_compose_create(error_to_wrap, err);
+}
+
svn_error_t *
svn_sqlite__begin_transaction(svn_sqlite__db_t *db)
{
@@ -1303,42 +1345,7 @@
/* Commit or rollback the sqlite transaction. */
if (err)
{
- svn_error_t *err2;
-
- err2 = get_internal_statement(&stmt, db,
- STMT_INTERNAL_ROLLBACK_TRANSACTION);
- if (!err2)
- err2 = svn_sqlite__step_done(stmt);
-
- if (err2 && err2->apr_err == SVN_ERR_SQLITE_BUSY)
- {
- /* ### Houston, we have a problem!
-
- We are trying to rollback but we can't because some
- statements are still busy. This leaves the database
- unusable for future transactions as the current transaction
- is still open.
-
- As we are returning the actual error as the most relevant
- error in the chain, our caller might assume that it can
- retry/compensate on this error (e.g. SVN_WC_LOCKED), while
- in fact the SQLite database is unusable until the statements
- started within this transaction are reset and the transaction
- aborted.
-
- We try to compensate by resetting all prepared but unreset
- statements; but we leave the busy error in the chain anyway to
- help diagnosing the original error and help in finding where
- a reset statement is missing. */
-
- err2 = reset_all_statements(db, err2);
- err2 = svn_error_compose_create(
- svn_sqlite__step_done(stmt),
- err2);
- }
-
- return svn_error_compose_create(err,
- err2);
+ return svn_error_trace(rollback_transaction(db, err));
}
SVN_ERR(get_internal_statement(&stmt, db, STMT_INTERNAL_COMMIT_TRANSACTION));
@@ -1359,18 +1366,22 @@
STMT_INTERNAL_ROLLBACK_TO_SAVEPOINT_SVN);
if (!err2)
- err2 = svn_sqlite__step_done(stmt);
-
- if (err2 && err2->apr_err == SVN_ERR_SQLITE_BUSY)
{
- /* Ok, we have a major problem. Some statement is still open, which
- makes it impossible to release this savepoint.
+ err2 = svn_error_trace(svn_sqlite__step_done(stmt));
- ### See huge comment in svn_sqlite__finish_transaction for
- further details */
+ if (err2 && err2->apr_err == SVN_ERR_SQLITE_BUSY)
+ {
+ /* Ok, we have a major problem. Some statement is still open,
+ which makes it impossible to release this savepoint.
- err2 = reset_all_statements(db, err2);
- err2 = svn_error_compose_create(svn_sqlite__step_done(stmt), err2);
+ ### See huge comment in svn_sqlite__finish_transaction for
+ further details */
+
+ err2 = svn_error_trace(reset_all_statements(db, err2));
+ err2 = svn_error_compose_create(
+ svn_error_trace(svn_sqlite__step_done(stmt)),
+ err2);
+ }
}
err = svn_error_compose_create(err, err2);
@@ -1378,9 +1389,9 @@
STMT_INTERNAL_RELEASE_SAVEPOINT_SVN);
if (!err2)
- err2 = svn_sqlite__step_done(stmt);
+ err2 = svn_error_trace(svn_sqlite__step_done(stmt));
- return svn_error_trace(svn_error_compose_create(err, err2));
+ return svn_error_compose_create(err, err2);
}
SVN_ERR(get_internal_statement(&stmt, db,
diff --git a/subversion/tests/libsvn_fs/fs-test.c b/subversion/tests/libsvn_fs/fs-test.c
index 835b3d2..36d3cfc 100644
--- a/subversion/tests/libsvn_fs/fs-test.c
+++ b/subversion/tests/libsvn_fs/fs-test.c
@@ -42,6 +42,7 @@
#include "private/svn_fs_util.h"
#include "private/svn_fs_private.h"
#include "private/svn_fspath.h"
+#include "private/svn_sqlite.h"
#include "../svn_test_fs.h"
@@ -7036,6 +7037,75 @@
return SVN_NO_ERROR;
}
+static svn_error_t *
+commit_with_locked_rep_cache(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *txn_root;
+ svn_revnum_t new_rev;
+ svn_sqlite__db_t *sdb;
+ svn_error_t *err;
+ const char *fs_path;
+ const char *statements[] = { "SELECT MAX(revision) FROM rep_cache", NULL };
+
+ if (strcmp(opts->fs_type, SVN_FS_TYPE_FSFS) != 0)
+ return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL,
+ "this will test FSFS repositories only");
+
+ if (opts->server_minor_version && (opts->server_minor_version < 6))
+ return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL,
+ "pre-1.6 SVN doesn't support FSFS rep-sharing");
+
+ fs_path = "test-repo-commit-with-locked-rep-cache";
+ SVN_ERR(svn_test__create_fs(&fs, fs_path, opts, pool));
+
+ /* r1: Add a file. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, 0, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_make_file(txn_root, "/foo", pool));
+ SVN_ERR(svn_test__set_file_contents(txn_root, "/foo", "a", pool));
+ SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
+ SVN_TEST_INT_ASSERT(new_rev, 1);
+
+ /* Begin a new transaction based on r1. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, 1, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_test__set_file_contents(txn_root, "/foo", "b", pool));
+
+ /* Obtain a shared lock on the rep-cache.db by starting a new read
+ * transaction. */
+ SVN_ERR(svn_sqlite__open(&sdb,
+ svn_dirent_join(fs_path, "rep-cache.db", pool),
+ svn_sqlite__mode_readonly, statements, 0, NULL,
+ 0, pool, pool));
+ SVN_ERR(svn_sqlite__begin_transaction(sdb));
+ SVN_ERR(svn_sqlite__exec_statements(sdb, 0));
+
+ /* Attempt to commit fs transaction. This should result in a commit
+ * post-processing error due to us still holding the shared lock on the
+ * rep-cache.db. */
+ err = svn_fs_commit_txn(NULL, &new_rev, txn, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_SQLITE_BUSY);
+ SVN_TEST_INT_ASSERT(new_rev, 2);
+
+ /* Release the shared lock. */
+ SVN_ERR(svn_sqlite__finish_transaction(sdb, SVN_NO_ERROR));
+ SVN_ERR(svn_sqlite__close(sdb));
+
+ /* Try an operation that reads from rep-cache.db.
+ *
+ * XFAIL: Around r1740802, this call was producing an error due to the
+ * svn_fs_t keeping an unusable db connection (and associated file
+ * locks) within it.
+ */
+ SVN_ERR(svn_fs_verify(fs_path, NULL, 0, SVN_INVALID_REVNUM, NULL, NULL,
+ NULL, NULL, pool));
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -7173,6 +7243,8 @@
"test svn_fs_check_related for transactions"),
SVN_TEST_OPTS_PASS(freeze_and_commit,
"freeze and commit"),
+ SVN_TEST_OPTS_XFAIL(commit_with_locked_rep_cache,
+ "test commit with locked rep-cache"),
SVN_TEST_NULL
};
diff --git a/subversion/tests/libsvn_subr/sqlite-test.c b/subversion/tests/libsvn_subr/sqlite-test.c
index c8313d3..a447693 100644
--- a/subversion/tests/libsvn_subr/sqlite-test.c
+++ b/subversion/tests/libsvn_subr/sqlite-test.c
@@ -26,8 +26,10 @@
static svn_error_t *
open_db(svn_sqlite__db_t **sdb,
+ const char **db_abspath_p,
const char *db_name,
const char *const *statements,
+ apr_int32_t timeout,
apr_pool_t *pool)
{
const char *db_dir, *db_abspath;
@@ -40,8 +42,10 @@
db_abspath = svn_dirent_join(db_dir, db_name, pool);
SVN_ERR(svn_sqlite__open(sdb, db_abspath, svn_sqlite__mode_rwcreate,
- statements, 0, NULL, 0, pool, pool));
+ statements, 0, NULL, timeout, pool, pool));
+ if (db_abspath_p)
+ *db_abspath_p = db_abspath;
return SVN_NO_ERROR;
}
@@ -83,7 +87,7 @@
NULL
};
- SVN_ERR(open_db(&sdb, "reset", statements, pool));
+ SVN_ERR(open_db(&sdb, NULL, "reset", statements, 0, pool));
SVN_ERR(svn_sqlite__create_scalar_function(sdb, "error_second",
1, FALSE /* deterministic */,
error_second, NULL));
@@ -113,6 +117,59 @@
return SVN_NO_ERROR;
}
+static svn_error_t *
+test_sqlite_txn_commit_busy(apr_pool_t *pool)
+{
+ svn_sqlite__db_t *sdb1;
+ svn_sqlite__db_t *sdb2;
+ const char *db_abspath;
+ svn_error_t *err;
+
+ static const char *const statements[] = {
+ "CREATE TABLE test (one TEXT NOT NULL PRIMARY KEY)",
+
+ "INSERT INTO test(one) VALUES ('foo')",
+
+ "SELECT one from test",
+
+ NULL
+ };
+
+ /* Open two db connections.
+
+ Use a small busy_timeout of 250ms, since we're about to receive an
+ SVN_ERR_SQLITE_BUSY error, and retrying for the default 10 seconds
+ would be a waste of time. */
+ SVN_ERR(open_db(&sdb1, &db_abspath, "txn_commit_busy",
+ statements, 250, pool));
+ SVN_ERR(svn_sqlite__open(&sdb2, db_abspath, svn_sqlite__mode_readwrite,
+ statements, 0, NULL, 250, pool, pool));
+ SVN_ERR(svn_sqlite__exec_statements(sdb1, 0));
+
+ /* Begin two deferred transactions. */
+ SVN_ERR(svn_sqlite__begin_transaction(sdb1));
+ SVN_ERR(svn_sqlite__exec_statements(sdb1, 1 /* INSERT */));
+ SVN_ERR(svn_sqlite__begin_transaction(sdb2));
+ SVN_ERR(svn_sqlite__exec_statements(sdb2, 2 /* SELECT */));
+
+ /* Try to COMMIT the first write transaction; this should fail due to
+ the concurrent read transaction that holds a shared lock on the db. */
+ err = svn_sqlite__finish_transaction(sdb1, SVN_NO_ERROR);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_SQLITE_BUSY);
+
+ /* We failed to COMMIT the first transaction, but COMMIT-ting the
+ second transaction through a different db connection should succeed.
+ Upgrade it to a write transaction by executing the INSERT statement,
+ and then commit. */
+ SVN_ERR(svn_sqlite__exec_statements(sdb2, 1 /* INSERT */));
+ SVN_ERR(svn_sqlite__finish_transaction(sdb2, SVN_NO_ERROR));
+
+ SVN_ERR(svn_sqlite__close(sdb2));
+ SVN_ERR(svn_sqlite__close(sdb1));
+
+ return SVN_NO_ERROR;
+}
+
static int max_threads = 1;
@@ -121,6 +178,8 @@
SVN_TEST_NULL,
SVN_TEST_PASS2(test_sqlite_reset,
"sqlite reset"),
+ SVN_TEST_XFAIL2(test_sqlite_txn_commit_busy,
+ "sqlite busy on transaction commit"),
SVN_TEST_NULL
};
