[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: 'svnsync_tests.py 23 --fs-type bdb' fails due to process abort

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: Thu, 24 Jan 2008 08:50:24 -0500

C. Michael Pilato wrote:
> C. Michael Pilato wrote:
>> C. Michael Pilato wrote:
>>> Specifically in this case, svn_fs_base__dag_commit_txn() (which is
>>> wrapped with a BDB transaction) calls
>>> svn_fs_mergeinfo__update_index(), which in turn calls
>>> index_txn_mergeinfo(), which calls (UH-OH!) svn_fs_revision_root().
>>>
>>> I started to fix this weeks ago on the reintegrate branch, but
>>> stopped because that branch took a turn towards removing sqlite from
>>> BDB altogether.
>>
>> I'm testing a patch for this right now. I hope it's temporary and
>> that David Glasser (or someone else) will be able to return to the
>> task of de-sqlite-ing our BDB mergeinfo logic Real Soon Now(tm).
>
> Oh, geez. I think there are other circular call paths to fix, too, as I
> see the mergeinfo util code also calls svn_fs_node_prop().

Attached is a patch that partially fixes the problems. But it's useless
without the fix to the svn_fs_node_prop-calling-codepath, too. I'm still
hoping this code will just go completely away soon, so I'm not going to
attack the rest of the problem right now.

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Fix abort()s triggered by illegal libsvn_fs_base call stacks (nested
BDB transactions are an absolute no-no!).

Sadly, this doesn't fix them all, as there is still a call to
svn_fs_node_prop() from inside the sqlite utility code. :-(

* subversion/include/private/svn_fs_mergeinfo.h
  (svn_fs_mergeinfo__update_index): Add 'prev_rev_root' parameter,

* subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
  (index_txn_mergeinfo): Add 'prev_rev_root' parameter, and use it
    instead of generating a new root object.
  (svn_fs_mergeinfo__update_index): Add 'prev_rev_root' parameter,
    passed to updated call to index_txn_mergeinfo().

* subversion/libsvn_fs_base/dag.c
  (svn_fs_base__dag_commit_txn): Don't do mergeinfo index updates here.

* subversion/libsvn_fs_base/tree.c
  (txn_body_commit): Instead, update mergeinfo indexes here.

* subversion/libsvn_fs_fs/fs_fs.c
  (commit_body): Update call to svn_fs_mergeinfo__update_index().

Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 29007)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -41,6 +41,7 @@
 #include "svn_mergeinfo.h"
 #include "svn_fs.h"
 #include "svn_sorts.h"
+#include "svn_hash.h"
 #include "fs.h"
 #include "err.h"
 #include "trail.h"
@@ -2372,10 +2373,12 @@
   svn_fs_txn_t *txn = args->txn;
   svn_fs_t *fs = txn->fs;
   const char *txn_name = txn->id;
-
   svn_revnum_t youngest_rev;
   const svn_fs_id_t *y_rev_root_id;
   dag_node_t *txn_base_root_node;
+ apr_hash_t *txnprops;
+ const svn_string_t *target_mergeinfo;
+ apr_pool_t *pool = trail->pool;
 
   /* Getting the youngest revision locks the revisions table until
      this trail is done. */
@@ -2408,10 +2411,33 @@
      discovered locks. */
   SVN_ERR(verify_locks(txn_name, trail, trail->pool));
 
- /* Else, commit the txn. */
+ /* Check for mergeinfo before finalizing the commit. */
+ SVN_ERR(svn_fs_base__txn_proplist_in_trail(&txnprops, txn_name, trail));
+ target_mergeinfo = apr_hash_get(txnprops, SVN_FS__PROP_TXN_MERGEINFO,
+ APR_HASH_KEY_STRING);
+
+ /* Now commit the txn. */
   SVN_ERR(svn_fs_base__dag_commit_txn(&(args->new_rev), txn, trail,
                                       trail->pool));
 
+ /* If there was mergeinfo, update the mergeinfo index. */
+ if (target_mergeinfo)
+ {
+ apr_hash_t *mergeinfo = apr_hash_make(pool);
+ svn_fs_root_t *prev_rev_root;
+ struct revision_root_args rr_args;
+ svn_stream_t *stream =
+ svn_stream_from_stringbuf(svn_stringbuf_create_from_string
+ (target_mergeinfo, pool), pool);
+
+ rr_args.root_p = &prev_rev_root;
+ rr_args.rev = args->new_rev;
+ SVN_ERR(txn_body_revision_root(&rr_args, trail));
+ SVN_ERR(svn_hash_read2(mergeinfo, stream, NULL, pool));
+ SVN_ERR(svn_fs_mergeinfo__update_index(txn, args->new_rev,
+ prev_rev_root, mergeinfo, pool));
+ }
+
   return SVN_NO_ERROR;
 }
 
