subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kot...@apache.org
Subject 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
Date Tue, 26 Apr 2016 20:11:31 GMT
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
   };
 



Mime
View raw message