[tarantool-patches] [PATCH v2 0/3] box: stacked diagnostics area in fiber

[tarantool-patches] [PATCH v2 0/3] box: stacked diagnostics area in fiber

[Kirill Shcherbatov]

Support stacked diagnostics for Tarantool allows to accumulate all
occurred errors during processing a request. This allows to better
understand what has happened and handle errors correspondingly.

This patch introduces core methods diag_add(), diag_svp(),
diag_rollback_to_svp() that allows you to produce a better
diagnostics information for a user.

There is an other new Lua endpoint
box.error.prev() that allows you to get a reason for a given
error object.

Changes in v2:
 - new API - user shouldn't have an ability to modify an existent
   error object that he didn't constructed.
 - a new detailed unit-test that performs a good coverage for
   many complicated reference-counting scenarios.

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-1148-stacked-errors
Issue: https://github.com/tarantool/tarantool/issues/1148

Kirill Shcherbatov (3):
  box: rfc for stacked diagnostic area in Tarantool
  box: rename diag_add_error to diag_set_error
  box: introduce stacked diagnostic area

 src/lib/core/diag.h                 |  82 ++++++++++++-
 src/lib/core/exception.h            |   2 +-
 src/lua/error.h                     |   3 +
 src/box/key_list.c                  |  16 +--
 src/box/lua/call.c                  |   6 +-
 src/box/vy_scheduler.c              |   6 +-
 src/lib/core/diag.c                 |   1 +
 src/lua/error.c                     |   2 +-
 src/lua/utils.c                     |   2 +-
 doc/rfc/1148-stacked-diagnostics.md | 175 ++++++++++++++++++++++++++++
 src/box/applier.cc                  |   2 +-
 src/box/error.cc                    |   2 +-
 src/box/lua/error.cc                |  20 ++++
 src/box/relay.cc                    |   4 +-
 src/lib/core/exception.cc           |   2 +-
 src/lua/error.lua                   |   2 +
 test/engine/func_index.result       |  48 ++++++--
 test/engine/func_index.test.lua     |   7 ++
 test/unit/CMakeLists.txt            |   2 +-
 test/unit/fiber.cc                  | 157 +++++++++++++++++++++++++
 test/unit/fiber.result              |  60 ++++++++++
 21 files changed, 566 insertions(+), 35 deletions(-)
 create mode 100644 doc/rfc/1148-stacked-diagnostics.md

-- 
2.22.1

[tarantool-patches] [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Kirill Shcherbatov]

