Re: svn commit: r34160 - trunk/subversion/libsvn_fs_fs

David Glasser wrote:
> On Wed, Nov 12, 2008 at 12:32 PM, <hwright_at_tigris.org> wrote:
>> Author: hwright
>> Date: Wed Nov 12 12:32:17 2008
>> New Revision: 34160
>>
>> Log:
>> Followup to r34147 by rolling back the FSFS sqlite transaction on a failed
>> commit.
>>
>> If at any point we decide that we have more places to used wrapped sqlite
>> transactions, this could probably be generalized in the sqlite APIs.
>>
>> * subversion/libsvn_fs_fs/fs_fs.c
>> (commit_body): Remove transaction handling.
>> (commit_body_wrapper): New.
>> (svn_fs_fs__commit): Call the new wrapper.
>>
>> Modified:
>> trunk/subversion/libsvn_fs_fs/fs_fs.c
>>
>> Modified: trunk/subversion/libsvn_fs_fs/fs_fs.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/fs_fs.c?pathrev=34160&r1=34159&r2=34160
>> ==============================================================================
>> --- trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Nov 12 12:09:48 2008 (r34159)
>> +++ trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Nov 12 12:32:17 2008 (r34160)
>> @@ -5287,10 +5287,6 @@ commit_body(void *baton, apr_pool_t *poo
>> apr_hash_t *txnprops;
>> svn_string_t date;
>>
>> - /* Start the sqlite transaction. */
>> - if (ffd->rep_cache.db)
>> - SVN_ERR(svn_sqlite__transaction_begin(ffd->rep_cache.db));
>> -
>> /* Get the current youngest revision. */
>> SVN_ERR(svn_fs_fs__youngest_rev(&old_rev, cb->fs, pool));
>>
>> @@ -5430,15 +5426,42 @@ commit_body(void *baton, apr_pool_t *poo
>> /* Remove this transaction directory. */
>> SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
>>
>> - /* Commit the sqlite transaction. */
>> - if (ffd->rep_cache.db)
>> - SVN_ERR(svn_sqlite__transaction_commit(ffd->rep_cache.db));
>> -
>> *cb->new_rev_p = new_rev;
>>
>> return SVN_NO_ERROR;
>> }
>>
>> +/* Wrapper around commit_body() which implements SQLite transactions. Arguments
>> + the same as commit_body().
>> +
>> + XXX: If we decide to wrap other stuff with sqlite transactions, this should
>> + probably be generalized and moved into libsvn_subr/sqlite.c.
>> + */
>> +static svn_error_t *
>> +commit_body_wrapper(void *baton, apr_pool_t *pool)
>> +{
>> + struct commit_baton *cb = baton;
>> + fs_fs_data_t *ffd = cb->fs->fsap_data;
>> + svn_error_t *err;
>> +
>> + /* Start the sqlite transaction. */
>> + if (ffd->rep_cache.db)
>> + SVN_ERR(svn_sqlite__transaction_begin(ffd->rep_cache.db));
>> +
>> + err = commit_body(baton, pool);
>> +
>> + /* Commit or rollback the sqlite transaction. */
>> + if (ffd->rep_cache.db)
>> + {
>> + if (err)
>> + SVN_ERR(svn_sqlite__transaction_rollback(ffd->rep_cache.db));
>
> Hmm, you really think an error from the ROLLBACK command should
> override the original error that caused you to issue the ROLLBACK?

Nope. I've got a follow up patch in testing right now, which addresses this, as
well as a couple of issues Greg mentioned in IRC.

-Hyrum