Index: subversion/libsvn_fs_base/dag.c
===================================================================
--- subversion/libsvn_fs_base/dag.c (revision 29007)
+++ subversion/libsvn_fs_base/dag.c (working copy)
@@ -1433,7 +1433,6 @@
   apr_hash_t *txnprops;
   svn_fs_t *fs = txn->fs;
   const char *txn_id = txn->id;
- const svn_string_t *target_mergeinfo;
 
   /* Remove any temporary transaction properties initially created by
      begin_txn(). */
@@ -1444,19 +1443,9 @@
   *new_rev = SVN_INVALID_REVNUM;
   SVN_ERR(svn_fs_bdb__put_rev(new_rev, fs, &revision, trail, pool));
 
- target_mergeinfo = apr_hash_get(txnprops, SVN_FS__PROP_TXN_MERGEINFO,
- APR_HASH_KEY_STRING);
- if (target_mergeinfo)
- {
- svn_stringbuf_t *buf =
- svn_stringbuf_create_from_string(target_mergeinfo, pool);
- svn_stream_t *stream = svn_stream_from_stringbuf(buf, pool);
- apr_hash_t *mergeinfo = apr_hash_make(pool);
- SVN_ERR(svn_hash_read2(mergeinfo, stream, NULL, pool));
- SVN_ERR(svn_fs_mergeinfo__update_index(txn, *new_rev, mergeinfo, pool));
- SVN_ERR(svn_fs_base__set_txn_prop
- (fs, txn_id, SVN_FS__PROP_TXN_MERGEINFO, NULL, trail, pool));
- }
+ if (apr_hash_get(txnprops, SVN_FS__PROP_TXN_MERGEINFO, APR_HASH_KEY_STRING))
+ SVN_ERR(svn_fs_base__set_txn_prop
+ (fs, txn_id, SVN_FS__PROP_TXN_MERGEINFO, NULL, trail, pool));
 
   if (apr_hash_get(txnprops, SVN_FS__PROP_TXN_CHECK_OOD, APR_HASH_KEY_STRING))
     SVN_ERR(svn_fs_base__set_txn_prop
Index: subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
===================================================================
--- subversion/libsvn_fs_util/mergeinfo-sqlite-index.c (revision 29007)
+++ subversion/libsvn_fs_util/mergeinfo-sqlite-index.c (working copy)
@@ -200,15 +200,13 @@
 static svn_error_t *
 index_txn_mergeinfo(sqlite3 *db,
                     svn_revnum_t new_rev,
+ svn_fs_root_t *prev_rev_root,
                     apr_hash_t *mergeinfo_for_paths,
                     svn_fs_t *fs,
                     apr_pool_t *pool)
 {
   apr_hash_index_t *hi;
- svn_fs_root_t *old_root;
 
- SVN_ERR(svn_fs_revision_root(&old_root, fs, new_rev - 1, pool));
-
   for (hi = apr_hash_first(pool, mergeinfo_for_paths);
        hi != NULL;
        hi = apr_hash_next(hi))
@@ -217,8 +215,8 @@
       void *mergeinfo;
 
       apr_hash_this(hi, &path, NULL, &mergeinfo);
- SVN_ERR(index_path_mergeinfo(new_rev, db, (const char *) path,
- (svn_string_t *) mergeinfo, old_root, pool));
+ SVN_ERR(index_path_mergeinfo(new_rev, db, path, mergeinfo,
+ prev_rev_root, pool));
     }
   return SVN_NO_ERROR;
 }