Part of #1148
---
 doc/rfc/1148-stacked-diagnostics.md | 175 ++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)
 create mode 100644 doc/rfc/1148-stacked-diagnostics.md

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
new file mode 100644
index 000000000..777b2440f
--- /dev/null
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -0,0 +1,175 @@
+# Stacked Diagnostics in Tarantool
+
+* **Status**: In progress
+* **Start date**: 30-07-2019
+* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org,Vladislav Shpilevoy @Gerold103 v.shpilevoy@tarantool.org
+* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
+
+## Summary
+Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
+correspondingly.
+
+## Background and motivation
+
+Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
+Diagnostics area stack must contain a diagnostics area for each nested execution context.
+
+### Current Tarantool's error diagnostics
+Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
+The last error is exported with `box.error.last()` endpoint.
+
+In total there are few error classes in Tarantool.
+```
+ClientError
+ | LoggedError
+ | AccessDeniedError
+ | UnsupportedIndexFeature
+
+XlogError
+ | XlogGapError
+SystemError
+ | SocketError
+ | OutOfMemory
+ | TimedOut
+
+ChannelIsClosed
+FiberIsCancelled
+LuajitError
+IllegalParams
+CollationError
+SwimError
+CryptoError
+```
+
+All ClientError codes are exported with their error codes in box.error folder by their NAMEs.
+
+You may use box.error.new endpoint to create a new error instance of predefined type:
+```
+tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
+tarantool> t:unpack()
+---
+- type: ClientError
+  code: 9
+  message: 'Failed to create space ''myspace'': just I can''t'
+  trace:
+  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
+    line: 1
+```
+
+User is allowed to define own errors with any code (including system errors) with
+```
+box.error.new{code = user_code, reason = user_error_msg}
+```
+where `user_code` must be greater that the biggest registered system error, say `10000`.
+
+Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
+
+
+## Proposal
+In some cases a diagnostic information must be more complicated.
+For example, when persistent Lua function referenced by functional index has a bug in it's definition, Lua handler sets an diag message. Then functional index extractor code setups an own, more specialized error.
+
+We need to introduce instruments to deal with it both in C and Lua API. Let's overview them one-by-one:
+
+### C API
+The existent `diag_set()` method should be kept as is: it should replace the last error in diagnostic area with a new one.
+
+Let's introduce a new method `diag_add()` that keeps an existent error message in diagnostic area (if any) and sets it as a reason error for a recently-constructed error object.
+
+We also need a way to reset last errors set. Let's introduce `diag_svp(diag) -> SVP`,
+`diag_rollback_to_svp(diag, SVP)`. The implementation proposal for SVP structure is given below:
+
+```
+                      truncate
+      xxxxxxxxxxxxxxxxxxxxxxxx SVP
+DIAG: +-------+               |
+      | err   |               |
+      +-------+----->+-------+ here
+      third    prev  | err   |
+                     +-------+------->+-------+
+                      second  prev    | err   |
+                                      +-------+
+                                       first
+```
+
+The diag_structure already has `struct error *next` pointer;
+
+Let's introduce a similar pointer in an error structure to organize a reason list:
+```
+struct error {
+   ...
+   struct error *prev;
+};
+```
+
+To implement a SVP object, we may return a last-set error pointer (diag::last):
+```
+diag_set(diag, FIRST)
+struct error *SVP = diag_svp(diag)
+       /**********************************\
+       * return diag->last                *
+       \**********************************/
+
+diag_add(diag, SECOND)
+diag_add(diag, THIRD)
+
+diag_rollback_to_svp(diag, svp)
+       /***********************************\
+       * struct error *tmp = diag->last;   *
+       * while (tmp != svp) {              *
+       *    diag->last = tmp->prev;        *
+       *    error_unref(tmp);              *
+       *    tmp = diag->last;              *
+       * }                                 *
+       \***********************************/
+```
+
+### Lua API
+Tarantool returns a last-set (diag::last) error object as `cdata` from central diagnostic
+area to Lua in case of error. User shouldn't have an ability to modify it
+(because he actually needn't and because it is dangerous - he doesn't own this object),
+but it need an ability to inspect a collected diagnostics information.
+
+Let's extend the `box.error` API with a couple of new functions:
+1) `prev` that allows to get a reason of given error:
+
+```
+-- Return a reason error object for given error
+-- (when exists, nil otherwise)
+box.error.prev(error)
+```
+### Binary protocol
+Currently errors are sent as `(IPROTO_ERROR | errcode)` messages with an error's message string payload.
+
+We must to design a backward-compatible error transfer specification. Imagine a net.box old-new Tarantool connection, same as existing connector: they mustn't broken.
+
+Let's send error messages as before, but add a field with `IPROTO_ERROR_LIST` key in addition to existing `IPROTO_ERROR (0x31)` to a body map.
+
+The error's payload would consists of backward-compatible last recent error in the beginning, and a list of reason's serialized errors then. Error packets are rare and not normal, so we may not optimize them too much in terms of payload's size.
+
+```
+header: [IPROTO_ERROR_LIST | IPROTO_ERROR |  error code]
+pyload: MSGPACK_ERR_MSG_STR..[{code = reason1Code, msg = reason1Msg},..
+                              {code = reasonNCode, msg = reasonNMsg}]
+```
+
+### SQL Warnings
+SQL Standard defines ["WARNINGS"](https://docs.microsoft.com/en-us/sql/t-sql/statements/set-ansi-warnings-transact-sql?view=sql-server-2017) term:
+- When set to ON, if null values appear in aggregate functions, such as SUM, AVG, MAX, MIN, STDEV, STDEVP, VAR, VARP, or COUNT, a warning message is generated. When set to OFF, no warning is issued.
+- When set to ON, the divide-by-zero and arithmetic overflow errors cause the statement to be rolled back and an error message is generated. When set to OFF, the divide-by-zero and arithmetic overflow errors cause null values to be returned. The behavior in which a divide-by-zero or arithmetic overflow error causes null values to be returned occurs if an INSERT or UPDATE is tried on a character, Unicode, or binary column in which the length of a new value exceeds the maximum size of the column. If SET ANSI_WARNINGS is ON, the INSERT or UPDATE is canceled as specified by the ISO standard. Trailing blanks are ignored for character columns and trailing nulls are ignored for binary columns. When OFF, data is truncated to the size of the column and the statement succeeds.
+
+According to the current convention, all Tarantool's function use "return not null error code on error" convention. Iff the global diagnostic area is valid (actually, Unix errno uses the same semantics). So we need an additional `Parser`/`VDBE` marker to indicate that the warning diagnostics
+is set. Say, in addition to `bool is_aborted` field we may introduce and additional field `bool is_warning_set`.
+
+To avoid a mess between SQL warnings and real errors let's better introduce an
+"diagnostics"-semantics area in Parser/VDBE classes where all warnings are
+organized in a list. Internal warning representation needn't follow error
+specification and may use other representation when it is reasonable.
+
+### Plans
+Ticket [#4398](https://github.com/tarantool/tarantool/issues/4398) "Expose box.error to be used by applications" requires to design a system to construct error objects with reasons in Lua.
+As it was described above, user shouldn't modify an original error, because he didn't construct it
+and doesn't hold it's life cycle. However he is able to take a reference on that error. Thus instead of
+Go-like `wrap()` semantics we plan to extend a `box.error.new()` constructor to
+support `reason` object-typed overload. In such case a new error object would be constructed with
+ a given reason reference.
-- 
2.22.1

[tarantool-patches] [PATCH v2 2/3] box: rename diag_add_error to diag_set_error

[Kirill Shcherbatov]

Renamed function diag_add_error to diag_set_error because it
actually replaces an error object in diagnostic area with a new
one and this name is not representative. Moreover, we are going
to introduce a new diag_add_error that extends error object in
diagnostic area in following patches.

Needed for #1148
---
 src/lib/core/diag.h       | 6 +++---
 src/lib/core/exception.h  | 2 +-
 src/box/vy_scheduler.c    | 6 +++---
 src/lua/utils.c           | 2 +-
 src/box/applier.cc        | 2 +-
 src/box/error.cc          | 2 +-
 src/box/relay.cc          | 4 ++--
 src/lib/core/exception.cc | 2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fd3831e66..02a67269f 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -162,12 +162,12 @@ diag_clear(struct diag *diag)
 }
 
 /**
- * Add a new error to the diagnostics area
+ * Set a new error to the diagnostics area, replacing existent.
  * \param diag diagnostics area
  * \param e error to add
  */
 static inline void
-diag_add_error(struct diag *diag, struct error *e)
+diag_set_error(struct diag *diag, struct error *e)
 {
 	assert(e != NULL);
 	error_ref(e);
@@ -269,7 +269,7 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
 	struct error *e;						\
 	e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__);		\
-	diag_add_error(diag_get(), e);					\
+	diag_set_error(diag_get(), e);					\
 	/* Restore the errno which might have been reset.  */           \
 	errno = save_errno;                                             \
 } while (0)
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index a29281427..e1a45b3fb 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -182,7 +182,7 @@ exception_init();
 #define tnt_error(class, ...) ({					\
 	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
 	class *e = new class(__FILE__, __LINE__, ##__VA_ARGS__);	\
-	diag_add_error(diag_get(), e);					\
+	diag_set_error(diag_get(), e);					\
 	e;								\
 })
 
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index ee361c31f..31d144cf3 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -618,7 +618,7 @@ vy_scheduler_dump(struct vy_scheduler *scheduler)
 		if (scheduler->is_throttled) {
 			/* Dump error occurred. */
 			struct error *e = diag_last_error(&scheduler->diag);
-			diag_add_error(diag_get(), e);
+			diag_set_error(diag_get(), e);
 			return -1;
 		}
 		fiber_cond_wait(&scheduler->dump_cond);
