Fix _local_docs end-point

[eiri]

Overview

This is a second attempt to fix _local_docs end-point. The previous one didn't work on big enough btree_local, because local btree doesn't have reduction fun, so reuse of couch_db_updater:btree_by_id_reduce/2 was crashing on a bad match as soon as btree_local was getting kp_node. Also using full fold to calculate total_rows value turned out to be resource expensive when a database have significant number of local documents.

This fix avoids calculating of total_rows and offset instead always setting them to null and also setting to null update_seq when requested, since it doesn't have meaning in context of local documents.

A fabric module fabric_view_all_docs.erl was copied and modified as fabric_view_local_docs.erl, because re-using it for processing of both types of the documents was getting rather convoluted.

Testing recommendations

Unit tests for all invoked dependencies could be run as make eunit apps=chttpd,fabric,couch_mrview. There are updated suite couch_mrview_local_docs_tests in couch_mrview.

From HTTP perspective end-point /{db}/_local_docs supports all the query parameters of /{db}/_all_docs with the difference that offset, 'total_rowsandupdate_seqalways returnnull`.

JIRA issue number

COUCHDB-3337

Checklist

• Code is written and works correctly;
• Changes are covered by tests;
• Documentation reflects the changes;

[davisp]

Mostly +1 but I'm not seeing the need for fabric_view_local_docs. There does seem to be a few places that'd have to get ironed out but that seems like a lot of code duplication for the few places I spotted skimming through. Can you elaborate which bits made it hard to include as part of fabric_view_all_docs?

[eiri]

@davisp sorry, I totally forgot about this PR!

I tried modifying fabric_view_all_docs initially and then went with new module for three reasons:

1. code got somehow confusing for first-time readers with usage of all null, numbers for offset and total and nil for exclusion of update_seq.
2. code around that spawned process for getting doc count in go/5 for POST with keys was rather ugly and running the whole thing as it is and then just throwing away the result felt like a waste.
3. shared config parameters and error log messages saying _all_docs were going to be confusing in production

I agree there are lot of repetition, but it felt more cleaner to separate this functionality. If you think it's excessive I can amend fabric_view_all_docs instead.

[eiri]

@davisp So I went ahead, reverted the new module and modified fabric_view_all_docs.erl instead. Does it looks better to you?

[davisp]

+1

Squerge away!

[nickva]

Squerged.

But also see a badarith exception when testing _local_docs endpoint by hand:

did a few:

http  post 'http://adm:pass@localhost:15984/d' x=1 _id=_local/1
{
    "id": "_local/1",
    "ok": true,
    "rev": "0-1"
}

Then see the exception with:

http get 'http://adm:pass@localhost:15984/d/_local_docs'
HTTP/1.1 500 Internal Server Error
{
    "error": "unknown_error",
    "reason": "badarith",
    "ref": 2604624769
}

Logs show:

2017-07-17T18:25:22.809235Z node1@127.0.0.1 <0.12977.0> e7fdbd92ef req_err(2604624769) unknown_error : badarith
    [<<"fabric_view_all_docs:handle_message/3 L157">>,<<"rexi_utils:process_mailbox/6 L55">>,<<"fabric_view_all_docs:go/6 L123">>,<<"fabric_view_all_docs:go/5 L32">>,<<"chttpd_db:all_docs_view/4 L644">>,<<"chttpd:process_request/1 L295">>,<<"chttpd:handle_request_int/1 L231">>,<<"mochiweb_http:headers/6 L122">>]

Will have to track this down

[davisp]

Huh. guessing that's in the totals or offset handling.

[nickva]

It looks like in fabic rpc view_cb/2 meta comes back with total=null, offset=null but sometimes as total=null, offset=0. Since offset=0 is a number we attempt to add to null in in the handle_message callback when we aggregate numbers: https://github.com/apache/couchdb/pull/526/files#diff-9d6b535641fc74e993a6c0a246313619R157 (there Offset0 is a number).

[nickva]

@davisp Take another look when you get a chance.

The reason for badarith was that couch_db:enum_docs was returning a 0 offset for _local docs if there were no docs in the shard range. If there were docs then it was correctly returning null. The coordinator ended up adding getting a mix of 0s and nulls and was trying to add them together.

[davisp]

+1

Good catch!