From b66e52ab9992654e1cb5ac626f0427270f0fd122 Mon Sep 17 00:00:00 2001 From: Eric Avdey Date: Thu, 30 Nov 2017 13:17:35 -0400 Subject: Add missing methods to fake index Mocked index module missing a couple of methods called on late compaction stages. This leads to a crash, but since it's happening after the test's assertions, it bring the test to fail. Also small refactoring to encapsulate all mocking in a single function and move unrelated parts back to test's setup. --- src/couch_index/test/couch_index_compaction_tests.erl | 12 +++++++----- src/couch_index/test/couch_index_ddoc_updated_tests.erl | 3 ++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/couch_index/test/couch_index_compaction_tests.erl b/src/couch_index/test/couch_index_compaction_tests.erl index 062be872a..164e9836a 100644 --- a/src/couch_index/test/couch_index_compaction_tests.erl +++ b/src/couch_index/test/couch_index_compaction_tests.erl @@ -21,12 +21,12 @@ setup() -> DbName = ?tempdb(), {ok, Db} = couch_db:create(DbName, [?ADMIN_CTX]), couch_db:close(Db), - {ok, IndexerPid} = fake_index(Db), + fake_index(DbName), + {ok, IndexerPid} = couch_index_server:get_index(test_index, Db, undefined), ?assertNot(is_opened(Db)), {Db, IndexerPid}. -fake_index(Db) -> - DbName = couch_db:name(Db), +fake_index(DbName) -> ok = meck:new([test_index], [non_strict]), ok = meck:expect(test_index, init, ['_', '_'], {ok, 10}), ok = meck:expect(test_index, open, fun(_Db, State) -> @@ -45,8 +45,10 @@ fake_index(Db) -> (update_seq, Seq) -> Seq end), - - couch_index_server:get_index(test_index, Db, undefined). + ok = meck:expect(test_index, close, ['_'], ok), + ok = meck:expect(test_index, swap_compacted, fun(_, NewState) -> + {ok, NewState} + end). teardown(_) -> (catch meck:unload(test_index)), diff --git a/src/couch_index/test/couch_index_ddoc_updated_tests.erl b/src/couch_index/test/couch_index_ddoc_updated_tests.erl index d1bbc43d2..aaf36c71f 100644 --- a/src/couch_index/test/couch_index_ddoc_updated_tests.erl +++ b/src/couch_index/test/couch_index_ddoc_updated_tests.erl @@ -121,7 +121,8 @@ fake_index() -> crypto:hash(md5, term_to_binary(DDoc)); (update_seq, Seq) -> Seq - end). + end), + ok = meck:expect(test_index, shutdown, ['_'], ok). get_indexes_by_ddoc(DDocID, N) -> -- cgit v1.2.1 From ccb657ea736d21c35ff967cabad149ee5bda4431 Mon Sep 17 00:00:00 2001 From: Eric Avdey Date: Thu, 30 Nov 2017 13:25:01 -0400 Subject: Remove invalid meck unload Remove invalid meck unload and catch around unload that was hiding this issue. --- src/couch_index/test/couch_index_compaction_tests.erl | 4 +--- src/couch_index/test/couch_index_ddoc_updated_tests.erl | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/couch_index/test/couch_index_compaction_tests.erl b/src/couch_index/test/couch_index_compaction_tests.erl index 164e9836a..53316d944 100644 --- a/src/couch_index/test/couch_index_compaction_tests.erl +++ b/src/couch_index/test/couch_index_compaction_tests.erl @@ -51,9 +51,7 @@ fake_index(DbName) -> end). teardown(_) -> - (catch meck:unload(test_index)), - (catch meck:unload(couch_util)), - ok. + meck:unload(test_index). compaction_test_() -> { diff --git a/src/couch_index/test/couch_index_ddoc_updated_tests.erl b/src/couch_index/test/couch_index_ddoc_updated_tests.erl index aaf36c71f..40dadcc62 100644 --- a/src/couch_index/test/couch_index_ddoc_updated_tests.erl +++ b/src/couch_index/test/couch_index_ddoc_updated_tests.erl @@ -25,7 +25,7 @@ start() -> stop({Ctx, DbName}) -> - (catch meck:unload(test_index)), + meck:unload(test_index), ok = fabric:delete_db(DbName, [?ADMIN_CTX]), DbDir = config:get("couchdb", "database_dir", "."), WaitFun = fun() -> -- cgit v1.2.1 From 743bd8820cf612e830c1ff6dd0b9b07c12aab8fb Mon Sep 17 00:00:00 2001 From: Will Holley Date: Thu, 30 Nov 2017 19:53:19 +0000 Subject: warn instead of error when use_index not valid (#962) If a user specifies a value for use_index that is not valid for the selector - i.e. it does not meet the coverage requirements of the selector or sort fields - attempt to fall back to a valid index (or database scan) rather than returning a 400 error. When a fallback occurs, populate the "warning" field in the response (as we already do when a full database scan takes place) with details of the fallback. This change is partially as mitigation for #816, which may lead to some previously valid indexes being deemed invalid, and also to make use_index less brittle in general. If an index that is used explicitly by active queries is removed, Couch will now generate warnings and there may be a performance impact, but the client will still get correct results. --- src/mango/src/mango_cursor.erl | 76 ++++++++++++++++++++++------- src/mango/src/mango_cursor_text.erl | 2 +- src/mango/src/mango_cursor_view.erl | 2 +- src/mango/src/mango_error.erl | 19 -------- src/mango/src/mango_idx.erl | 14 ++---- src/mango/test/05-index-selection-test.py | 80 ++++++++++++++++++------------- 6 files changed, 112 insertions(+), 81 deletions(-) diff --git a/src/mango/src/mango_cursor.erl b/src/mango/src/mango_cursor.erl index 7997d9ada..5108d36b2 100644 --- a/src/mango/src/mango_cursor.erl +++ b/src/mango/src/mango_cursor.erl @@ -18,6 +18,7 @@ explain/1, execute/3, maybe_filter_indexes_by_ddoc/2, + remove_indexes_with_partial_filter_selector/1, maybe_add_warning/3 ]). @@ -47,16 +48,18 @@ create(Db, Selector0, Opts) -> Selector = mango_selector:normalize(Selector0), UsableIndexes = mango_idx:get_usable_indexes(Db, Selector, Opts), - - {use_index, IndexSpecified} = proplists:lookup(use_index, Opts), - case {length(UsableIndexes), length(IndexSpecified)} of - {0, 0} -> + case length(UsableIndexes) of + 0 -> AllDocs = mango_idx:special(Db), create_cursor(Db, AllDocs, Selector, Opts); - {0, _} -> - ?MANGO_ERROR({no_usable_index, selector_unsupported}); _ -> - create_cursor(Db, UsableIndexes, Selector, Opts) + case mango_cursor:maybe_filter_indexes_by_ddoc(UsableIndexes, Opts) of + [] -> + % use_index doesn't match a valid index - fall back to a valid one + create_cursor(Db, UsableIndexes, Selector, Opts); + UserSpecifiedIndex -> + create_cursor(Db, UserSpecifiedIndex, Selector, Opts) + end end. @@ -90,9 +93,7 @@ execute(#cursor{index=Idx}=Cursor, UserFun, UserAcc) -> maybe_filter_indexes_by_ddoc(Indexes, Opts) -> case lists:keyfind(use_index, 1, Opts) of {use_index, []} -> - % We remove any indexes that have a selector - % since they are only used when specified via use_index - remove_indexes_with_partial_filter_selector(Indexes); + []; {use_index, [DesignId]} -> filter_indexes(Indexes, DesignId); {use_index, [DesignId, ViewName]} -> @@ -150,12 +151,53 @@ group_indexes_by_type(Indexes) -> end, ?CURSOR_MODULES). -maybe_add_warning(UserFun, #idx{type = IndexType}, UserAcc) -> - case IndexType of +maybe_add_warning(UserFun, #cursor{index = Index, opts = Opts}, UserAcc) -> + NoIndexWarning = case Index#idx.type of <<"special">> -> - Arg = {add_key, warning, <<"no matching index found, create an index to optimize query time">>}, - {_Go, UserAcc0} = UserFun(Arg, UserAcc), - UserAcc0; + <<"no matching index found, create an index to optimize query time">>; _ -> - UserAcc - end. \ No newline at end of file + ok + end, + + UseIndexInvalidWarning = case lists:keyfind(use_index, 1, Opts) of + {use_index, []} -> + NoIndexWarning; + {use_index, [DesignId]} -> + case filter_indexes([Index], DesignId) of + [] -> + fmt("_design/~s was not used because it does not contain a valid index for this query.", + [ddoc_name(DesignId)]); + _ -> + NoIndexWarning + end; + {use_index, [DesignId, ViewName]} -> + case filter_indexes([Index], DesignId, ViewName) of + [] -> + fmt("_design/~s, ~s was not used because it is not a valid index for this query.", + [ddoc_name(DesignId), ViewName]); + _ -> + NoIndexWarning + end + end, + + maybe_add_warning_int(UseIndexInvalidWarning, UserFun, UserAcc). + + +maybe_add_warning_int(ok, _, UserAcc) -> + UserAcc; + +maybe_add_warning_int(Warning, UserFun, UserAcc) -> + Arg = {add_key, warning, Warning}, + {_Go, UserAcc0} = UserFun(Arg, UserAcc), + UserAcc0. + + +fmt(Format, Args) -> + iolist_to_binary(io_lib:format(Format, Args)). + + +ddoc_name(<<"_design/", Name/binary>>) -> + Name; + +ddoc_name(Name) -> + Name. diff --git a/src/mango/src/mango_cursor_text.erl b/src/mango/src/mango_cursor_text.erl index 88abfc00a..3883bc8f2 100644 --- a/src/mango/src/mango_cursor_text.erl +++ b/src/mango/src/mango_cursor_text.erl @@ -124,7 +124,7 @@ execute(Cursor, UserFun, UserAcc) -> Arg = {add_key, bookmark, JsonBM}, {_Go, FinalUserAcc} = UserFun(Arg, LastUserAcc), FinalUserAcc0 = mango_execution_stats:maybe_add_stats(Opts, UserFun, Stats0, FinalUserAcc), - FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Idx, FinalUserAcc0), + FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Cursor, FinalUserAcc0), {ok, FinalUserAcc1} end. diff --git a/src/mango/src/mango_cursor_view.erl b/src/mango/src/mango_cursor_view.erl index 7c57b1414..1e2108b7d 100644 --- a/src/mango/src/mango_cursor_view.erl +++ b/src/mango/src/mango_cursor_view.erl @@ -137,7 +137,7 @@ execute(#cursor{db = Db, index = Idx, execution_stats = Stats} = Cursor0, UserFu {_Go, FinalUserAcc} = UserFun(Arg, LastCursor#cursor.user_acc), Stats0 = LastCursor#cursor.execution_stats, FinalUserAcc0 = mango_execution_stats:maybe_add_stats(Opts, UserFun, Stats0, FinalUserAcc), - FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Idx, FinalUserAcc0), + FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Cursor, FinalUserAcc0), {ok, FinalUserAcc1}; {error, Reason} -> {error, Reason} diff --git a/src/mango/src/mango_error.erl b/src/mango/src/mango_error.erl index 4c55ef3f6..ad665e2f3 100644 --- a/src/mango/src/mango_error.erl +++ b/src/mango/src/mango_error.erl @@ -21,31 +21,12 @@ ]). -info(mango_idx, {no_usable_index, no_indexes_defined}) -> - { - 400, - <<"no_usable_index">>, - <<"There are no indexes defined in this database.">> - }; -info(mango_idx, {no_usable_index, no_index_matching_name}) -> - { - 400, - <<"no_usable_index">>, - <<"No index matches the index specified with \"use_index\"">> - }; info(mango_idx, {no_usable_index, missing_sort_index}) -> { 400, <<"no_usable_index">>, <<"No index exists for this sort, try indexing by the sort fields.">> }; -info(mango_cursor, {no_usable_index, selector_unsupported}) -> - { - 400, - <<"no_usable_index">>, - <<"The index specified with \"use_index\" is not usable for the query.">> - }; - info(mango_json_bookmark, {invalid_bookmark, BadBookmark}) -> { 400, diff --git a/src/mango/src/mango_idx.erl b/src/mango/src/mango_idx.erl index 8e19ebff8..ea5949c02 100644 --- a/src/mango/src/mango_idx.erl +++ b/src/mango/src/mango_idx.erl @@ -59,20 +59,16 @@ list(Db) -> get_usable_indexes(Db, Selector, Opts) -> ExistingIndexes = mango_idx:list(Db), - if ExistingIndexes /= [] -> ok; true -> - ?MANGO_ERROR({no_usable_index, no_indexes_defined}) - end, - FilteredIndexes = mango_cursor:maybe_filter_indexes_by_ddoc(ExistingIndexes, Opts), - if FilteredIndexes /= [] -> ok; true -> - ?MANGO_ERROR({no_usable_index, no_index_matching_name}) - end, + GlobalIndexes = mango_cursor:remove_indexes_with_partial_filter_selector(ExistingIndexes), + UserSpecifiedIndex = mango_cursor:maybe_filter_indexes_by_ddoc(ExistingIndexes, Opts), + UsableIndexes0 = lists:usort(GlobalIndexes ++ UserSpecifiedIndex), SortFields = get_sort_fields(Opts), UsableFilter = fun(I) -> is_usable(I, Selector, SortFields) end, - UsableIndexes0 = lists:filter(UsableFilter, FilteredIndexes), + UsableIndexes1 = lists:filter(UsableFilter, UsableIndexes0), - case maybe_filter_by_sort_fields(UsableIndexes0, SortFields) of + case maybe_filter_by_sort_fields(UsableIndexes1, SortFields) of {ok, SortIndexes} -> SortIndexes; {error, no_usable_index} -> diff --git a/src/mango/test/05-index-selection-test.py b/src/mango/test/05-index-selection-test.py index ef662a918..eec0bd9a6 100644 --- a/src/mango/test/05-index-selection-test.py +++ b/src/mango/test/05-index-selection-test.py @@ -66,12 +66,8 @@ class IndexSelectionTests: def test_invalid_use_index(self): # ddoc id for the age index ddocid = "_design/ad3d537c03cd7c6a43cf8dff66ef70ea54c2b40f" - try: - self.db.find({}, use_index=ddocid) - except Exception as e: - self.assertEqual(e.response.status_code, 400) - else: - raise AssertionError("bad find") + r = self.db.find({}, use_index=ddocid, return_raw=True) + self.assertEqual(r["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid)) def test_uses_index_when_no_range_or_equals(self): # index on ["manager"] should be valid because @@ -87,19 +83,18 @@ class IndexSelectionTests: resp_explain = self.db.find(selector, explain=True) self.assertEqual(resp_explain["index"]["type"], "json") - def test_reject_use_index_invalid_fields(self): # index on ["company","manager"] which should not be valid ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198" selector = { "company": "Pharmex" } - try: - self.db.find(selector, use_index=ddocid) - except Exception as e: - self.assertEqual(e.response.status_code, 400) - else: - raise AssertionError("did not reject bad use_index") + r = self.db.find(selector, use_index=ddocid, return_raw=True) + self.assertEqual(r["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid)) + + # should still return a correct result + for d in r["docs"]: + self.assertEqual(d["company"], "Pharmex") def test_reject_use_index_ddoc_and_name_invalid_fields(self): # index on ["company","manager"] which should not be valid @@ -108,41 +103,58 @@ class IndexSelectionTests: selector = { "company": "Pharmex" } - try: - self.db.find(selector, use_index=[ddocid,name]) - except Exception as e: - self.assertEqual(e.response.status_code, 400) - else: - raise AssertionError("did not reject bad use_index") + + resp = self.db.find(selector, use_index=[ddocid,name], return_raw=True) + self.assertEqual(resp["warning"], "{0}, {1} was not used because it is not a valid index for this query.".format(ddocid, name)) + + # should still return a correct result + for d in resp["docs"]: + self.assertEqual(d["company"], "Pharmex") def test_reject_use_index_sort_order(self): # index on ["company","manager"] which should not be valid + # and there is no valid fallback (i.e. an index on ["company"]) ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198" selector = { - "company": {"$gt": None}, - "manager": {"$gt": None} + "company": {"$gt": None} } try: - self.db.find(selector, use_index=ddocid, sort=[{"manager":"desc"}]) + self.db.find(selector, use_index=ddocid, sort=[{"company":"desc"}]) except Exception as e: self.assertEqual(e.response.status_code, 400) else: raise AssertionError("did not reject bad use_index") - def test_reject_use_index_ddoc_and_name_sort_order(self): - # index on ["company","manager"] which should not be valid - ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198" - name = "a0c425a60cf3c3c09e3c537c9ef20059dcef9198" + def test_use_index_fallback_if_valid_sort(self): + ddocid_valid = "_design/fallbackfoo" + ddocid_invalid = "_design/fallbackfoobar" + self.db.create_index(fields=["foo"], ddoc=ddocid_invalid) + self.db.create_index(fields=["foo", "bar"], ddoc=ddocid_valid) selector = { - "company": {"$gt": None}, - "manager": {"$gt": None} + "foo": {"$gt": None} } - try: - self.db.find(selector, use_index=[ddocid,name], sort=[{"manager":"desc"}]) - except Exception as e: - self.assertEqual(e.response.status_code, 400) - else: - raise AssertionError("did not reject bad use_index") + + resp_explain = self.db.find(selector, sort=["foo", "bar"], use_index=ddocid_invalid, explain=True) + self.assertEqual(resp_explain["index"]["ddoc"], ddocid_valid) + + resp = self.db.find(selector, sort=["foo", "bar"], use_index=ddocid_invalid, return_raw=True) + self.assertEqual(resp["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid_invalid)) + self.assertEqual(len(resp["docs"]), 0) + + def test_prefer_use_index_over_optimal_index(self): + # index on ["company"] even though index on ["company", "manager"] is better + ddocid_preferred = "_design/testsuboptimal" + self.db.create_index(fields=["baz"], ddoc=ddocid_preferred) + self.db.create_index(fields=["baz", "bar"]) + selector = { + "baz": {"$gt": None}, + "bar": {"$gt": None} + } + resp = self.db.find(selector, use_index=ddocid_preferred, return_raw=True) + self.assertTrue("warning" not in resp) + + resp_explain = self.db.find(selector, use_index=ddocid_preferred, explain=True) + self.assertEqual(resp_explain["index"]["ddoc"], ddocid_preferred) # This doc will not be saved given the new ddoc validation code # in couch_mrview -- cgit v1.2.1