@@ -692,7 +692,7 @@ vy_scheduler_begin_checkpoint(struct vy_scheduler *scheduler)
 	 */
 	if (scheduler->is_throttled) {
 		struct error *e = diag_last_error(&scheduler->diag);
-		diag_add_error(diag_get(), e);
+		diag_set_error(diag_get(), e);
 		say_error("cannot checkpoint vinyl, "
 			  "scheduler is throttled with: %s", e->errmsg);
 		return -1;
@@ -728,7 +728,7 @@ vy_scheduler_wait_checkpoint(struct vy_scheduler *scheduler)
 		if (scheduler->is_throttled) {
 			/* A dump error occurred, abort checkpoint. */
 			struct error *e = diag_last_error(&scheduler->diag);
-			diag_add_error(diag_get(), e);
+			diag_set_error(diag_get(), e);
 			say_error("vinyl checkpoint failed: %s", e->errmsg);
 			return -1;
 		}
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 75efe0ed2..22b0f0424 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1004,7 +1004,7 @@ luaT_toerror(lua_State *L)
 	struct error *e = luaL_iserror(L, -1);
 	if (e != NULL) {
 		/* Re-throw original error */
-		diag_add_error(&fiber()->diag, e);
+		diag_set_error(&fiber()->diag, e);
 	} else {
 		/* Convert Lua error to a Tarantool exception. */
 		diag_set(LuajitError, luaT_tolstring(L, -1, NULL));
diff --git a/src/box/applier.cc b/src/box/applier.cc
index 4304ff055..d34086681 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -762,7 +762,7 @@ applier_on_rollback(struct trigger *trigger, void *event)
 	struct applier *applier = (struct applier *)trigger->data;
 	/* Setup a shared error. */
 	if (!diag_is_empty(&replicaset.applier.diag)) {
-		diag_add_error(&applier->diag,
+		diag_set_error(&applier->diag,
 			       diag_last_error(&replicaset.applier.diag));
 	}
 	/* Stop the applier fiber. */
diff --git a/src/box/error.cc b/src/box/error.cc
index 47dce3305..7dfe1b3ee 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -82,7 +82,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 		error_vformat_msg(e, fmt, ap);
 		va_end(ap);
 	}
-	diag_add_error(&fiber()->diag, e);
+	diag_set_error(&fiber()->diag, e);
 	return -1;
 }
 
diff --git a/src/box/relay.cc b/src/box/relay.cc
index a19abf6a9..1f6cb28b7 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -449,7 +449,7 @@ relay_set_error(struct relay *relay, struct error *e)
 {
 	/* Don't override existing error. */
 	if (diag_is_empty(&relay->diag))
-		diag_add_error(&relay->diag, e);
+		diag_set_error(&relay->diag, e);
 }
 
 static void
@@ -623,7 +623,7 @@ relay_subscribe_f(va_list ap)
 	 * Don't clear the error for status reporting.
 	 */
 	assert(!diag_is_empty(&relay->diag));
-	diag_add_error(diag_get(), diag_last_error(&relay->diag));
+	diag_set_error(diag_get(), diag_last_error(&relay->diag));
 	diag_log();
 	say_crit("exiting the relay loop");
 
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index a6999af43..55ee8cd40 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -99,7 +99,7 @@ Exception::operator new(size_t size)
 	void *buf = malloc(size);
 	if (buf != NULL)
 		return buf;
-	diag_add_error(diag_get(), &out_of_memory);
+	diag_set_error(diag_get(), &out_of_memory);
 	throw &out_of_memory;
 }
 
-- 
2.22.1

[tarantool-patches] [PATCH v2 3/3] box: introduce stacked diagnostic area

[Kirill Shcherbatov]

Closes #1148

@TarantoolBot document
Title: Stacked Diagnostics for Tarantool

Tarantool statements must produce diagnostic information that
populates the diagnostics area. Diagnostics area stack must
contain a diagnostics area for each nested execution context.

Tarantool used to have the only diag_set() mechanism to set a
diagnostic error. In some cases a diagnostic information must be
more complicated. A new method diag_add() allows to extend
global diagnostics with a new error: the previous set becomes
a reason for a recently-constructed error object. This commit
also introduce a savepoint-semantics mechanism to remove errors
when they are redundant. Thus,
diag_set(diag, FIRST)
struct error *SVP = diag_svp(diag)
diag_add(diag, SECOND)
diag_add(diag, THIRD)
diag_rollback_to_svp(diag, svp) // only the FIRST error is set

