[GitHub] [couchdb] kocolosk commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps

[GitBox <gi...@apache.org>]

kocolosk commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588890781



##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -126,14 +127,29 @@ get_total_view_size(TxDb, Mrst) ->
     end, 0, Mrst#mrst.views).
 
 
-read_view(Db, Mrst, ViewName, Callback, Acc0, Args) ->
+read_view(Db, Mrst, ViewName, Callback, Acc0, Args, {_, _} = IdxVStamps) ->
+    {IndexerGRV, CommittedVer} = IdxVStamps,

Review comment:
       Here and elsewhere in the `read_view` code path I think it might help future devs to use names that more directly indicate what the purpose of these two versions are, e.g. `DbReadVsn` and `ViewReadVsn` or similar.
   

##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
                     update_stats := UpdateStats
                 }}
         end
-    end).
+    end),
+    case TxResult of
+        {Mrst, continue, State} ->
+            {Mrst, State};
+        {Mrst, finished, State} ->
+            #{tx_db := OldDb} = State,
+            CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+            fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->

Review comment:
       It'd be good to avoid the roundtrip to acquire a new read version here. Can we configure the new transaction to use the previous one's committed version as its read version?

##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -53,11 +53,12 @@ query(Db, DDoc, ViewName, Callback, Acc0, Args0) ->
     try
         fabric2_fdb:transactional(Db, fun(TxDb) ->
             ok = maybe_update_view(TxDb, Mrst, IsInteractive, Args3),
-            read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3)
+            IdxVStamps = {current, current},
+            read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)
         end)
     catch throw:{build_view, WaitSeq} ->
-        couch_views_jobs:build_view(Db, Mrst, WaitSeq),
-        read_view(Db, Mrst, ViewName, Callback, Acc0, Args3)
+        {ok, IdxVStamps} = couch_views_jobs:build_view(Db, Mrst, WaitSeq),
+        read_view(Db, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)

Review comment:
       I don't know the answer to this question, but is there an extra unneeded GRV roundtrip here? The `read_view` function is going to `fabric2_fdb:transactional` to start a transaction, but we already have the read version we want to use so a roundtrip to FDB to acquire one would be a waste. I just don't know if that GRV operation happens before we set the read version inside the transaction.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org