[Zodb-checkins] CVS: Zope3/src/zodb/btrees - MergeTemplate.c:1.3 interfaces.py:1.5

Tim Peters tim.one@comcast.net
Wed, 15 Jan 2003 23:47:05 -0500


Update of /cvs-repository/Zope3/src/zodb/btrees
In directory cvs.zope.org:/tmp/cvs-serv15528/src/zodb/btrees

Modified Files:
	MergeTemplate.c interfaces.py 
Log Message:
Tried to sort out the current reasons for raising ConflictError during
bucket merges.  No actual code changes, but added lots of XXX comments
about things that aren't clear to me, and added the "bucket split"
reason to interfaces.py's list of English explanations.


=== Zope3/src/zodb/btrees/MergeTemplate.c 1.2 => 1.3 ===
--- Zope3/src/zodb/btrees/MergeTemplate.c:1.2	Wed Dec 25 09:12:16 2002
+++ Zope3/src/zodb/btrees/MergeTemplate.c	Wed Jan 15 23:47:01 2003
@@ -33,6 +33,10 @@
     return 0;
 }
 
+/* The "reason" argument is a little integer giving "a reason" for the
+ * error.  In the Zope3 codebase, these are mapped to explanatory strings
+ * via zodb/btrees/interfaces.py.
+ */
 static PyObject *
 merge_error(int p1, int p2, int p3, int reason)
 {
@@ -84,6 +88,10 @@
   if (i3.next(&i3) < 0)
       goto err;
 
+  /* Consult zodb/btrees/interfaces.py for the meaning of the last
+   * argument passed to merge_error().
+   */
+  /* XXX This isn't passing on errors raised by value comparisons. */
   while (i1.position >= 0 && i2.position >= 0 && i3.position >= 0)
     {
       TEST_KEY_SET_OR(cmp12, i1.key, i2.key) goto err;
@@ -93,15 +101,15 @@
           if (cmp13==0)
             {
               if (set || (TEST_VALUE(i1.value, i2.value) == 0))
-                {               /* change in i3 or all same */
+                {               /* change in i3 value or all same */
                   if (merge_output(r, &i3, mapping) < 0) goto err;
                 }
               else if (set || (TEST_VALUE(i1.value, i3.value) == 0))
-                {               /* change in i2 */
+                {               /* change in i2 value */
                   if (merge_output(r, &i2, mapping) < 0) goto err;
                 }
               else
-                {               /* conflicting changes in i2 and i3 */
+                {               /* conflicting value changes in i2 and i3 */
                   merge_error(i1.position, i2.position, i3.position, 1);
                   goto err;
                 }
@@ -148,6 +156,14 @@
           TEST_KEY_SET_OR(cmp23, i2.key, i3.key) goto err;
           if (cmp23==0)
             {                   /* dualing inserts or deletes */
+              /* XXX Why is this a conflict?  Suppose i2 and i3 both
+               * XXX added the same key.  If this is a set, that's not
+               * XXX a conflict.  Or if it's a mapping and they both added
+               * XXX the same value too, ditto.  If it's a mapping and they
+               * XXX added different values, *that* would be a conflict.
+               * XXX Or if they both deleted the same key, that's not a
+               * XXX conflict either.
+               */
               merge_error(i1.position, i2.position, i3.position, 4);
               goto err;
             }
@@ -171,6 +187,11 @@
             }
           else
             {                   /* Dueling deletes */
+              /* XXX I don't think this is a conflict:  i1.key < i2.key,
+               * XXX and i1.key < i3.key here.  That just means i1.key
+               * XXX was deleted by both, and we should simply advance i1
+               * XXX without merging anything into the output.
+               */
               merge_error(i1.position, i2.position, i3.position, 5);
               goto err;
             }
@@ -182,6 +203,10 @@
       TEST_KEY_SET_OR(cmp23, i2.key, i3.key) goto err;
       if (cmp23==0)
         {                       /* dualing inserts */
+          /* XXX I don't believe this should be a conflict for sets,
+           * XXX and it should be a conflict for mappings iff the
+           * XXX associated values differ.
+           */
           merge_error(i1.position, i2.position, i3.position, 6);
           goto err;
         }
@@ -199,6 +224,10 @@
 
   while (i1.position >= 0 && i2.position >= 0)
     {                           /* deleting i3 */
+      /* XXX I don't understand the "deleting i3" comment.  It seems more
+       * XXX that i3 deleted some keys from the tail end of i1, and i2 may
+       * XXX have added some keys that i3 didn't add.
+       */
       TEST_KEY_SET_OR(cmp12, i1.key, i2.key) goto err;
       if (cmp12 > 0)
         {                       /* insert i2 */
@@ -212,6 +241,10 @@
         }
       else
         {                       /* Dueling deletes or delete and change */
+          /* XXX If i1.key < i2.key at this point, I think that just means
+           * XXX that i2 and i3 both deleted i1.key, and that shouldn't be
+           * XXX a conflict.
+           */
           merge_error(i1.position, i2.position, i3.position, 7);
           goto err;
         }
@@ -219,6 +252,10 @@
 
   while (i1.position >= 0 && i3.position >= 0)
     {                           /* deleting i2 */
+      /* XXX I don't understand the "deleting i2" comment.  It seems more
+       * XXX that i2 deleted some keys from the tail end of i1, and i3 may
+       * XXX have added some keys that i2 didn't add.
+       */
       TEST_KEY_SET_OR(cmp13, i1.key, i3.key) goto err;
       if (cmp13 > 0)
         {                       /* insert i3 */
@@ -232,6 +269,10 @@
         }
       else
         {                       /* Dueling deletes or delete and change */
+          /* XXX If i1.key < i3.key at this point, I think that just means
+           * XXX that i2 and i3 both deleted i1.key, and that shouldn't be
+           * XXX a conflict.
+           */
           merge_error(i1.position, i2.position, i3.position, 8);
           goto err;
         }
@@ -239,6 +280,9 @@
 
   if (i1.position >= 0)
     {                           /* Dueling deletes */
+      /* XXX If i2 and i3 both deleted these guys, why is that a conflict?
+       * XXX I think we should ignore this.
+       */
       merge_error(i1.position, i2.position, i3.position, 9);
       goto err;
     }


=== Zope3/src/zodb/btrees/interfaces.py 1.4 => 1.5 ===
--- Zope3/src/zodb/btrees/interfaces.py:1.4	Wed Jan 15 14:11:54 2003
+++ Zope3/src/zodb/btrees/interfaces.py	Wed Jan 15 23:47:01 2003
@@ -16,15 +16,54 @@
 
 class BTreesConflictError(ConflictError):
 
-    msgs = ['',
-            'Conflicting changes', # in i2 & i3
-            'Conflicting del and change', # in i3 & i2
-            'Conflicting del and change', # in i2 & i3
+    msgs = [# 0; i2 or i3 bucket split; positions are all -1
+            'Conflicting bucket split',
+
+            # 1; keys the same, but i2 and i3 values differ, and both values
+            # differ from i1's value
+            'Conflicting changes',
+
+            # 2; i1's value changed in i2, but key+value deleted in i3
+            'Conflicting del and change',
+
+            # 3; i1's value changed in i3, but key+value deleted in i2
+            'Conflicting del and change',
+
+            # 4; see XXX comments in bucket_merge:  this seems to be raised
+            # if:
+            #    i1 and i2 both add the same key to a set
+            #    i1 and i2 both add the same <key, value> pair to a mapping
+            #    i1 and i2 both delete the same key from a set or mapping
+            #    i1 and i2 both add the same key to a mapping but with
+            #        different values
+            # I believe only the last should be called a conflict, and in
+            # that case this msg should be "conflicting inserts".
             'Conflicting inserts or deletes',
+
+            # 5; see XXX comments in bucket_merge; i2 and i3 both deleted
+            # the same key if we get here, and I don't believe that should
+            # be a conflict.
             'Conflicting deletes',
+
+            # 6; see XXX comments in bucket_merge; i2 and i3 both added the
+            # same key, but this is considered a conflict even if it's a set,
+            # or if it's a mapping and the values also match.
             'Conflicting inserts',
-            'Conflicting deletes or delete and change',
-            'Conflicting deletes or delete and change',
+
+            # 7; see XXX comments in bucket_merge; I don't believe it should
+            # be a conflict to delete the same key, and then this msg should
+            # be "Conflicting del and change".
+            'Conflicting dels or del and change',
+
+            # 8; see XXX comments in bucket_merge; I don't believe it should
+            # be a conflict to delete the same key, and then this msg should
+            # be "Conflicting del and change".
+            'Conflicting dels or del and change',
+
+            # 9; see XXX comments in bucket_merge; i2 and i3 both deleted
+            # the same key if we get here, and I don't believe that should
+            # be a conflict.
+            'Conflicting deletes',
             ]
 
     def __init__(self, p1, p2, p3, reason):