To work with errors having a reason efficiently, box.error
endpoint was extended with a new method prev that returns a
reason error object for a given error, when exists or nil
otherwise:
err = box.error.last()
err:unpack()
reason = box.error.prev(err)
reason:unpack()
---
 src/lib/core/diag.h             |  78 +++++++++++++++-
 src/lua/error.h                 |   3 +
 src/box/key_list.c              |  16 ++--
 src/box/lua/call.c              |   6 +-
 src/lib/core/diag.c             |   1 +
 src/lua/error.c                 |   2 +-
 src/box/lua/error.cc            |  20 ++++
 src/lua/error.lua               |   2 +
 test/engine/func_index.result   |  48 ++++++++--
 test/engine/func_index.test.lua |   7 ++
 test/unit/CMakeLists.txt        |   2 +-
 test/unit/fiber.cc              | 157 ++++++++++++++++++++++++++++++++
 test/unit/fiber.result          |  60 ++++++++++++
 13 files changed, 379 insertions(+), 23 deletions(-)

diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 02a67269f..6d50e94e7 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -78,6 +78,8 @@ struct error {
 	char file[DIAG_FILENAME_MAX];
 	/* Error description. */
 	char errmsg[DIAG_ERRMSG_MAX];
+	/* A pointer to the reason error. */
+	struct error *reason;
 };
 
 static inline void
@@ -90,9 +92,11 @@ static inline void
 error_unref(struct error *e)
 {
 	assert(e->refs > 0);
-	--e->refs;
-	if (e->refs == 0)
+	if (--e->refs == 0) {
+		if (e->reason != NULL)
+			error_unref(e->reason);
 		e->destroy(e);
+	}
 }
 
 NORETURN static inline void
@@ -175,6 +179,26 @@ diag_set_error(struct diag *diag, struct error *e)
 	diag->last = e;
 }
 
+/**
+ * Add a new error to the diagnostics area: the previous error
+ * becomes a reason of a current.
+ * \param diag diagnostics area
+ * \param e error to add
+ */
+static inline void
+diag_add_error(struct diag *diag, struct error *e)
+{
+	assert(e != NULL);
+	error_ref(e);
+	/*
+	 * Nominally e takes a reason's reference while diag
+	 * releases it's reference because it holds e now
+	 * instead. I.e. reason->refs kept unchanged.
+	 */
+	e->reason = diag->last;
+	diag->last = e;
+}
+
 /**
  * Move all errors from \a from to \a to.
  * \param from source
@@ -212,6 +236,45 @@ diag_last_error(struct diag *diag)
 	return diag->last;
 }
 
+/**
+ * Get a diagnostic savepoint: a marker that allows to reset all
+ * errors set after that moment.
+ */
+static inline struct error *
+diag_svp(struct diag *diag)
+{
+	return diag_last_error(diag);
+}
+
+/**
+ * Remove all errors set in a given diagnostics area after a
+ * given savepoint.
+ *
+ * Operation removes reason for the error
+ * preceding the savepoint and releases a diagnostic area's
+ * reference on the most recent error (diag::last for the
+ * rollback beginning). This means that if user code have a
+ * pointer and have a reference to an error object from the
+ * rollback zone, this pointer and the following "reason" error
+ * objects are a valid error list.
+ */
+static inline void
+diag_rollback_to_svp(struct diag *diag, struct error *svp)
+{
+	struct error *begin = diag->last, *prev = NULL;
+	while (diag->last != svp) {
+		prev = diag->last;
+		diag->last = diag->last->reason;
+	}
+	if (diag->last != begin) {
+		assert(prev != NULL && prev->reason == svp);
+		prev->reason = NULL;
+		error_unref(begin);
+		prev->reason = svp;
+		error_ref(svp);
+	}
+}
+
 struct diag *
 diag_get();
 
@@ -274,6 +337,17 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	errno = save_errno;                                             \
 } while (0)
 
+#define diag_add(class, ...) do {					\
+	/* Preserve the original errno. */                              \
+	int save_errno = errno;                                         \
+	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
+	struct error *e;						\
+	e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__);		\
+	diag_add_error(diag_get(), e);					\
+	/* Restore the errno which might have been reset.  */           \
+	errno = save_errno;                                             \
+} while (0)
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/lua/error.h b/src/lua/error.h
index 64fa5eba3..16cdaf7fe 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
 struct error *
 luaL_iserror(struct lua_State *L, int narg);
 
+struct error *
+luaL_checkerror(struct lua_State *L, int narg);
+
 void
 tarantool_lua_error_init(struct lua_State *L);
 
diff --git a/src/box/key_list.c b/src/box/key_list.c
index e130d1c8c..c3de262cc 100644
--- a/src/box/key_list.c
+++ b/src/box/key_list.c
@@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (rc != 0) {
 		/* Can't evaluate function. */
 		struct space *space = space_by_id(index_def->space_id);
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't evaluate function");
 		return -1;
 	}
 	uint32_t key_data_sz;
@@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (key_data == NULL) {
 		struct space *space = space_by_id(index_def->space_id);
 		/* Can't get a result returned by function . */
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't get a value returned by function");
 		return -1;
 	}
 
@@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
 		 * The key doesn't follow functional index key
 		 * definition.
 		 */
-		diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
+		diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
 			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+			 "key does not follow functional index definition");
 		return -1;
 	}
 
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 0ac2eb7a6..cebd8a680 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -680,9 +680,9 @@ func_persistent_lua_load(struct func_lua *func)
 	if (func->base.def->is_sandboxed) {
 		if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
 					nelem(default_sandbox_exports)) != 0) {
-			diag_set(ClientError, ER_LOAD_FUNCTION,
-				func->base.def->name,
-				diag_last_error(diag_get())->errmsg);
+			diag_add(ClientError, ER_LOAD_FUNCTION,
+				 func->base.def->name,
+				 "can't prepare a Lua sandbox");
 			goto end;
 		}
 	} else {
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index 248277e74..c0aeb52b9 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -52,6 +52,7 @@ error_create(struct error *e,
 		e->line = 0;
 	}
 	e->errmsg[0] = '\0';
+	e->reason = NULL;
 }
 
 struct diag *
