Happy Trails to you!

["Glenn A. Thompson" <gt...@cdr.net>]

Ha, made you look!

happy "trail.c" that is:-)

Below is a change to trail.c that I think is "more" correct than the
current implemention.
While creating an abstracted trail implemention, I noticed that the undo
logic was not completely correct.
I beleive my change is more correct.  Given the sparse use of the
"record" functions this change will not yield a fix to any existing
problem. However, it will be more correct for future use.
I can submit a real patch if you like.  Otherwise it will be represented
(differently however) in a coming fs-refactoring patch.

Thanks,
gat

Index: trail.c
===================================================================
--- trail.c
+++ trail.c Wed Oct 16 11:14:41 2002
@@ -86,17 +86,18 @@
 {
   struct undo *undo;

+  /* According to the example in the Berkeley DB manual, txn_commit
+     doesn't return DB_LOCK_DEADLOCK --- all deadlocks are reported
+     earlier.  */
+  SVN_ERR (DB_WRAP (fs, "committing Berkeley DB transaction",
+                    trail->db_txn->commit (trail->db_txn, 0)));
+
   /* Undo those changes which should persist only while the
      transaction is active.  */
   for (undo = trail->undo; undo; undo = undo->prev)
     if (undo->when & undo_on_success)
       undo->func (undo->baton);

-  /* According to the example in the Berkeley DB manual, txn_commit
-     doesn't return DB_LOCK_DEADLOCK --- all deadlocks are reported
-     earlier.  */
-  SVN_ERR (DB_WRAP (fs, "committing Berkeley DB transaction",
-                    trail->db_txn->commit (trail->db_txn, 0)));

   /* Do a checkpoint here, if enough has gone on.
      The checkpoint parameters below are pretty arbitrary.  Perhaps
@@ -128,10 +129,20 @@

       if (! svn_err)
         {
+          svn_error_t *commit_err;
           /* The transaction succeeded!  Commit it.  */
-          SVN_ERR (commit_trail (trail, fs));
-
-          return SVN_NO_ERROR;
+          commit_err = commit_trail (trail, fs);
+          if ( commit_err == SVN_NO_ERROR )
+            {
+              return SVN_NO_ERROR;
+            }
+          else
+            {
+              for (undo = trail->undo; undo; undo = undo->prev)
+                if (undo->when & undo_on_failure)
+                  undo->func (undo->baton);
+              return commit_err;
+            }
         }

       /* Is this a real error, or do we just need to retry?  */



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Happy Trails to you!

["Glenn A. Thompson" <gt...@cdr.net>]

>
> I beleive my change is more correct.  Given the sparse use of the
> "record" functions this change will not yield a fix to any existing
> problem. However, it will be more correct for future use.
> I can submit a real patch if you like.

Not!  I have a little problem with my logic also.
The checkpoint bothers me.  It is possible, "reading bdb docs" that a
checkpoint could fail after a commit succeeded.
This is not goodness. This would/could create an unexpected undo behavior.
Two things in particular:
1) svn_fs__record_completion recorded functions could get called twice.
2) svn_fs__record_undo recorded functions would be called even though the
commit succeeded.

As I understand it. Even if the checkpoint fails the commit information has
not been lost.  Oncde the recovery is run it should be there.  Right?

So, I think the checkpoint should be run after the commit_trail function
returns.

This is the way I'm coding it in my new version.
It will be called as part of an internal flush_trail function.

cheerio,
gat


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org