Fix concurrent exception related to multi-match statement

[czpmango]

What type of PR is this?

• bug
• feature
• enhancement

What problem(s) does this PR solve?

Issue(s) number:

#4430

Description:

Plan of match (v:player) where v.player.name == "Tim Duncan" MATCH (v)-[e:serve]->(n:team) return v :

In the execution plan of the above multiple match query, _AppendVertices_3 dataset is depended on by operators Filter_14 and Arguement_6 at the same time, and operators Filter_14 and Arguement_6 are scheduled concurrently, where operator Filter_14 will modify _AppendVertices_3 dataset, which cause write-read conflict. If operator Arguement_6 is scheduled after operator Filter_14, it may be undefined behavior (crash or output incorrect results).

How do you solve it?

I have considered the following options:
Option 1. Customize a special Iterator view for filter/limit, because these two operators will reuse the dataset of the pre-operator
Option 2. Use the readby member of dataset to determine the scenarios that may cause concurrent exceptions and eliminate errors that may be caused by write-read conflicts by copying the dataset
Option 3. Set the validity flag in the dataset to handle write-read conflicts
Option 4. Make the scheduler aware of possible concurrent write-read conflicts and reasonably schedule the execution order of operators

Option 1 Due to the complicated generation of various statement execution plans, for example, the iterator type of the filter of the go statement may be getNeighborsIter. If it is consistent with the previous behavior, the modification may be complicated.
Option 3 will affect the way all operators access the dataset, and the impact is relatively large, making it difficult to assess the performance impact.
Option 4. May lead to complex scheduling logic.

Therefore, the current pr adopts option 2 to ensure that it does not affect the irrelevant implementation as much as possible, and only handles scenarios with concurrent scheduling exceptions, and the performance impact is relatively small.

Checklist:

Tests:

• Unit test(positive and negative cases)
• Function test
• Performance test
• N/A

Affects:

• Documentation affected (Please add the label if documentation needs to be modified.)
• Incompatibility (If it breaks the compatibility, please describe it and add the label.）
• If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
• Performance impacted: Consumes more CPU/Memory

Special notes for reviewer

In the case of possible concurrency exceptions, it is necessary to copy data and bring additional overhead, but this overhead is necessary for correctness.
This is a patch, and a better fix may exist.
Since the problem caused by concurrency is difficult to reproduce, no test case is provided to constrain it.

[Shylock-Hg]

Please add a comment.

[czpmango]

Please add a comment.

I explained in the description of pr why I need to fix it this way, as for the logic of fixing the code is relatively simple.

[Shylock-Hg]

I think you could simply remove modify related operations from Iterator of DataSet, and keep Iterator is immutable. For Filter you could copy valid rows to new DataSet, and same for Limit or other operators need modify input DataSet.

[czpmango]

I think you could simply remove modify related operations from Iterator of DataSet, and keep Iterator is immutable. For Filter you could copy valid rows to new DataSet, and same for Limit or other operators need modify input DataSet.

Some of the previous revisions considered performance issues more comprehensively, and it is feasible to revert this refactoring if the previous considerations can be covered.
I found some previous pr for your reference:
vesoft-inc/nebula-graph#789
vesoft-inc/nebula-graph#757
vesoft-inc/nebula-graph#791

The current pr fix is feasible, and the code changes and performance impact are manageable.

[Shylock-Hg]

Intersection will call unstableErase too, I think you should check all operators call erase and eraseRange

[czpmango]

Intersection will call unstableErase too, I think you should check all operators call erase and eraseRange

The IntersectExecutor will only consume data through erase function, and will not cause concurrent read-write conflicts.

[Shylock-Hg]

Intersection will call unstableErase too, I think you should check all operators call erase and eraseRange

The IntersectExecutor will only consume data through erase function, and will not cause concurrent read-write conflicts.

If other operator reference input of Intersection, it's of course read-write conflicts.

[czpmango]

Intersection will call unstableErase too, I think you should check all operators call erase and eraseRange

The IntersectExecutor will only consume data through erase function, and will not cause concurrent read-write conflicts.

This issue will only be reproduced by the arguement operator of multiple match statements.
It will only crash in the following code block:

nebula/src/graph/executor/logic/ArgumentExecutor.cpp

Lines 18 to 36 in fc5735a

	auto iter = ectx_->getResult(argNode->inputVar()).iter();
	DCHECK(iter != nullptr);
	DataSet ds;
	ds.colNames = argNode->colNames();
	ds.rows.reserve(iter->size());
	std::unordered_set<Value> unique;
	for (; iter->valid(); iter->next()) {
	auto val = iter->getColumn(alias);
	if (!val.isVertex()) {
	continue;
	}
	if (unique.emplace(val.getVertex().vid).second) {
	Row row;
	row.values.emplace_back(std::move(val));
	ds.rows.emplace_back(std::move(row));
	}
	}
	return finish(ResultBuilder().value(Value(std::move(ds))).build());

[Shylock-Hg]

Why add this?

[czpmango]

Reuse getNeighbors's dataset. Go statements do not cause write-read conflicts.

[codecov-commenter]

Codecov Report

Merging #4605 (a051931) into master (3157fad) will increase coverage by 0.03%.
The diff coverage is 86.82%.

@@            Coverage Diff             @@
##           master    #4605      +/-   ##
==========================================
+ Coverage   84.72%   84.75%   +0.03%     
==========================================
  Files        1357     1358       +1     
  Lines      135323   135531     +208     
==========================================
+ Hits       114652   114870     +218     
+ Misses      20671    20661      -10     

Impacted Files	Coverage Δ
src/common/expression/LabelAttributeExpression.h	77.41% <0.00%> (ø)
src/graph/planner/plan/PlanNodeVisitor.h	100.00% <ø> (ø)
src/graph/planner/plan/Query.h	96.41% <ø> (+0.53%)	⬆️
src/graph/visitor/PropertyTrackerVisitor.h	100.00% <ø> (ø)
src/graph/visitor/PrunePropertiesVisitor.h	50.00% <ø> (ø)
src/daemons/MetaDaemon.cpp	67.18% <50.00%> (ø)
src/daemons/StorageDaemon.cpp	65.59% <50.00%> (ø)
src/graph/visitor/PropertyTrackerVisitor.cpp	85.11% <69.76%> (-0.68%)	⬇️
src/graph/context/Result.h	98.30% <80.00%> (-1.70%)	⬇️
...aph/optimizer/rule/PushFilterDownInnerJoinRule.cpp	85.71% <85.71%> (ø)
... and 56 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.