diff --git a/src/lua/error.c b/src/lua/error.c
index d82e78dc4..18a990a88 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -53,7 +53,7 @@ luaL_iserror(struct lua_State *L, int narg)
 	return e;
 }
 
-static struct error *
+struct error *
 luaL_checkerror(struct lua_State *L, int narg)
 {
 	struct error *error = luaL_iserror(L, narg);
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 230d51dec..da0a3f52c 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -144,6 +144,22 @@ luaT_error_new(lua_State *L)
 	return luaT_error_last(L);
 }
 
+static int
+luaT_error_prev(lua_State *L)
+{
+	if (lua_gettop(L) == 0)
+		return luaL_error(L, "Usage: box.error.prev(error)");
+	struct error *e = luaL_checkerror(L, 1);
+	if (e == NULL)
+		return luaT_error(L);
+
+	if (e->reason != NULL)
+		luaT_pusherror(L, e->reason);
+	else
+		lua_pushnil(L);
+	return 1;
+}
+
 static int
 luaT_error_clear(lua_State *L)
 {
@@ -250,6 +266,10 @@ box_lua_error_init(struct lua_State *L) {
 			lua_pushcfunction(L, luaT_error_new);
 			lua_setfield(L, -2, "new");
 		}
+		{
+			lua_pushcfunction(L, luaT_error_prev);
+			lua_setfield(L, -2, "prev");
+		}
 		lua_setfield(L, -2, "__index");
 	}
 	lua_setmetatable(L, -2);
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 28fc0377d..7a4d19c5a 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -23,6 +23,8 @@ struct error {
     char _file[DIAG_FILENAME_MAX];
     /* Error description. */
     char _errmsg[DIAG_ERRMSG_MAX];
+    /* A pointer to the reason error. */
+    struct error *_reason;
 };
 
 char *
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index bb4200f7a..5ca7fcb66 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -5,6 +5,10 @@ test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
  | ---
  | ...
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+ | ---
+ | - true
+ | ...
 
 --
 -- gh-1260: Func index.
@@ -158,8 +162,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn1.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -197,8 +200,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -217,8 +219,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn4.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -264,8 +265,39 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri
 s:insert({1})
  | ---
  | - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- |     [string "return function(tuple)                 local ..."]:1: attempt to call
- |     global ''require'' (a nil value)'
+ |     can''t evaluate function'
+ | ...
+e = box.error.last()
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - type: ClientError
+ |   code: 198
+ |   message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ |     can''t evaluate function'
+ |   trace:
+ |   - file: <filename>
+ |     line: 68
+ | ...
+e = box.error.prev(e)
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - type: LuajitError
+ |   message: '[string "return function(tuple)                 local ..."]:1: attempt
+ |     to call global ''require'' (a nil value)'
+ |   trace:
+ |   - file: <filename>
+ |     line: 1010
+ | ...
+e = box.error.prev(e)
+ | ---
+ | ...
+e == nil
+ | ---
+ | - true
  | ...
 idx:drop()
  | ---
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index f31162c97..ccbc9822d 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -1,5 +1,6 @@
 test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
 
 --
 -- gh-1260: Func index.
@@ -99,6 +100,12 @@ test_run:cmd("setopt delimiter ''");
 box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
 idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
 s:insert({1})
+e = box.error.last()
+e:unpack()
+e = box.error.prev(e)
+e:unpack()
+e = box.error.prev(e)
+e == nil
 idx:drop()
 
 -- Remove old persistent functions
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 4a57597e9..6dfe1f6d9 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -72,7 +72,7 @@ target_link_libraries(decimal.test core unit)
 
 add_executable(fiber.test fiber.cc)
 set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
-target_link_libraries(fiber.test core unit)
+target_link_libraries(fiber.test core box unit)
 
 if (NOT ENABLE_GCOV)
     # This test is known to be broken with GCOV
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 91f7d43f9..9ff1cc643 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -1,3 +1,6 @@
+#include "box/error.h"
+#include "diag.h"
+#include "errcode.h"
 #include "memory.h"
 #include "fiber.h"
 #include "unit.h"
@@ -181,12 +184,166 @@ fiber_name_test()
 	footer();
 }
 
