Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 18079 bytes --]

Hi! Thanks for the fixes!

See 2 comments below.

> static inline void
> error_unref(struct error *e)
> {
> 	assert(e->refs > 0);
> 	struct error *to_delete = e;
> 	while (--to_delete->refs == 0) {
> 		/* Unlink error from lists completely.*/
> 		struct error *cause = to_delete->cause;
> 		if (to_delete->effect != NULL)
> 			to_delete->effect->cause = to_delete->cause;

1. Looks like 'effect' should be always NULL here. Because
if it is not NULL, then effect->cause == e, and it would keep a
reference, making e->refs == 0 impossible. I applied the diff
below and all the tests passed.

====================
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 3a817a659..276bf2362 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -117,12 +117,11 @@ error_unref(struct error *e)
 	while (--to_delete->refs == 0) {
 		/* Unlink error from lists completely.*/
 		struct error *cause = to_delete->cause;
-		if (to_delete->effect != NULL)
-			to_delete->effect->cause = to_delete->cause;
-		if (to_delete->cause != NULL)
-			to_delete->cause->effect = to_delete->effect;
-		to_delete->cause = NULL;
-		to_delete->effect = NULL;
+		assert(to_delete->effect == NULL);
+		if (to_delete->cause != NULL) {
+			to_delete->cause->effect = NULL;
+			to_delete->cause = NULL;
+		}
 		to_delete->destroy(to_delete);
 		if (cause == NULL)
 			return;
====================

> 		if (to_delete->cause != NULL)
> 			to_delete->cause->effect = to_delete->effect;
> 		to_delete->cause = NULL;
> 		to_delete->effect = NULL;
> 		to_delete->destroy(to_delete);
> 		if (cause == NULL)
> 			return;
> 		to_delete = cause;
> 	}
> }>>> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
>>> index c350abb4a..57da5da44 100644
>>> --- a/src/lib/core/diag.c
>>> +++ b/src/lib/core/diag.c
>>> @@ -34,6 +34,43 @@
>>>  /* Must be set by the library user */
>>>  struct error_factory *error_factory = NULL;
>>>  
>>> +int
>>> +error_set_prev(struct error *e, struct error *prev)
>>> +{
>>> +	/*
>>> +	 * Make sure that adding error won't result in cycles.
>>> +	 * Don't bother with sophisticated cycle-detection
>>> +	 * algorithms, simple iteration is OK since as a rule
>>> +	 * list contains a dozen errors at maximum.
>>> +	 */
>>> +	struct error *tmp = prev;
>>> +	while (tmp != NULL) {
>>> +		if (tmp == e)
>>> +			return -1;
>>> +		tmp = tmp->cause;
>>> +	}
>>
>> 3. Or we could remove 'prev' from its current stack gracefully,
>> since the list is doubly linked. Then cycles become impossible.
>> Is there a reason not to do that? I mean, we remove 'prev'
>> from its old stack by changing its 'effect' already anyway.
>>
>> Can do the same for the other pointer, and no need to check
>> for cycles anymore.
>> I can only think of a problem when someone makes
>>
>>     e1 = box.error.new()
>>     e2 = box.error.new()
>>     e2:set_prev(e1)
>>
>> and then
>>
>>     e4 = box.error.new()
>>     e4:set_prev(e1)
>>
>> assuming that e1 will be copied. But it will steal e1 from
>> its old stack. Even with your current solution. Maybe to make
>> it more protected against dummies we should forbid moving error
>> from one stack to another? So an error can't change its 'cause'
>> and 'effect' once they become not NULL. In that way we eliminate
>> any uncertainties and in future can decide whether we want to copy
>> in set_prev() when necessary, or move from previous stack. Or we
>> will keep this forbidden because probably no one does moving
>> between stacks, nor the example I showed above. This also
>> significantly simplifies tests and reduces probability of having
>> a bug there. What are your thoughts?
> 
> I'm okay with current approach. It is all about documentation - 
> the better feature is documented, the easier it turns out to
> be in terms of usage. There's always way to misuse any feature
> however well designed. If you insist on some changes - let me know.

2. I don't insist. Can only recommend. And I would strongly recommend
to cut the ability to move errors between stacks. I see only pros in
cutting that. Because it is never needed (I can't imagine a sane case),
can be surprising for someone, and complicates the code. The server
code. Makes it easier to leave a bug here. Another reason is that it is
easier to add something later if someone will need it, than to forbid
something after introduction.

Besides, it seems we could even make the error list unidirectional then.
Instead of 'effect' pointer we could keep a flag saying whether this error
belongs to a stack. No more complex handling of when to do ref/unref/cycles
and so on. Even bigger simplification.

I decided to try that and got a patch. See it attached to the email as
'unidirect.txt', and below. I didn't push it on the branch so as not to
change next commits.

On the summary, I made the list unidirectional, removed the cycle
detection and forbidden any chance of creating it by banning change of
error object when it is not on top of the stack. So a stack should be
built always in one order - from the cause of all causes to top. The
only thing you can do is to cut a tail of a stack. All more complex
actions which may be needed in very rare cases still can be implemented
by users using the provided basic API.

That made the patch 110 lines shorter.


Also a small comment about cycle detection in your patch. Here I
would strongly-strongly recommend to avoid it when it is clearly
not necessary.

You can't create a cycle, if e->effect and prev->effect are NULL.
Which is the case in 99% of cases. Even if prev->cause is not NULL,
you still can't create a cycle when both effects are NULL. See diff
below and attached as 'opt-cycle.txt'. That skips 'prev' iteration
even if it is a whole another stack, but 'prev' is its top.

Optimized cycle detection:
====================
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index ed8beb58d..64ff5e4d6 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -43,11 +43,28 @@ error_set_prev(struct error *e, struct error *prev)
 	 * algorithms, simple iteration is OK since as a rule
 	 * list contains a dozen errors at maximum.
 	 */
-	struct error *tmp = prev;
-	while (tmp != NULL) {
-		if (tmp == e)
+	if (prev != NULL) {
+		if (e == prev)
 			return -1;
-		tmp = tmp->cause;
+		if (prev->effect != NULL || e->effect != NULL) {
+			/*
+			 * e and prev already compared, so start
+			 * from prev->cause.
+			 */
+			struct error *tmp = prev->cause;
+			while (tmp != NULL) {
+				if (tmp == e)
+					return -1;
+				tmp = tmp->cause;
+			}
+			/*
+			 * Unlink new 'effect' node from its old
+			 * list of 'cause' errors.
+			 */
+			error_unlink_effect(prev);
+		}
+		error_ref(prev);
+		prev->effect = e;
 	}
 	/*
 	 * At once error can feature only one reason.
@@ -59,15 +76,6 @@ error_set_prev(struct error *e, struct error *prev)
 	}
 	/* Set new 'prev' node. */
 	e->cause = prev;
-	/*
-	 * Unlink new 'effect' node from its old list of 'cause'
-	 * errors. nil can be also passed as an argument.
-	 */
-	if (prev != NULL) {
-		error_ref(prev);
-		error_unlink_effect(prev);
-		prev->effect = e;
-	}
 	return 0;
 }
====================


Simplified error list patch:
====================
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index ed8beb58d..69a19d91f 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -37,37 +37,32 @@ struct error_factory *error_factory = NULL;
 int
 error_set_prev(struct error *e, struct error *prev)
 {
-	/*
-	 * Make sure that adding error won't result in cycles.
-	 * Don't bother with sophisticated cycle-detection
-	 * algorithms, simple iteration is OK since as a rule
-	 * list contains a dozen errors at maximum.
-	 */
-	struct error *tmp = prev;
-	while (tmp != NULL) {
-		if (tmp == e)
+	if (e == prev)
+		return -1;
+	if (prev != NULL) {
+		/*
+		 * It is not allowed to change middle of the error
+		 * stack. Except when the tail is cut. Not
+		 * replaced. Reason is to make the code simpler,
+		 * and avoid any necessity to detect cycles. In
+		 * that implementation cycles are not allowed, and
+		 * this guarantee costs nothing.
+		 */
+		if (prev->has_effect || e->has_effect)
 			return -1;
-		tmp = tmp->cause;
+		prev->has_effect = true;
+		error_ref(prev);
 	}
 	/*
 	 * At once error can feature only one reason.
 	 * So unlink previous 'cause' node.
 	 */
 	if (e->cause != NULL) {
-		e->cause->effect = NULL;
+		e->cause->has_effect = false;
 		error_unref(e->cause);
 	}
 	/* Set new 'prev' node. */
 	e->cause = prev;
-	/*
-	 * Unlink new 'effect' node from its old list of 'cause'
-	 * errors. nil can be also passed as an argument.
-	 */
-	if (prev != NULL) {
-		error_ref(prev);
-		error_unlink_effect(prev);
-		prev->effect = e;
-	}
 	return 0;
 }
 
@@ -91,7 +86,7 @@ error_create(struct error *e,
 	}
 	e->errmsg[0] = '\0';
 	e->cause = NULL;
-	e->effect = NULL;
+	e->has_effect = false;
 }
 
 struct diag *
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 3a817a659..52987befa 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -85,22 +85,20 @@ struct error {
 	/* Error description. */
 	char errmsg[DIAG_ERRMSG_MAX];
 	/**
-	 * Link to the cause and effect of given error. The cause
-	 * creates the effect:
+	 * Cause of the given error..
 	 * e1 = box.error.new({code = 0, reason = 'e1'})
 	 * e2 = box.error.new({code = 0, reason = 'e2'})
-	 * e1:set_prev(e2) -- Now e2 is the cause of e1 and e1 is
-	 * the effect of e2.
-	 * Only cause keeps reference to avoid cyclic dependence.
-	 * RLIST implementation is not really suitable here
-	 * since it is organized as circular list. In such
-	 * a case it is impossible to start an iteration
-	 * from any node and finish at the logical end of the
-	 * list. Double-linked list is required to allow deletion
-	 * from the middle of the list.
+	 * e1:set_prev(e2) -- Now e2 is the cause of e1.
 	 */
 	struct error *cause;
-	struct error *effect;
+	/**
+	 * Flag whether this error is not top of the error stack.
+	 * I.e. it has an 'effect'. Effect is e1 in the example
+	 * above. The flag is used to prevent changing middle of
+	 * the stack (except when it is cut off - this easy to
+	 * support, and can't create a cycle).
+	 */
+	bool has_effect;
 };
 
 static inline void
@@ -115,51 +113,24 @@ error_unref(struct error *e)
 	assert(e->refs > 0);
 	struct error *to_delete = e;
 	while (--to_delete->refs == 0) {
-		/* Unlink error from lists completely.*/
 		struct error *cause = to_delete->cause;
-		if (to_delete->effect != NULL)
-			to_delete->effect->cause = to_delete->cause;
-		if (to_delete->cause != NULL)
-			to_delete->cause->effect = to_delete->effect;
 		to_delete->cause = NULL;
-		to_delete->effect = NULL;
 		to_delete->destroy(to_delete);
 		if (cause == NULL)
 			return;
+		cause->has_effect = false;
 		to_delete = cause;
 	}
 }
 
 /**
- * Unlink error from its effect. For instance:
- * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
- * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
- */
-static inline void
-error_unlink_effect(struct error *e)
-{
-	if (e->effect != NULL) {
-		assert(e->refs > 1);
-		error_unref(e);
-		e->effect->cause = NULL;
-	}
-	e->effect = NULL;
-}
-
-/**
- * Set previous error: cut @a prev from its previous 'tail' of
- * causes and link to the one @a e belongs to. Note that all
- * previous errors starting from @a prev->cause are transferred
- * with it as well (i.e. causes for given error are not erased).
- * For instance:
- * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
- * e2:set_effect(e3): e1 -> e2 -> e3 -> e4 -> NULL
- *
- * @a effect can be NULL. To be used as ffi method in
- * lua/error.lua.
+ * Set previous error. It can be NULL to make @a not having any
+ * previous error. In case @a prev is not NULL, it should not be
+ * already belong to another stack, and @a e should be top of the
+ * stack.
  *
- * @retval -1 in case adding @a effect results in list cycles;
- *          0 otherwise.
+ * @retval -1 In case adding @a prev is not possible.
+ * @retval 0 Success.
  */
 int
 error_set_prev(struct error *e, struct error *prev);
@@ -241,7 +212,6 @@ diag_set_error(struct diag *diag, struct error *e)
 	assert(e != NULL);
 	error_ref(e);
 	diag_clear(diag);
-	error_unlink_effect(e);
 	diag->last = e;
 }
 
@@ -255,11 +225,11 @@ static inline void
 diag_add_error(struct diag *diag, struct error *e)
 {
 	assert(e != NULL);
+	assert(e->cause == NULL);
 	error_ref(e);
-	error_unlink_effect(e);
 	e->cause = diag->last;
 	if (diag->last != NULL)
-		diag->last->effect = e;
+		diag->last->has_effect = true;
 	diag->last = e;
 }
 
diff --git a/src/lua/error.lua b/src/lua/error.lua
index bdc9c714d..3d0b75689 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -25,7 +25,7 @@ struct error {
     /* Error description. */
     char _errmsg[DIAG_ERRMSG_MAX];
     struct error *_cause;
-    struct error *_effect;
+    bool has_effect;
 };
 
 char *
diff --git a/test/box/error.result b/test/box/error.result
index 4f0f30491..300def647 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -502,6 +502,10 @@ box.error()
  | ---
  | ...
 
+space:drop()
+ | ---
+ | ...
+
 -- gh-1148: errors can be arranged into list (so called
 -- stacked diagnostics).
 --
@@ -540,14 +544,17 @@ assert(e2.prev == nil)
  | ...
 -- At this point stack is following: e1 -> e2
 -- Let's test following cases:
--- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
 -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
 -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
--- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
 --
 e3 = box.error.new({code = 111, reason = "another cause"})
  | ---
  | ...
+e1:set_prev(nil)
+ | ---
+ | ...
 e3:set_prev(e2)
  | ---
  | ...
@@ -566,6 +573,9 @@ assert(e1.prev == nil)
 
 -- Reset stack to e1 -> e2 and test case 2.
 --
+e3:set_prev(nil)
+ | ---
+ | ...
 e1:set_prev(e2)
  | ---
  | ...
@@ -628,19 +638,19 @@ assert(e3.prev == e1)
 
 -- Unlink errors and test case 4.
 --
-e1:set_prev(nil)
+e3:set_prev(nil)
  | ---
  | ...
 e2:set_prev(nil)
  | ---
  | ...
-e3:set_prev(nil)
+e1:set_prev(nil)
  | ---
  | ...
-e1:set_prev(e2)
+e2:set_prev(e3)
  | ---
  | ...
-e2:set_prev(e3)
+e1:set_prev(e2)
  | ---
  | ...
 assert(e1.prev == e2)
@@ -676,72 +686,6 @@ assert(e3.prev == nil)
  | - true
  | ...
 
--- Test splitting list into two ones.
--- After that we will get two lists: e1->e2->e5 and e3->e4
---
-e4 = box.error.new({code = 111, reason = "yet another cause"})
- | ---
- | ...
-e5 = box.error.new({code = 111, reason = "and another one"})
- | ---
- | ...
-e3:set_prev(e4)
- | ---
- | ...
-e2:set_prev(e5)
- | ---
- | ...
-assert(e1.prev == e2)
- | ---
- | - true
- | ...
-assert(e2.prev == e5)
- | ---
- | - true
- | ...
-assert(e3.prev == e4)
- | ---
- | - true
- | ...
-assert(e5.prev == nil)
- | ---
- | - true
- | ...
-assert(e4.prev == nil)
- | ---
- | - true
- | ...
-
--- Another splitting option: e1->e2 and e5->e3->e4
--- But firstly restore to one single list e1->e2->e3->e4
---
-e2:set_prev(e3)
- | ---
- | ...
-e5:set_prev(e3)
- | ---
- | ...
-assert(e1.prev == e2)
- | ---
- | - true
- | ...
-assert(e2.prev == nil)
- | ---
- | - true
- | ...
-assert(e5.prev == e3)
- | ---
- | - true
- | ...
-assert(e3.prev == e4)
- | ---
- | - true
- | ...
-assert(e4.prev == nil)
- | ---
- | - true
- | ...
-
 -- In case error is destroyed, it unrefs reference counter
 -- of its previous error. In turn, box.error.clear() refs/unrefs
 -- only head and doesn't touch other errors.
@@ -785,7 +729,3 @@ assert(e1.prev == e2)
  | ---
  | - true
  | ...
-
-space:drop()
- | ---
- | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 66a22db90..04e00168e 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -108,6 +108,8 @@ box.error.new(err)
 box.error.clear()
 box.error()
 
+space:drop()
+
 -- gh-1148: errors can be arranged into list (so called
 -- stacked diagnostics).
 --
@@ -122,12 +124,13 @@ e2:set_prev(e1)
 assert(e2.prev == nil)
 -- At this point stack is following: e1 -> e2
 -- Let's test following cases:
--- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
 -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
 -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
--- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
 --
 e3 = box.error.new({code = 111, reason = "another cause"})
+e1:set_prev(nil)
 e3:set_prev(e2)
 assert(e3.prev == e2)
 assert(e2.prev == nil)
@@ -135,6 +138,7 @@ assert(e1.prev == nil)
 
 -- Reset stack to e1 -> e2 and test case 2.
 --
+e3:set_prev(nil)
 e1:set_prev(e2)
 assert(e2.prev == nil)
 assert(e3.prev == nil)
@@ -156,11 +160,11 @@ assert(e3.prev == e1)
 
 -- Unlink errors and test case 4.
 --
-e1:set_prev(nil)
-e2:set_prev(nil)
 e3:set_prev(nil)
-e1:set_prev(e2)
+e2:set_prev(nil)
+e1:set_prev(nil)
 e2:set_prev(e3)
+e1:set_prev(e2)
 assert(e1.prev == e2)
 assert(e2.prev == e3)
 assert(e3.prev == nil)
@@ -173,30 +177,6 @@ assert(e3.prev == nil)
 e3:set_prev(e2)
 assert(e3.prev == nil)
 
--- Test splitting list into two ones.
--- After that we will get two lists: e1->e2->e5 and e3->e4
---
-e4 = box.error.new({code = 111, reason = "yet another cause"})
-e5 = box.error.new({code = 111, reason = "and another one"})
-e3:set_prev(e4)
-e2:set_prev(e5)
-assert(e1.prev == e2)
-assert(e2.prev == e5)
-assert(e3.prev == e4)
-assert(e5.prev == nil)
-assert(e4.prev == nil)
-
--- Another splitting option: e1->e2 and e5->e3->e4
--- But firstly restore to one single list e1->e2->e3->e4
---
-e2:set_prev(e3)
-e5:set_prev(e3)
-assert(e1.prev == e2)
-assert(e2.prev == nil)
-assert(e5.prev == e3)
-assert(e3.prev == e4)
-assert(e4.prev == nil)
-
 -- In case error is destroyed, it unrefs reference counter
 -- of its previous error. In turn, box.error.clear() refs/unrefs
 -- only head and doesn't touch other errors.
@@ -212,5 +192,3 @@ assert(e2.code == 111)
 box.error.set(e1)
 box.error.clear()
 assert(e1.prev == e2)
-
-space:drop()

[-- Attachment #2: opt-cycle.txt --]
[-- Type: text/plain, Size: 749 bytes --]

diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 3a817a659..276bf2362 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -117,12 +117,11 @@ error_unref(struct error *e)
 	while (--to_delete->refs == 0) {
 		/* Unlink error from lists completely.*/
 		struct error *cause = to_delete->cause;
-		if (to_delete->effect != NULL)
-			to_delete->effect->cause = to_delete->cause;
-		if (to_delete->cause != NULL)
-			to_delete->cause->effect = to_delete->effect;
-		to_delete->cause = NULL;
-		to_delete->effect = NULL;
+		assert(to_delete->effect == NULL);
+		if (to_delete->cause != NULL) {
+			to_delete->cause->effect = NULL;
+			to_delete->cause = NULL;
+		}
 		to_delete->destroy(to_delete);
 		if (cause == NULL)
 			return;

[-- Attachment #3: unidirect.txt --]
[-- Type: text/plain, Size: 11079 bytes --]

commit eaada025e478aeb66d0b8f8f31c91045b0f0089f
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Apr 2 00:52:54 2020 +0200

    Diag stack simplification

diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index ed8beb58d..69a19d91f 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -37,37 +37,32 @@ struct error_factory *error_factory = NULL;
 int
 error_set_prev(struct error *e, struct error *prev)
 {
-	/*
-	 * Make sure that adding error won't result in cycles.
-	 * Don't bother with sophisticated cycle-detection
-	 * algorithms, simple iteration is OK since as a rule
-	 * list contains a dozen errors at maximum.
-	 */
-	struct error *tmp = prev;
-	while (tmp != NULL) {
-		if (tmp == e)
+	if (e == prev)
+		return -1;
+	if (prev != NULL) {
+		/*
+		 * It is not allowed to change middle of the error
+		 * stack. Except when the tail is cut. Not
+		 * replaced. Reason is to make the code simpler,
+		 * and avoid any necessity to detect cycles. In
+		 * that implementation cycles are not allowed, and
+		 * this guarantee costs nothing.
+		 */
+		if (prev->has_effect || e->has_effect)
 			return -1;
-		tmp = tmp->cause;
+		prev->has_effect = true;
+		error_ref(prev);
 	}
 	/*
 	 * At once error can feature only one reason.
 	 * So unlink previous 'cause' node.
 	 */
 	if (e->cause != NULL) {
-		e->cause->effect = NULL;
+		e->cause->has_effect = false;
 		error_unref(e->cause);
 	}
 	/* Set new 'prev' node. */
 	e->cause = prev;
-	/*
-	 * Unlink new 'effect' node from its old list of 'cause'
-	 * errors. nil can be also passed as an argument.
-	 */
-	if (prev != NULL) {
-		error_ref(prev);
-		error_unlink_effect(prev);
-		prev->effect = e;
-	}
 	return 0;
 }
 
@@ -91,7 +86,7 @@ error_create(struct error *e,
 	}
 	e->errmsg[0] = '\0';
 	e->cause = NULL;
-	e->effect = NULL;
+	e->has_effect = false;
 }
 
 struct diag *
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 3a817a659..52987befa 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -85,22 +85,20 @@ struct error {
 	/* Error description. */
 	char errmsg[DIAG_ERRMSG_MAX];
 	/**
-	 * Link to the cause and effect of given error. The cause
-	 * creates the effect:
+	 * Cause of the given error..
 	 * e1 = box.error.new({code = 0, reason = 'e1'})
 	 * e2 = box.error.new({code = 0, reason = 'e2'})
-	 * e1:set_prev(e2) -- Now e2 is the cause of e1 and e1 is
-	 * the effect of e2.
-	 * Only cause keeps reference to avoid cyclic dependence.
-	 * RLIST implementation is not really suitable here
-	 * since it is organized as circular list. In such
-	 * a case it is impossible to start an iteration
-	 * from any node and finish at the logical end of the
-	 * list. Double-linked list is required to allow deletion
-	 * from the middle of the list.
+	 * e1:set_prev(e2) -- Now e2 is the cause of e1.
 	 */
 	struct error *cause;
-	struct error *effect;
+	/**
+	 * Flag whether this error is not top of the error stack.
+	 * I.e. it has an 'effect'. Effect is e1 in the example
+	 * above. The flag is used to prevent changing middle of
+	 * the stack (except when it is cut off - this easy to
+	 * support, and can't create a cycle).
+	 */
+	bool has_effect;
 };
 
 static inline void
@@ -115,51 +113,24 @@ error_unref(struct error *e)
 	assert(e->refs > 0);
 	struct error *to_delete = e;
 	while (--to_delete->refs == 0) {
-		/* Unlink error from lists completely.*/
 		struct error *cause = to_delete->cause;
-		if (to_delete->effect != NULL)
-			to_delete->effect->cause = to_delete->cause;
-		if (to_delete->cause != NULL)
-			to_delete->cause->effect = to_delete->effect;
 		to_delete->cause = NULL;
-		to_delete->effect = NULL;
 		to_delete->destroy(to_delete);
 		if (cause == NULL)
 			return;
+		cause->has_effect = false;
 		to_delete = cause;
 	}
 }
 
 /**
- * Unlink error from its effect. For instance:
- * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
- * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
- */
-static inline void
-error_unlink_effect(struct error *e)
-{
-	if (e->effect != NULL) {
-		assert(e->refs > 1);
-		error_unref(e);
-		e->effect->cause = NULL;
-	}
-	e->effect = NULL;
-}
-
-/**
- * Set previous error: cut @a prev from its previous 'tail' of
- * causes and link to the one @a e belongs to. Note that all
- * previous errors starting from @a prev->cause are transferred
- * with it as well (i.e. causes for given error are not erased).
- * For instance:
- * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
- * e2:set_effect(e3): e1 -> e2 -> e3 -> e4 -> NULL
- *
- * @a effect can be NULL. To be used as ffi method in
- * lua/error.lua.
+ * Set previous error. It can be NULL to make @a not having any
+ * previous error. In case @a prev is not NULL, it should not be
+ * already belong to another stack, and @a e should be top of the
+ * stack.
  *
- * @retval -1 in case adding @a effect results in list cycles;
- *          0 otherwise.
+ * @retval -1 In case adding @a prev is not possible.
+ * @retval 0 Success.
  */
 int
 error_set_prev(struct error *e, struct error *prev);
@@ -241,7 +212,6 @@ diag_set_error(struct diag *diag, struct error *e)
 	assert(e != NULL);
 	error_ref(e);
 	diag_clear(diag);
-	error_unlink_effect(e);
 	diag->last = e;
 }
 
@@ -255,11 +225,11 @@ static inline void
 diag_add_error(struct diag *diag, struct error *e)
 {
 	assert(e != NULL);
+	assert(e->cause == NULL);
 	error_ref(e);
-	error_unlink_effect(e);
 	e->cause = diag->last;
 	if (diag->last != NULL)
-		diag->last->effect = e;
+		diag->last->has_effect = true;
 	diag->last = e;
 }
 
diff --git a/src/lua/error.lua b/src/lua/error.lua
index bdc9c714d..3d0b75689 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -25,7 +25,7 @@ struct error {
     /* Error description. */
     char _errmsg[DIAG_ERRMSG_MAX];
     struct error *_cause;
-    struct error *_effect;
+    bool has_effect;
 };
 
 char *
diff --git a/test/box/error.result b/test/box/error.result
index 4f0f30491..300def647 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -502,6 +502,10 @@ box.error()
  | ---
  | ...
 
+space:drop()
+ | ---
+ | ...
+
 -- gh-1148: errors can be arranged into list (so called
 -- stacked diagnostics).
 --
@@ -540,14 +544,17 @@ assert(e2.prev == nil)
  | ...
 -- At this point stack is following: e1 -> e2
 -- Let's test following cases:
--- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
 -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
 -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
--- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
 --
 e3 = box.error.new({code = 111, reason = "another cause"})
  | ---
  | ...
+e1:set_prev(nil)
+ | ---
+ | ...
 e3:set_prev(e2)
  | ---
  | ...
@@ -566,6 +573,9 @@ assert(e1.prev == nil)
 
 -- Reset stack to e1 -> e2 and test case 2.
 --
+e3:set_prev(nil)
+ | ---
+ | ...
 e1:set_prev(e2)
  | ---
  | ...
@@ -628,19 +638,19 @@ assert(e3.prev == e1)
 
 -- Unlink errors and test case 4.
 --
-e1:set_prev(nil)
+e3:set_prev(nil)
  | ---
  | ...
 e2:set_prev(nil)
  | ---
  | ...
-e3:set_prev(nil)
+e1:set_prev(nil)
  | ---
  | ...
-e1:set_prev(e2)
+e2:set_prev(e3)
  | ---
  | ...
-e2:set_prev(e3)
+e1:set_prev(e2)
  | ---
  | ...
 assert(e1.prev == e2)
@@ -676,72 +686,6 @@ assert(e3.prev == nil)
  | - true
  | ...
 
--- Test splitting list into two ones.
--- After that we will get two lists: e1->e2->e5 and e3->e4
---
-e4 = box.error.new({code = 111, reason = "yet another cause"})
- | ---
- | ...
-e5 = box.error.new({code = 111, reason = "and another one"})
- | ---
- | ...
-e3:set_prev(e4)
- | ---
- | ...
-e2:set_prev(e5)
- | ---
- | ...
-assert(e1.prev == e2)
- | ---
- | - true
- | ...
-assert(e2.prev == e5)
- | ---
- | - true
- | ...
-assert(e3.prev == e4)
- | ---
- | - true
- | ...
-assert(e5.prev == nil)
- | ---
- | - true
- | ...
-assert(e4.prev == nil)
- | ---
- | - true
- | ...
-
--- Another splitting option: e1->e2 and e5->e3->e4
--- But firstly restore to one single list e1->e2->e3->e4
---
-e2:set_prev(e3)
- | ---
- | ...
-e5:set_prev(e3)
- | ---
- | ...
-assert(e1.prev == e2)
- | ---
- | - true
- | ...
-assert(e2.prev == nil)
- | ---
- | - true
- | ...
-assert(e5.prev == e3)
- | ---
- | - true
- | ...
-assert(e3.prev == e4)
- | ---
- | - true
- | ...
-assert(e4.prev == nil)
- | ---
- | - true
- | ...
-
 -- In case error is destroyed, it unrefs reference counter
 -- of its previous error. In turn, box.error.clear() refs/unrefs
 -- only head and doesn't touch other errors.
@@ -785,7 +729,3 @@ assert(e1.prev == e2)
  | ---
  | - true
  | ...
-
-space:drop()
- | ---
- | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 66a22db90..04e00168e 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -108,6 +108,8 @@ box.error.new(err)
 box.error.clear()
 box.error()
 
+space:drop()
+
 -- gh-1148: errors can be arranged into list (so called
 -- stacked diagnostics).
 --
@@ -122,12 +124,13 @@ e2:set_prev(e1)
 assert(e2.prev == nil)
 -- At this point stack is following: e1 -> e2
 -- Let's test following cases:
--- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
 -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
 -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
--- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
 --
 e3 = box.error.new({code = 111, reason = "another cause"})
+e1:set_prev(nil)
 e3:set_prev(e2)
 assert(e3.prev == e2)
 assert(e2.prev == nil)
@@ -135,6 +138,7 @@ assert(e1.prev == nil)
 
 -- Reset stack to e1 -> e2 and test case 2.
 --
+e3:set_prev(nil)
 e1:set_prev(e2)
 assert(e2.prev == nil)
 assert(e3.prev == nil)
@@ -156,11 +160,11 @@ assert(e3.prev == e1)
 
 -- Unlink errors and test case 4.
 --
-e1:set_prev(nil)
-e2:set_prev(nil)
 e3:set_prev(nil)
-e1:set_prev(e2)
+e2:set_prev(nil)
+e1:set_prev(nil)
 e2:set_prev(e3)
+e1:set_prev(e2)
 assert(e1.prev == e2)
 assert(e2.prev == e3)
 assert(e3.prev == nil)
@@ -173,30 +177,6 @@ assert(e3.prev == nil)
 e3:set_prev(e2)
 assert(e3.prev == nil)
 
--- Test splitting list into two ones.
--- After that we will get two lists: e1->e2->e5 and e3->e4
---
-e4 = box.error.new({code = 111, reason = "yet another cause"})
-e5 = box.error.new({code = 111, reason = "and another one"})
-e3:set_prev(e4)
-e2:set_prev(e5)
-assert(e1.prev == e2)
-assert(e2.prev == e5)
-assert(e3.prev == e4)
-assert(e5.prev == nil)
-assert(e4.prev == nil)
-
--- Another splitting option: e1->e2 and e5->e3->e4
--- But firstly restore to one single list e1->e2->e3->e4
---
-e2:set_prev(e3)
-e5:set_prev(e3)
-assert(e1.prev == e2)
-assert(e2.prev == nil)
-assert(e5.prev == e3)
-assert(e3.prev == e4)
-assert(e4.prev == nil)
-
 -- In case error is destroyed, it unrefs reference counter
 -- of its previous error. In turn, box.error.clear() refs/unrefs
 -- only head and doesn't touch other errors.
@@ -212,5 +192,3 @@ assert(e2.code == 111)
 box.error.set(e1)
 box.error.clear()
 assert(e1.prev == e2)
-
-space:drop()