fix(mango): formatting of text bookmarks in _explain output

[pgj]

In the Explain output, bookmarks for queries on text (Search) indexes are added in their unpacked format to the options. This leads to at least two problems:

• it is inconsistent with how empty bookmarks are rendered for json indexes, where the string "nil" is used,

• for non-trivial bookmarks, contents cannot be translated by jiffy:encode/1 to a JSON representation so that it will crash.

Mitigate this issue by deferring the unpacking of bookmarks to the execution phase so that explain will get affected by that.

Thanks @mojito317 for reporting this issue, and thanks @rnewson for revisiting my original fix and guiding me about it.

Testing recommendations

The change comes with tests that may be run as follows.

make elixir EXUNIT_OPTS="test/elixir/test/mango/02_basic_find_test.exs test/elixir/test/partition_mango_test.exs"

and

make elixir-search EXUNIT_OPTS="test/elixir/test/mango/06_basic_text_test.exs test/elixir/test/partition_mango_test.exs"

Otherwise just try to start a _find query on a text index, grab the bookmark, and feed it into an _explain request with the same query. It should not crash any more. When running a _explain query on a text with no bookmark provided, the opts field in the body should show nil instead of an empty array as it did previously.

Checklist

• Code is written and works correctly
• Changes are covered by tests

[rnewson]

_explain works for me without this patch;

  "opts": {
    "use_index": [],
    "bookmark": "g2wAAAABaANkAA9ub2RlMUAxMjcuMC4wLjFsAAAAAmEAYn____9qaAJGP9OjegAAAABhAGo",
    "limit": 25,
    "skip": 0,
    "sort": {},
    "fields": [],

I do get this sometimes;

  "error": "invalid_ejson",
  "reason": "{{shard,<<\"shards/00000000-7fffffff/db1.1769515564\">>,'[email protected]',\n        <<\"db1\">>,\n        [0,2147483647],\n        undefined,[]},\n {0.3068528175354004,0}}",
  "ref": 1564355139

which is a different bug. attempting to jiffy:encode a shard record.

[rnewson]

also the fixup code, if any is needed, should be in mango_cursor_text module, not the generic mango_cursor module.

[rnewson]

as commented

[pgj]

_explain works for me without this patch

Hrm, so may it be that simply dreyfus_bookmark:unpack/1 is not called on some of the branches?

[rnewson]

It seems to me that we unpack the provided bookmark too early, in the create function. It is then unpacked when explain returns. The patch below moves the unpack logic to the execute function.

diff --git i/src/mango/src/mango_cursor_text.erl w/src/mango/src/mango_cursor_text.erl
index 0fc92092d..9b2e44838 100644
--- i/src/mango/src/mango_cursor_text.erl
+++ w/src/mango/src/mango_cursor_text.erl
@@ -38,7 +38,7 @@
     documents_seen
 }).

-create(Db, {Indexes, Trace}, Selector, Opts0) ->
+create(Db, {Indexes, Trace}, Selector, Opts) ->
     Index =
         case Indexes of
             [Index0] ->
@@ -48,7 +48,6 @@ create(Db, {Indexes, Trace}, Selector, Opts0) ->
         end,

     DbName = couch_db:name(Db),
-    Opts = unpack_bookmark(DbName, Opts0),
     Stats = mango_execution_stats:stats_init(DbName),

     DreyfusLimit = get_dreyfus_limit(),
@@ -87,9 +86,12 @@ execute(Cursor, UserFun, UserAcc) ->
         limit = Limit,
         skip = Skip,
         selector = Selector,
-        opts = Opts,
+        opts = Opts0,
         execution_stats = Stats
     } = Cursor,
+
+    DbName = couch_db:name(Db),
+    Opts = unpack_bookmark(DbName, Opts0),
     Query = mango_selector_text:convert(Selector),
     QueryArgs = #index_query_args{
         q = Query,

[rnewson]

(_explain worked for me briefly with dev/run because mango won't choose a search index if search is down, and it now takes longer to start clouseau, so there's a 15-20 second window after dev/run where the query falls back to _all_docs).

[pgj]

we unpack the provided bookmark too early

Excellent. I was suspecting that there may be other places where the missing conversion can be injected, but I was not sure. Thanks for checking.

[pgj]

mango won't choose a search index if search is down

Yes, I also noticed this one. I am inclined to believe that it is a regression from the Elixir conversion (hence I am cc'ing @janl here) because the Python tests bailed out before any Search-based Mango tests could be run while the Elixir tests do not seem to care about that.

A mitigation of the weird selection logic might be utilizing the allow_fallback=false configuration where both _find and _explain would be coerced to use that single text index.

[pgj]

it now takes longer to start clouseau, so there's a 15-20 second window after dev/run

I did not have problems with that. For the reference, once dev/run starts up, the search feature is immediately reported.

% dev/run -a adm:pass -n 1 --with-clouseau --no-eval "curl -s http://127.0.0.1:15984/ | jq -r '.features'"
[ * ] Setup log dir: /Users/gaborpali/projects/github/couchdb/dev/logs ... ok
[ * ] Ensure CouchDB is built ... ok
[ * ] Ensure Erlang boot script exists ... ok
[ * ] Prepare configuration files ... ok
[ * ] Start node node1 ... ok
[ * ] Start Clouseau node clouseau1 ... ok
[ * ] Check node at http://127.0.0.1:15984/ ... ok
[ * ] Check Clouseau node clouseau1 ... ok
[ * ] Running cluster setup ... ok
[ * ] Exec command curl -s http://127.0.0.1:15984/ | jq -r '.features' ... [
  "search",
  "quickjs",
  "access-ready",
  "partitioned",
  "pluggable-storage-engines",
  "reshard",
  "scheduler"
]

[rnewson]

I'm using the --with-clouseau --clouseau-dir=../clouseau so, if necessary, I can modify clouseau.

[pgj]

I'm using the --with-clouseau --clouseau-dir=../clouseau so, if necessary, I can modify clouseau.

Well, if Clouseau is run from source (basically) then it could indeed take more time because SBT will be called instead of running the JAR file immediately. That is the same for Clouseau 2.x too, but with Maven.