@@ -292,7 +290,9 @@
    revision number as NEW_REV, and if the current transaction contains
    mergeinfo, record it. */
 svn_error_t *
-svn_fs_mergeinfo__update_index(svn_fs_txn_t *txn, svn_revnum_t new_rev,
+svn_fs_mergeinfo__update_index(svn_fs_txn_t *txn,
+ svn_revnum_t new_rev,
+ svn_fs_root_t *old_rev_root,
                                apr_hash_t *mergeinfo_for_paths,
                                apr_pool_t *pool)
 {
@@ -306,17 +306,14 @@
 
   /* Clean up old data. (If we're going to write to the DB anyway,
      there's no reason to do extra checks to avoid no-op DELETEs.) */
- err = clean_tables(db,
- new_rev,
- (mergeinfo_for_paths == NULL),
- subpool);
+ err = clean_tables(db, new_rev, (mergeinfo_for_paths == NULL), subpool);
   MAYBE_CLEANUP;
 
   /* Record any mergeinfo from the current transaction. */
   if (mergeinfo_for_paths)
     {
- err = index_txn_mergeinfo(db, new_rev, mergeinfo_for_paths, txn->fs,
- subpool);
+ err = index_txn_mergeinfo(db, new_rev, old_rev_root,
+ mergeinfo_for_paths, txn->fs, subpool);
       MAYBE_CLEANUP;
     }
 
Index: subversion/include/private/svn_fs_mergeinfo.h
===================================================================
--- subversion/include/private/svn_fs_mergeinfo.h (revision 29007)
+++ subversion/include/private/svn_fs_mergeinfo.h (working copy)
@@ -33,17 +33,23 @@
 
 
 /* Update the mergeinfo index according to the changes made in
- transaction TXN for revision NEW_REV. MERGEINFO_FOR_PATHS is the
- mergeinfo for each path changed in the transaction (a mapping of
- const char * -> svn_string_t *), or NULL if there was no mergeinfo
- recorded for that transaction. Use POOL for any temporary allocations.
+ transaction TXN for the revision NEW_REV. MERGEINFO_FOR_PATHS is
+ the mergeinfo for each path changed in the transaction (a mapping
+ of const char * -> svn_string_t *), or NULL if there was no
+ mergeinfo recorded for that transaction.
+
+ PREV_REV_ROOT is a revision root constructed for the revision
+ NEW_REV - 1.
 
+ Use POOL for any temporary allocations.
+
    NOTE: Even if there is no mergeinfo, this function should be
    called to make sure there is no stray mergeinfo for this revision
    left from a previous failed commit. */
 svn_error_t *
 svn_fs_mergeinfo__update_index(svn_fs_txn_t *txn,
                                svn_revnum_t new_rev,
+ svn_fs_root_t *prev_rev_root,
                                apr_hash_t *mergeinfo_for_paths,
                                apr_pool_t *pool);
 
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 29007)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -4997,6 +4997,7 @@
   const char *revprop_filename, *final_revprop;
   const svn_fs_id_t *root_id, *new_root_id;
   const char *start_node_id, *start_copy_id;
+ svn_fs_root_t *prev_rev_root;
   svn_revnum_t old_rev, new_rev;
   apr_file_t *proto_file;
   void *proto_file_lockcookie;
@@ -5130,8 +5131,9 @@
                                      old_rev_filename, pool));
 
   /* Update the merge tracking information index. */
- SVN_ERR(svn_fs_mergeinfo__update_index(cb->txn, new_rev, target_mergeinfo,
- pool));
+ SVN_ERR(svn_fs_revision_root(&prev_rev_root, cb->fs, new_rev - 1, pool));
+ SVN_ERR(svn_fs_mergeinfo__update_index(cb->txn, new_rev, prev_rev_root,
+ target_mergeinfo, pool));
 
   /* Update the 'current' file. */
   SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,

Received on 2008-01-24 17:14:50 CET

This is an archived mail posted to the Subversion Dev mailing list.