+void
+diag_test()
+{
+	header();
+	plan(28);
+
+	note("constract e1 in global diag, share with local diag");
+	diag_set(ClientError, ER_PROC_LUA, "runtime error");
+	struct diag local_diag;
+	diag_create(&local_diag);
+	/* Copy a last error to the local diagnostics area. */
+	diag_add_error(&local_diag, diag_last_error(diag_get()));
+
+	struct error *e1 = diag_last_error(&local_diag);
+	is(e1, diag_last_error(diag_get()),
+	   "e1 is an error shared between local and global diag");
+	is(e1->refs, 2, "e1::refs: global+local diag");
+
+	note("append e2 to global diag, usr error_ref(e2)");
+	diag_add(ClientError, ER_LOAD_FUNCTION, "test", "internal error");
+	struct error *e2 = diag_last_error(diag_get());
+	error_ref(e2);
+
+	is(e2->reason, e1, "e2::reason == e1");
+	is(e1->reason, NULL, "e1::reason == NULL");
+	is(e1->refs, 2, "e1::refs: e2 + global diag");
+	is(e2->refs, 2, "e2::refs: usr + global diag");
+
+	note("diag_clean global diag");
+	diag_clear(diag_get());
+	is(e1->refs, 2, "e1::refs: e2 + local");
+	is(e2->refs, 1, "e2::refs: usr");
+	note("error_unref(e2) -> e2 destruction");
+	error_unref(e2);
+	e2 = NULL;
+	is(e1->refs, 1, "e1::refs: local diag");
+
+	/* Test rollback to SVP. */
+	note("diag_move(from = local, to = global): move e1 to global");
+	diag_move(&local_diag, diag_get());
+	is(diag_is_empty(&local_diag), true, "local diag is empty");
+	is(diag_is_empty(diag_get()), false, "global diag is not empty");
+	is(diag_last_error(diag_get()), e1, "global diag::last == e1");
+
+	note("svp = diag_svp(global), i.e. 'diag_last(global) = e1' state");
+	struct error *svp = diag_svp(diag_get());
+	fail_if(svp != e1);
+	fail_if(diag_last_error(diag_get()) != e1);
+	fail_if(e1->reason != NULL);
+	note("append e3, e4 to global diag");
+	note("usr error_ref(e1), error_ref(e3), error_ref(e4)");
+	diag_add(ClientError, ER_LOAD_FUNCTION, "test", "internal error");
+	struct error *e3 = diag_last_error(diag_get());
+	error_ref(e3);
+	diag_add(ClientError, ER_FUNC_INDEX_FUNC, "func_idx", "space",
+		 "everything is bad");
+	struct error *e4 = diag_last_error(diag_get());
+	error_ref(e4);
+	is(e1->refs, 1, "e1::refs: e3");
+	is(e3->refs, 2, "e3::refs: usr + e4");
+	is(e4->refs, 2, "e4::refs: usr + global diag");
+	is(e4->reason, e3, "e4::reason == e3");
+	is(e3->reason, e1, "e3::reason == e1");
+	note("diag_rollback_to_svp(global, svp)");
+	/*
+	 * Before rollback there is a sequence
+	 * DIAG[e4]->e3->e1->NULL;
+	 * After rollback there would be DIAG[e1]->NULL and
+	 * a sequence e4->e3->e1->NULL.
+	 */
+	diag_rollback_to_svp(diag_get(), svp);
+	is(e1->refs, 2, "e1::refs: e3 + global diag %d/%d", e1->refs, 2);
+	is(e3->refs, 2, "e3::refs: usr + e4");
+	is(e4->refs, 1, "e4::refs: usr");
+	is(diag_last_error(diag_get()), e1, "diag_last(global) = e1");
+	/* Rollback doesn't change error objects itself. */
+	is(e4->reason, e3, "e4::reason == e3");
+	is(e3->reason, e1, "e3::reason == e1");
+	error_unref(e4);
+	e4 = NULL;
+	is(e3->refs, 1, "e3::refs: usr");
+	error_unref(e3);
+	e3 = NULL;
+
+	note("ensure that sequential rollback is no-op");
+	diag_rollback_to_svp(diag_get(), svp);
+	is(e1->refs, 1, "e1::refs: global diag");
+
+	diag_clear(diag_get());
+	/*
+	 *           usr ref    SVP
+	 *   DEL!       |        |
+	 *  DIAG[#5] -> #4 -> DIAG'[#3] -> #2 -> #1
+	 *
+	 * 1) diag_rollback_to_svp
+	 * del   <-----------<------>
+	 */
+	note("test partial list destruction on rollback");
+	diag_add(ClientError, ER_PROC_LUA, "#1");
+	struct error *er1 = diag_last_error(diag_get());
+	diag_add(ClientError, ER_PROC_LUA, "#2");
+	svp = diag_svp(diag_get());
+	diag_add(ClientError, ER_PROC_LUA, "#3");
+	struct error *er3 = diag_last_error(diag_get());
+	diag_add(ClientError, ER_PROC_LUA, "#4");
+	struct error *er4 = diag_last_error(diag_get());
+	error_ref(er4);
+	diag_add(ClientError, ER_PROC_LUA, "#5");
+	is(er4->refs, 2, "er4:refs: usr + er5 %d/%d", er4->refs, 2);
+
+	diag_rollback_to_svp(diag_get(), svp);
+	note("rollback to svp(er2) -> e5:refs == 0, destruction");
+	is(er4->reason, er3, "er4->reason == er3");
+	is(er3->refs, 1, "er3:refs: er4");
+	is(er3->reason, svp, "er3->reason == svp");
+	is(svp->refs, 2, "svp->refs: global diag + er3");
+	is(svp->reason, er1, "svp->reason == er1");
+	is(er1->refs, 1, "er1->refs: err2");
+
+	/*
+	 * usr ref       SVP
+	 *  |             |
+	 * #4 -> #3 -> DIAG'[#2] -> #1
+	 *                 |
+	 * DIAG[#7] -> #6 -/
+	 *   |
+	 * usr ref
+	 */
+	note("multiple error sequences after rollback");
+	diag_add(ClientError, ER_PROC_LUA, "#6");
+	diag_add(ClientError, ER_PROC_LUA, "#7");
+	struct error *er7 = diag_last_error(diag_get());
+	error_ref(er7);
+	is(er4->refs, 1, "er4->refs: usr");
+	is(er7->refs, 2, "er7->refs: global diag + usr");
+	is(svp->refs, 2, "svp->refs: er3 + er6");
+	is(svp->reason->refs, 1, "svp->reason->refs: svp");
+	diag_rollback_to_svp(diag_get(), svp);
+	is(er4->refs, 1, "er4->refs: usr");
+	is(er7->refs, 1, "er7->refs: usr");
+	is(svp->refs, 3, "svp->refs: global diag + er3 + er6");
+	is(svp->reason->refs, 1, "svp->reason->refs: svp");
+	diag_clear(diag_get());
+	is(svp->refs, 2, "svp->refs: er3 + er6");
+	is(svp->reason->refs, 1, "svp->reason->refs: svp");
+	error_unref(er4);
+	is(svp->refs, 1, "svp->refs: er6");
+	is(svp->reason->refs, 1, "svp->reason->refs: svp");
+	error_unref(er7);
+
+	footer();
+}
+
 static int
 main_f(va_list ap)
 {
 	fiber_name_test();
 	fiber_join_test();
 	fiber_stack_test();
+	diag_test();
 	ev_break(loop(), EVBREAK_ALL);
 	return 0;
 }
