You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2016/04/26 22:11:31 UTC
svn commit: r1741096 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_fs_fs/rep-cache.c libsvn_fs_fs/rep-cache.h
libsvn_fs_fs/transaction.c libsvn_subr/sqlite.c tests/libsvn_fs/fs-test.c
tests/libsvn_subr/sqlite-test.c
Author: kotkov
Date: Tue Apr 26 20:11:30 2016
New Revision: 1741096
URL: http://svn.apache.org/viewvc?rev=1741096&view=rev
Log:
Rollback an sqlite transaction in case we fail to COMMIT it.
Otherwise, the db connection might be left in an unusable state and can
be causing different issues, especially in case the connection is a
long-living one.
See r1741071 and the commit_with_locked_rep_cache() test.
* subversion/include/svn_error_codes.h
(SVN_ERR_SQLITE_ROLLBACK_FAILED): New error code.
* subversion/libsvn_subr/sqlite.c
(rollback_transaction): Use the new error code in case we fail to
rollback the transaction.
(svn_sqlite__finish_transaction): Rollback the transaction if the
commit fails. Add a comment explaining why this is required.
* subversion/libsvn_fs_fs/rep-cache.h
(svn_fs_fs__close_rep_cache): New function.
* subversion/libsvn_fs_fs/rep-cache.c
(svn_fs_fs__close_rep_cache): Implement this new function.
* subversion/libsvn_fs_fs/transaction.c
(svn_fs_fs_commit): Unroll the SVN_SQLITE__WITH_TXN() macro.
Handle the edge case when we fail to rollback the transaction by
immediately closing the db connection.
* subversion/tests/libsvn_fs/fs-test.c
(test_funcs): The commit_with_locked_rep_cache() test now passes.
* subversion/tests/libsvn_subr/sqlite-test.c
(test_funcs): The test_sqlite_txn_commit_busy() test now passes.
Modified:
subversion/trunk/subversion/include/svn_error_codes.h
subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
subversion/trunk/subversion/libsvn_fs_fs/rep-cache.h
subversion/trunk/subversion/libsvn_fs_fs/transaction.c
subversion/trunk/subversion/libsvn_subr/sqlite.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
subversion/trunk/subversion/tests/libsvn_subr/sqlite-test.c
Modified: subversion/trunk/subversion/include/svn_error_codes.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_error_codes.h?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_error_codes.h (original)
+++ subversion/trunk/subversion/include/svn_error_codes.h Tue Apr 26 20:11:30 2016
@@ -1457,6 +1457,11 @@ SVN_ERROR_START
SVN_ERR_MISC_CATEGORY_START + 43,
"Parser error: invalid input")
+ /** @since New in 1.10. */
+ SVN_ERRDEF(SVN_ERR_SQLITE_ROLLBACK_FAILED,
+ SVN_ERR_MISC_CATEGORY_START + 44,
+ "SQLite transaction rollback failed")
+
/* command-line client errors */
SVN_ERRDEF(SVN_ERR_CL_ARG_PARSING_ERROR,
Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c Tue Apr 26 20:11:30 2016
@@ -126,6 +126,21 @@ svn_fs_fs__open_rep_cache(svn_fs_t *fs,
}
svn_error_t *
+svn_fs_fs__close_rep_cache(svn_fs_t *fs)
+{
+ fs_fs_data_t *ffd = fs->fsap_data;
+
+ if (ffd->rep_cache_db)
+ {
+ SVN_ERR(svn_sqlite__close(ffd->rep_cache_db));
+ ffd->rep_cache_db = NULL;
+ ffd->rep_cache_db_opened = 0;
+ }
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
svn_fs_fs__exists_rep_cache(svn_boolean_t *exists,
svn_fs_t *fs, apr_pool_t *pool)
{
Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rep-cache.h?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/rep-cache.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/rep-cache.h Tue Apr 26 20:11:30 2016
@@ -40,6 +40,10 @@ svn_error_t *
svn_fs_fs__open_rep_cache(svn_fs_t *fs,
apr_pool_t *pool);
+/* Close the rep cache database associated with FS. */
+svn_error_t *
+svn_fs_fs__close_rep_cache(svn_fs_t *fs);
+
/* Set *EXISTS to TRUE iff the rep-cache DB file exists. */
svn_error_t *
svn_fs_fs__exists_rep_cache(svn_boolean_t *exists,
Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Tue Apr 26 20:11:30 2016
@@ -3824,6 +3824,8 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
if (ffd->rep_sharing_allowed)
{
+ svn_error_t *err;
+
SVN_ERR(svn_fs_fs__open_rep_cache(fs, pool));
/* Write new entries to the rep-sharing database.
@@ -3834,9 +3836,21 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
/* ### A commit that touches thousands of files will starve other
(reader/writer) commits for the duration of the below call.
Maybe write in batches? */
- SVN_SQLITE__WITH_TXN(
- write_reps_to_cache(fs, cb.reps_to_cache, pool),
- ffd->rep_cache_db);
+ SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
+ err = write_reps_to_cache(fs, cb.reps_to_cache, pool);
+ err = svn_sqlite__finish_transaction(ffd->rep_cache_db, err);
+
+ if (err && svn_error_find_cause(err, SVN_ERR_SQLITE_ROLLBACK_FAILED))
+ {
+ /* Failed rollback means that our db connection is unusable, and
+ the only thing we can do is close it. The connection will be
+ reopened during the next operation with rep-cache.db. */
+ return svn_error_trace(
+ svn_error_compose_create(err,
+ svn_fs_fs__close_rep_cache(fs)));
+ }
+ else if (err)
+ return svn_error_trace(err);
}
return SVN_NO_ERROR;
Modified: subversion/trunk/subversion/libsvn_subr/sqlite.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sqlite.c?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/sqlite.c (original)
+++ subversion/trunk/subversion/libsvn_subr/sqlite.c Tue Apr 26 20:11:30 2016
@@ -1300,6 +1300,12 @@ rollback_transaction(svn_sqlite__db_t *d
}
}
+ if (err)
+ {
+ /* Rollback failed, use a specific error code. */
+ err = svn_error_create(SVN_ERR_SQLITE_ROLLBACK_FAILED, err, NULL);
+ }
+
return svn_error_compose_create(error_to_wrap, err);
}
@@ -1347,9 +1353,35 @@ svn_sqlite__finish_transaction(svn_sqlit
{
return svn_error_trace(rollback_transaction(db, err));
}
+ else
+ {
+ err = get_internal_statement(&stmt, db,
+ STMT_INTERNAL_COMMIT_TRANSACTION);
+ if (!err)
+ err = svn_error_trace(svn_sqlite__step_done(stmt));
+
+ /* Need to rollback if the commit fails as well, because otherwise the
+ db connection will be left in an unusable state.
+
+ One important case to keep in mind is trying to COMMIT with concurrent
+ readers. In case the commit fails, because someone else is holding a
+ shared lock, sqlite keeps the transaction, and *also* keeps the file
+ locks on the database. While the first part only prevents from using
+ this connection, the second part prevents everyone else from accessing
+ the database while the connection is open.
+
+ See https://www.sqlite.org/lang_transaction.html
+
+ COMMIT might also result in an SQLITE_BUSY return code if an another
+ thread or process has a shared lock on the database that prevented
+ the database from being updated. When COMMIT fails in this way, the
+ transaction remains active and the COMMIT can be retried later after
+ the reader has had a chance to clear. */
+ if (err)
+ return svn_error_trace(rollback_transaction(db, err));
+ }
- SVN_ERR(get_internal_statement(&stmt, db, STMT_INTERNAL_COMMIT_TRANSACTION));
- return svn_error_trace(svn_sqlite__step_done(stmt));
+ return SVN_NO_ERROR;
}
svn_error_t *
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Apr 26 20:11:30 2016
@@ -7329,8 +7329,8 @@ static struct svn_test_descriptor_t test
"freeze and commit"),
SVN_TEST_OPTS_PASS(test_large_changed_paths_list,
"test reading a large changed paths list"),
- SVN_TEST_OPTS_XFAIL(commit_with_locked_rep_cache,
- "test commit with locked rep-cache"),
+ SVN_TEST_OPTS_PASS(commit_with_locked_rep_cache,
+ "test commit with locked rep-cache"),
SVN_TEST_NULL
};
Modified: subversion/trunk/subversion/tests/libsvn_subr/sqlite-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/sqlite-test.c?rev=1741096&r1=1741095&r2=1741096&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/sqlite-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/sqlite-test.c Tue Apr 26 20:11:30 2016
@@ -178,8 +178,8 @@ static struct svn_test_descriptor_t test
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_PASS2(test_sqlite_txn_commit_busy,
+ "sqlite busy on transaction commit"),
SVN_TEST_NULL
};
Re: svn commit: r1741096 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_fs_fs/rep-cache.c libsvn_fs_fs/rep-cache.h
libsvn_fs_fs/transaction.c libsvn_subr/sqlite.c tests/libsvn_fs/fs-test.c tests/libsvn_subr/sqlite-test.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
>> + if (err && svn_error_find_cause(err, SVN_ERR_SQLITE_ROLLBACK_FAILED))
>
> There is no need check for 'err' since svn_error_find_cause() already
> checks for SVN_NO_ERROR.
Thanks, should be fixed in r1741200.
Regards,
Evgeny Kotkov
Re: svn commit: r1741096 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_fs_fs/rep-cache.c libsvn_fs_fs/rep-cache.h
libsvn_fs_fs/transaction.c libsvn_subr/sqlite.c tests/libsvn_fs/fs-test.c tests/libsvn_subr/sqlite-test.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 April 2016 at 23:11, <ko...@apache.org> wrote:
> Author: kotkov
> Date: Tue Apr 26 20:11:30 2016
> New Revision: 1741096
>
> URL: http://svn.apache.org/viewvc?rev=1741096&view=rev
> Log:
> Rollback an sqlite transaction in case we fail to COMMIT it.
>
> Otherwise, the db connection might be left in an unusable state and can
> be causing different issues, especially in case the connection is a
> long-living one.
>
> See r1741071 and the commit_with_locked_rep_cache() test.
>
[...]
[...]
> Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1741096&r1=1741095&r2=1741096&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Tue Apr 26 20:11:30 2016
> @@ -3824,6 +3824,8 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
>
> if (ffd->rep_sharing_allowed)
> {
> + svn_error_t *err;
> +
> SVN_ERR(svn_fs_fs__open_rep_cache(fs, pool));
>
> /* Write new entries to the rep-sharing database.
> @@ -3834,9 +3836,21 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
> /* ### A commit that touches thousands of files will starve other
> (reader/writer) commits for the duration of the below call.
> Maybe write in batches? */
> - SVN_SQLITE__WITH_TXN(
> - write_reps_to_cache(fs, cb.reps_to_cache, pool),
> - ffd->rep_cache_db);
> + SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
> + err = write_reps_to_cache(fs, cb.reps_to_cache, pool);
> + err = svn_sqlite__finish_transaction(ffd->rep_cache_db, err);
> +
> + if (err && svn_error_find_cause(err, SVN_ERR_SQLITE_ROLLBACK_FAILED))
There is no need check for 'err' since svn_error_find_cause() already
checks for SVN_NO_ERROR.
> + {
> + /* Failed rollback means that our db connection is unusable, and
> + the only thing we can do is close it. The connection will be
> + reopened during the next operation with rep-cache.db. */
> + return svn_error_trace(
> + svn_error_compose_create(err,
> + svn_fs_fs__close_rep_cache(fs)));
> + }
> + else if (err)
> + return svn_error_trace(err);
> }
>
> return SVN_NO_ERROR;
>
--
Ivan Zhakov