iilyak commented on code in PR #5558:
URL: https://github.com/apache/couchdb/pull/5558#discussion_r2155273580
##########
src/couch_mrview/src/couch_mrview_index.erl:
##########
@@ -210,7 +215,17 @@ finish_update(State) ->
commit(State) ->
Header = {State#mrst.sig, couch_mrview_util:make_header(State)},
- couch_file:write_header(State#mrst.fd, Header).
+ ok = couch_file:sync(State#mrst.fd),
Review Comment:
I understand why you do the sync here before and after. Although it feels
like we are breaking the abstraction boundaries (and bypassing ioq). The
decision to do `sync` belongs to couch application or more specifically to the
storage engine. As we do in
[`couch_bt_engine:commit_data/1`](https://github.com/apache/couchdb/blob/91bd2edb6406ddd39d735548774a384b0ec3a46f/src/couch/src/couch_bt_engine.erl#L563C1-L567C33).
Unfortunately we don't have something like it for indexes.
So I was thinking maybe we should update `couch_file` to keep the old header
checksum in the state. The checksum is a prefix of the header so it is easy to
get access to (we need to know the size of the checksum though). However
maintaining the Checksum to be able to bypass the IO is just one side of the
coin. The other is the need to wrap the write_header in two sync calls to do a
commit. This is where I stopped my train of thought. Because we would need to
be able to pass the `NeedsCommit` flag.
Maybe we should add a separate call to `couch_file`. We might have
`couch_file:write_header/2` which we wouldn't do the sync (no change) and
separate `couch_file:commit_header/2` which would do sync calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