diff --git a/test/unit/fiber.result b/test/unit/fiber.result
index 7c9f85dcd..0047426d1 100644
--- a/test/unit/fiber.result
+++ b/test/unit/fiber.result
@@ -17,3 +17,63 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc
 # normal-stack fiber not crashed
 # big-stack fiber not crashed
 	*** fiber_stack_test: done ***
+	*** diag_test ***
+1..28
+# constract e1 in global diag, share with local diag
+ok 1 - e1 is an error shared between local and global diag
+ok 2 - e1::refs: global+local diag
+# append e2 to global diag, usr error_ref(e2)
+ok 3 - e2::reason == e1
+ok 4 - e1::reason == NULL
+ok 5 - e1::refs: e2 + global diag
+ok 6 - e2::refs: usr + global diag
+# diag_clean global diag
+ok 7 - e1::refs: e2 + local
+ok 8 - e2::refs: usr
+# error_unref(e2) -> e2 destruction
+ok 9 - e1::refs: local diag
+# diag_move(from = local, to = global): move e1 to global
+ok 10 - local diag is empty
+ok 11 - global diag is not empty
+ok 12 - global diag::last == e1
+# svp = diag_svp(global), i.e. 'diag_last(global) = e1' state
+# append e3, e4 to global diag
+# usr error_ref(e1), error_ref(e3), error_ref(e4)
+ok 13 - e1::refs: e3
+ok 14 - e3::refs: usr + e4
+ok 15 - e4::refs: usr + global diag
+ok 16 - e4::reason == e3
+ok 17 - e3::reason == e1
+# diag_rollback_to_svp(global, svp)
+ok 18 - e1::refs: e3 + global diag 2/2
+ok 19 - e3::refs: usr + e4
+ok 20 - e4::refs: usr
+ok 21 - diag_last(global) = e1
+ok 22 - e4::reason == e3
+ok 23 - e3::reason == e1
+ok 24 - e3::refs: usr
+# ensure that sequential rollback is no-op
+ok 25 - e1::refs: global diag
+# test partial list destruction on rollback
+ok 26 - er4:refs: usr + er5 2/2
+# rollback to svp(er2) -> e5:refs == 0, destruction
+ok 27 - er4->reason == er3
+ok 28 - er3:refs: er4
+ok 29 - er3->reason == svp
+ok 30 - svp->refs: global diag + er3
+ok 31 - svp->reason == er1
+ok 32 - er1->refs: err2
+# multiple error sequences after rollback
+ok 33 - er4->refs: usr
+ok 34 - er7->refs: global diag + usr
+ok 35 - svp->refs: er3 + er6
+ok 36 - svp->reason->refs: svp
+ok 37 - er4->refs: usr
+ok 38 - er7->refs: usr
+ok 39 - svp->refs: global diag + er3 + er6
+ok 40 - svp->reason->refs: svp
+ok 41 - svp->refs: er3 + er6
+ok 42 - svp->reason->refs: svp
+ok 43 - svp->refs: er6
+ok 44 - svp->reason->refs: svp
+	*** diag_test: done ***
-- 
2.22.1

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
> Part of #1148
> ---
>  doc/rfc/1148-stacked-diagnostics.md | 175 ++++++++++++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 doc/rfc/1148-stacked-diagnostics.md

as I mentioned before, lgtm. I agree with PeterG comments, but I
think we can worry about them when we get to implementing SQL
warnings.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 2/3] box: rename diag_add_error to diag_set_error

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
> Renamed function diag_add_error to diag_set_error because it
> actually replaces an error object in diagnostic area with a new
> one and this name is not representative. Moreover, we are going
> to introduce a new diag_add_error that extends error object in
> diagnostic area in following patches.

lgtm, it's obvious.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
>  static inline void
> @@ -90,9 +92,11 @@ static inline void
>  error_unref(struct error *e)
>  {
>  	assert(e->refs > 0);
> -	--e->refs;
> -	if (e->refs == 0)
> +	if (--e->refs == 0) {
> +		if (e->reason != NULL)
> +			error_unref(e->reason);
>  		e->destroy(e);
> +	}

This looks fragile, can you rewrite this place without recursion?
> + * Remove all errors set in a given diagnostics area after a
> + * given savepoint.
> + *
> + * Operation removes reason for the error
> + * preceding the savepoint and releases a diagnostic area's
> + * reference on the most recent error (diag::last for the
> + * rollback beginning). This means that if user code have a
> + * pointer and have a reference to an error object from the
> + * rollback zone, this pointer and the following "reason" error
> + * objects are a valid error list.
> + */

BTW, did you check if anyone likes the name 'reason'? I'm not very
fond of it, previous error is not always the reason of the current
one. I would simply call it 'prev' or 'next'.

> +	if (diag->last != begin) {
> +		assert(prev != NULL && prev->reason == svp);
> +		prev->reason = NULL;
> +		error_unref(begin);

OK, this works because you terminate the list with NULL first...

> +		prev->reason = svp;
> +		error_ref(svp);
> +	}
> +}
> +
> +++ b/src/box/key_list.c
> @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (rc != 0) {
>  		/* Can't evaluate function. */
>  		struct space *space = space_by_id(index_def->space_id);
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't evaluate function");
>  		return -1;
>  	}
>  	uint32_t key_data_sz;
> @@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (key_data == NULL) {
>  		struct space *space = space_by_id(index_def->space_id);
>  		/* Can't get a result returned by function . */
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't get a value returned by function");
>  		return -1;
>  	}

