CouchDB
  1. CouchDB
  2. COUCHDB-902

Attachments that have recovered from conflict do not accept attachments.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.2, 1.1, 1.2
    • Component/s: Database Core
    • Labels:
      None
    • Environment:

      trunk

    • Skill Level:
      Committers Level (Medium to Hard)

      Description

      Apparently if a document has been in a conflict, they will reject requests to add an attachment with a conflict error.

      I've tracked this down to couch_db_updater.erl line 501, but I'm not too familiar with this part of the code so I figured I'd fill out a ticket in case anyone else can go through this more quickly than me.

      Sure would be nice if I could attach a file when I create an issue...

      1. 988.patch
        19 kB
        Bob Dionne
      2. 902.patch
        1 kB
        Bob Dionne
      3. 0001-Fix-COUCHDB-902.patch
        7 kB
        Klaus Trainer
      4. couchdb-902-test-case.py
        4 kB
        Paul Joseph Davis

        Issue Links

          Activity

          Hide
          Bob Dionne added a comment -

          Note: 988.patch is also attached to couchdb-988 where it belongs. It's attached here by mistake but JIRA doesn't seems to allow delete.

          Show
          Bob Dionne added a comment - Note: 988.patch is also attached to couchdb-988 where it belongs. It's attached here by mistake but JIRA doesn't seems to allow delete.
          Hide
          Bob Dionne added a comment -

          more refactoring of couch_key_tree, new etaps, test for couchdb-902

          Show
          Bob Dionne added a comment - more refactoring of couch_key_tree, new etaps, test for couchdb-902
          Hide
          Bob Dionne added a comment -

          Klaus,

          I have two more patches coming, one for COUCHDB-988 that includes new versions of the kt etaps, including the one for this fail.

          Cheers,

          Bob

          Show
          Bob Dionne added a comment - Klaus, I have two more patches coming, one for COUCHDB-988 that includes new versions of the kt etaps, including the one for this fail. Cheers, Bob
          Hide
          Klaus Trainer added a comment -

          Yeah, we should get that into trunk soon.

          The only question that I still see as open is about testing. In the corresponding fix for 1.0.2 (see https://github.com/apache/couchdb/commit/8432c0e8f31a683b13419dc591edca49933d1f81), we have accordant futon tests in the (new) file `attachment_conflicts.js`. Also, there's Bob's new etap test (see https://github.com/bdionne/couchdb/commit/1a276a#diff-7) for couch_key_tree.

          Shouldn't we include those tests in the fix?

          Show
          Klaus Trainer added a comment - Yeah, we should get that into trunk soon. The only question that I still see as open is about testing. In the corresponding fix for 1.0.2 (see https://github.com/apache/couchdb/commit/8432c0e8f31a683b13419dc591edca49933d1f81 ), we have accordant futon tests in the (new) file `attachment_conflicts.js`. Also, there's Bob's new etap test (see https://github.com/bdionne/couchdb/commit/1a276a#diff-7 ) for couch_key_tree. Shouldn't we include those tests in the fix?
          Hide
          Bob Dionne added a comment -

          This is the fix proposed by Adam, that we agreed fixes the underlying issue couch_key_tree

          Show
          Bob Dionne added a comment - This is the fix proposed by Adam, that we agreed fixes the underlying issue couch_key_tree
          Hide
          Paul Joseph Davis added a comment -

          Awesome sauce. I'll make a new release thinger tonight.

          Show
          Paul Joseph Davis added a comment - Awesome sauce. I'll make a new release thinger tonight.
          Hide
          Adam Kocoloski added a comment -

          Bob, excellent. I can confirm that the test fails on 1.0.x and passes with the patch. So, here is my proposed workaround for 1.0.x:

          https://github.com/kocolosk/couchdb/commit/4663fa

          I'll commit in a couple of hours if there are no objections.

          Show
          Adam Kocoloski added a comment - Bob, excellent. I can confirm that the test fails on 1.0.x and passes with the patch. So, here is my proposed workaround for 1.0.x: https://github.com/kocolosk/couchdb/commit/4663fa I'll commit in a couple of hours if there are no objections.
          Hide
          Bob Dionne added a comment -

          Adam,

          Here's a failing Javascript test that mimics the second python test. I forgot to mention the original JS test wasn't working. Perhaps the code had changed since Klaus published it.

          Cheers,

          Bob

          [1] https://github.com/bdionne/couchdb/commit/8a1a3d

          Show
          Bob Dionne added a comment - Adam, Here's a failing Javascript test that mimics the second python test. I forgot to mention the original JS test wasn't working. Perhaps the code had changed since Klaus published it. Cheers, Bob [1] https://github.com/bdionne/couchdb/commit/8a1a3d
          Hide
          Adam Kocoloski added a comment -

          I tried the python test cases from Paul / Bob at at https://github.com/bdionne/couchdb/commit/1a276a ... cases 1 and 2 fail with stock 1.0.x but pass with the above patch. Case 3 always passes, but that seems to be expected (it doesn't do anything with attachments).

          I'd be grateful if someone could take a look at those and see what the problem is with Klaus' attachment_conflicts.js.

          Show
          Adam Kocoloski added a comment - I tried the python test cases from Paul / Bob at at https://github.com/bdionne/couchdb/commit/1a276a ... cases 1 and 2 fail with stock 1.0.x but pass with the above patch. Case 3 always passes, but that seems to be expected (it doesn't do anything with attachments). I'd be grateful if someone could take a look at those and see what the problem is with Klaus' attachment_conflicts.js.
          Hide
          Robert Newson added a comment -

          +1, this feels like the right approach, let's get 1.0.2 moving.

          Show
          Robert Newson added a comment - +1, this feels like the right approach, let's get 1.0.2 moving.
          Hide
          Adam Kocoloski added a comment -

          The patch I'm proposing to couch_httpd_db is pretty simple:

          diff --git a/src/couchdb/couch_httpd_db.erl b/src/couchdb/couch_httpd_db.erl
          index 1767d9c..02e1aa4 100644
          — a/src/couchdb/couch_httpd_db.erl
          +++ b/src/couchdb/couch_httpd_db.erl
          @@ -1020,8 +1020,10 @@ db_attachment_req(#httpd

          {method=Method,mochi_req=MochiReq}

          =Req, Db, DocId, FileN
          end
          end,

          • #doc {atts=Atts}

            = Doc,
            + #doc{atts=Atts, revs = {Pos, Revs}} = Doc,
            DocEdited = Doc#doc

            Unknown macro: {+ % prune revision list as a workaround for key tree bug (COUCHDB-902)+ revs = case Revs of [] -> {Pos, []}; _ -> {Pos, [hd(Revs)]} end, atts = NewAtt ++ [A || A <- Atts, A#att.name /= FileName] }

            ,

            {ok, UpdatedRev}

            = couch_db:update_doc(Db, DocEdited, []),

          However, my current stumbling block is that I cannot get attachment_conflicts.js to fail on a stock 1.0.x build. I haven't looked into it further yet. Will look again in a couple of hours.

          Show
          Adam Kocoloski added a comment - The patch I'm proposing to couch_httpd_db is pretty simple: diff --git a/src/couchdb/couch_httpd_db.erl b/src/couchdb/couch_httpd_db.erl index 1767d9c..02e1aa4 100644 — a/src/couchdb/couch_httpd_db.erl +++ b/src/couchdb/couch_httpd_db.erl @@ -1020,8 +1020,10 @@ db_attachment_req(#httpd {method=Method,mochi_req=MochiReq} =Req, Db, DocId, FileN end end, #doc {atts=Atts} = Doc, + #doc{atts=Atts, revs = {Pos, Revs}} = Doc, DocEdited = Doc#doc Unknown macro: {+ % prune revision list as a workaround for key tree bug (COUCHDB-902)+ revs = case Revs of [] -> {Pos, []}; _ -> {Pos, [hd(Revs)]} end, atts = NewAtt ++ [A || A <- Atts, A#att.name /= FileName] } , {ok, UpdatedRev} = couch_db:update_doc(Db, DocEdited, []), However, my current stumbling block is that I cannot get attachment_conflicts.js to fail on a stock 1.0.x build. I haven't looked into it further yet. Will look again in a couple of hours.
          Hide
          Adam Kocoloski added a comment -

          Thanks, Klaus!

          However, I'm a little uneasy making a change to a core module like that just before a release. I'd prefer it if the change cooked in trunk for a little while. That's why I'm proposing the following:

          a) we commit Bob's change to couch_key_tree to trunk

          b) we patch 1.0.x and 1.1.x at the HTTP layer by pruning the revision list during an attachment update

          We aren't in any danger of breaking something that isn't already broken, but we still fix the user-facing issue of being unable recover attachments from a conflict state. CouchDB 1.2 will have an improved Erlang API that never returns a spurious 'conflict' result from a merge when the full revision lists are used.

          Anyone opposed? I'd like to release 1.0.2 ASAP after the HTTP fix is committed.

          Show
          Adam Kocoloski added a comment - Thanks, Klaus! However, I'm a little uneasy making a change to a core module like that just before a release. I'd prefer it if the change cooked in trunk for a little while. That's why I'm proposing the following: a) we commit Bob's change to couch_key_tree to trunk b) we patch 1.0.x and 1.1.x at the HTTP layer by pruning the revision list during an attachment update We aren't in any danger of breaking something that isn't already broken, but we still fix the user-facing issue of being unable recover attachments from a conflict state. CouchDB 1.2 will have an improved Erlang API that never returns a spurious 'conflict' result from a merge when the full revision lists are used. Anyone opposed? I'd like to release 1.0.2 ASAP after the HTTP fix is committed.
          Hide
          Klaus Trainer added a comment -

          Bob, Adam, this one is an awesome solution! I've scrutinized it and haven't found any flaw in it.

          What I like most about it, is that with it, couch_key_tree:merge/3 always returns a correct `conflicts | no_conflicts` flag.

          If you can identify further test cases, we should surely include them. Anyway, I'd be fine with the bugfix and tests in your couchdb-902 branch.

          Please tell me when you've prepared a patch. I'll then have a look at it once again.

          Show
          Klaus Trainer added a comment - Bob, Adam, this one is an awesome solution! I've scrutinized it and haven't found any flaw in it. What I like most about it, is that with it, couch_key_tree:merge/3 always returns a correct `conflicts | no_conflicts` flag. If you can identify further test cases, we should surely include them. Anyway, I'd be fine with the bugfix and tests in your couchdb-902 branch. Please tell me when you've prepared a patch. I'll then have a look at it once again.
          Hide
          Nikolai Teofilov added a comment -

          Any chance to get this issue fixed for 1.0.2?

          Show
          Nikolai Teofilov added a comment - Any chance to get this issue fixed for 1.0.2?
          Hide
          Bob Dionne added a comment -

          Klaus et. al.,

          I think we found the crux of the matter. As you suspected, the merge_simple clause involving A < B reports conflicts even when no new conflicts are present and the merge is successful. Adding an attachment to an existing conflict situation just exposes the issue. Kocolosk found a nice simple solution[1] that runs all the tests including the new etap we added. See what you think and if you agree we can turn it into a patch. I'm going to add some more tests there while I'm at it.

          Cheers,

          Bob

          [1] https://github.com/bdionne/couchdb/commit/5e424d118615ca14ee5

          Show
          Bob Dionne added a comment - Klaus et. al., I think we found the crux of the matter. As you suspected, the merge_simple clause involving A < B reports conflicts even when no new conflicts are present and the merge is successful. Adding an attachment to an existing conflict situation just exposes the issue. Kocolosk found a nice simple solution [1] that runs all the tests including the new etap we added. See what you think and if you agree we can turn it into a patch. I'm going to add some more tests there while I'm at it. Cheers, Bob [1] https://github.com/bdionne/couchdb/commit/5e424d118615ca14ee5
          Hide
          Klaus Trainer added a comment -

          Here is a new version of my patch I'd propose. In contrast to the previous version, this one seems to be fine.

          Show
          Klaus Trainer added a comment - Here is a new version of my patch I'd propose. In contrast to the previous version, this one seems to be fine.
          Hide
          Klaus Trainer added a comment - - edited

          I just found the reason for the incorrect responses with status code 201 described in my previous comment.

          The problem is that responses that indicate conflicts are ignored, as the key in the dict:find/2 function call would never match.

          A possible fix could e.g. look like:

          diff --git a/src/couchdb/couch_db.erl b/src/couchdb/couch_db.erl
          index 20379ca..2bff4ab 100644
          — a/src/couchdb/couch_db.erl
          +++ b/src/couchdb/couch_db.erl
          @@ -727,8 +727,17 @@ update_docs(Db, Docs, Options, interactive_edit) ->
          ResultsDict = dict:from_list(IdRevs ++ CommitResults ++ PreCommitFailures),
          {ok, lists:map(
          fun(#doc{id=Id,revs={Pos, RevIds}}) ->

          • {ok, Result} = dict:find({Id, {Pos, RevIds}}, ResultsDict),
            - Result
            + Rev = case RevIds of
            + [] -> [];
            + _ -> hd(RevIds)
            + end,
            + case lists:member({Id, {Pos, [Rev]}, conflict}, CommitResults) of
            + false ->
            + {ok, Result}

            = dict:find({Id, {Pos, RevIds}}, ResultsDict),
            + Result;
            + true ->
            +

            Unknown macro: {Id, {Pos, [Rev]}, conflict}

            + end
            end, Docs)}
            end.

          I'll attach a revised version of my previously attached patch later.

          Show
          Klaus Trainer added a comment - - edited I just found the reason for the incorrect responses with status code 201 described in my previous comment. The problem is that responses that indicate conflicts are ignored, as the key in the dict:find/2 function call would never match. A possible fix could e.g. look like: diff --git a/src/couchdb/couch_db.erl b/src/couchdb/couch_db.erl index 20379ca..2bff4ab 100644 — a/src/couchdb/couch_db.erl +++ b/src/couchdb/couch_db.erl @@ -727,8 +727,17 @@ update_docs(Db, Docs, Options, interactive_edit) -> ResultsDict = dict:from_list(IdRevs ++ CommitResults ++ PreCommitFailures), {ok, lists:map( fun(#doc{id=Id,revs={Pos, RevIds}}) -> {ok, Result} = dict:find({Id, {Pos, RevIds}}, ResultsDict), - Result + Rev = case RevIds of + [] -> []; + _ -> hd(RevIds) + end, + case lists:member({Id, {Pos, [Rev]}, conflict}, CommitResults) of + false -> + {ok, Result} = dict:find({Id, {Pos, RevIds}}, ResultsDict), + Result; + true -> + Unknown macro: {Id, {Pos, [Rev]}, conflict} + end end, Docs)} end. I'll attach a revised version of my previously attached patch later.
          Hide
          Klaus Trainer added a comment -

          I just discovered that this issue is a bit trickier...

          Simply pruning #doc.revs tree doesn't seem to work for updates with attachments.

          When having multiple concurrent writers trying to attach a file to the same version, it's possible that more than one response with status code 201 and the same update sequence number is returned, even though only the first update succeeded for for real

          Here an excerpt from my logs.

          CouchDB log:

          [info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 201

          [info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 201

          [info] [<0.377.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 201

          [info] [<0.376.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 201

          Client log:

          'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 "85-31c1a9fb50b253aa68ac632f6e1a7626"

          'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 "86-1e38b77c2d0f1160b12cb8f48d2506bc"

          'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 "86-f907a3b56661163f9fa5aeb732a282f2"

          'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 "86-a81f66b089296771b23aa6b04f5abb28"

          This is reproducible. I'm now trying to understand why it occurs for updates with attachment, but not for updates of document bodies (where #doc.revs only consists of the current and new rev as well). Maybe I can find a fix then.

          Show
          Klaus Trainer added a comment - I just discovered that this issue is a bit trickier... Simply pruning #doc.revs tree doesn't seem to work for updates with attachments. When having multiple concurrent writers trying to attach a file to the same version, it's possible that more than one response with status code 201 and the same update sequence number is returned, even though only the first update succeeded for for real Here an excerpt from my logs. CouchDB log: [info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 201 [info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 201 [info] [<0.377.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 201 [info] [<0.376.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 201 Client log: 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 "85-31c1a9fb50b253aa68ac632f6e1a7626" 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 "86-1e38b77c2d0f1160b12cb8f48d2506bc" 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 "86-f907a3b56661163f9fa5aeb732a282f2" 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 "86-a81f66b089296771b23aa6b04f5abb28" This is reproducible. I'm now trying to understand why it occurs for updates with attachment, but not for updates of document bodies (where #doc.revs only consists of the current and new rev as well). Maybe I can find a fix then.
          Hide
          Klaus Trainer added a comment -

          Adam Kocoloski wrote:
          > Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?

          Yeah, I'll do that. The tests in the attached patch will detect spurious conflicts. However it's probably not the worst idea to write an additional etap test for the couch_key_tree module only.

          Paul Joseph Davis wrote:
          > Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.

          As opposed to Adam's speculation, there is an actual reason for that.

          When updating a document body, the new document body is supplied by the client. The new #doc{} that is passed to couch_doc:to_path/1 consists of the new document body supplied by the client and the revs tree containing the current, if available, and the new rev. Therefore, the resulting path contains at most two revs.

          In contrast, when updating with an attachment, the new document body is equal to the current one, and therefore isn't supplied by the client. That's the reason why couch_db:open_doc_revs/4 is called in couch_httpd_db:db_attachment_req/4. As the #doc{} that is returned by the function call contains the full revs tree, the #doc{} that's passed to couch_doc:to_path/1 does as well.

          Let me propose another (probably nicer) solution.

          We could introduce another function couch_key_tree:merge/4. The fourth argument would be a flag telling whether merge conflicts are allowed or not.

          merge/4 would call merge/3. When calling merge/4, the only difference from directly calling merge/3 would be that if the `MergeConflicts` flag is false, the revs tree is pruned.

          What do you think about that?

          Show
          Klaus Trainer added a comment - Adam Kocoloski wrote: > Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries? Yeah, I'll do that. The tests in the attached patch will detect spurious conflicts. However it's probably not the worst idea to write an additional etap test for the couch_key_tree module only. Paul Joseph Davis wrote: > Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug. As opposed to Adam's speculation, there is an actual reason for that. When updating a document body, the new document body is supplied by the client. The new #doc{} that is passed to couch_doc:to_path/1 consists of the new document body supplied by the client and the revs tree containing the current, if available, and the new rev. Therefore, the resulting path contains at most two revs. In contrast, when updating with an attachment, the new document body is equal to the current one, and therefore isn't supplied by the client. That's the reason why couch_db:open_doc_revs/4 is called in couch_httpd_db:db_attachment_req/4. As the #doc{} that is returned by the function call contains the full revs tree, the #doc{} that's passed to couch_doc:to_path/1 does as well. Let me propose another (probably nicer) solution. We could introduce another function couch_key_tree:merge/4. The fourth argument would be a flag telling whether merge conflicts are allowed or not. merge/4 would call merge/3. When calling merge/4, the only difference from directly calling merge/3 would be that if the `MergeConflicts` flag is false, the revs tree is pruned. What do you think about that?
          Hide
          Adam Kocoloski added a comment -

          I don't have a failing test, but the working assumption is that including the full #doc.revs list in an update_docs call generates spurious conflicts. Attachments are tangential, it's just the only place that we include the full #doc.revs in handling an HTTP request.

          Show
          Adam Kocoloski added a comment - I don't have a failing test, but the working assumption is that including the full #doc.revs list in an update_docs call generates spurious conflicts. Attachments are tangential, it's just the only place that we include the full #doc.revs in handling an HTTP request.
          Hide
          Paul Joseph Davis added a comment -

          Does that not work either? I can never keep things straight when we have full paths or not. If that is just as borked as the attachment stuff then I'd agree that we should fix it. Though I'm still wondering if this isn't two separate bugs.

          Show
          Paul Joseph Davis added a comment - Does that not work either? I can never keep things straight when we have full paths or not. If that is just as borked as the attachment stuff then I'd agree that we should fix it. Though I'm still wondering if this isn't two separate bugs.
          Hide
          Adam Kocoloski added a comment - - edited

          Hi Paul, I don't think there's any real reason why the full history is present for attachments. But I think a sane API should allow me to do the following as many times as I want in a loop without generating a conflict (if I'm the only writer):

          {ok, Doc}

          = couch_db:open_doc(Db, Id, []),

          {ok, _NewRev}

          = couch_db:update_doc(Db, Doc#doc{body = {[

          {<<"random">>, random:uniform()}

          ]}}, [])

          Show
          Adam Kocoloski added a comment - - edited Hi Paul, I don't think there's any real reason why the full history is present for attachments. But I think a sane API should allow me to do the following as many times as I want in a loop without generating a conflict (if I'm the only writer): {ok, Doc} = couch_db:open_doc(Db, Id, []), {ok, _NewRev} = couch_db:update_doc(Db, Doc#doc{body = {[ {<<"random">>, random:uniform()} ]}}, [])
          Hide
          Paul Joseph Davis added a comment -

          Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.

          Show
          Paul Joseph Davis added a comment - Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.
          Hide
          Adam Kocoloski added a comment -

          Hi Klaus, thanks. I'm not sure I followed your entire explanation, though. I think your point #2 is the crux of the matter. Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?

          I've definitely seen these spurious conflicts myself, and I'm excited that you're drilling down and trying to solve it.

          Show
          Adam Kocoloski added a comment - Hi Klaus, thanks. I'm not sure I followed your entire explanation, though. I think your point #2 is the crux of the matter. Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries? I've definitely seen these spurious conflicts myself, and I'm excited that you're drilling down and trying to solve it.
          Hide
          Klaus Trainer added a comment -

          First of all, thank you, Adam! Your rewrite of couch_key_tree made it way more readable.

          I think that I got to the point where I understand how the merging logic works.

          To me, the merging logic in couch_key_tree looks absolutely correct. Given the fact that I now understand what's going on there, I don't see any reasonable change we can make in couch_key_tree in order to solve this issue. We rather have to make a change somewhere else.

          To anticipate my conclusion: Pruning the revs tree of a #doc{} is necessary for updates that cannot introduce new conflicts.

          In the following, I'd like to explain why. I've attached a new patch, which fixes the issue in a more reasonable way.

          couch_key_tree:merge/3 returns an atom (`conflicts | no_conflicts`) that tells us whether an update creates a conflict or not. The returned atom is ignored as long as a certain flag `MergeConflicts` is set. More precisely, conflict checking is only active for normal interactive edits, but not for replication or bulk insertions with `all_or_nothing: true`.

          Currently, the only case where an interactive edit is done with the #doc{} containing the full revision history (instead of just the current and the new rev) is when there's an attachment (see couch_httpd_db:db_attachment_req/4).

          couch_key_tree:merge/3 is called with a list of paths and a path, whereupon the latter is the return value of couch_doc:to_path/1. Looking at couch_doc:to_path/1, one can see that the start sequence number that is computed there depends on the length of the revision history. Also, the revisions list `RevIds` is reversed. That means that for interactive edits the computed paths may be different (namely when length(RevIds) > 2), depending on whether it's an update with an attachment or not.

          To summarize my above explanation:

          1) The result of the conflict detection algorithm is only relevant for interactive edits with `MergeConflicts =:= false`. Otherwise, the result may be not correct and it's ignored anyway.

          2) The correctness of the conflict detections in couch_key_tree (namely merge_at/2 and merge_simple/2) relies on the fact that no more than two revision ids, namely the new and the current one (if available), are contained in the revs tree.

          Although my previous patch was somehow correct, and pruning the revs tree seems to be the way to go indeed, Filipe is right when saying that the fix should be applied in couch_db_updater:merge_rev_trees/7 (as opposed to in couch_httpd_db), which is what I've done now.

          Show
          Klaus Trainer added a comment - First of all, thank you, Adam! Your rewrite of couch_key_tree made it way more readable. I think that I got to the point where I understand how the merging logic works. To me, the merging logic in couch_key_tree looks absolutely correct. Given the fact that I now understand what's going on there, I don't see any reasonable change we can make in couch_key_tree in order to solve this issue. We rather have to make a change somewhere else. To anticipate my conclusion: Pruning the revs tree of a #doc{} is necessary for updates that cannot introduce new conflicts. In the following, I'd like to explain why. I've attached a new patch, which fixes the issue in a more reasonable way. couch_key_tree:merge/3 returns an atom (`conflicts | no_conflicts`) that tells us whether an update creates a conflict or not. The returned atom is ignored as long as a certain flag `MergeConflicts` is set. More precisely, conflict checking is only active for normal interactive edits, but not for replication or bulk insertions with `all_or_nothing: true`. Currently, the only case where an interactive edit is done with the #doc{} containing the full revision history (instead of just the current and the new rev) is when there's an attachment (see couch_httpd_db:db_attachment_req/4). couch_key_tree:merge/3 is called with a list of paths and a path, whereupon the latter is the return value of couch_doc:to_path/1. Looking at couch_doc:to_path/1, one can see that the start sequence number that is computed there depends on the length of the revision history. Also, the revisions list `RevIds` is reversed. That means that for interactive edits the computed paths may be different (namely when length(RevIds) > 2), depending on whether it's an update with an attachment or not. To summarize my above explanation: 1) The result of the conflict detection algorithm is only relevant for interactive edits with `MergeConflicts =:= false`. Otherwise, the result may be not correct and it's ignored anyway. 2) The correctness of the conflict detections in couch_key_tree (namely merge_at/2 and merge_simple/2) relies on the fact that no more than two revision ids, namely the new and the current one (if available), are contained in the revs tree. Although my previous patch was somehow correct, and pruning the revs tree seems to be the way to go indeed, Filipe is right when saying that the fix should be applied in couch_db_updater:merge_rev_trees/7 (as opposed to in couch_httpd_db), which is what I've done now.
          Hide
          Klaus Trainer added a comment -

          Oh, that's funny indeed. I should try to keep up again Your code and the comments on COUCHDB-968 are an interesting reading.

          Show
          Klaus Trainer added a comment - Oh, that's funny indeed. I should try to keep up again Your code and the comments on COUCHDB-968 are an interesting reading.
          Hide
          Adam Kocoloski added a comment -

          Funny you should mention that, Klaus - I just rewrote all of the merging logic in an attempt to fix COUCHDB-968.

          Show
          Adam Kocoloski added a comment - Funny you should mention that, Klaus - I just rewrote all of the merging logic in an attempt to fix COUCHDB-968 .
          Hide
          Klaus Trainer added a comment -

          Thanks Adam for bringing this issue back to my attention.

          My "point" on open_doc_revs/4 and open_doc/2 was pointless, as open_doc/2 isn't used for neither doc nor doc attachment updates.

          The point that I actually wanted to make, however, is that couch_db:update_docs/4 gets a #doc{} with only the latest rev in #doc.revs when there's a doc update without attachment, but it gets a #doc{} with the full rev list in #doc.revs when there's an update with attachment.

          To conclude that, doc updates with attachments are (AFAIK) the only situation where pruning the older revisions would be necessary. My patch would basically solve the problem, but I agree that it's better to not require that "#doc.revs must only include the latest rev".

          Also, I've tried to identify what's wrong in the conflict resolution logic, i.e., why there's a spurious conflict when the revs list > 1.

          Here are the results that I've gained so far:

          I can tell for sure that the problem is somewhere behind the couch_key_tree:merge function. It returns {_NewTree, conflict} to the couch_db_updater:merge_rev_trees function.

          I terminated my debugging session in couch_key_tree:merge_simple/2 (line 108): the second element of the tuple

          {[ATree | MTree], true}

          represents the conflicts flag. The code here is a bit tricky; I can't tell what exactly I can change to eliminate that spurious conflict without breaking anything. Maybe we should grab the guy who wrote that piece of code in order to solve this issue

          Show
          Klaus Trainer added a comment - Thanks Adam for bringing this issue back to my attention. My "point" on open_doc_revs/4 and open_doc/2 was pointless, as open_doc/2 isn't used for neither doc nor doc attachment updates. The point that I actually wanted to make, however, is that couch_db:update_docs/4 gets a #doc{} with only the latest rev in #doc.revs when there's a doc update without attachment, but it gets a #doc{} with the full rev list in #doc.revs when there's an update with attachment. To conclude that, doc updates with attachments are (AFAIK) the only situation where pruning the older revisions would be necessary. My patch would basically solve the problem, but I agree that it's better to not require that "#doc.revs must only include the latest rev". Also, I've tried to identify what's wrong in the conflict resolution logic, i.e., why there's a spurious conflict when the revs list > 1. Here are the results that I've gained so far: I can tell for sure that the problem is somewhere behind the couch_key_tree:merge function. It returns {_NewTree, conflict} to the couch_db_updater:merge_rev_trees function. I terminated my debugging session in couch_key_tree:merge_simple/2 (line 108): the second element of the tuple {[ATree | MTree], true} represents the conflicts flag. The code here is a bit tricky; I can't tell what exactly I can change to eliminate that spurious conflict without breaking anything. Maybe we should grab the guy who wrote that piece of code in order to solve this issue
          Hide
          Adam Kocoloski added a comment -

          I tend to agree with Filipe here. I've encountered the need to prune the revisions of a #doc{} before updating in the past, and it's always bothered me. It shouldn't be necessary. I also can't figure out when it is and is not needed.

          Show
          Adam Kocoloski added a comment - I tend to agree with Filipe here. I've encountered the need to prune the revisions of a #doc{} before updating in the past, and it's always bothered me. It shouldn't be necessary. I also can't figure out when it is and is not needed.
          Hide
          Filipe Manana added a comment -

          Well, yes. But that alone doesn't fix the issue. For the test Paul gave, open_doc/2 will give you the exact same value for the revs value of the #doc record.

          Prunning the revs tree (removing all parent revisions) doesn't look like to me a clean/correct way of doing it. I think the fix should be somewhere in the prep_and_validate_doc_updates functions or in couch_db_updater merge_rev_trees (or couch_key_tree)

          Show
          Filipe Manana added a comment - Well, yes. But that alone doesn't fix the issue. For the test Paul gave, open_doc/2 will give you the exact same value for the revs value of the #doc record. Prunning the revs tree (removing all parent revisions) doesn't look like to me a clean/correct way of doing it. I think the fix should be somewhere in the prep_and_validate_doc_updates functions or in couch_db_updater merge_rev_trees (or couch_key_tree)
          Hide
          Klaus Trainer added a comment -

          A remark from davisp on #couchdb IRC made we wondering why in

          Doc = case extract_header_rev(Req, couch_httpd:qs_value(Req, "rev")) of
          missing_rev -> % make the new doc
          couch_doc:validate_docid(DocId),
          #doc

          {id=DocId}

          ;
          Rev ->
          case couch_db:open_doc_revs(Db, DocId, [Rev], []) of
          {ok, [

          {ok, Doc0}

          ]} -> Doc0;

          {ok, [Error]}

          -> throw(Error)
          end
          end,

          couch_db:open_doc_revs/4 is used instead of couch_db:open_doc/2.

          I suggest that, if nobody can tell a reason for using open_doc_revs/4 instead of open_doc/2, the patch should be changed accordingly.

          Anyway, I think that this deserves a closer look.

          Show
          Klaus Trainer added a comment - A remark from davisp on #couchdb IRC made we wondering why in Doc = case extract_header_rev(Req, couch_httpd:qs_value(Req, "rev")) of missing_rev -> % make the new doc couch_doc:validate_docid(DocId), #doc {id=DocId} ; Rev -> case couch_db:open_doc_revs(Db, DocId, [Rev] , []) of {ok, [ {ok, Doc0} ]} -> Doc0; {ok, [Error]} -> throw(Error) end end, couch_db:open_doc_revs/4 is used instead of couch_db:open_doc/2. I suggest that, if nobody can tell a reason for using open_doc_revs/4 instead of open_doc/2, the patch should be changed accordingly. Anyway, I think that this deserves a closer look.
          Hide
          Klaus Trainer added a comment -

          Renamed Seq to Pos.

          Show
          Klaus Trainer added a comment - Renamed Seq to Pos.
          Hide
          Filipe Manana added a comment -

          Klaus,

          In the code, where you have Seq, it should be Pos.

          Show
          Filipe Manana added a comment - Klaus, In the code, where you have Seq, it should be Pos.
          Hide
          Klaus Trainer added a comment -

          After some debugging, I found out that the difference between an ordinary update and an update with attachment was that in the former case the doc only contained the most current rev in #doc.revs, whereas in the latter case, all revs were contained in #doc.revs.

          This is because in the latter case case, the document is retrieved with couch_db:open_doc_revs/4, whereas in the former case, the doc record is constructed using couch_httpd_db:couch_doc_from_req/3.

          If all revs are included in #doc.revs (instead of only the latest one), the merge* functions in couch_key_tree will come to the conclusion that there's a conflict. As I didn't see anything wrong with the merge* functions, I assume that the flaw lies in including more than the latest rev in #doc.revs.

          Attached is a patch that fixes the issue and provides corresponding tests as well. Testing with make check and the Futon tests suggests that everything is fine with it.

          Show
          Klaus Trainer added a comment - After some debugging, I found out that the difference between an ordinary update and an update with attachment was that in the former case the doc only contained the most current rev in #doc.revs, whereas in the latter case, all revs were contained in #doc.revs. This is because in the latter case case, the document is retrieved with couch_db:open_doc_revs/4, whereas in the former case, the doc record is constructed using couch_httpd_db:couch_doc_from_req/3. If all revs are included in #doc.revs (instead of only the latest one), the merge* functions in couch_key_tree will come to the conclusion that there's a conflict. As I didn't see anything wrong with the merge* functions, I assume that the flaw lies in including more than the latest rev in #doc.revs. Attached is a patch that fixes the issue and provides corresponding tests as well. Testing with make check and the Futon tests suggests that everything is fine with it.
          Hide
          Klaus Trainer added a comment -

          I've figured out what the actual problem here is.

          ========================================================================
          Deleting conflicts on database two:
          ========================================================================
          GET - /two/test?conflicts=true - "some plain text"
          200 -

          {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_conflicts":["2-2ee767305024673cfb3f5af037cd2729"]}

          DELETE - /two/test?rev=2-2ee767305024673cfb3f5af037cd2729 -
          200 -

          {"ok":true,"id":"test","rev":"3-89c964c5556795ee0fc45fd5213013b4"}

          ========================================================================
          Attaching to document without conflicts on database two:
          ========================================================================
          GET - /two/test - "some plain text"
          200 -

          {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3}

          PUT - /two/test/some_attachment?rev=2-871d61d84063e8fc02d64acbcdaad8ba - some plain text
          409 -

          {"error":"conflict","reason":"Document update conflict."}

          The problem is that when deleting the conflicting revision 2-2ee767305024673cfb3f5af037cd2729, just a new revision 3-89c964c5556795ee0fc45fd5213013b4 (with "_deleted":true) is created. The old revision 2-871d61d84063e8fc02d64acbcdaad8ba (that's not been deleted) is now in a kind of "deleted conflict" state with it. That is, updating revision 2-871d61d84063e8fc02d64acbcdaad8ba would create a new revision that's in conflict with the deleted one.

          You can check this e.g. with

          curl -X GET http://127.0.0.1:5984/two/test?deleted_conflicts=true

          {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_deleted_conflicts":["3-89c964c5556795ee0fc45fd5213013b4"]}

          The actual problem here is that when updating a document with an attachment, deleted conflicts are treated like normal conflicts. Supposably there's an easy solution to that issue.

          Show
          Klaus Trainer added a comment - I've figured out what the actual problem here is. ======================================================================== Deleting conflicts on database two: ======================================================================== GET - /two/test?conflicts=true - "some plain text" 200 - {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_conflicts":["2-2ee767305024673cfb3f5af037cd2729"]} DELETE - /two/test?rev=2-2ee767305024673cfb3f5af037cd2729 - 200 - {"ok":true,"id":"test","rev":"3-89c964c5556795ee0fc45fd5213013b4"} ======================================================================== Attaching to document without conflicts on database two: ======================================================================== GET - /two/test - "some plain text" 200 - {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3} PUT - /two/test/some_attachment?rev=2-871d61d84063e8fc02d64acbcdaad8ba - some plain text 409 - {"error":"conflict","reason":"Document update conflict."} The problem is that when deleting the conflicting revision 2-2ee767305024673cfb3f5af037cd2729, just a new revision 3-89c964c5556795ee0fc45fd5213013b4 (with "_deleted":true) is created. The old revision 2-871d61d84063e8fc02d64acbcdaad8ba (that's not been deleted) is now in a kind of "deleted conflict" state with it. That is, updating revision 2-871d61d84063e8fc02d64acbcdaad8ba would create a new revision that's in conflict with the deleted one. You can check this e.g. with curl -X GET http://127.0.0.1:5984/two/test?deleted_conflicts=true {"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_deleted_conflicts":["3-89c964c5556795ee0fc45fd5213013b4"]} The actual problem here is that when updating a document with an attachment, deleted conflicts are treated like normal conflicts. Supposably there's an easy solution to that issue.
          Hide
          Paul Joseph Davis added a comment -

          At first glance its not out of the realm of possibility.

          Show
          Paul Joseph Davis added a comment - At first glance its not out of the realm of possibility.
          Hide
          Sebastian Cohnen added a comment -

          Could this be related to COUCHDB-885?

          Show
          Sebastian Cohnen added a comment - Could this be related to COUCHDB-885 ?
          Hide
          Paul Joseph Davis added a comment -

          Simple python script that reproduces the issue.

          Show
          Paul Joseph Davis added a comment - Simple python script that reproduces the issue.

            People

            • Assignee:
              Unassigned
              Reporter:
              Paul Joseph Davis
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development