Nice. I bet there is a bunch of places like that, forgotten by
now.
With these two comments, LGTM.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
> Closes #1148

Please write a follow up patch for IPROTO, will you?


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area

[Alexander Turenko]

> > + * Remove all errors set in a given diagnostics area after a
> > + * given savepoint.
> > + *
> > + * Operation removes reason for the error
> > + * preceding the savepoint and releases a diagnostic area's
> > + * reference on the most recent error (diag::last for the
> > + * rollback beginning). This means that if user code have a
> > + * pointer and have a reference to an error object from the
> > + * rollback zone, this pointer and the following "reason" error
> > + * objects are a valid error list.
> > + */
> 
> BTW, did you check if anyone likes the name 'reason'? I'm not very
> fond of it, previous error is not always the reason of the current
> one. I would simply call it 'prev' or 'next'.

Can you give an example of the error that definitely should be previous
for another one, but is not its reason?

It is so for warnings, but this is possibly because they just should not
be mixed with errors within one stack.

WBR, Alexander Turenko.

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Alexander Turenko]

On Tue, Aug 27, 2019 at 01:26:07AM +0300, Konstantin Osipov wrote:
> * Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
> > Part of #1148
> > ---
> >  doc/rfc/1148-stacked-diagnostics.md | 175 ++++++++++++++++++++++++++++
> >  1 file changed, 175 insertions(+)
> >  create mode 100644 doc/rfc/1148-stacked-diagnostics.md
> 
> as I mentioned before, lgtm. I agree with PeterG comments, but I
> think we can worry about them when we get to implementing SQL
> warnings.

I think the important question here is when we clear a diagnostic area
(if we ever want to decide now anything about warnings). Say, we
possibly will need to entirely split warning stacks from the error stack
and possibly will need to operate on several warning stacks.

I'm tentative that we can decide now whether we should store warnings
within the same stack as errors or not. Maybe we should postpone all
warnings questions now and concentrate on errors within the scope #1148.

WBR, Alexander Turenko.

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/08/27 09:50]:
> > as I mentioned before, lgtm. I agree with PeterG comments, but I
> > think we can worry about them when we get to implementing SQL
> > warnings.
> 
> I think the important question here is when we clear a diagnostic area
> (if we ever want to decide now anything about warnings). Say, we
> possibly will need to entirely split warning stacks from the error stack
> and possibly will need to operate on several warning stacks.
> 
> I'm tentative that we can decide now whether we should store warnings
> within the same stack as errors or not. Maybe we should postpone all
> warnings questions now and concentrate on errors within the scope #1148.

MySQL uses a statement counter which is incremented on every
external call, and triggers a reset of the area. 

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/08/27 09:50]:
> > > + * Operation removes reason for the error
> > > + * preceding the savepoint and releases a diagnostic area's
> > > + * reference on the most recent error (diag::last for the
> > > + * rollback beginning). This means that if user code have a
> > > + * pointer and have a reference to an error object from the
> > > + * rollback zone, this pointer and the following "reason" error
> > > + * objects are a valid error list.
> > > + */
> > 
> > BTW, did you check if anyone likes the name 'reason'? I'm not very
> > fond of it, previous error is not always the reason of the current
> > one. I would simply call it 'prev' or 'next'.
> 
> Can you give an example of the error that definitely should be previous
> for another one, but is not its reason?
> 
> It is so for warnings, but this is possibly because they just should not
> be mixed with errors within one stack.

You try to do some useful work and get an error. You unwind, and
get another one during cleanup.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Alexander Turenko]

> +### Lua API
> +Tarantool returns a last-set (diag::last) error object as `cdata` from central diagnostic
> +area to Lua in case of error. User shouldn't have an ability to modify it
> +(because he actually needn't and because it is dangerous - he doesn't own this object),
> +but it need an ability to inspect a collected diagnostics information.
> +
> +Let's extend the `box.error` API with a couple of new functions:
> +1) `prev` that allows to get a reason of given error:
> +
> +```
> +-- Return a reason error object for given error
> +-- (when exists, nil otherwise)
> +box.error.prev(error)
> +```

I would also add err:prev() method like we did for, say,
box.tuple.update(t, <...>) and t:update(<...>).

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Kirill Shcherbatov]

> I would also add err:prev() method like we did for, say,
> box.tuple.update(t, <...>) and t:update(<...>).
I am not sure, that it is a good idea. Obviously the list must be double-linked.
Then, in current implementation, an error has a reference for it's reason.
With double-linked list this would become more complicated and I really don't
think that it is reasonable.  

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Alexander Turenko]

On Fri, Aug 30, 2019 at 04:24:03PM +0300, Kirill Shcherbatov wrote:
> > I would also add err:prev() method like we did for, say,
> > box.tuple.update(t, <...>) and t:update(<...>).
> I am not sure, that it is a good idea. Obviously the list must be double-linked.
> Then, in current implementation, an error has a reference for it's reason.
> With double-linked list this would become more complicated and I really don't
> think that it is reasonable.  

I'm about a metamethod :prev() (in addition to box.error.prev()), not
about adding :next() or so.

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Kirill Shcherbatov]

> I'm about a metamethod :prev() (in addition to box.error.prev()), not
> about adding :next() or so.
But having a cdata<struct error> object in Lua I also need a way to obtain prev
item, haven't I? 

[tarantool-patches] Re: [PATCH v2 1/3] box: rfc for stacked diagnostic area in Tarantool

[Alexander Turenko]

On Fri, Aug 30, 2019 at 05:13:44PM +0300, Kirill Shcherbatov wrote:
> > I'm about a metamethod :prev() (in addition to box.error.prev()), not
> > about adding :next() or so.
> But having a cdata<struct error> object in Lua I also need a way to obtain prev
> item, haven't I? 

Yep, from 'prev' field.