Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason
@ 2021-11-11 23:54 Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 01/11] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
                   ` (12 more replies)
  0 siblings, 13 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

Changes in v2:
- A lot of minor fixes in all the commits;
- libuuid is moved into libcore;
- ER_READONLY message got the same details as in the fields.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5568-err-readonly-reason
Issue: https://github.com/tarantool/tarantool/issues/5568

Vladislav Shpilevoy (11):
  diag: return created error from diag_set()
  uuid: move into libcore
  error: introduce error_payload
  error: move code to struct error from ClientError
  error: use error_payload to store optional members
  error: use error_payload in MessagePack codecs
  error: use error_payload in Lua
  luatest: copy config in cluster:build_server()
  luatest: add new helpers for 'server' object
  box: enrich ER_READONLY with new details
  error: report ER_READONLY reason in message

 .../unreleased/gh-5568-readonly-reason.md     |   4 +
 extra/exports                                 |   3 +-
 src/CMakeLists.txt                            |   3 +-
 src/box/applier.h                             |   2 +-
 src/box/bind.h                                |   2 +-
 src/box/box.cc                                |  69 ++-
 src/box/errcode.h                             |   2 +-
 src/box/error.cc                              |  57 +--
 src/box/error.h                               |  60 +--
 src/box/field_def.c                           |   4 +-
 src/box/index.cc                              |   4 +-
 src/box/lua/serialize_lua.c                   |   2 +-
 src/box/mp_error.cc                           | 197 +++-----
 src/box/msgpack.c                             |   2 +-
 src/box/replication.h                         |   2 +-
 src/box/sql/mem.c                             |   2 +-
 src/box/sql/mem.h                             |   2 +-
 src/box/tuple.h                               |   2 +-
 src/box/tuple_compare.cc                      |   8 +-
 src/box/xlog.h                                |   2 +-
 src/box/xrow.h                                |   2 +-
 src/lib/CMakeLists.txt                        |   1 -
 src/lib/core/CMakeLists.txt                   |   3 +
 src/lib/core/crash.c                          |   2 +-
 src/lib/core/diag.c                           |  37 +-
 src/lib/core/diag.h                           | 109 +++-
 src/lib/core/error_payload.c                  | 301 +++++++++++
 src/lib/core/error_payload.h                  | 190 +++++++
 src/lib/core/exception.cc                     |   8 +-
 src/lib/core/exception.h                      |  66 +++
 src/lib/{uuid => core}/mp_uuid.c              |   0
 src/lib/{uuid => core}/mp_uuid.h              |   0
 src/lib/{uuid => core}/tt_uuid.c              |   0
 src/lib/{uuid => core}/tt_uuid.h              |   0
 src/lib/mpstream/CMakeLists.txt               |   2 +-
 src/lib/mpstream/mpstream.c                   |   2 +-
 src/lib/swim/CMakeLists.txt                   |   2 +-
 src/lib/swim/swim_io.h                        |   2 +-
 src/lib/swim/swim_proto.h                     |   2 +-
 src/lib/uuid/CMakeLists.txt                   |   2 -
 src/lua/error.lua                             |  84 ++--
 src/lua/init.lua                              |  24 -
 src/lua/msgpack.c                             |   6 +-
 src/lua/tnt_msgpuck.h                         |   2 +-
 src/lua/utils.c                               |   2 +-
 test/box/error.result                         |   4 +-
 test/box/error.test.lua                       |   2 +-
 test/engine/func_index.result                 |   3 +-
 test/luatest_helpers/cluster.lua              |   1 +
 test/luatest_helpers/server.lua               |  66 ++-
 .../gh_5568_read_only_reason_test.lua         | 304 ++++++++++++
 test/replication/anon.result                  |   6 +-
 test/replication/catch.result                 |   4 +-
 test/replication/election_qsync.result        |  11 +-
 test/replication/election_qsync.test.lua      |   4 +-
 .../gh-6034-qsync-limbo-ownership.result      |  22 +-
 .../gh-6034-qsync-limbo-ownership.test.lua    |   8 +-
 test/sql-tap/CMakeLists.txt                   |   2 +-
 test/sql-tap/gh-6024-funcs-return-bin.c       |   2 +-
 test/sql-tap/sql_uuid.c                       |   2 +-
 test/unit/CMakeLists.txt                      |   4 +-
 test/unit/error.c                             | 466 ++++++++++++++++++
 test/unit/error.result                        | 161 ++++++
 test/unit/mp_error.cc                         |  76 ++-
 test/unit/mp_error.result                     |  27 +-
 test/unit/swim_proto.c                        |   2 +-
 test/unit/swim_test_utils.c                   |   2 +-
 test/unit/swim_test_utils.h                   |   2 +-
 test/unit/uuid.c                              |   4 +-
 test/unit/vy_iterators_helper.c               |   2 +-
 test/unit/xrow.cc                             |   2 +-
 test/vinyl/misc.result                        |   2 +-
 third_party/lua-cjson/lua_cjson.c             |   2 +-
 third_party/lua-yaml/lyaml.cc                 |   2 +-
 74 files changed, 2069 insertions(+), 404 deletions(-)
 create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
 create mode 100644 src/lib/core/error_payload.c
 create mode 100644 src/lib/core/error_payload.h
 rename src/lib/{uuid => core}/mp_uuid.c (100%)
 rename src/lib/{uuid => core}/mp_uuid.h (100%)
 rename src/lib/{uuid => core}/tt_uuid.c (100%)
 rename src/lib/{uuid => core}/tt_uuid.h (100%)
 delete mode 100644 src/lib/uuid/CMakeLists.txt
 create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
 create mode 100644 test/unit/error.c
 create mode 100644 test/unit/error.result

-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 01/11] diag: return created error from diag_set()
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

And from diag_add(). This will be helpful not to bother with
box_error_last() and diag_last_error(diag_get()) in the future
patch. It will change some attributes of a just created
ER_READONLY error to add more details.

Part of #5568
---
 src/lib/core/diag.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fa85326ad..d7d09f6fb 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -347,7 +347,7 @@ struct error *
 BuildSocketError(const char *file, unsigned line, const char *socketname,
 		 const char *format, ...);
 
-#define diag_set_detailed(file, line, class, ...) do {			\
+#define diag_set_detailed(file, line, class, ...) ({			\
 	/* Preserve the original errno. */                              \
 	int save_errno = errno;                                         \
 	say_debug("%s at %s:%i", #class, file, line);			\
@@ -356,19 +356,21 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	diag_set_error(diag_get(), e);					\
 	/* Restore the errno which might have been reset.  */           \
 	errno = save_errno;                                             \
-} while (0)
+	e;								\
+})
 
 #define diag_set(...)							\
 	diag_set_detailed(__FILE__, __LINE__, __VA_ARGS__)
 
-#define diag_add(class, ...) do {					\
+#define diag_add(class, ...) ({						\
 	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);					\
 	errno = save_errno;						\
-} while (0)
+	e;								\
+})
 
 #if defined(__cplusplus)
 } /* extern "C" */
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 01/11] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-12  7:30   ` Serge Petrenko via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 11/11] error: report ER_READONLY reason in message Vladislav Shpilevoy via Tarantool-patches
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

ER_READONLY used not to have any details about the exact reason
why the instance is read-only. The patch changes that by adding
new fields into the error which explain why the error happened and
even help to avoid it for next requests.

Now from the error object can tell whether it was raised because
of box.cfg.read_only = true, or the instance is an orphan, or it
has election enabled but is not a leader, or the transaction limbo
belongs to another instance.

The alternative to ClientError alteration via error_payload was
not to touch struct error and introduce a new error type
specifically for ER_READONLY via a new C++ class like
ReadOnlyError. But it had drawbacks:

- There may be clients who expect ER_READONLY to have ClientError
  type. For instance, they check err.code == ER_READONLY only for
  err.type == 'ClientError';

- Having to introduce a new C++ class each time when want to add a
  new field into an error has to end. Rather sooner than later.

Closes #5568

@TarantoolBot document
Title: box.error.READONLY new attributes

Users could see the error code as `box.error.READONLY` in `.code`
field of an error object. The error didn't have any other
attributes except common ones like 'type'.

Now from the `box.error.READONLY` error users can see why it
happened. The reasons can be the following:

* The instance has `box.cfg.read_only = true`. Then the `READONLY`
error has at least these fields:
```
tarantool> err:unpack()
---
- state: read_only
  reason: state
  code: 7
  type: ClientError
...
```

* The instance is an orphan. It enters that state if number of
connected replicas is < `box.cfg.replication_connect_quorum`. Then
`READONLY` error has at least these fields:
```
tarantool> err:unpack()
---
- state: orphan
  reason: state
  code: 7
  type: ClientError
...
```

* The synchro queue has an owner which is not the given instance.
It usually happens if synchronous replication is used and there is
another instance who called `box.ctl.promote()`. Then `READONLY`
error has at least these fields:
```
tarantool> err:unpack()
---
- queue_owner_id: <box.info.id of the queue owner>
  queue_owner_uuid: <box.info.uuid of the queue owner>
  reason: synchro
  term: <box.info.election.term of this instance>
  code: 7
  type: ClientError
...
```
Note than `queue_owner_uuid` sometimes might be not present.

* The instance has `box.cfg.election_mode` not `off` and it is not
a leader. Then `READONLY` error has at least these fields:
```
tarantool> err:unpack()
---
- state: <box.info.election.state of this instance>
  leader_id: <box.info.id of the leader>
  leader_uuid: <box.info.uuid of the leader>
  reason: election
  term: <box.info.election.term of this instance>
  code: 7
  type: ClientError
...
```
`leader_id` and `leader_uuid` might be absent if the leader is not
known. For example, an election is still in progress. Note, than
`leader_uuid` sometimes might be not present even if `leader_id`
is.

If multiple reasons are true at the same time, then only one is
returned in order reversed from how they are listed above. IOW,
election, synchro, box.cfg.read_only, orphan.
---
 .../unreleased/gh-5568-readonly-reason.md     |   4 +
 src/box/box.cc                                |  54 +++-
 .../gh_5568_read_only_reason_test.lua         | 285 ++++++++++++++++++
 3 files changed, 335 insertions(+), 8 deletions(-)
 create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
 create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua

diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
new file mode 100644
index 000000000..f3a2db986
--- /dev/null
+++ b/changelogs/unreleased/gh-5568-readonly-reason.md
@@ -0,0 +1,4 @@
+## feature/core
+
+* Error objects with the code `box.error.READONLY` now have additional fields
+  explaining why the error happened (gh-5568).
diff --git a/src/box/box.cc b/src/box/box.cc
index 1ed1ce3f8..323982969 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -177,16 +177,54 @@ box_update_ro_summary(void)
 static int
 box_check_writable(void)
 {
-	if (is_ro_summary) {
+	if (!is_ro_summary)
+		return 0;
+	struct error *e = diag_set(ClientError, ER_READONLY);
+	struct raft *raft = box_raft();
+	/*
+	 * In case of multiple reasons at the same time only one is reported.
+	 * But the order is important. For example, if the instance has election
+	 * enabled, for the client it is better to see that it is a 'follower'
+	 * and who is the leader than just see cfg 'read_only' is true.
+	 */
+	if (raft_is_ro(raft)) {
+		error_set_str(e, "reason", "election");
+		error_set_str(e, "state", raft_state_str(raft->state));
+		error_set_uint(e, "term", raft->volatile_term);
+		uint32_t id = raft->leader;
+		if (id != REPLICA_ID_NIL) {
+			error_set_uint(e, "leader_id", id);
+			struct replica *r = replica_by_id(id);
+			/*
+			 * XXX: when the leader is dropped from _cluster, it
+			 * is not reported to Raft.
+			 */
+			if (r != NULL)
+				error_set_uuid(e, "leader_uuid", &r->uuid);
+		}
+	} else if (txn_limbo_is_ro(&txn_limbo)) {
+		error_set_str(e, "reason", "synchro");
+		uint32_t id = txn_limbo.owner_id;
+		error_set_uint(e, "queue_owner_id", id);
+		error_set_uint(e, "term", raft->volatile_term);
+		struct replica *r = replica_by_id(id);
 		/*
-		 * XXX: return a special error when the node is not a leader to
-		 * reroute to the leader node.
+		 * XXX: when an instance is deleted from _cluster, its limbo's
+		 * ownership is not cleared.
 		 */
-		diag_set(ClientError, ER_READONLY);
-		diag_log();
-		return -1;
-	}
-	return 0;
+		if (r != NULL)
+			error_set_uuid(e, "queue_owner_uuid", &r->uuid);
+	} else {
+		error_set_str(e, "reason", "state");
+		if (is_ro)
+			error_set_str(e, "state", "read_only");
+		else if (is_orphan)
+			error_set_str(e, "state", "orphan");
+		else
+			assert(false);
+	}
+	diag_log();
+	return -1;
 }
 
 static void
diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
new file mode 100644
index 000000000..8effa69e8
--- /dev/null
+++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
@@ -0,0 +1,285 @@
+local t = require('luatest')
+local cluster = require('test.luatest_helpers.cluster')
+local helpers = require('test.luatest_helpers')
+
+--
+-- gh-5568: ER_READONLY needs to carry additional info explaining the reason
+-- why the error happened. It should help the clients to do better error logging
+-- and to use the provided info to find the actual leader if the reason is in
+-- being not a leader.
+--
+
+--
+-- Make functions which will create and destroy a cluster. Usage of the closures
+-- is a workaround for luatest hooks not accepting their group as a parameter.
+--
+local function make_create_cluster(g) return function()
+    g.cluster = cluster:new({})
+    local master_uri = helpers.instance_uri('master')
+    local replica_uri = helpers.instance_uri('replica')
+    local replication = {master_uri, replica_uri}
+    local box_cfg = {
+        listen = master_uri,
+        replication = replication,
+        -- To speed up new term when try to elect a new leader.
+        replication_timeout = 0.1,
+    }
+    g.master = g.cluster:build_server({alias = 'master', box_cfg = box_cfg})
+
+    box_cfg.listen = replica_uri
+    g.replica = g.cluster:build_server({alias = 'replica', box_cfg = box_cfg})
+
+    g.cluster:add_server(g.master)
+    g.cluster:add_server(g.replica)
+    g.cluster:start()
+end end
+
+local function make_destroy_cluster(g) return function()
+    g.cluster:drop()
+end end
+
+--
+-- This group's test cases leave instances in a valid state after which they can
+-- be reused.
+--
+local g = t.group('gh-5568-read-only-reason1')
+
+g.before_all(make_create_cluster(g))
+g.after_all(make_destroy_cluster(g))
+
+--
+-- Read-only because of box.cfg{read_only = true}.
+--
+g.test_read_only_reason_cfg = function(g)
+    local ok, err = g.master:exec(function()
+        box.cfg{read_only = true}
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert_covers(err, {
+        reason = 'state',
+        state = 'read_only',
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is read_only')
+
+    -- Cleanup.
+    g.master:exec(function()
+        box.cfg{read_only = false}
+    end)
+end
+
+--
+-- Read-only because is an orphan.
+--
+g.test_read_only_reason_orphan = function(g)
+    local fake_uri = helpers.instance_uri('fake')
+    local old_timeout, ok, err = g.master:exec(function(fake_uri)
+        -- Make connect-quorum impossible to satisfy using a fake instance.
+        local old_timeout = box.cfg.replication_connect_timeout
+        local repl = table.copy(box.cfg.replication)
+        table.insert(repl, fake_uri)
+        box.cfg{
+            replication = repl,
+            replication_connect_timeout = 0.001,
+        }
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return old_timeout, ok, err:unpack()
+    end, {fake_uri})
+    t.assert(not ok, 'fail ddl')
+    t.assert_covers(err, {
+        reason = 'state',
+        state = 'orphan',
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is orphan')
+
+    -- Cleanup.
+    g.master:exec(function(old_timeout)
+        local repl = table.copy(box.cfg.replication)
+        table.remove(repl)
+        box.cfg{
+            replication = repl,
+            replication_connect_timeout = old_timeout,
+        }
+    end, {old_timeout})
+end
+
+--
+-- Read-only because is not a leader. Does not know the leader.
+--
+g.test_read_only_reason_election_no_leader = function(g)
+    local ok, err = g.master:exec(function()
+        box.cfg{election_mode = 'voter'}
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert(err.term, 'has term')
+    t.assert_covers(err, {
+        reason = 'election',
+        state = 'follower',
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is election, no leader')
+    t.assert(not err.leader_id, 'no leader id')
+    t.assert(not err.leader_uuid, 'no leader uuid')
+
+    -- Cleanup.
+    g.master:exec(function()
+        box.cfg{election_mode = 'off'}
+    end)
+end
+
+--
+-- Read-only because is not a leader. Knows the leader.
+--
+g.test_read_only_reason_election_has_leader = function(g)
+    g.master:exec(function()
+        box.cfg{election_mode = 'candidate'}
+    end)
+    g.master:wait_election_leader()
+    g.replica:wait_election_leader_found()
+    local ok, err = g.replica:exec(function()
+        box.cfg{election_mode = 'voter'}
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert(err.term, 'has term')
+    t.assert_covers(err, {
+        reason = 'election',
+        state = 'follower',
+        leader_id = g.master:instance_id(),
+        leader_uuid = g.master:instance_uuid(),
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is election, has leader')
+
+    -- Cleanup.
+    g.master:exec(function()
+        box.cfg{election_mode = 'off'}
+        box.ctl.demote()
+    end)
+    g.replica:exec(function()
+        box.cfg{election_mode = 'off'}
+    end)
+end
+
+--
+-- Read-only because does not own the limbo. Knows the owner.
+--
+g.test_read_only_reason_synchro = function(g)
+    g.master:exec(function()
+        box.ctl.promote()
+    end)
+
+    t.helpers.retrying({}, function()
+        assert(g.replica:exec(function()
+            return box.info.synchro.queue.owner ~= 0
+        end))
+    end)
+
+    local ok, err = g.replica:exec(function()
+        local ok, err = pcall(box.schema.create_space, 'test2')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert(err.term, 'has term')
+    t.assert_covers(err, {
+        reason = 'synchro',
+        queue_owner_id = g.master:instance_id(),
+        queue_owner_uuid = g.master:instance_uuid(),
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is synchro, has owner')
+
+    -- Cleanup.
+    g.master:exec(function()
+        box.ctl.demote()
+    end)
+end
+
+--
+-- This group's test cases leave instances in an invalid state after which they
+-- should be re-created.
+--
+g = t.group('gh-5568-read-only-reason2')
+
+g.before_each(make_create_cluster(g))
+g.after_each(make_destroy_cluster(g))
+
+--
+-- Read-only because is not a leader. Knows the leader, but not its UUID.
+--
+g.test_read_only_reason_election_has_leader_no_uuid = function(g)
+    g.replica:exec(function()
+        box.cfg{election_mode = 'voter'}
+    end)
+    g.master:exec(function()
+        box.cfg{election_mode = 'candidate'}
+    end)
+    g.master:wait_election_leader()
+    g.replica:wait_election_leader_found()
+    local leader_id = g.master:instance_id()
+
+    g.master:exec(function()
+        box.space._cluster:run_triggers(false)
+        box.space._cluster:delete{box.info.id}
+    end)
+
+    t.helpers.retrying({}, function()
+        assert(g.replica:exec(function(leader_id)
+            return box.space._cluster:get{leader_id} == nil
+        end, {leader_id}))
+    end)
+
+    local ok, err = g.replica:exec(function()
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert(err.term, 'has term')
+    t.assert(not err.leader_uuid, 'has no leader uuid')
+    t.assert_covers(err, {
+        reason = 'election',
+        state = 'follower',
+        leader_id = leader_id,
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is election, has leader but no uuid')
+end
+
+--
+-- Read-only because does not own the limbo. Knows the owner, but not its UUID.
+--
+g.test_read_only_reason_synchro_no_uuid = function(g)
+    g.master:exec(function()
+        box.ctl.promote()
+        box.space._cluster:run_triggers(false)
+        box.space._cluster:delete{box.info.id}
+    end)
+
+    local leader_id = g.master:instance_id()
+    t.helpers.retrying({}, function()
+        assert(g.replica:exec(function(leader_id)
+            return box.info.synchro.queue.owner ~= 0 and
+                   box.space._cluster:get{leader_id} == nil
+        end, {leader_id}))
+    end)
+
+    local ok, err = g.replica:exec(function()
+        local ok, err = pcall(box.schema.create_space, 'test')
+        return ok, err:unpack()
+    end)
+    t.assert(not ok, 'fail ddl')
+    t.assert(err.term, 'has term')
+    t.assert(not err.queue_owner_uuid)
+    t.assert_covers(err, {
+        reason = 'synchro',
+        queue_owner_id = leader_id,
+        code = box.error.READONLY,
+        type = 'ClientError'
+    }, 'reason is synchro, has owner but no uuid')
+end
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 11/11] error: report ER_READONLY reason in message
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 01/11] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 02/11] uuid: move into libcore Vladislav Shpilevoy via Tarantool-patches
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

A previous commit added reason details as error fields. But error
objects are serialized as strings by default thus loosing all the
details.

This commit makes ER_READONLY message contain the reason why it
happened in a human-readable form. That way it will be well
visible in the logs at least.

Follow-up #5568
---
 src/box/box.cc                                | 33 ++++++++++++++-----
 src/box/errcode.h                             |  2 +-
 src/lib/core/diag.c                           | 11 +++++++
 src/lib/core/diag.h                           |  3 ++
 .../gh_5568_read_only_reason_test.lua         | 19 +++++++++++
 test/replication/anon.result                  |  6 ++--
 test/replication/catch.result                 |  4 +--
 test/replication/election_qsync.result        | 11 +++++--
 test/replication/election_qsync.test.lua      |  4 ++-
 .../gh-6034-qsync-limbo-ownership.result      | 22 ++++++++++---
 .../gh-6034-qsync-limbo-ownership.test.lua    |  8 +++--
 test/vinyl/misc.result                        |  2 +-
 12 files changed, 101 insertions(+), 24 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 323982969..f5162e67c 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -181,6 +181,7 @@ box_check_writable(void)
 		return 0;
 	struct error *e = diag_set(ClientError, ER_READONLY);
 	struct raft *raft = box_raft();
+	error_append_msg(e, " - ");
 	/*
 	 * In case of multiple reasons at the same time only one is reported.
 	 * But the order is important. For example, if the instance has election
@@ -188,40 +189,56 @@ box_check_writable(void)
 	 * and who is the leader than just see cfg 'read_only' is true.
 	 */
 	if (raft_is_ro(raft)) {
+		const char *state = raft_state_str(raft->state);
+		uint64_t term = raft->volatile_term;
 		error_set_str(e, "reason", "election");
-		error_set_str(e, "state", raft_state_str(raft->state));
-		error_set_uint(e, "term", raft->volatile_term);
+		error_set_str(e, "state", state);
+		error_set_uint(e, "term", term);
+		error_append_msg(e, "state is election %s with term %llu",
+				 state, term);
 		uint32_t id = raft->leader;
 		if (id != REPLICA_ID_NIL) {
 			error_set_uint(e, "leader_id", id);
+			error_append_msg(e, ", leader is %u", id);
 			struct replica *r = replica_by_id(id);
 			/*
 			 * XXX: when the leader is dropped from _cluster, it
 			 * is not reported to Raft.
 			 */
-			if (r != NULL)
+			if (r != NULL) {
 				error_set_uuid(e, "leader_uuid", &r->uuid);
+				error_append_msg(e, " (%s)",
+						 tt_uuid_str(&r->uuid));
+			}
 		}
 	} else if (txn_limbo_is_ro(&txn_limbo)) {
 		error_set_str(e, "reason", "synchro");
 		uint32_t id = txn_limbo.owner_id;
+		uint64_t term = raft->volatile_term;
 		error_set_uint(e, "queue_owner_id", id);
-		error_set_uint(e, "term", raft->volatile_term);
+		error_set_uint(e, "term", term);
+		error_append_msg(e, "synchro queue with term %llu belongs "
+				 "to %u", term, id);
 		struct replica *r = replica_by_id(id);
 		/*
 		 * XXX: when an instance is deleted from _cluster, its limbo's
 		 * ownership is not cleared.
 		 */
-		if (r != NULL)
+		if (r != NULL) {
 			error_set_uuid(e, "queue_owner_uuid", &r->uuid);
+			error_append_msg(e, " (%s)", tt_uuid_str(&r->uuid));
+		}
 	} else {
 		error_set_str(e, "reason", "state");
-		if (is_ro)
+		if (is_ro) {
 			error_set_str(e, "state", "read_only");
-		else if (is_orphan)
+			error_append_msg(e, "box.cfg.read_only is true");
+		} else if (is_orphan) {
 			error_set_str(e, "state", "orphan");
-		else
+			error_append_msg(e, "it is an orphan");
+		} else {
 			assert(false);
+		}
 	}
 	diag_log();
 	return -1;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index ac178f1f1..66c335c20 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -59,7 +59,7 @@ struct errcode_record {
 	/*  4 */_(ER_TUPLE_NOT_FOUND,		"Tuple doesn't exist in index '%s' in space '%s'") \
 	/*  5 */_(ER_UNSUPPORTED,		"%s does not support %s") \
 	/*  6 */_(ER_NONMASTER,			"Can't modify data on a replication slave. My master is: %s") \
-	/*  7 */_(ER_READONLY,			"Can't modify data because this instance is in read-only mode.") \
+	/*  7 */_(ER_READONLY,			"Can't modify data on a read-only instance") \
 	/*  8 */_(ER_INJECTION,			"Error injection '%s'") \
 	/*  9 */_(ER_CREATE_SPACE,		"Failed to create space '%s': %s") \
 	/* 10 */_(ER_SPACE_EXISTS,		"Space '%s' already exists") \
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index b05f2793f..2eda31ae9 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -155,6 +155,17 @@ error_format_msg(struct error *e, const char *format, ...)
 	va_end(ap);
 }
 
+void
+error_append_msg(struct error *e, const char *format, ...)
+{
+	va_list ap;
+	va_start(ap, format);
+	int prefix_len = strlen(e->errmsg);
+	char *msg = e->errmsg + prefix_len;
+	vsnprintf(msg, sizeof(e->errmsg) - prefix_len, format, ap);
+	va_end(ap);
+}
+
 void
 error_vformat_msg(struct error *e, const char *format, va_list ap)
 {
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index be81689ce..0522d9e5a 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -273,6 +273,9 @@ error_format_msg(struct error *e, const char *format, ...);
 void
 error_vformat_msg(struct error *e, const char *format, va_list ap);
 
+void
+error_append_msg(struct error *e, const char *format, ...);
+
 /**
  * Diagnostics Area - a container for errors
  */
diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
index 8effa69e8..5e67029c0 100644
--- a/test/replication-luatest/gh_5568_read_only_reason_test.lua
+++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
@@ -47,6 +47,8 @@ local g = t.group('gh-5568-read-only-reason1')
 g.before_all(make_create_cluster(g))
 g.after_all(make_destroy_cluster(g))
 
+local read_only_msg = "Can't modify data on a read-only instance - "
+
 --
 -- Read-only because of box.cfg{read_only = true}.
 --
@@ -57,6 +59,8 @@ g.test_read_only_reason_cfg = function(g)
         return ok, err:unpack()
     end)
     t.assert(not ok, 'fail ddl')
+    t.assert_str_contains(err.message, read_only_msg..
+                          'box.cfg.read_only is true')
     t.assert_covers(err, {
         reason = 'state',
         state = 'read_only',
@@ -88,6 +92,7 @@ g.test_read_only_reason_orphan = function(g)
         return old_timeout, ok, err:unpack()
     end, {fake_uri})
     t.assert(not ok, 'fail ddl')
+    t.assert_str_contains(err.message, read_only_msg..'it is an orphan')
     t.assert_covers(err, {
         reason = 'state',
         state = 'orphan',
@@ -117,6 +122,8 @@ g.test_read_only_reason_election_no_leader = function(g)
     end)
     t.assert(not ok, 'fail ddl')
     t.assert(err.term, 'has term')
+    t.assert_str_contains(err.message, read_only_msg..('state is election '..
+                          '%s with term %s'):format(err.state, err.term))
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -148,6 +155,9 @@ g.test_read_only_reason_election_has_leader = function(g)
     end)
     t.assert(not ok, 'fail ddl')
     t.assert(err.term, 'has term')
+    t.assert_str_contains(err.message, read_only_msg..('state is election '..
+                          '%s with term %s, leader is %s (%s)'):format(
+                          err.state, err.term, err.leader_id, err.leader_uuid))
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -187,6 +197,9 @@ g.test_read_only_reason_synchro = function(g)
     end)
     t.assert(not ok, 'fail ddl')
     t.assert(err.term, 'has term')
+    t.assert_str_contains(err.message, read_only_msg..('synchro queue with '..
+                          'term %s belongs to %s (%s)'):format(err.term,
+                          err.queue_owner_id, err.queue_owner_uuid))
     t.assert_covers(err, {
         reason = 'synchro',
         queue_owner_id = g.master:instance_id(),
@@ -242,6 +255,9 @@ g.test_read_only_reason_election_has_leader_no_uuid = function(g)
     t.assert(not ok, 'fail ddl')
     t.assert(err.term, 'has term')
     t.assert(not err.leader_uuid, 'has no leader uuid')
+    t.assert_str_contains(err.message, read_only_msg..('state is election %s '..
+                          'with term %s, leader is %s'):format(err.state,
+                          err.term, err.leader_id))
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -276,6 +292,9 @@ g.test_read_only_reason_synchro_no_uuid = function(g)
     t.assert(not ok, 'fail ddl')
     t.assert(err.term, 'has term')
     t.assert(not err.queue_owner_uuid)
+    t.assert_str_contains(err.message, read_only_msg..('synchro queue with '..
+                          'term %s belongs to %s'):format(err.term,
+                          err.queue_owner_id))
     t.assert_covers(err, {
         reason = 'synchro',
         queue_owner_id = leader_id,
diff --git a/test/replication/anon.result b/test/replication/anon.result
index dd470797c..3359be1d1 100644
--- a/test/replication/anon.result
+++ b/test/replication/anon.result
@@ -136,16 +136,16 @@ box.cfg{read_only=false}
 
 box.space.test:insert{2}
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | - error: Can't modify data on a read-only instance - box.cfg.read_only is true
  | ...
 
 box.space.loc:drop()
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | - error: Can't modify data on a read-only instance - box.cfg.read_only is true
  | ...
 box.space.loc:truncate()
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | - error: Can't modify data on a read-only instance - box.cfg.read_only is true
  | ...
 
 test_run:cmd('switch default')
diff --git a/test/replication/catch.result b/test/replication/catch.result
index e1b2995ec..3fbc07fbd 100644
--- a/test/replication/catch.result
+++ b/test/replication/catch.result
@@ -92,7 +92,7 @@ box.space.test ~= nil
 ...
 box.space.test:replace{1}
 ---
-- error: Can't modify data because this instance is in read-only mode.
+- error: Can't modify data on a read-only instance - it is an orphan
 ...
 -- Case #2: replace tuple on replica by net.box.
 test_run:cmd("switch default")
@@ -108,7 +108,7 @@ c = net_box.connect(r_uri)
 ...
 d = c.space.test:replace{1}
 ---
-- error: Can't modify data because this instance is in read-only mode.
+- error: Can't modify data on a read-only instance - it is an orphan
 ...
 -- Resume replication.
 errinj.set('ERRINJ_RELAY_SEND_DELAY', false)
diff --git a/test/replication/election_qsync.result b/test/replication/election_qsync.result
index c6ec5e352..798f9d493 100644
--- a/test/replication/election_qsync.result
+++ b/test/replication/election_qsync.result
@@ -115,9 +115,16 @@ test_run:cmd('stop server replica')
  | - true
  | ...
 -- Will fail - the node is not a leader.
-box.space.test:replace{2}
+ok, err = pcall(box.space.test.replace, box.space.test, {2})
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | ...
+assert(not ok)
+ | ---
+ | - true
+ | ...
+assert(err.code == box.error.READONLY)
+ | ---
+ | - true
  | ...
 
 -- Set synchro timeout to a huge value to ensure, that when a leader is elected,
diff --git a/test/replication/election_qsync.test.lua b/test/replication/election_qsync.test.lua
index f3c7c290b..3fb6a9be7 100644
--- a/test/replication/election_qsync.test.lua
+++ b/test/replication/election_qsync.test.lua
@@ -58,7 +58,9 @@ test_run:wait_lsn('default', 'replica')
 test_run:switch('default')
 test_run:cmd('stop server replica')
 -- Will fail - the node is not a leader.
-box.space.test:replace{2}
+ok, err = pcall(box.space.test.replace, box.space.test, {2})
+assert(not ok)
+assert(err.code == box.error.READONLY)
 
 -- Set synchro timeout to a huge value to ensure, that when a leader is elected,
 -- it won't wait for this timeout.
diff --git a/test/replication/gh-6034-qsync-limbo-ownership.result b/test/replication/gh-6034-qsync-limbo-ownership.result
index b4f53cd2a..58acf7db2 100644
--- a/test/replication/gh-6034-qsync-limbo-ownership.result
+++ b/test/replication/gh-6034-qsync-limbo-ownership.result
@@ -94,9 +94,16 @@ assert(box.info.synchro.queue.owner == test_run:get_server_id('default'))
  | ---
  | - true
  | ...
-box.space.async:insert{2} -- failure.
+ok, err = pcall(box.space.async.insert, box.space.async, {2})
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | ...
+assert(not ok)
+ | ---
+ | - true
+ | ...
+assert(err.code == box.error.READONLY)
+ | ---
+ | - true
  | ...
 
 -- Promotion on the other node. Default should become ro.
@@ -131,9 +138,16 @@ assert(box.info.synchro.queue.owner == test_run:get_server_id('replica'))
  | ---
  | - true
  | ...
-box.space.sync:insert{3} -- failure.
+ok, err = pcall(box.space.sync.insert, box.space.sync, {3})
  | ---
- | - error: Can't modify data because this instance is in read-only mode.
+ | ...
+assert(not ok)
+ | ---
+ | - true
+ | ...
+assert(err.code == box.error.READONLY)
+ | ---
+ | - true
  | ...
 
 box.ctl.promote()
diff --git a/test/replication/gh-6034-qsync-limbo-ownership.test.lua b/test/replication/gh-6034-qsync-limbo-ownership.test.lua
index f9ef5ca41..0f62ba6a4 100644
--- a/test/replication/gh-6034-qsync-limbo-ownership.test.lua
+++ b/test/replication/gh-6034-qsync-limbo-ownership.test.lua
@@ -34,7 +34,9 @@ box.cfg{replication=rpl_listen}
 test_run:switch('replica')
 assert(box.info.ro)
 assert(box.info.synchro.queue.owner == test_run:get_server_id('default'))
-box.space.async:insert{2} -- failure.
+ok, err = pcall(box.space.async.insert, box.space.async, {2})
+assert(not ok)
+assert(err.code == box.error.READONLY)
 
 -- Promotion on the other node. Default should become ro.
 box.ctl.promote()
@@ -46,7 +48,9 @@ test_run:switch('default')
 test_run:wait_lsn('default', 'replica')
 assert(box.info.ro)
 assert(box.info.synchro.queue.owner == test_run:get_server_id('replica'))
-box.space.sync:insert{3} -- failure.
+ok, err = pcall(box.space.sync.insert, box.space.sync, {3})
+assert(not ok)
+assert(err.code == box.error.READONLY)
 
 box.ctl.promote()
 box.ctl.demote()
diff --git a/test/vinyl/misc.result b/test/vinyl/misc.result
index e5a56ee62..3bb4e379c 100644
--- a/test/vinyl/misc.result
+++ b/test/vinyl/misc.result
@@ -359,7 +359,7 @@ ch2:put(true)
 ...
 ch1:get()
 ---
-- Can't modify data because this instance is in read-only mode.
+- Can't modify data on a read-only instance - box.cfg.read_only is true
 ...
 ch1:get()
 ---
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 02/11] uuid: move into libcore
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 11/11] error: report ER_READONLY reason in message Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 03/11] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

libuuid used to be a separate library since
de11d68a48e397b555731be04ba695b2ba1a0ccf ("CMake: refactor
dependencies of internal libraries").

Unclear what was it done for. The commit says "fir unit tests".
But they perfectly fine can depend on libcore like many of them
do already.

Because of libuuid being a separate library, libcore can't use
tt_uuid, because that would be a cyclic dependency. And that won't
allow to introduce tt_uuid-dependent API in a next patch.

Hence libuuid is merged into libcore.

Needed for #5568
---
 src/CMakeLists.txt                      | 3 +--
 src/box/applier.h                       | 2 +-
 src/box/bind.h                          | 2 +-
 src/box/field_def.c                     | 4 ++--
 src/box/lua/serialize_lua.c             | 2 +-
 src/box/msgpack.c                       | 2 +-
 src/box/replication.h                   | 2 +-
 src/box/sql/mem.c                       | 2 +-
 src/box/sql/mem.h                       | 2 +-
 src/box/tuple.h                         | 2 +-
 src/box/tuple_compare.cc                | 8 ++++----
 src/box/xlog.h                          | 2 +-
 src/box/xrow.h                          | 2 +-
 src/lib/CMakeLists.txt                  | 1 -
 src/lib/core/CMakeLists.txt             | 2 ++
 src/lib/core/crash.c                    | 2 +-
 src/lib/{uuid => core}/mp_uuid.c        | 0
 src/lib/{uuid => core}/mp_uuid.h        | 0
 src/lib/{uuid => core}/tt_uuid.c        | 0
 src/lib/{uuid => core}/tt_uuid.h        | 0
 src/lib/mpstream/CMakeLists.txt         | 2 +-
 src/lib/mpstream/mpstream.c             | 2 +-
 src/lib/swim/CMakeLists.txt             | 2 +-
 src/lib/swim/swim_io.h                  | 2 +-
 src/lib/swim/swim_proto.h               | 2 +-
 src/lib/uuid/CMakeLists.txt             | 2 --
 src/lua/msgpack.c                       | 6 +++---
 src/lua/tnt_msgpuck.h                   | 2 +-
 src/lua/utils.c                         | 2 +-
 test/sql-tap/CMakeLists.txt             | 2 +-
 test/sql-tap/gh-6024-funcs-return-bin.c | 2 +-
 test/sql-tap/sql_uuid.c                 | 2 +-
 test/unit/CMakeLists.txt                | 2 +-
 test/unit/swim_proto.c                  | 2 +-
 test/unit/swim_test_utils.c             | 2 +-
 test/unit/swim_test_utils.h             | 2 +-
 test/unit/uuid.c                        | 4 ++--
 test/unit/vy_iterators_helper.c         | 2 +-
 test/unit/xrow.cc                       | 2 +-
 third_party/lua-cjson/lua_cjson.c       | 2 +-
 third_party/lua-yaml/lyaml.cc           | 2 +-
 41 files changed, 43 insertions(+), 45 deletions(-)
 rename src/lib/{uuid => core}/mp_uuid.c (100%)
 rename src/lib/{uuid => core}/mp_uuid.h (100%)
 rename src/lib/{uuid => core}/tt_uuid.c (100%)
 rename src/lib/{uuid => core}/tt_uuid.h (100%)
 delete mode 100644 src/lib/uuid/CMakeLists.txt

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 168ac2a52..2e20c06ee 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -190,7 +190,7 @@ endif()
 set_source_files_compile_flags(${server_sources})
 add_library(server STATIC ${server_sources})
 add_dependencies(server build_bundled_libs)
-target_link_libraries(server core coll http_parser bit uri uuid swim swim_udp
+target_link_libraries(server core coll http_parser bit uri swim swim_udp
                       swim_ev crypto mpstream crc32 tzcode)
 
 # Rule of thumb: if exporting a symbol from a static library, list the
@@ -220,7 +220,6 @@ if (TARGET_OS_FREEBSD AND NOT TARGET_OS_DEBIAN_FREEBSD)
     endif()
 endif()
 
-set (common_libraries ${common_libraries} ${LIBUUID_LIBRARIES})
 set (common_libraries ${common_libraries} PARENT_SCOPE)
 
 add_subdirectory(lib)
diff --git a/src/box/applier.h b/src/box/applier.h
index 15ca1fcfd..9a7497ba5 100644
--- a/src/box/applier.h
+++ b/src/box/applier.h
@@ -40,7 +40,7 @@
 #include "fiber_cond.h"
 #include "trigger.h"
 #include "trivia/util.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "uri/uri.h"
 
 #include "xrow.h"
diff --git a/src/box/bind.h b/src/box/bind.h
index 1ab8ea72d..45d390314 100644
--- a/src/box/bind.h
+++ b/src/box/bind.h
@@ -41,8 +41,8 @@ extern "C" {
 
 #include "msgpuck.h"
 #include "decimal.h"
-#include "uuid/tt_uuid.h"
 #include "mp_extension_types.h"
+#include "tt_uuid.h"
 
 struct sql_stmt;
 
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 51acb8025..780729854 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -33,8 +33,8 @@
 #include "trivia/util.h"
 #include "key_def.h"
 #include "mp_extension_types.h"
-#include "uuid/mp_uuid.h"
-#include "uuid/tt_uuid.h"
+#include "mp_uuid.h"
+#include "tt_uuid.h"
 
 const char *mp_type_strs[] = {
 	/* .MP_NIL    = */ "nil",
diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c
index 1f791980f..b3a7ea09a 100644
--- a/src/box/lua/serialize_lua.c
+++ b/src/box/lua/serialize_lua.c
@@ -39,7 +39,7 @@
 
 #include "lib/core/decimal.h"
 #include "mp_extension_types.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 
 #include "lua-yaml/b64.h"
 
diff --git a/src/box/msgpack.c b/src/box/msgpack.c
index 1723dea4c..06775c1fe 100644
--- a/src/box/msgpack.c
+++ b/src/box/msgpack.c
@@ -33,8 +33,8 @@
 
 #include "mp_extension_types.h"
 #include "mp_decimal.h"
-#include "uuid/mp_uuid.h"
 #include "mp_error.h"
+#include "mp_uuid.h"
 
 static int
 msgpack_fprint_ext(FILE *file, const char **data, int depth)
diff --git a/src/box/replication.h b/src/box/replication.h
index a8fed45e8..95563e811 100644
--- a/src/box/replication.h
+++ b/src/box/replication.h
@@ -30,7 +30,6 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
-#include "uuid/tt_uuid.h"
 #include "trigger.h"
 #include <stdint.h>
 #define RB_COMPACT 1
@@ -38,6 +37,7 @@
 #include <small/rlist.h>
 #include "applier.h"
 #include "fiber_cond.h"
+#include "tt_uuid.h"
 #include "vclock/vclock.h"
 #include "latch.h"
 
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index dc629aee3..2d3a82122 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -43,8 +43,8 @@
 #include "lua/serializer.h"
 #include "lua/msgpack.h"
 #include "lua/decimal.h"
-#include "uuid/mp_uuid.h"
 #include "mp_decimal.h"
+#include "mp_uuid.h"
 
 #define CMP_OLD_NEW(a, b, type) (((a) > (type)(b)) - ((a) < (type)(b)))
 
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 242f910db..07d975163 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -30,8 +30,8 @@
  * SUCH DAMAGE.
  */
 #include "box/field_def.h"
-#include "uuid/tt_uuid.h"
 #include "decimal.h"
+#include "tt_uuid.h"
 
 struct sql;
 struct Vdbe;
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 5d4a19a19..1278866c8 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -34,8 +34,8 @@
 #include "say.h"
 #include "diag.h"
 #include "error.h"
-#include "uuid/tt_uuid.h" /* tuple_field_uuid */
 #include "tt_static.h"
+#include "tt_uuid.h"
 #include "tuple_format.h"
 
 #if defined(__cplusplus)
diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
index 43cd29ce9..8acdd42bd 100644
--- a/src/box/tuple_compare.cc
+++ b/src/box/tuple_compare.cc
@@ -28,15 +28,15 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#include "decimal.h"
 #include "tuple_compare.h"
 #include "tuple.h"
 #include "coll/coll.h"
 #include "trivia/util.h" /* NOINLINE */
 #include <math.h>
-#include "lib/core/decimal.h"
-#include "lib/core/mp_decimal.h"
-#include "uuid/mp_uuid.h"
-#include "lib/core/mp_extension_types.h"
+#include "mp_decimal.h"
+#include "mp_extension_types.h"
+#include "mp_uuid.h"
 
 /* {{{ tuple_compare */
 
diff --git a/src/box/xlog.h b/src/box/xlog.h
index a8bc4a330..b525fd68e 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -33,7 +33,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <sys/stat.h>
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "vclock/vclock.h"
 
 #define ZSTD_STATIC_LINKING_ONLY
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 762b6e36b..6431f708f 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -35,9 +35,9 @@
 #include <stddef.h>
 #include <sys/uio.h> /* struct iovec */
 
-#include "uuid/tt_uuid.h"
 #include "diag.h"
 #include "iproto_features.h"
+#include "tt_uuid.h"
 #include "vclock/vclock.h"
 
 #if defined(__cplusplus)
diff --git a/src/lib/CMakeLists.txt b/src/lib/CMakeLists.txt
index 81ccc585d..b973dc501 100644
--- a/src/lib/CMakeLists.txt
+++ b/src/lib/CMakeLists.txt
@@ -8,7 +8,6 @@ add_subdirectory(json)
 add_subdirectory(uri)
 add_subdirectory(http_parser)
 add_subdirectory(core)
-add_subdirectory(uuid)
 add_subdirectory(coll)
 add_subdirectory(crypto)
 add_subdirectory(swim)
diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index 0cc742a1c..fb640461d 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -31,6 +31,8 @@ set(core_sources
     mp_decimal.c
     cord_buf.c
     datetime.c
+    tt_uuid.c
+    mp_uuid.c
 )
 
 if (TARGET_OS_NETBSD)
diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c
index abb7837e6..826c140b5 100644
--- a/src/lib/core/crash.c
+++ b/src/lib/core/crash.c
@@ -14,12 +14,12 @@
 
 #include "small/static.h"
 #include "trivia/util.h"
-#include "uuid/tt_uuid.h"
 
 #include "box/replication.h"
 #include "backtrace.h"
 #include "crash.h"
 #include "say.h"
+#include "tt_uuid.h"
 
 #define pr_fmt(fmt)		"crash: " fmt
 #define pr_debug(fmt, ...)	say_debug(pr_fmt(fmt), ##__VA_ARGS__)
diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/core/mp_uuid.c
similarity index 100%
rename from src/lib/uuid/mp_uuid.c
rename to src/lib/core/mp_uuid.c
diff --git a/src/lib/uuid/mp_uuid.h b/src/lib/core/mp_uuid.h
similarity index 100%
rename from src/lib/uuid/mp_uuid.h
rename to src/lib/core/mp_uuid.h
diff --git a/src/lib/uuid/tt_uuid.c b/src/lib/core/tt_uuid.c
similarity index 100%
rename from src/lib/uuid/tt_uuid.c
rename to src/lib/core/tt_uuid.c
diff --git a/src/lib/uuid/tt_uuid.h b/src/lib/core/tt_uuid.h
similarity index 100%
rename from src/lib/uuid/tt_uuid.h
rename to src/lib/core/tt_uuid.h
diff --git a/src/lib/mpstream/CMakeLists.txt b/src/lib/mpstream/CMakeLists.txt
index 60ed20030..bd8dc9981 100644
--- a/src/lib/mpstream/CMakeLists.txt
+++ b/src/lib/mpstream/CMakeLists.txt
@@ -1,2 +1,2 @@
 add_library(mpstream STATIC mpstream.c)
-target_link_libraries(mpstream core uuid ${MSGPUCK_LIBRARIES})
+target_link_libraries(mpstream core ${MSGPUCK_LIBRARIES})
diff --git a/src/lib/mpstream/mpstream.c b/src/lib/mpstream/mpstream.c
index 70ca29889..758bf5e55 100644
--- a/src/lib/mpstream/mpstream.c
+++ b/src/lib/mpstream/mpstream.c
@@ -34,7 +34,7 @@
 #include <stdint.h>
 #include "msgpuck.h"
 #include "mp_decimal.h"
-#include "uuid/mp_uuid.h"
+#include "mp_uuid.h"
 
 void
 mpstream_reserve_slow(struct mpstream *stream, size_t size)
diff --git a/src/lib/swim/CMakeLists.txt b/src/lib/swim/CMakeLists.txt
index 11202dce3..873cf6e5b 100644
--- a/src/lib/swim/CMakeLists.txt
+++ b/src/lib/swim/CMakeLists.txt
@@ -5,7 +5,7 @@ set(lib_swim_ev_sources swim_ev.c)
 set_source_files_compile_flags(${lib_swim_sources} ${lib_swim_udp_sources}
                                ${lib_swim_ev_sources})
 add_library(swim STATIC ${lib_swim_sources})
-target_link_libraries(swim core misc uuid crypto)
+target_link_libraries(swim core misc crypto)
 add_library(swim_udp STATIC ${lib_swim_udp_sources})
 target_link_libraries(swim_udp core)
 add_library(swim_ev STATIC ${lib_swim_ev_sources})
diff --git a/src/lib/swim/swim_io.h b/src/lib/swim/swim_io.h
index bf5a1389f..83c415541 100644
--- a/src/lib/swim/swim_io.h
+++ b/src/lib/swim/swim_io.h
@@ -36,7 +36,7 @@
 #include "crypto/crypto.h"
 #include "swim_transport.h"
 #include "tarantool_ev.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include <stdbool.h>
 #include <arpa/inet.h>
 #include <netinet/in.h>
diff --git a/src/lib/swim/swim_proto.h b/src/lib/swim/swim_proto.h
index 595606499..070f4b47b 100644
--- a/src/lib/swim/swim_proto.h
+++ b/src/lib/swim/swim_proto.h
@@ -31,7 +31,7 @@
  * SUCH DAMAGE.
  */
 #include "tt_static.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include <arpa/inet.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
diff --git a/src/lib/uuid/CMakeLists.txt b/src/lib/uuid/CMakeLists.txt
deleted file mode 100644
index a3316647b..000000000
--- a/src/lib/uuid/CMakeLists.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-add_library(uuid STATIC tt_uuid.c mp_uuid.c)
-target_link_libraries(uuid core bit)
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index 73a9eb074..1953fc407 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -42,10 +42,10 @@
 #include <small/region.h>
 #include <small/ibuf.h>
 
+#include "decimal.h" /* decimal_unpack() */
 #include "lua/decimal.h" /* lua_pushdecimal() */
-#include "lib/core/decimal.h" /* decimal_unpack() */
-#include "lib/uuid/mp_uuid.h" /* mp_decode_uuid() */
-#include "lib/core/mp_extension_types.h"
+#include "mp_extension_types.h"
+#include "mp_uuid.h" /* mp_decode_uuid() */
 
 #include "cord_buf.h"
 #include <fiber.h>
diff --git a/src/lua/tnt_msgpuck.h b/src/lua/tnt_msgpuck.h
index 2eb5dad8a..ac898a9c0 100644
--- a/src/lua/tnt_msgpuck.h
+++ b/src/lua/tnt_msgpuck.h
@@ -43,7 +43,7 @@ extern "C" {
 
 #include "mp_decimal.h"
 #include "box/mp_error.h"
-#include "uuid/mp_uuid.h"
+#include "mp_uuid.h"
 
 char *
 tnt_mp_encode_decimal(char *data, const decimal_t *dec);
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 979716758..ae8fe468c 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -37,7 +37,7 @@
 #include <trivia/util.h>
 #include <diag.h>
 #include <fiber.h>
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 
 int luaL_nil_ref = LUA_REFNIL;
 
diff --git a/test/sql-tap/CMakeLists.txt b/test/sql-tap/CMakeLists.txt
index 74543becd..c4ec1214a 100644
--- a/test/sql-tap/CMakeLists.txt
+++ b/test/sql-tap/CMakeLists.txt
@@ -4,6 +4,6 @@ target_link_libraries(gh-5938-wrong-string-length msgpuck)
 build_module(gh-6024-funcs-return-bin gh-6024-funcs-return-bin.c)
 target_link_libraries(gh-6024-funcs-return-bin msgpuck)
 build_module(sql_uuid sql_uuid.c)
-target_link_libraries(sql_uuid msgpuck uuid)
+target_link_libraries(sql_uuid msgpuck core)
 build_module(decimal decimal.c)
 target_link_libraries(decimal msgpuck)
diff --git a/test/sql-tap/gh-6024-funcs-return-bin.c b/test/sql-tap/gh-6024-funcs-return-bin.c
index 3bcd67136..f05924337 100644
--- a/test/sql-tap/gh-6024-funcs-return-bin.c
+++ b/test/sql-tap/gh-6024-funcs-return-bin.c
@@ -1,8 +1,8 @@
 #include "msgpuck.h"
 #include "module.h"
-#include "uuid/mp_uuid.h"
 #include "mp_decimal.h"
 #include "lua/tnt_msgpuck.h"
+#include "mp_uuid.h"
 
 enum {
 	BUF_SIZE = 512,
diff --git a/test/sql-tap/sql_uuid.c b/test/sql-tap/sql_uuid.c
index 9debfb9d2..d681a4d6c 100644
--- a/test/sql-tap/sql_uuid.c
+++ b/test/sql-tap/sql_uuid.c
@@ -1,8 +1,8 @@
 #include "msgpuck.h"
 #include "module.h"
-#include "uuid/mp_uuid.h"
 #include "mp_extension_types.h"
 #include "lua/tnt_msgpuck.h"
+#include "mp_uuid.h"
 
 enum {
 	BUF_SIZE = 512,
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index ae8b5b9ac..10e1f4395 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -53,7 +53,7 @@ target_link_libraries(bitset_index.test bitset)
 add_executable(base64.test base64.c)
 target_link_libraries(base64.test misc unit)
 add_executable(uuid.test uuid.c core_test_utils.c)
-target_link_libraries(uuid.test uuid unit)
+target_link_libraries(uuid.test core unit)
 add_executable(random.test random.c core_test_utils.c)
 target_link_libraries(random.test core unit)
 add_executable(xmalloc.test xmalloc.c core_test_utils.c)
diff --git a/test/unit/swim_proto.c b/test/unit/swim_proto.c
index d7fafc7f0..6aab07d5a 100644
--- a/test/unit/swim_proto.c
+++ b/test/unit/swim_proto.c
@@ -31,7 +31,7 @@
 
 #include "memory.h"
 #include "fiber.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "version.h"
 #include "msgpuck.h"
 #include "unit.h"
diff --git a/test/unit/swim_test_utils.c b/test/unit/swim_test_utils.c
index 188ca5e5f..6badabcbd 100644
--- a/test/unit/swim_test_utils.c
+++ b/test/unit/swim_test_utils.c
@@ -30,7 +30,7 @@
  */
 #include "swim_test_utils.h"
 #include "swim/swim_ev.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "trivia/util.h"
 #include "msgpuck.h"
 #include "trigger.h"
diff --git a/test/unit/swim_test_utils.h b/test/unit/swim_test_utils.h
index 61a6787d9..4e5c1aec1 100644
--- a/test/unit/swim_test_utils.h
+++ b/test/unit/swim_test_utils.h
@@ -31,7 +31,7 @@
  * SUCH DAMAGE.
  */
 #include <stdbool.h>
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "unit.h"
 #include "fiber.h"
 #include "uri/uri.h"
diff --git a/test/unit/uuid.c b/test/unit/uuid.c
index fdf263092..e58baa728 100644
--- a/test/unit/uuid.c
+++ b/test/unit/uuid.c
@@ -1,9 +1,9 @@
 #include "unit.h"
-#include "uuid/tt_uuid.h"
-#include "uuid/mp_uuid.h"
 #include "core/random.h"
+#include "mp_uuid.h"
 #include "msgpuck/msgpuck.h"
 #include "mp_extension_types.h"
+#include "tt_uuid.h"
 #include <string.h>
 
 static void
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 0d20f19ef..63982bdb1 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -1,8 +1,8 @@
 #include "vy_iterators_helper.h"
 #include "memory.h"
 #include "fiber.h"
-#include "uuid/tt_uuid.h"
 #include "say.h"
+#include "tt_uuid.h"
 
 struct tt_uuid INSTANCE_UUID;
 
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index 2c0dd88b6..56a0ec3ef 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -35,7 +35,7 @@ extern "C" {
 #include "box/error.h"
 #include "box/xrow.h"
 #include "box/iproto_constants.h"
-#include "uuid/tt_uuid.h"
+#include "tt_uuid.h"
 #include "version.h"
 #include "random.h"
 #include "memory.h"
diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index 6c4ec3736..67b97a0ce 100644
--- a/third_party/lua-cjson/lua_cjson.c
+++ b/third_party/lua-cjson/lua_cjson.c
@@ -52,8 +52,8 @@
 #include "mp_extension_types.h" /* MP_DECIMAL, MP_UUID */
 #include "diag.h"
 #include "tt_static.h"
-#include "uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */
 #include "cord_buf.h"
+#include "tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */
 
 typedef enum {
     T_OBJ_BEGIN,
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index a0e4de41f..59995f37c 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -54,7 +54,7 @@ extern "C" {
 #include "diag.h"
 #include "tt_static.h"
 #include "mp_extension_types.h" /* MP_DECIMAL, MP_UUID */
-#include "uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */
+#include "tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */
 
 #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:"
 
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 03/11] error: introduce error_payload
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 02/11] uuid: move into libcore Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 04/11] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

It is a dictionary-like struct which stores keys with binary data
values. The values are supposed to be arbitrary MessagePack blobs:
number, string, bool, UUID, array, map, anything.

The payload is an array inside instead of a hash table because
number of keys will be <= 3 in all cases. And even when it will be
public, it is very unlikely it will be bigger.

Object of error_payload in a future patch will be stored in struct
error and will allow to extend it dynamically with more members.

This in turn is needed to extend ER_READONLY error with more
details about why it happened.

Part of #5568
---
 src/lib/core/CMakeLists.txt  |   1 +
 src/lib/core/error_payload.c | 301 ++++++++++++++++++++++
 src/lib/core/error_payload.h | 190 ++++++++++++++
 test/unit/CMakeLists.txt     |   2 +
 test/unit/error.c            | 466 +++++++++++++++++++++++++++++++++++
 test/unit/error.result       | 161 ++++++++++++
 6 files changed, 1121 insertions(+)
 create mode 100644 src/lib/core/error_payload.c
 create mode 100644 src/lib/core/error_payload.h
 create mode 100644 test/unit/error.c
 create mode 100644 test/unit/error.result

diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index fb640461d..17197a371 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -21,6 +21,7 @@ set(core_sources
     fio.c
     exception.cc
     errinj.c
+    error_payload.c
     reflection.c
     assoc.c
     util.c
diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
new file mode 100644
index 000000000..e5c2efd1a
--- /dev/null
+++ b/src/lib/core/error_payload.c
@@ -0,0 +1,301 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#include "error_payload.h"
+#include "mp_uuid.h"
+#include "msgpuck.h"
+
+void
+error_payload_create(struct error_payload *p)
+{
+	p->count = 0;
+	p->fields = NULL;
+}
+
+void
+error_payload_destroy(struct error_payload *p)
+{
+	for (int i = 0; i < p->count; ++i) {
+		TRASH(p->fields[i]);
+		free(p->fields[i]);
+	}
+	free(p->fields);
+	TRASH(p);
+}
+
+const char *
+error_payload_get_str(const struct error_payload *e, const char *name)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		return NULL;
+	const char *data = f->data;
+	if (mp_typeof(*data) != MP_STR)
+		return NULL;
+	uint32_t len;
+	data = mp_decode_str(&data, &len);
+	assert(data[len] == 0);
+	return data;
+}
+
+void
+error_payload_set_str(struct error_payload *e, const char *name,
+		      const char *value)
+{
+	uint32_t len = strlen(value);
+	char *data = error_payload_prepare(
+		e, name, mp_sizeof_str(len), 1)->data;
+	/*
+	 * 1 extra byte in the end is reserved to append 0 after the encoded
+	 * string. To be able to return it from get() without copying.
+	 */
+	*mp_encode_str(data, value, len) = 0;
+}
+
+bool
+error_payload_get_uint(const struct error_payload *e, const char *name,
+		       uint64_t *value)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		goto not_found;
+	const char *data = f->data;
+	if (mp_typeof(*data) != MP_UINT)
+		goto not_found;
+	*value = mp_decode_uint(&data);
+	return true;
+
+not_found:
+	*value = 0;
+	return false;
+}
+
+void
+error_payload_set_uint(struct error_payload *e, const char *name,
+		       uint64_t value)
+{
+	char *data = error_payload_prepare(
+		e, name, mp_sizeof_uint(value), 0)->data;
+	mp_encode_uint(data, value);
+}
+
+bool
+error_payload_get_int(const struct error_payload *e, const char *name,
+		      int64_t *value)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		goto not_found;
+	const char *data = f->data;
+	if (mp_typeof(*data) == MP_UINT) {
+		uint64_t u = mp_decode_uint(&data);
+		if (u > INT64_MAX)
+			goto not_found;
+		*value = u;
+		return true;
+	} else if (mp_typeof(*data) == MP_INT) {
+		*value = mp_decode_int(&data);
+		return true;
+	}
+not_found:
+	*value = 0;
+	return false;
+}
+
+void
+error_payload_set_int(struct error_payload *e, const char *name, int64_t value)
+{
+	if (value >= 0)
+		return error_payload_set_uint(e, name, value);
+	char *data = error_payload_prepare(
+		e, name, mp_sizeof_int(value), 0)->data;
+	mp_encode_int(data, value);
+}
+
+bool
+error_payload_get_double(const struct error_payload *e, const char *name,
+			 double *value)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		goto not_found;
+	const char *data = f->data;
+	if (mp_typeof(*data) == MP_DOUBLE) {
+		*value = mp_decode_double(&data);
+		return true;
+	} else if (mp_typeof(*data) == MP_FLOAT) {
+		*value = mp_decode_float(&data);
+		return true;
+	}
+not_found:
+	*value = 0;
+	return false;
+}
+
+void
+error_payload_set_double(struct error_payload *e, const char *name,
+			 double value)
+{
+	char *data = error_payload_prepare(
+		e, name, mp_sizeof_double(value), 0)->data;
+	mp_encode_double(data, value);
+}
+
+bool
+error_payload_get_bool(const struct error_payload *e, const char *name,
+		       bool *value)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		goto not_found;
+	const char *data = f->data;
+	if (mp_typeof(*data) != MP_BOOL)
+		goto not_found;
+	*value = mp_decode_bool(&data);
+	return true;
+
+not_found:
+	*value = false;
+	return false;
+}
+
+void
+error_payload_set_bool(struct error_payload *e, const char *name, bool value)
+{
+	char *data = error_payload_prepare(
+		e, name, mp_sizeof_bool(value), 0)->data;
+	mp_encode_bool(data, value);
+}
+
+bool
+error_payload_get_uuid(const struct error_payload *e, const char *name,
+		       struct tt_uuid *uuid)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL)
+		goto not_found;
+	const char *data = f->data;
+	if (mp_decode_uuid(&data, uuid) == NULL)
+		goto not_found;
+	return true;
+
+not_found:
+	*uuid = uuid_nil;
+	return false;
+}
+
+void
+error_payload_set_uuid(struct error_payload *e, const char *name,
+		       const struct tt_uuid *uuid)
+{
+	char *data = error_payload_prepare(e, name, mp_sizeof_uuid(), 0)->data;
+	mp_encode_uuid(data, uuid);
+}
+
+const char *
+error_payload_get_mp(const struct error_payload *e, const char *name,
+		     uint32_t *size)
+{
+	const struct error_field *f = error_payload_find(e, name);
+	if (f == NULL) {
+		*size = 0;
+		return NULL;
+	}
+	*size = f->size;
+	return f->data;
+}
+
+void
+error_payload_set_mp(struct error_payload *e, const char *name,
+		     const char *src, uint32_t size)
+{
+	char *data;
+	if (mp_typeof(*src) == MP_STR) {
+		data = error_payload_prepare(e, name, size, 1)->data;
+		/* @sa error_payload_set_str(). */
+		data[size] = 0;
+	} else {
+		data = error_payload_prepare(e, name, size, 0)->data;
+	}
+	memcpy(data, src, size);
+}
+
+void
+error_payload_clear(struct error_payload *e, const char *name)
+{
+	struct error_field **fields = e->fields;
+	for (int i = 0; i < e->count; ++i) {
+		struct error_field *f = fields[i];
+		if (strcmp(name, f->name) != 0)
+			continue;
+		TRASH(f);
+		free(f);
+		int count = --e->count;
+		if (count == 0) {
+			free(fields);
+			e->fields = NULL;
+			return;
+		}
+		/* Cyclic deletion. Order does not matter in a dictionary. */
+		fields[i] = fields[count];
+		return;
+	}
+}
+
+void
+error_payload_move(struct error_payload *dst, struct error_payload *src)
+{
+	for (int i = 0; i < dst->count; ++i) {
+		TRASH(dst->fields[i]);
+		free(dst->fields[i]);
+	}
+	free(dst->fields);
+	dst->fields = src->fields;
+	dst->count = src->count;
+	src->fields = NULL;
+	src->count = 0;
+}
+
+const struct error_field *
+error_payload_find(const struct error_payload *e, const char *name)
+{
+	for (int i = 0; i < e->count; ++i) {
+		const struct error_field *f = e->fields[i];
+		if (strcmp(name, f->name) == 0)
+			return f;
+	}
+	return NULL;
+}
+
+struct error_field *
+error_payload_prepare(struct error_payload *e, const char *name,
+		      uint32_t value_size, uint32_t extra)
+{
+	struct error_field *f;
+	uint32_t name_size = strlen(name) + 1;
+	uint32_t data_offset = offsetof(struct error_field, name[name_size]);
+	uint32_t total = data_offset + value_size + extra;
+
+	struct error_field **fields = e->fields;
+	int count = e->count;
+	int i;
+	for (i = 0; i < count; ++i) {
+		f = fields[i];
+		if (strcmp(name, f->name) != 0)
+			continue;
+		f = xrealloc(f, total);
+		goto set;
+	}
+	e->count = ++count;
+	fields = xrealloc(fields, sizeof(fields[0]) * count);
+	e->fields = fields;
+	f = xmalloc(total);
+	memcpy(f->name, name, name_size);
+set:
+	f->data = (char *)f + data_offset;
+	f->size = value_size;
+	fields[i] = f;
+	return f;
+}
diff --git a/src/lib/core/error_payload.h b/src/lib/core/error_payload.h
new file mode 100644
index 000000000..23c75064b
--- /dev/null
+++ b/src/lib/core/error_payload.h
@@ -0,0 +1,190 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#pragma once
+
+#include "trivia/util.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct tt_uuid;
+
+/** Single field of error payload. */
+struct error_field {
+	/** MessagePack field value. */
+	char *data;
+	/** Data size. */
+	uint32_t size;
+	/** Zero terminated field name. */
+	char name[1];
+};
+
+/**
+ * Key-value pairs to store dynamic fields of an error object. The ones which
+ * are defined not for all error types and user-defined ones.
+ */
+struct error_payload {
+	/** Number of fields. */
+	int count;
+	/**
+	 * Array of field pointers. Not very optimized, but
+	 * - The errors are supposed to be rare;
+	 * - Number of fields is around 3 tops - linear search can be even
+	 *   faster than a generic hash table then;
+	 * - Not storing them by values simplifies addition of new fields and
+	 *   their removal.
+	 */
+	struct error_field **fields;
+};
+
+/** Create error payload. */
+void
+error_payload_create(struct error_payload *p);
+
+/** Destroy error payload. */
+void
+error_payload_destroy(struct error_payload *p);
+
+/**
+ * Get value of a payload field as a string. If it is not string or is not
+ * found - return NULL.
+ */
+const char *
+error_payload_get_str(const struct error_payload *e, const char *name);
+
+/**
+ * Set value of a payload field to a string. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_str(struct error_payload *e, const char *name,
+		      const char *value);
+
+/**
+ * Get value of a payload field as a uint. If it is not uint or is not found -
+ * return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_uint(const struct error_payload *e, const char *name,
+		       uint64_t *value);
+
+/**
+ * Set value of a payload field to a uint. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_uint(struct error_payload *e, const char *name,
+		       uint64_t value);
+
+/**
+ * Get value of a payload field as an int. If it is not int, or is not found, or
+ * does not fit int64_t - return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_int(const struct error_payload *e, const char *name,
+		      int64_t *value);
+
+/**
+ * Set value of a payload field to an int. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_int(struct error_payload *e, const char *name, int64_t value);
+
+/**
+ * Get value of a payload field as a double. If it is not double or is not
+ * found - return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_double(const struct error_payload *e, const char *name,
+			 double *value);
+
+/**
+ * Set value of a payload field to a double. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_double(struct error_payload *e, const char *name,
+			 double value);
+
+/**
+ * Get value of a payload field as a bool. If it is not bool or is not found -
+ * return false and the out parameter is set to false.
+ */
+bool
+error_payload_get_bool(const struct error_payload *e, const char *name,
+		       bool *value);
+
+/**
+ * Set value of a payload field to a bool. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_bool(struct error_payload *e, const char *name, bool value);
+
+/**
+ * Get value of a payload field as a UUID. If it is not UUID or is not found -
+ * return false and the out parameter is set to UUID with zeros.
+ */
+bool
+error_payload_get_uuid(const struct error_payload *e, const char *name,
+		       struct tt_uuid *uuid);
+
+/**
+ * Set value of a payload field to a UUID. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_uuid(struct error_payload *e, const char *name,
+		       const struct tt_uuid *uuid);
+
+/**
+ * Get MessagePack value of a payload field. If it is not found - return NULL
+ * and the out parameter is set to 0.
+ */
+const char *
+error_payload_get_mp(const struct error_payload *e, const char *name,
+		     uint32_t *size);
+
+/**
+ * Set value of a payload field to a MessagePack buffer. If the field existed
+ * before, it is overwritten.
+ */
+void
+error_payload_set_mp(struct error_payload *e, const char *name,
+		     const char *src, uint32_t size);
+
+/** Remove the given field from the payload. */
+void
+error_payload_clear(struct error_payload *e, const char *name);
+
+/**
+ * Move all fields of one payload into another. Old fields of the destination
+ * are all deleted. The source stays valid but empty.
+ */
+void
+error_payload_move(struct error_payload *dst, struct error_payload *src);
+
+/** Find a payload field by name. */
+const struct error_field *
+error_payload_find(const struct error_payload *e, const char *name);
+
+/**
+ * Prepare a payload field to get a new value. If the field didn't exist, it is
+ * added. If it existed then it is reallocated if was necessary and should be
+ * overwritten. Name is already filled in the field. Only need to fill the
+ * value.
+ * Extra parameter allows to allocate extra memory for arbitrary usage after the
+ * error field and its value.
+ */
+struct error_field *
+error_payload_prepare(struct error_payload *e, const char *name,
+		      uint32_t value_size, uint32_t extra);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 10e1f4395..19674d3fe 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -60,6 +60,8 @@ add_executable(xmalloc.test xmalloc.c core_test_utils.c)
 target_link_libraries(xmalloc.test unit)
 add_executable(datetime.test datetime.c)
 target_link_libraries(datetime.test tzcode core cdt unit)
+add_executable(error.test error.c core_test_utils.c)
+target_link_libraries(error.test unit core)
 
 add_executable(bps_tree.test bps_tree.cc)
 target_link_libraries(bps_tree.test small misc)
diff --git a/test/unit/error.c b/test/unit/error.c
new file mode 100644
index 000000000..55f8f6798
--- /dev/null
+++ b/test/unit/error.c
@@ -0,0 +1,466 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#include "error_payload.h"
+#include "mp_uuid.h"
+#include "msgpuck.h"
+#include "random.h"
+#include "unit.h"
+
+#include <float.h>
+
+static void
+test_payload_field_str(void)
+{
+	header();
+	plan(15);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	is(p.count, 0, "no fields in the beginning");
+	is(error_payload_get_str(&p, "key"), NULL, "get_str() empty");
+
+	error_payload_set_str(&p, "key1", "value1");
+	is(p.count, 1, "++count");
+	is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
+	   "get_str() finds");
+
+	error_payload_set_str(&p, "key2", "value2");
+	is(p.count, 2, "++count");
+	is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
+	   "get_str() finds old");
+	is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
+	   "get_str() finds new");
+	is(error_payload_find(&p, "key1")->size,
+	   mp_sizeof_str(strlen("value1")),
+	   "size does not include terminating zero");
+
+	error_payload_set_str(&p, "key1", "new_value1");
+	is(p.count, 2, "field count is same");
+	is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
+	   "get_str() finds new value");
+	is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
+	   "get_str() finds other key old value");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
+	   "get_str() finds new value");
+	is(error_payload_get_str(&p, "key2"), NULL,
+	   "get_str() can't find deleted value");
+
+	error_payload_set_uint(&p, "key2", 1);
+	is(error_payload_get_str(&p, "key2"), NULL, "wrong type");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_uint(void)
+{
+	header();
+	plan(17);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	uint64_t val = 1;
+	ok(!error_payload_get_uint(&p, "key", &val) && val == 0,
+	   "get_uint() empty");
+
+	error_payload_set_uint(&p, "key1", 1);
+	is(p.count, 1, "++count");
+	ok(error_payload_get_uint(&p, "key1", &val), "get_uint() finds");
+	is(val, 1, "value match");
+
+	val = 100;
+	ok(!error_payload_get_uint(&p, "key2", &val), "get_uint() fails");
+	is(val, 0, "value is default");
+
+	is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
+	   "small number size");
+
+	error_payload_set_uint(&p, "key2", UINT32_MAX);
+	ok(error_payload_get_uint(&p, "key2", &val), "get_uint() 32 bit");
+	is(val, UINT32_MAX, "value match");
+	is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
+	   "middle number size");
+	is(p.count, 2, "field count is same");
+
+	error_payload_set_uint(&p, "key1", UINT64_MAX);
+	ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
+	   "value 1");
+	ok(error_payload_get_uint(&p, "key2", &val) && val == UINT32_MAX,
+	   "value 2");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
+	   "remained value");
+	ok(!error_payload_get_uint(&p, "key2", &val) && val == 0,
+	   "deleted value");
+
+	error_payload_set_str(&p, "key2", "1");
+	ok(!error_payload_get_uint(&p, "key2", &val) && val == 0, "wrong type");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_int(void)
+{
+	header();
+	plan(20);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	int64_t val = 1;
+	ok(!error_payload_get_int(&p, "key", &val) && val == 0,
+	   "get_int() empty");
+
+	error_payload_set_int(&p, "key1", 1);
+	is(p.count, 1, "++count");
+	ok(error_payload_get_int(&p, "key1", &val), "get_int() finds");
+	is(val, 1, "value match");
+
+	val = 100;
+	ok(!error_payload_get_int(&p, "key2", &val), "get_int() fails");
+	is(val, 0, "value is default");
+
+	is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
+	   "small number size");
+
+	error_payload_set_int(&p, "key2", UINT32_MAX);
+	ok(error_payload_get_int(&p, "key2", &val), "get_int() 32 bit");
+	is(val, UINT32_MAX, "value match");
+	is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
+	   "middle number size");
+	is(p.count, 2, "field count is same");
+
+	error_payload_set_int(&p, "key1", INT64_MAX);
+	ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
+	   "value 1");
+	ok(error_payload_get_int(&p, "key2", &val) && val == UINT32_MAX,
+	   "value 2");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
+	   "remained value");
+	ok(!error_payload_get_int(&p, "key2", &val) && val == 0,
+	   "deleted value");
+
+	error_payload_set_str(&p, "key2", "1");
+	ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "wrong type");
+
+	error_payload_set_uint(&p, "key2", (uint64_t)INT64_MAX + 1);
+	ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "overflow");
+
+	error_payload_set_uint(&p, "key2", 100);
+	ok(error_payload_get_int(&p, "key2", &val) && val == 100, "conversion");
+
+	error_payload_set_int(&p, "key2", INT64_MIN);
+	ok(error_payload_get_int(&p, "key2", &val) && val == INT64_MIN,
+	   "min value");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_double(void)
+{
+	header();
+	plan(14);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	double val = 1;
+	ok(!error_payload_get_double(&p, "key", &val) && val == 0,
+	   "get_double() empty");
+
+	error_payload_set_double(&p, "key1", 1.5);
+	is(p.count, 1, "++count");
+	ok(error_payload_get_double(&p, "key1", &val), "get_double() finds");
+	is(val, 1.5, "value match");
+
+	val = 1;
+	ok(!error_payload_get_double(&p, "key2", &val), "get_double() fails");
+	is(val, 0, "value is default");
+
+	is(error_payload_find(&p, "key1")->size, mp_sizeof_double(1.5), "size");
+
+	error_payload_set_double(&p, "key2", DBL_MAX);
+	ok(error_payload_get_double(&p, "key1", &val) && val == 1.5, "value 1");
+	ok(error_payload_get_double(&p, "key2", &val) && val == DBL_MAX,
+	   "value 2");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	ok(error_payload_get_double(&p, "key1", &val) && val == 1.5,
+	   "remained value");
+	ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
+	   "deleted value");
+
+	error_payload_set_str(&p, "key2", "1");
+	ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
+	   "wrong type");
+
+	char buffer[16];
+	char *data = mp_encode_float(buffer, 0.5);
+	error_payload_set_mp(&p, "key2", buffer, data - buffer);
+	ok(error_payload_get_double(&p, "key2", &val) && val == 0.5,
+	   "float 0.5");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_bool(void)
+{
+	header();
+	plan(13);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	bool val = true;
+	ok(!error_payload_get_bool(&p, "key", &val) && !val,
+	   "get_bool() empty");
+
+	error_payload_set_bool(&p, "key1", true);
+	is(p.count, 1, "++count");
+	ok(error_payload_get_bool(&p, "key1", &val), "get_bool() finds");
+	ok(val, "value match");
+
+	val = true;
+	ok(!error_payload_get_bool(&p, "key2", &val), "get_bool() fails");
+	ok(!val, "value is default");
+
+	error_payload_set_bool(&p, "key2", false);
+	ok(error_payload_get_bool(&p, "key2", &val), "get_bool() finds");
+	ok(!val, "value match");
+
+	is(error_payload_find(&p, "key1")->size, mp_sizeof_bool(true), "size");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	ok(error_payload_get_bool(&p, "key1", &val) && val, "remained value");
+	ok(!error_payload_get_bool(&p, "key2", &val) && !val, "deleted value");
+
+	error_payload_set_str(&p, "key2", "true");
+	ok(!error_payload_get_bool(&p, "key2", &val) && !val, "wrong type");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_uuid(void)
+{
+	header();
+	plan(17);
+
+	struct error_payload p;
+	error_payload_create(&p);
+	struct tt_uuid val1;
+	tt_uuid_create(&val1);
+	ok(!error_payload_get_uuid(&p, "key", &val1), "get_uuid() empty");
+	ok(tt_uuid_is_nil(&val1), "default value");
+
+	tt_uuid_create(&val1);
+	error_payload_set_uuid(&p, "key1", &val1);
+	is(p.count, 1, "++count");
+	struct tt_uuid val2;
+	ok(error_payload_get_uuid(&p, "key1", &val2), "get_uuid() finds");
+	ok(tt_uuid_is_equal(&val1, &val2), "value match");
+
+	ok(!error_payload_get_uuid(&p, "key2", &val2), "get_uuid() fails");
+	ok(tt_uuid_is_nil(&val2), "value is default");
+
+	tt_uuid_create(&val2);
+	error_payload_set_uuid(&p, "key2", &val2);
+	struct tt_uuid val3;
+	ok(error_payload_get_uuid(&p, "key2", &val3), "get_uuid() finds");
+	ok(tt_uuid_is_equal(&val3, &val2), "value match");
+
+	is(error_payload_find(&p, "key1")->size, mp_sizeof_uuid(), "size");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 1, "--count");
+	ok(error_payload_get_uuid(&p, "key1", &val3), "remained value");
+	ok(tt_uuid_is_equal(&val1, &val3), "value match");
+	ok(!error_payload_get_uuid(&p, "key2", &val3), "deleted value");
+	ok(tt_uuid_is_nil(&val3), "value match");
+
+	error_payload_set_str(&p, "key2", "1");
+	ok(!error_payload_get_uuid(&p, "key2", &val3), "wrong type");
+	ok(tt_uuid_is_nil(&val3), "value match");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_field_mp(void)
+{
+	header();
+	plan(6);
+	char buf[1024];
+	char *data;
+	const char *cdata;
+	uint32_t size;
+
+	struct error_payload p;
+	error_payload_create(&p);
+
+	data = mp_encode_str(buf, "value1", 6);
+	error_payload_set_mp(&p, "key1", buf, data - buf);
+	is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0, "mp str");
+
+	cdata = error_payload_get_mp(&p, "key1", &size);
+	is(memcmp(cdata, buf, size), 0, "mp str cmp");
+
+	data = mp_encode_uint(buf, 100);
+	error_payload_set_mp(&p, "key2", buf, data - buf);
+	uint64_t val;
+	ok(error_payload_get_uint(&p, "key2", &val) && val == 100, "mp uint");
+
+	cdata = error_payload_get_mp(&p, "key2", &size);
+	is(memcmp(cdata, buf, size), 0, "mp uint cmp");
+
+	data = mp_encode_array(buf, 1);
+	data = mp_encode_uint(data, 2);
+	error_payload_set_mp(&p, "key3", buf, data - buf);
+
+	cdata = error_payload_get_mp(&p, "key3", &size);
+	is(memcmp(cdata, buf, size), 0, "mp array");
+
+	ok(!error_payload_get_uint(&p, "key3", &val) && val == 0,
+	   "mp uint from array");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_clear(void)
+{
+	header();
+	plan(13);
+
+	struct error_payload p;
+	error_payload_create(&p);
+
+	error_payload_set_uint(&p, "key1", 1);
+	error_payload_set_uint(&p, "key2", 2);
+	error_payload_set_uint(&p, "key3", 3);
+	error_payload_set_uint(&p, "key4", 4);
+	error_payload_set_uint(&p, "key5", 5);
+
+	error_payload_clear(&p, "key5");
+	is(p.count, 4, "clear last, count");
+	is(error_payload_find(&p, "key5"), NULL, "clear last, key");
+
+	error_payload_clear(&p, "key1");
+	is(p.count, 3, "clear first, count");
+	is(error_payload_find(&p, "key1"), NULL, "clear first, key");
+
+	uint64_t val;
+	ok(error_payload_get_uint(&p, "key2", &val) && val == 2, "check key2");
+	ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
+	ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
+
+	is(strcmp(p.fields[0]->name, "key4"), 0, "deletion is cyclic");
+
+	error_payload_clear(&p, "key2");
+	is(p.count, 2, "clear middle, count");
+	is(error_payload_find(&p, "key2"), NULL, "clear middle, key");
+	ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
+	ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
+
+	error_payload_clear(&p, "key3");
+	error_payload_clear(&p, "key4");
+
+	is(p.count, 0, "clear all");
+
+	error_payload_destroy(&p);
+
+	check_plan();
+	footer();
+}
+
+static void
+test_payload_move(void)
+{
+	header();
+	plan(7);
+
+	struct error_payload p1, p2;
+	error_payload_create(&p1);
+	error_payload_create(&p2);
+
+	error_payload_move(&p1, &p2);
+	ok(p1.count == 0 && p1.fields == NULL, "empty");
+
+	error_payload_set_str(&p1, "key", "value");
+	error_payload_move(&p1, &p2);
+	ok(p1.count == 0 && p1.fields == NULL, "emptied on move");
+
+	error_payload_set_str(&p1, "key", "value");
+	error_payload_set_str(&p2, "key1", "value1");
+	error_payload_set_str(&p2, "key2", "value2");
+	error_payload_move(&p1, &p2);
+	is(p1.count, 2, "got 2 fields");
+	isnt(p1.fields, NULL, "got 2 fields");
+	is(strcmp(error_payload_get_str(&p1, "key1"), "value1"), 0, "key1");
+	is(strcmp(error_payload_get_str(&p1, "key2"), "value2"), 0, "key2");
+	is(error_payload_get_str(&p1, "key"), NULL, "key");
+
+	error_payload_destroy(&p2);
+	error_payload_destroy(&p1);
+
+	check_plan();
+	footer();
+}
+
+int
+main(void)
+{
+	header();
+	plan(9);
+
+	random_init();
+
+	test_payload_field_str();
+	test_payload_field_uint();
+	test_payload_field_int();
+	test_payload_field_double();
+	test_payload_field_bool();
+	test_payload_field_uuid();
+	test_payload_field_mp();
+	test_payload_clear();
+	test_payload_move();
+
+	random_free();
+
+	footer();
+	return check_plan();
+}
diff --git a/test/unit/error.result b/test/unit/error.result
new file mode 100644
index 000000000..daf4e6eb0
--- /dev/null
+++ b/test/unit/error.result
@@ -0,0 +1,161 @@
+	*** main ***
+1..9
+	*** test_payload_field_str ***
+    1..15
+    ok 1 - no fields in the beginning
+    ok 2 - get_str() empty
+    ok 3 - ++count
+    ok 4 - get_str() finds
+    ok 5 - ++count
+    ok 6 - get_str() finds old
+    ok 7 - get_str() finds new
+    ok 8 - size does not include terminating zero
+    ok 9 - field count is same
+    ok 10 - get_str() finds new value
+    ok 11 - get_str() finds other key old value
+    ok 12 - --count
+    ok 13 - get_str() finds new value
+    ok 14 - get_str() can't find deleted value
+    ok 15 - wrong type
+ok 1 - subtests
+	*** test_payload_field_str: done ***
+	*** test_payload_field_uint ***
+    1..17
+    ok 1 - get_uint() empty
+    ok 2 - ++count
+    ok 3 - get_uint() finds
+    ok 4 - value match
+    ok 5 - get_uint() fails
+    ok 6 - value is default
+    ok 7 - small number size
+    ok 8 - get_uint() 32 bit
+    ok 9 - value match
+    ok 10 - middle number size
+    ok 11 - field count is same
+    ok 12 - value 1
+    ok 13 - value 2
+    ok 14 - --count
+    ok 15 - remained value
+    ok 16 - deleted value
+    ok 17 - wrong type
+ok 2 - subtests
+	*** test_payload_field_uint: done ***
+	*** test_payload_field_int ***
+    1..20
+    ok 1 - get_int() empty
+    ok 2 - ++count
+    ok 3 - get_int() finds
+    ok 4 - value match
+    ok 5 - get_int() fails
+    ok 6 - value is default
+    ok 7 - small number size
+    ok 8 - get_int() 32 bit
+    ok 9 - value match
+    ok 10 - middle number size
+    ok 11 - field count is same
+    ok 12 - value 1
+    ok 13 - value 2
+    ok 14 - --count
+    ok 15 - remained value
+    ok 16 - deleted value
+    ok 17 - wrong type
+    ok 18 - overflow
+    ok 19 - conversion
+    ok 20 - min value
+ok 3 - subtests
+	*** test_payload_field_int: done ***
+	*** test_payload_field_double ***
+    1..14
+    ok 1 - get_double() empty
+    ok 2 - ++count
+    ok 3 - get_double() finds
+    ok 4 - value match
+    ok 5 - get_double() fails
+    ok 6 - value is default
+    ok 7 - size
+    ok 8 - value 1
+    ok 9 - value 2
+    ok 10 - --count
+    ok 11 - remained value
+    ok 12 - deleted value
+    ok 13 - wrong type
+    ok 14 - float 0.5
+ok 4 - subtests
+	*** test_payload_field_double: done ***
+	*** test_payload_field_bool ***
+    1..13
+    ok 1 - get_bool() empty
+    ok 2 - ++count
+    ok 3 - get_bool() finds
+    ok 4 - value match
+    ok 5 - get_bool() fails
+    ok 6 - value is default
+    ok 7 - get_bool() finds
+    ok 8 - value match
+    ok 9 - size
+    ok 10 - --count
+    ok 11 - remained value
+    ok 12 - deleted value
+    ok 13 - wrong type
+ok 5 - subtests
+	*** test_payload_field_bool: done ***
+	*** test_payload_field_uuid ***
+    1..17
+    ok 1 - get_uuid() empty
+    ok 2 - default value
+    ok 3 - ++count
+    ok 4 - get_uuid() finds
+    ok 5 - value match
+    ok 6 - get_uuid() fails
+    ok 7 - value is default
+    ok 8 - get_uuid() finds
+    ok 9 - value match
+    ok 10 - size
+    ok 11 - --count
+    ok 12 - remained value
+    ok 13 - value match
+    ok 14 - deleted value
+    ok 15 - value match
+    ok 16 - wrong type
+    ok 17 - value match
+ok 6 - subtests
+	*** test_payload_field_uuid: done ***
+	*** test_payload_field_mp ***
+    1..6
+    ok 1 - mp str
+    ok 2 - mp str cmp
+    ok 3 - mp uint
+    ok 4 - mp uint cmp
+    ok 5 - mp array
+    ok 6 - mp uint from array
+ok 7 - subtests
+	*** test_payload_field_mp: done ***
+	*** test_payload_clear ***
+    1..13
+    ok 1 - clear last, count
+    ok 2 - clear last, key
+    ok 3 - clear first, count
+    ok 4 - clear first, key
+    ok 5 - check key2
+    ok 6 - check key3
+    ok 7 - check key4
+    ok 8 - deletion is cyclic
+    ok 9 - clear middle, count
+    ok 10 - clear middle, key
+    ok 11 - check key3
+    ok 12 - check key4
+    ok 13 - clear all
+ok 8 - subtests
+	*** test_payload_clear: done ***
+	*** test_payload_move ***
+    1..7
+    ok 1 - empty
+    ok 2 - emptied on move
+    ok 3 - got 2 fields
+    ok 4 - got 2 fields
+    ok 5 - key1
+    ok 6 - key2
+    ok 7 - key
+ok 9 - subtests
+	*** test_payload_move: done ***
+	*** main: done ***
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 04/11] error: move code to struct error from ClientError
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 03/11] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 05/11] error: use error_payload to store optional members Vladislav Shpilevoy via Tarantool-patches
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

All optional fields soon will be moved into error_payload. Code
was optional too. But it is needed too often, the most used field.
The patch moves it into struct error to make it more accessible.

Also in future it should allow to drop the hack
ClientError::get_errcode() which tries to return error code
depending on error type. But could just store the code right away.

As a consequence of the patch, errors which didn't have an error
code at all before, such as LuajitError, now have it 0 in Lua.

Part of #5568
---
 src/box/error.cc              | 15 +++++++--------
 src/box/error.h               |  4 +---
 src/box/index.cc              |  4 ++--
 src/box/mp_error.cc           |  7 +++----
 src/lib/core/diag.c           |  1 +
 src/lib/core/diag.h           |  2 ++
 src/lua/error.lua             |  3 ++-
 test/engine/func_index.result |  3 ++-
 test/unit/mp_error.cc         |  2 +-
 9 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/src/box/error.cc b/src/box/error.cc
index 225b377b7..bc0d46601 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -76,7 +76,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
 	ClientError *client_error = type_cast(ClientError, e);
 	if (client_error) {
-		client_error->m_errcode = code;
+		client_error->code = code;
 		va_list ap;
 		va_start(ap, fmt);
 		error_vformat_msg(e, fmt, ap);
@@ -94,7 +94,7 @@ box_error_new_va(const char *file, unsigned line, uint32_t code,
 		struct error *e = BuildClientError(file, line, ER_UNKNOWN);
 		ClientError *client_error = type_cast(ClientError, e);
 		if (client_error != NULL) {
-			client_error->m_errcode = code;
+			client_error->code = code;
 			error_vformat_msg(e, fmt, ap);
 		}
 		return e;
@@ -170,7 +170,7 @@ ClientError::ClientError(const type_info *type, const char *file, unsigned line,
 			 uint32_t errcode)
 	:Exception(type, file, line)
 {
-	m_errcode = errcode;
+	code = errcode;
 	if (rmean_error)
 		rmean_collect(rmean_error, RMEAN_ERROR, 1);
 }
@@ -181,7 +181,7 @@ ClientError::ClientError(const char *file, unsigned line,
 {
 	va_list ap;
 	va_start(ap, errcode);
-	error_vformat_msg(this, tnt_errcode_desc(m_errcode), ap);
+	error_vformat_msg(this, tnt_errcode_desc(errcode), ap);
 	va_end(ap);
 }
 
@@ -194,7 +194,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
 		va_start(ap, errcode);
 		error_vformat_msg(e, tnt_errcode_desc(errcode), ap);
 		va_end(ap);
-		e->m_errcode = errcode;
+		e->code = errcode;
 		return e;
 	} catch (OutOfMemory *e) {
 		return e;
@@ -204,8 +204,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
 void
 ClientError::log() const
 {
-	say_file_line(S_ERROR, file, line, errmsg, "%s",
-		      tnt_errcode_str(m_errcode));
+	say_file_line(S_ERROR, file, line, errmsg, "%s", tnt_errcode_str(code));
 }
 
 
@@ -296,7 +295,7 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
 				     bool run_trigers)
 	:ClientError(&type_AccessDeniedError, file, line, ER_ACCESS_DENIED)
 {
-	error_format_msg(this, tnt_errcode_desc(m_errcode),
+	error_format_msg(this, tnt_errcode_desc(code),
 			 access_type, object_type, object_name, user_name);
 
 	struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
diff --git a/src/box/error.h b/src/box/error.h
index 338121dd9..30ab36a97 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -208,14 +208,12 @@ public:
 	int
 	errcode() const
 	{
-		return m_errcode;
+		return code;
 	}
 
 	ClientError(const char *file, unsigned line, uint32_t errcode, ...);
 
 	static uint32_t get_errcode(const struct error *e);
-	/* client errno code */
-	int m_errcode;
 protected:
 	ClientError(const type_info *type, const char *file, unsigned line,
 		    uint32_t errcode);
diff --git a/src/box/index.cc b/src/box/index.cc
index 0651868d3..be14827be 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -47,8 +47,8 @@ UnsupportedIndexFeature::UnsupportedIndexFeature(const char *file,
 	: ClientError(file, line, ER_UNKNOWN)
 {
 	struct space *space = space_cache_find_xc(index_def->space_id);
-	m_errcode = ER_UNSUPPORTED_INDEX_FEATURE;
-	error_format_msg(this, tnt_errcode_desc(m_errcode), index_def->name,
+	code = ER_UNSUPPORTED_INDEX_FEATURE;
+	error_format_msg(this, tnt_errcode_desc(code), index_def->name,
 			 index_type_strs[index_def->type],
 			 space->def->name, space->def->engine_name, what);
 }
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 059f9bc85..35c770400 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -261,10 +261,8 @@ missing_fields:
 	}
 
 	if (strcmp(mp_error->type, "ClientError") == 0) {
-		ClientError *e = new ClientError(mp_error->file, mp_error->line,
-						 ER_UNKNOWN);
-		e->m_errcode = mp_error->code;
-		err = e;
+		err = new ClientError(mp_error->file, mp_error->line,
+				      ER_UNKNOWN);
 	} else if (strcmp(mp_error->type, "CustomError") == 0) {
 		if (mp_error->custom_type == NULL)
 			goto missing_fields;
@@ -320,6 +318,7 @@ missing_fields:
 		err = new ClientError(mp_error->file, mp_error->line,
 				      ER_UNKNOWN);
 	}
+	err->code = mp_error->code;
 	err->saved_errno = mp_error->saved_errno;
 	error_format_msg(err, "%s", mp_error->message);
 	return err;
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index f872be6a7..b557ca667 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -116,6 +116,7 @@ error_create(struct error *e,
 	e->type = type;
 	e->refs = 0;
 	e->saved_errno = 0;
+	e->code = 0;
 	if (file != NULL) {
 		snprintf(e->file, sizeof(e->file), "%s", file);
 		e->line = line;
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index d7d09f6fb..30b8d2f5d 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -87,6 +87,8 @@ struct error {
 	 * to the standard library, then it is 0.
 	 */
 	int saved_errno;
+	/** Error code. Shortest possible description of error's reason. */
+	int code;
 	/** Line number. */
 	unsigned line;
 	/* Source file name. */
diff --git a/src/lua/error.lua b/src/lua/error.lua
index ebc784f07..9fb227df4 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -18,6 +18,7 @@ struct error {
     const struct type_info *_type;
     int64_t _refs;
     int _saved_errno;
+    int code;
     /** Line number. */
     unsigned _line;
     /* Source file name. */
@@ -158,7 +159,7 @@ local function error_unpack(err)
     if not ffi.istype('struct error', err) then
         error("Usage: error:unpack()")
     end
-    local result = {}
+    local result = {code = err.code}
     for key, getter in pairs(error_fields)  do
         result[key] = getter(err)
     end
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index b689c68ec..d3f44f7be 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -292,7 +292,8 @@ e = e.prev
  | ...
 e:unpack()
  | ---
- | - base_type: LuajitError
+ | - code: 0
+ |   base_type: LuajitError
  |   type: LuajitError
  |   message: '[string "return function(tuple)                 local ..."]:1: attempt
  |     to call global ''require'' (a nil value)'
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
index e85d282e7..fe0cd49b9 100644
--- a/test/unit/mp_error.cc
+++ b/test/unit/mp_error.cc
@@ -349,7 +349,7 @@ test_decode_unknown_type()
 	const char *pos = buffer;
 	struct error *unpacked = error_unpack(&pos, len);
 	error_ref(unpacked);
-	err.code = 0;
+	err.code = 1;
 	err.type = "ClientError";
 	ok(error_is_eq_mp_error(unpacked, &err), "check SomeNewError");
 	error_unref(unpacked);
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 05/11] error: use error_payload to store optional members
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (5 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 04/11] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 06/11] error: use error_payload in MessagePack codecs Vladislav Shpilevoy via Tarantool-patches
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

error_payload is a part of struct error now. All the fields stored
in C++ classes on top of struct error are moved into the payload.

Part of #5568
---
 src/box/error.cc        | 11 +++---
 src/box/error.h         | 34 ++++-------------
 src/lib/core/diag.c     |  2 +
 src/lib/core/diag.h     | 82 +++++++++++++++++++++++++++++++++++++++++
 src/lua/error.lua       | 12 ++++++
 test/box/error.result   |  4 +-
 test/box/error.test.lua |  2 +-
 7 files changed, 112 insertions(+), 35 deletions(-)

diff --git a/src/box/error.cc b/src/box/error.cc
index bc0d46601..34e807eb3 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -305,9 +305,9 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
 	 */
 	if (run_trigers)
 		trigger_run(&on_access_denied, (void *) &ctx);
-	m_object_type = strdup(object_type);
-	m_access_type = strdup(access_type);
-	m_object_name = strdup(object_name);
+	error_set_str(this, "object_type", object_type);
+	error_set_str(this, "object_name", object_name);
+	error_set_str(this, "access_type", access_type);
 }
 
 struct error *
@@ -337,15 +337,14 @@ CustomError::CustomError(const char *file, unsigned int line,
 			 const char *custom_type, uint32_t errcode)
 	:ClientError(&type_CustomError, file, line, errcode)
 {
-	strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
-	m_custom_type[sizeof(m_custom_type) - 1] = '\0';
+	error_set_str(this, "custom_type", custom_type);
 }
 
 void
 CustomError::log() const
 {
 	say_file_line(S_ERROR, file, line, errmsg,
-		      "Custom type %s", m_custom_type);
+		      "Custom type %s", custom_type());
 }
 
 struct error *
diff --git a/src/box/error.h b/src/box/error.h
index 30ab36a97..cc37412d7 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -243,38 +243,23 @@ public:
 			  const char *object_name, const char *user_name,
 			  bool run_trigers = true);
 
-	~AccessDeniedError()
-	{
-		free(m_object_name);
-		free(m_object_type);
-		free(m_access_type);
-	}
-
 	const char *
-	object_type()
+	object_type() const
 	{
-		return m_object_type;
+		return error_get_str(this, "object_type");
 	}
 
 	const char *
-	object_name()
+	object_name() const
 	{
-		return m_object_name?:"(nil)";
+		return error_get_str(this, "object_name");
 	}
 
 	const char *
-	access_type()
+	access_type() const
 	{
-		return m_access_type;
+		return error_get_str(this, "access_type");
 	}
-
-private:
-	/** Type of object the required access was denied to */
-	char *m_object_type;
-	/** Name of object the required access was denied to */
-	char *m_object_name;
-	/** Type of declined access */
-	char *m_access_type;
 };
 
 /**
@@ -319,13 +304,10 @@ public:
 	virtual void log() const;
 
 	const char*
-	custom_type()
+	custom_type() const
 	{
-		return m_custom_type;
+		return error_get_str(this, "custom_type");
 	}
-private:
-	/** Custom type name. */
-	char m_custom_type[64];
 };
 
 #endif /* defined(__cplusplus) */
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index b557ca667..217c3ae7e 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -53,6 +53,7 @@ error_unref(struct error *e)
 			to_delete->cause->effect = NULL;
 			to_delete->cause = NULL;
 		}
+		error_payload_destroy(&to_delete->payload);
 		to_delete->destroy(to_delete);
 		if (cause == NULL)
 			return;
@@ -117,6 +118,7 @@ error_create(struct error *e,
 	e->refs = 0;
 	e->saved_errno = 0;
 	e->code = 0;
+	error_payload_create(&e->payload);
 	if (file != NULL) {
 		snprintf(e->file, sizeof(e->file), "%s", file);
 		e->line = line;
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 30b8d2f5d..c3e7470e8 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -36,6 +36,8 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <assert.h>
+
+#include "error_payload.h"
 #include "say.h"
 
 #if defined(__cplusplus)
@@ -89,6 +91,8 @@ struct error {
 	int saved_errno;
 	/** Error code. Shortest possible description of error's reason. */
 	int code;
+	/** Key-value storage with error's dynamic fields. */
+	struct error_payload payload;
 	/** Line number. */
 	unsigned line;
 	/* Source file name. */
@@ -120,6 +124,84 @@ error_ref(struct error *e);
 void
 error_unref(struct error *e);
 
+static inline const char *
+error_get_str(const struct error *e, const char *name)
+{
+	return error_payload_get_str(&e->payload, name);
+}
+
+static inline void
+error_set_str(struct error *e, const char *name, const char *value)
+{
+	error_payload_set_str(&e->payload, name, value);
+}
+
+static inline bool
+error_get_uint(const struct error *e, const char *name, uint64_t *value)
+{
+	return error_payload_get_uint(&e->payload, name, value);
+}
+
+static inline void
+error_set_uint(struct error *e, const char *name, uint64_t value)
+{
+	error_payload_set_uint(&e->payload, name, value);
+}
+
+static inline bool
+error_get_int(const struct error *e, const char *name, int64_t *value)
+{
+	return error_payload_get_int(&e->payload, name, value);
+}
+
+static inline void
+error_set_int(struct error *e, const char *name, int64_t value)
+{
+	error_payload_set_int(&e->payload, name, value);
+}
+
+static inline bool
+error_get_double(const struct error *e, const char *name, double *value)
+{
+	return error_payload_get_double(&e->payload, name, value);
+}
+
+static inline void
+error_set_double(struct error *e, const char *name, double value)
+{
+	error_payload_set_double(&e->payload, name, value);
+}
+
+static inline bool
+error_get_bool(const struct error *e, const char *name, bool *value)
+{
+	return error_payload_get_bool(&e->payload, name, value);
+}
+
+static inline void
+error_set_bool(struct error *e, const char *name, bool value)
+{
+	error_payload_set_bool(&e->payload, name, value);
+}
+
+static inline bool
+error_get_uuid(const struct error *e, const char *name, struct tt_uuid *value)
+{
+	return error_payload_get_uuid(&e->payload, name, value);
+}
+
+static inline void
+error_set_uuid(struct error *e, const char *name, const struct tt_uuid *value)
+{
+	error_payload_set_uuid(&e->payload, name, value);
+}
+
+static inline void
+error_clear_field(struct error *e, const char *name)
+{
+	error_payload_clear(&e->payload, name);
+}
+
 /**
  * Unlink error from its effect. For instance:
  * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 9fb227df4..fb48a9b18 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -11,6 +11,17 @@ enum {
 
 typedef void (*error_f)(struct error *e);
 
+struct error_field {
+    char *_data;
+    uint32_t _size;
+    char _name[1];
+};
+
+struct error_payload {
+    int _count;
+    struct error_field **_fields;
+};
+
 struct error {
     error_f _destroy;
     error_f _raise;
@@ -19,6 +30,7 @@ struct error {
     int64_t _refs;
     int _saved_errno;
     int code;
+    struct error_payload _payload;
     /** Line number. */
     unsigned _line;
     /* Source file name. */
diff --git a/test/box/error.result b/test/box/error.result
index 19ea53873..7583306a6 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -899,13 +899,13 @@ e:unpack()
  |   - file: '[string "e = box.error.new({type = ''TestType''}) "]'
  |     line: 1
  | ...
--- Try too long type name.
+-- Try long type name.
 e = box.error.new({type = string.rep('a', 128)})
  | ---
  | ...
 #e.type
  | ---
- | - 63
+ | - 128
  | ...
 
 -- gh-4887: accessing 'prev' member also refs it so that after
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index ed95d1d41..068212d96 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -241,7 +241,7 @@ e:unpack()
 -- Try to omit message.
 e = box.error.new({type = 'TestType'})
 e:unpack()
--- Try too long type name.
+-- Try long type name.
 e = box.error.new({type = string.rep('a', 128)})
 #e.type
 
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 06/11] error: use error_payload in MessagePack codecs
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (6 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 05/11] error: use error_payload to store optional members Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 07/11] error: use error_payload in Lua Vladislav Shpilevoy via Tarantool-patches
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

Before this patch mp_error API could only encode and decode
hardcoded fields from the C++ classes inheriting struct error.

The fields are gone from the classes in a previous patch - moved
into error_payload. Now to be able to support arbitrary fields in
the payload the MessagePack encoding/decoding must use its content
instead of hardcoded fields depending on error type.

Part of #5568
---
 src/box/error.cc          |   7 --
 src/box/error.h           |  28 +++++-
 src/box/mp_error.cc       | 194 +++++++++++---------------------------
 src/lib/core/diag.c       |  17 ++--
 src/lib/core/diag.h       |   9 ++
 src/lib/core/exception.h  |  66 +++++++++++++
 test/unit/mp_error.cc     |  74 ++++++++++++++-
 test/unit/mp_error.result |  27 +++++-
 8 files changed, 260 insertions(+), 162 deletions(-)

diff --git a/src/box/error.cc b/src/box/error.cc
index 34e807eb3..3b5fa1c95 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -256,13 +256,6 @@ XlogGapError::XlogGapError(const char *file, unsigned line,
 		 (long long) vclock_sum(to), s_to ? s_to : "");
 }
 
-XlogGapError::XlogGapError(const char *file, unsigned line,
-			   const char *msg)
-		: XlogError(&type_XlogGapError, file, line)
-{
-	error_format_msg(this, "%s", msg);
-}
-
 struct error *
 BuildXlogGapError(const char *file, unsigned line,
 		  const struct vclock *from, const struct vclock *to)
diff --git a/src/box/error.h b/src/box/error.h
index cc37412d7..fbfcec995 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -213,6 +213,11 @@ public:
 
 	ClientError(const char *file, unsigned line, uint32_t errcode, ...);
 
+	ClientError()
+		:Exception(&type_ClientError, NULL, 0)
+	{
+	}
+
 	static uint32_t get_errcode(const struct error *e);
 protected:
 	ClientError(const type_info *type, const char *file, unsigned line,
@@ -243,6 +248,11 @@ public:
 			  const char *object_name, const char *user_name,
 			  bool run_trigers = true);
 
+	AccessDeniedError()
+		:ClientError(&type_AccessDeniedError, NULL, 0, 0)
+	{
+	}
+
 	const char *
 	object_type() const
 	{
@@ -276,6 +286,12 @@ struct XlogError: public Exception
 	{
 		error_vformat_msg(this, format, ap);
 	}
+
+	XlogError()
+		:Exception(&type_XlogError, NULL, 0)
+	{
+	}
+
 	XlogError(const struct type_info *type, const char *file,
 		  unsigned line)
 		:Exception(type, file, line)
@@ -289,8 +305,11 @@ struct XlogGapError: public XlogError
 {
 	XlogGapError(const char *file, unsigned line,
 		     const struct vclock *from, const struct vclock *to);
-	XlogGapError(const char *file, unsigned line,
-		     const char *msg);
+
+	XlogGapError()
+		:XlogError(&type_XlogGapError, NULL, 0)
+	{
+	}
 
 	virtual void raise() { throw this; }
 };
@@ -301,6 +320,11 @@ public:
 	CustomError(const char *file, unsigned int line,
 		    const char *custom_type, uint32_t errcode);
 
+	CustomError()
+		:ClientError(&type_CustomError, NULL, 0, 0)
+	{
+	}
+
 	virtual void log() const;
 
 	const char*
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 35c770400..fba562a84 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -118,35 +118,31 @@ struct mp_error {
 	const char *type;
 	const char *file;
 	const char *message;
-	const char *custom_type;
-	const char *ad_object_type;
-	const char *ad_object_name;
-	const char *ad_access_type;
+	struct error_payload payload;
 };
 
 static void
 mp_error_create(struct mp_error *mp_error)
 {
 	memset(mp_error, 0, sizeof(*mp_error));
+	error_payload_create(&mp_error->payload);
+}
+
+static void
+mp_error_destroy(struct mp_error *mp_error)
+{
+	error_payload_destroy(&mp_error->payload);
 }
 
 static uint32_t
 mp_sizeof_error_one(const struct error *error)
 {
 	uint32_t errcode = box_error_code(error);
-
-	bool is_custom = false;
-	bool is_access_denied = false;
-
-	if (strcmp(error->type->name, "CustomError") == 0) {
-		is_custom = true;
-	} else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
-		is_access_denied = true;
-	}
-
-	uint32_t details_num = 6;
+	int field_count = error->payload.count;
+	uint32_t map_size = 6 + (field_count > 0);
 	uint32_t data_size = 0;
 
+	data_size += mp_sizeof_map(map_size);
 	data_size += mp_sizeof_uint(MP_ERROR_TYPE);
 	data_size += mp_sizeof_str(strlen(error->type->name));
 	data_size += mp_sizeof_uint(MP_ERROR_LINE);
@@ -160,28 +156,15 @@ mp_sizeof_error_one(const struct error *error)
 	data_size += mp_sizeof_uint(MP_ERROR_CODE);
 	data_size += mp_sizeof_uint(errcode);
 
-	if (is_access_denied) {
-		++details_num;
-		data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
-		data_size += mp_sizeof_map(3);
-		AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
-		data_size += mp_sizeof_str(strlen("object_type"));
-		data_size += mp_sizeof_str(strlen(ad_err->object_type()));
-		data_size += mp_sizeof_str(strlen("object_name"));
-		data_size += mp_sizeof_str(strlen(ad_err->object_name()));
-		data_size += mp_sizeof_str(strlen("access_type"));
-		data_size += mp_sizeof_str(strlen(ad_err->access_type()));
-	} else if (is_custom) {
-		++details_num;
+	if (field_count > 0) {
 		data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
-		data_size += mp_sizeof_map(1);
-		data_size += mp_sizeof_str(strlen("custom_type"));
-		data_size +=
-			mp_sizeof_str(strlen(box_error_custom_type(error)));
+		data_size += mp_sizeof_map(field_count);
+		for (int i = 0; i < field_count; ++i) {
+			const struct error_field *f = error->payload.fields[i];
+			data_size += mp_sizeof_str(strlen(f->name));
+			data_size += f->size;
+		}
 	}
-
-	data_size += mp_sizeof_map(details_num);
-
 	return data_size;
 }
 
@@ -195,21 +178,10 @@ static char *
 mp_encode_error_one(char *data, const struct error *error)
 {
 	uint32_t errcode = box_error_code(error);
+	int field_count = error->payload.count;
+	uint32_t map_size = 6 + (field_count > 0);
 
-	bool is_custom = false;
-	bool is_access_denied = false;
-
-	if (strcmp(error->type->name, "CustomError") == 0) {
-		is_custom = true;
-	} else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
-		is_access_denied = true;
-	}
-
-	uint32_t details_num = 6;
-	if (is_access_denied || is_custom)
-		++details_num;
-
-	data = mp_encode_map(data, details_num);
+	data = mp_encode_map(data, map_size);
 	data = mp_encode_uint(data, MP_ERROR_TYPE);
 	data = mp_encode_str0(data, error->type->name);
 	data = mp_encode_uint(data, MP_ERROR_LINE);
@@ -223,21 +195,15 @@ mp_encode_error_one(char *data, const struct error *error)
 	data = mp_encode_uint(data, MP_ERROR_CODE);
 	data = mp_encode_uint(data, errcode);
 
-	if (is_access_denied) {
-		data = mp_encode_uint(data, MP_ERROR_FIELDS);
-		data = mp_encode_map(data, 3);
-		AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
-		data = mp_encode_str0(data, "object_type");
-		data = mp_encode_str0(data, ad_err->object_type());
-		data = mp_encode_str0(data, "object_name");
-		data = mp_encode_str0(data, ad_err->object_name());
-		data = mp_encode_str0(data, "access_type");
-		data = mp_encode_str0(data, ad_err->access_type());
-	} else if (is_custom) {
+	if (field_count > 0) {
 		data = mp_encode_uint(data, MP_ERROR_FIELDS);
-		data = mp_encode_map(data, 1);
-		data = mp_encode_str0(data, "custom_type");
-		data = mp_encode_str0(data, box_error_custom_type(error));
+		data = mp_encode_map(data, field_count);
+		for (int i = 0; i < field_count; ++i) {
+			const struct error_field *f = error->payload.fields[i];
+			data = mp_encode_str0(data, f->name);
+			memcpy(data, f->data, f->size);
+			data += f->size;
+		}
 	}
 	return data;
 }
@@ -254,72 +220,50 @@ error_build_xc(struct mp_error *mp_error)
 	struct error *err = NULL;
 	if (mp_error->type == NULL || mp_error->message == NULL ||
 	    mp_error->file == NULL) {
-missing_fields:
 		diag_set(ClientError, ER_INVALID_MSGPACK,
 			 "Missing mandatory error fields");
 		return NULL;
 	}
 
 	if (strcmp(mp_error->type, "ClientError") == 0) {
-		err = new ClientError(mp_error->file, mp_error->line,
-				      ER_UNKNOWN);
+		err = new ClientError();
 	} else if (strcmp(mp_error->type, "CustomError") == 0) {
-		if (mp_error->custom_type == NULL)
-			goto missing_fields;
-		err = new CustomError(mp_error->file, mp_error->line,
-				      mp_error->custom_type, mp_error->code);
+		err = new CustomError();
 	} else if (strcmp(mp_error->type, "AccessDeniedError") == 0) {
-		if (mp_error->ad_access_type == NULL ||
-		    mp_error->ad_object_type == NULL ||
-		    mp_error->ad_object_name == NULL)
-			goto missing_fields;
-		err = new AccessDeniedError(mp_error->file, mp_error->line,
-					    mp_error->ad_access_type,
-					    mp_error->ad_object_type,
-					    mp_error->ad_object_name, "",
-					    false);
+		err = new AccessDeniedError();
 	} else if (strcmp(mp_error->type, "XlogError") == 0) {
-		err = new XlogError(&type_XlogError, mp_error->file,
-				    mp_error->line);
+		err = new XlogError();
 	} else if (strcmp(mp_error->type, "XlogGapError") == 0) {
-		err = new XlogGapError(mp_error->file, mp_error->line,
-				       mp_error->message);
+		err = new XlogGapError();
 	} else if (strcmp(mp_error->type, "SystemError") == 0) {
-		err = new SystemError(mp_error->file, mp_error->line,
-				      "%s", mp_error->message);
+		err = new SystemError();
 	} else if (strcmp(mp_error->type, "SocketError") == 0) {
-		err = new SocketError(mp_error->file, mp_error->line, "", "");
-		error_format_msg(err, "%s", mp_error->message);
+		err = new SocketError();
 	} else if (strcmp(mp_error->type, "OutOfMemory") == 0) {
-		err = new OutOfMemory(mp_error->file, mp_error->line,
-				      0, "", "");
+		err = new OutOfMemory();
 	} else if (strcmp(mp_error->type, "TimedOut") == 0) {
-		err = new TimedOut(mp_error->file, mp_error->line);
+		err = new TimedOut();
 	} else if (strcmp(mp_error->type, "ChannelIsClosed") == 0) {
-		err = new ChannelIsClosed(mp_error->file, mp_error->line);
+		err = new ChannelIsClosed();
 	} else if (strcmp(mp_error->type, "FiberIsCancelled") == 0) {
-		err = new FiberIsCancelled(mp_error->file, mp_error->line);
+		err = new FiberIsCancelled();
 	} else if (strcmp(mp_error->type, "LuajitError") == 0) {
-		err = new LuajitError(mp_error->file, mp_error->line,
-				      mp_error->message);
+		err = new LuajitError();
 	} else if (strcmp(mp_error->type, "IllegalParams") == 0) {
-		err = new IllegalParams(mp_error->file, mp_error->line,
-					"%s", mp_error->message);
+		err = new IllegalParams();
 	} else if (strcmp(mp_error->type, "CollationError") == 0) {
-		err = new CollationError(mp_error->file, mp_error->line,
-					 "%s", mp_error->message);
+		err = new CollationError();
 	} else if (strcmp(mp_error->type, "SwimError") == 0) {
-		err = new SwimError(mp_error->file, mp_error->line,
-				    "%s", mp_error->message);
+		err = new SwimError();
 	} else if (strcmp(mp_error->type, "CryptoError") == 0) {
-		err = new CryptoError(mp_error->file, mp_error->line,
-				      "%s", mp_error->message);
+		err = new CryptoError();
 	} else {
-		err = new ClientError(mp_error->file, mp_error->line,
-				      ER_UNKNOWN);
+		err = new ClientError();
 	}
 	err->code = mp_error->code;
 	err->saved_errno = mp_error->saved_errno;
+	error_set_location(err, mp_error->file, mp_error->line);
+	error_move_payload(err, &mp_error->payload);
 	error_format_msg(err, "%s", mp_error->message);
 	return err;
 }
@@ -350,12 +294,6 @@ mp_decode_and_copy_str(const char **data, struct region *region)
 	return region_strdup(region, str, str_len);;
 }
 
-static inline bool
-str_nonterm_is_eq(const char *l, const char *r, uint32_t r_len)
-{
-	return r_len == strlen(l) && memcmp(l, r, r_len) == 0;
-}
-
 static int
 mp_decode_error_fields(const char **data, struct mp_error *mp_err,
 		       struct region *region)
@@ -363,35 +301,16 @@ mp_decode_error_fields(const char **data, struct mp_error *mp_err,
 	if (mp_typeof(**data) != MP_MAP)
 		return -1;
 	uint32_t map_sz = mp_decode_map(data);
-	const char *key;
-	uint32_t key_len;
 	for (uint32_t i = 0; i < map_sz; ++i) {
-		if (mp_typeof(**data) != MP_STR)
+		uint32_t svp = region_used(region);
+		const char *key = mp_decode_and_copy_str(data, region);
+		if (key == NULL)
 			return -1;
-		key = mp_decode_str(data, &key_len);
-		if (str_nonterm_is_eq("object_type", key, key_len)) {
-			mp_err->ad_object_type =
-				mp_decode_and_copy_str(data, region);
-			if (mp_err->ad_object_type == NULL)
-				return -1;
-		} else if (str_nonterm_is_eq("object_name", key, key_len)) {
-			mp_err->ad_object_name =
-				mp_decode_and_copy_str(data, region);
-			if (mp_err->ad_object_name == NULL)
-				return -1;
-		} else if (str_nonterm_is_eq("access_type", key, key_len)) {
-			mp_err->ad_access_type =
-				mp_decode_and_copy_str(data, region);
-			if (mp_err->ad_access_type == NULL)
-				return -1;
-		} else if (str_nonterm_is_eq("custom_type", key, key_len)) {
-			mp_err->custom_type =
-				mp_decode_and_copy_str(data, region);
-			if (mp_err->custom_type == NULL)
-				return -1;
-		} else {
-			mp_next(data);
-		}
+		const char *value = *data;
+		mp_next(data);
+		uint32_t value_len = *data - value;
+		error_payload_set_mp(&mp_err->payload, key, value, value_len);
+		region_truncate(region, svp);
 	}
 	return 0;
 }
@@ -462,6 +381,7 @@ mp_decode_error_one(const char **data)
 	}
 finish:
 	region_truncate(region, region_svp);
+	mp_error_destroy(&mp_err);
 	return err;
 
 error:
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index 217c3ae7e..b6fa1f5bb 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -119,18 +119,21 @@ error_create(struct error *e,
 	e->saved_errno = 0;
 	e->code = 0;
 	error_payload_create(&e->payload);
-	if (file != NULL) {
-		snprintf(e->file, sizeof(e->file), "%s", file);
-		e->line = line;
-	} else {
-		e->file[0] = '\0';
-		e->line = 0;
-	}
+	if (file == NULL)
+		file = "";
+	error_set_location(e, file, line);
 	e->errmsg[0] = '\0';
 	e->cause = NULL;
 	e->effect = NULL;
 }
 
+void
+error_set_location(struct error *e, const char *file, int line)
+{
+	snprintf(e->file, sizeof(e->file), "%s", file);
+	e->line = line;
+}
+
 struct diag *
 diag_get(void)
 {
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index c3e7470e8..933ba3ae3 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -202,6 +202,12 @@ error_clear_field(struct error *e, const char *name)
 	error_payload_clear(&e->payload, name);
 }
 
+static inline void
+error_move_payload(struct error *e, struct error_payload *src)
+{
+	error_payload_move(&e->payload, src);
+}
+
 /**
  * Unlink error from its effect. For instance:
  * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
@@ -255,6 +261,9 @@ error_create(struct error *e,
 	     const struct type_info *type, const char *file,
 	     unsigned line);
 
+void
+error_set_location(struct error *e, const char *file, int line);
+
 void
 error_format_msg(struct error *e, const char *format, ...);
 
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index 7277b2784..0e9e05f45 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -91,6 +91,12 @@ public:
 
 	SystemError(const char *file, unsigned line,
 		    const char *format, ...);
+
+	SystemError()
+		:Exception(&type_SystemError, NULL, 0)
+	{
+	}
+
 protected:
 	SystemError(const struct type_info *type, const char *file, unsigned line);
 };
@@ -100,6 +106,12 @@ class SocketError: public SystemError {
 public:
 	SocketError(const char *file, unsigned line, const char *socketname,
 		    const char *format, ...);
+
+	SocketError()
+		:SystemError(&type_SocketError, NULL, 0)
+	{
+	}
+
 	virtual void raise()
 	{
 		throw this;
@@ -111,18 +123,36 @@ public:
 	OutOfMemory(const char *file, unsigned line,
 		    size_t amount, const char *allocator,
 		    const char *object);
+
+	OutOfMemory()
+		:SystemError(&type_OutOfMemory, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
 class TimedOut: public SystemError {
 public:
 	TimedOut(const char *file, unsigned line);
+
+	TimedOut()
+		:SystemError(&type_TimedOut, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
 class ChannelIsClosed: public Exception {
 public:
 	ChannelIsClosed(const char *file, unsigned line);
+
+	ChannelIsClosed()
+		:Exception(&type_ChannelIsClosed, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
@@ -133,6 +163,12 @@ public:
 class FiberIsCancelled: public Exception {
 public:
 	FiberIsCancelled(const char *file, unsigned line);
+
+	FiberIsCancelled()
+		:Exception(&type_FiberIsCancelled, NULL, 0)
+	{
+	}
+
 	virtual void log() const;
 	virtual void raise() { throw this; }
 };
@@ -141,12 +177,24 @@ class LuajitError: public Exception {
 public:
 	LuajitError(const char *file, unsigned line,
 		    const char *msg);
+
+	LuajitError()
+		:Exception(&type_LuajitError, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
 class IllegalParams: public Exception {
 public:
 	IllegalParams(const char *file, unsigned line, const char *format, ...);
+
+	IllegalParams()
+		:Exception(&type_IllegalParams, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
@@ -154,18 +202,36 @@ class CollationError: public Exception {
 public:
 	CollationError(const char *file, unsigned line, const char *format,
 		       ...);
+
+	CollationError()
+		:Exception(&type_CollationError, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
 class SwimError: public Exception {
 public:
 	SwimError(const char *file, unsigned line, const char *format, ...);
+
+	SwimError()
+		:Exception(&type_SwimError, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
 class CryptoError: public Exception {
 public:
 	CryptoError(const char *file, unsigned line, const char *format, ...);
+
+	CryptoError()
+		:Exception(&type_CryptoError, NULL, 0)
+	{
+	}
+
 	virtual void raise() { throw this; }
 };
 
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
index fe0cd49b9..5e68b34d2 100644
--- a/test/unit/mp_error.cc
+++ b/test/unit/mp_error.cc
@@ -34,9 +34,11 @@
 #include "memory.h"
 #include "msgpuck.h"
 #include "mp_extension_types.h"
+#include "random.h"
 #include "tt_static.h"
 #include "small/ibuf.h"
 #include "mpstream/mpstream.h"
+#include "tt_uuid.h"
 
 #include "box/error.h"
 #include "box/mp_error.h"
@@ -362,7 +364,7 @@ void
 test_fail_not_enough_fields()
 {
 	header();
-	plan(2);
+	plan(4);
 	char buffer[2048];
 	memset(buffer, 0, sizeof(buffer));
 
@@ -383,8 +385,12 @@ test_fail_not_enough_fields()
 	const char *pos = buffer;
 	struct error *unpacked = error_unpack(&pos, len);
 
-	is(unpacked, NULL, "check not enough additional fields");
-	ok(!diag_is_empty(diag_get()), "error about parsing problem is set");
+	isnt(unpacked, NULL, "check lack of fields");
+	is(strcmp(error_get_str(unpacked, "object_type"), err.ad_object_type),
+	   0, "object type");
+	is(strcmp(error_get_str(unpacked, "access_type"), err.ad_access_type),
+	   0, "access type");
+	is(error_get_str(unpacked, "object_name"), NULL, "object name");
 	check_plan();
 	footer();
 }
@@ -455,6 +461,63 @@ test_unknown_additional_fields()
 	footer();
 }
 
+static void
+test_payload(void)
+{
+	header();
+	plan(11);
+	char buffer[2048];
+	memset(buffer, 0, sizeof(buffer));
+
+	struct error *e = new ClientError();
+	error_ref(e);
+	e->code = 42;
+	e->saved_errno = 1;
+	error_format_msg(e, "msg");
+	error_set_location(e, "file", 2);
+	error_set_str(e, "key1", "1");
+	error_set_uint(e, "key2", 1);
+	error_set_int(e, "key3", -1);
+	error_set_double(e, "key4", 1.5);
+	error_set_bool(e, "key5", true);
+	struct tt_uuid uuid;
+	tt_uuid_create(&uuid);
+	error_set_uuid(e, "key6", &uuid);
+
+	mp_encode_error(buffer, e);
+	error_unref(e);
+
+	int8_t type;
+	const char *data = buffer;
+	mp_decode_extl(&data, &type);
+	e = error_unpack_unsafe(&data);
+	error_ref(e);
+
+	is(e->code, 42, "code");
+	is(e->saved_errno, 1, "errno");
+	is(strcmp(e->errmsg, "msg"), 0, "msg");
+	is(e->line, 2, "line");
+	is(strcmp(e->file, "file"), 0, "file");
+	is(strcmp(error_get_str(e, "key1"), "1"), 0, "key str");
+	uint64_t val_uint;
+	ok(error_get_uint(e, "key2", &val_uint) && val_uint == 1, "key uint");
+	int64_t val_int;
+	ok(error_get_int(e, "key3", &val_int) && val_int == -1, "key int");
+	double val_dbl;
+	ok(error_get_double(e, "key4", &val_dbl) && val_dbl == 1.5,
+	   "key double");
+	bool val_bool;
+	ok(error_get_bool(e, "key5", &val_bool) && val_bool, "key bool");
+	struct tt_uuid val_uuid;
+	ok(error_get_uuid(e, "key6", &val_uuid) &&
+	   tt_uuid_is_equal(&uuid, &val_uuid), "key uuid");
+
+	error_unref(e);
+
+	check_plan();
+	footer();
+}
+
 static int
 mp_fprint_ext_test(FILE *file, const char **data, int depth)
 {
@@ -723,7 +786,8 @@ int
 main(void)
 {
 	header();
-	plan(6);
+	plan(7);
+	random_init();
 	memory_init();
 	fiber_init(fiber_c_invoke);
 
@@ -732,10 +796,12 @@ main(void)
 	test_fail_not_enough_fields();
 	test_unknown_fields();
 	test_unknown_additional_fields();
+	test_payload();
 	test_mp_print();
 
 	fiber_free();
 	memory_free();
+	random_free();
 	footer();
 	return check_plan();
 }
diff --git a/test/unit/mp_error.result b/test/unit/mp_error.result
index 0582458d3..356d2dc97 100644
--- a/test/unit/mp_error.result
+++ b/test/unit/mp_error.result
@@ -1,5 +1,5 @@
 	*** main ***
-1..6
+1..7
 	*** test_stack_error_decode ***
     1..17
     ok 1 - check CustomError
@@ -27,9 +27,11 @@ ok 1 - subtests
 ok 2 - subtests
 	*** test_decode_unknown_type: done ***
 	*** test_fail_not_enough_fields ***
-    1..2
-    ok 1 - check not enough additional fields
-    ok 2 - error about parsing problem is set
+    1..4
+    ok 1 - check lack of fields
+    ok 2 - object type
+    ok 3 - access type
+    ok 4 - object name
 ok 3 - subtests
 	*** test_fail_not_enough_fields: done ***
 	*** test_unknown_fields ***
@@ -41,6 +43,21 @@ ok 4 - subtests
     ok 1 - check unknown additional field
 ok 5 - subtests
 	*** test_unknown_additional_fields: done ***
+	*** test_payload ***
+    1..11
+    ok 1 - code
+    ok 2 - errno
+    ok 3 - msg
+    ok 4 - line
+    ok 5 - file
+    ok 6 - key str
+    ok 7 - key uint
+    ok 8 - key int
+    ok 9 - key double
+    ok 10 - key bool
+    ok 11 - key uuid
+ok 6 - subtests
+	*** test_payload: done ***
 	*** test_mp_print ***
     1..60
     # zero depth, normal error
@@ -109,6 +126,6 @@ ok 5 - subtests
     ok 58 - mp_fprint depth 0 correct length
     ok 59 - mp_fprint depth 0 correct prefix and suffix
     ok 60 - mp_fprint depth 0 correct object in the middle
-ok 6 - subtests
+ok 7 - subtests
 	*** test_mp_print: done ***
 	*** main: done ***
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 07/11] error: use error_payload in Lua
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (7 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 06/11] error: use error_payload in MessagePack codecs Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 08/11] luatest: copy config in cluster:build_server() Vladislav Shpilevoy via Tarantool-patches
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

In Lua struct error used RTTI to return members of the error
depending on its type. If a field was added to error's payload, it
wasn't visible. The patch makes Lua use error_payload instead of
RTTI. Now if the payload gets a new field, it becomes
automatically visible in Lua without need to introduce a new
method for it.

Part of #5568
---
 extra/exports             |  3 +-
 src/box/error.cc          | 24 ++------------
 src/lib/core/diag.c       |  6 ++++
 src/lib/core/diag.h       |  3 ++
 src/lib/core/exception.cc |  8 +----
 src/lua/error.lua         | 69 +++++++++------------------------------
 src/lua/init.lua          | 24 --------------
 7 files changed, 29 insertions(+), 108 deletions(-)

diff --git a/extra/exports b/extra/exports
index 19e23f5db..4ca513680 100644
--- a/extra/exports
+++ b/extra/exports
@@ -151,12 +151,11 @@ csv_next
 csv_setopt
 decimal_from_string
 decimal_unpack
+error_find_field
 error_ref
 error_set_prev
 error_unpack_unsafe
 error_unref
-exception_get_int
-exception_get_string
 fiber_attr_delete
 fiber_attr_getstacksize
 fiber_attr_new
diff --git a/src/box/error.cc b/src/box/error.cc
index 3b5fa1c95..5097a076d 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -158,13 +158,8 @@ const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
 	"ERROR"
 };
 
-static struct method_info clienterror_methods[] = {
-	make_method(&type_ClientError, "code", &ClientError::errcode),
-	METHODS_SENTINEL
-};
-
 const struct type_info type_ClientError =
-	make_type("ClientError", &type_Exception, clienterror_methods);
+	make_type("ClientError", &type_Exception);
 
 ClientError::ClientError(const type_info *type, const char *file, unsigned line,
 			 uint32_t errcode)
@@ -269,16 +264,8 @@ BuildXlogGapError(const char *file, unsigned line,
 
 struct rlist on_access_denied = RLIST_HEAD_INITIALIZER(on_access_denied);
 
-static struct method_info accessdeniederror_methods[] = {
-	make_method(&type_AccessDeniedError, "access_type", &AccessDeniedError::access_type),
-	make_method(&type_AccessDeniedError, "object_type", &AccessDeniedError::object_type),
-	make_method(&type_AccessDeniedError, "object_name", &AccessDeniedError::object_name),
-	METHODS_SENTINEL
-};
-
 const struct type_info type_AccessDeniedError =
-	make_type("AccessDeniedError", &type_ClientError,
-		  accessdeniederror_methods);
+	make_type("AccessDeniedError", &type_ClientError);
 
 AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
 				     const char *access_type,
@@ -318,13 +305,8 @@ BuildAccessDeniedError(const char *file, unsigned int line,
 	}
 }
 
-static struct method_info customerror_methods[] = {
-	make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
-	METHODS_SENTINEL
-};
-
 const struct type_info type_CustomError =
-	make_type("CustomError", &type_ClientError, customerror_methods);
+	make_type("CustomError", &type_ClientError);
 
 CustomError::CustomError(const char *file, unsigned int line,
 			 const char *custom_type, uint32_t errcode)
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index b6fa1f5bb..b05f2793f 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -61,6 +61,12 @@ error_unref(struct error *e)
 	}
 }
 
+const struct error_field *
+error_find_field(const struct error *e, const char *name)
+{
+	return error_payload_find(&e->payload, name);
+}
+
 int
 error_set_prev(struct error *e, struct error *prev)
 {
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 933ba3ae3..be81689ce 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -208,6 +208,9 @@ error_move_payload(struct error *e, struct error_payload *src)
 	error_payload_move(&e->payload, src);
 }
 
+const struct error_field *
+error_find_field(const struct error *e, const char *name);
+
 /**
  * Unlink error from its effect. For instance:
  * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index 395baff6f..1895c50b8 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -85,13 +85,7 @@ exception_get_int(struct error *e, const struct method_info *method)
 static OutOfMemory out_of_memory(__FILE__, __LINE__,
 				 sizeof(OutOfMemory), "malloc", "exception");
 
-static const struct method_info exception_methods[] = {
-	make_method(&type_Exception, "message", &Exception::get_errmsg),
-	make_method(&type_Exception, "log", &Exception::log),
-	METHODS_SENTINEL
-};
-const struct type_info type_Exception = make_type("Exception", NULL,
-	exception_methods);
+const struct type_info type_Exception = make_type("Exception", NULL);
 
 void *
 Exception::operator new(size_t size)
diff --git a/src/lua/error.lua b/src/lua/error.lua
index fb48a9b18..cd27be97c 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -1,6 +1,10 @@
 -- error.lua (internal file)
 
 local ffi = require('ffi')
+local msgpack = require('msgpack')
+
+local mp_decode = msgpack.decode_unchecked
+
 ffi.cdef[[
 struct type_info;
 
@@ -41,11 +45,6 @@ struct error {
     struct error *_effect;
 };
 
-char *
-exception_get_string(struct error *e, const struct method_info *method);
-int
-exception_get_int(struct error *e, const struct method_info *method);
-
 int
 error_set_prev(struct error *e, struct error *prev);
 
@@ -57,45 +56,10 @@ error_ref(struct error *e);
 
 void
 error_unref(struct error *e);
-]]
-
-local REFLECTION_CACHE = {}
 
-local function reflection_enumerate(err)
-    local key = tostring(err._type)
-    local result = REFLECTION_CACHE[key]
-    if result ~= nil then
-        return result
-    end
-    result = {}
-    -- See type_foreach_method() in reflection.h
-    local t = err._type
-    while t ~= nil do
-        local m = t.methods
-        while m.name ~= nil do
-            result[ffi.string(m.name)] = m
-            m = m + 1
-        end
-        t = t.parent
-    end
-    REFLECTION_CACHE[key] = result
-    return result
-end
-
-local function reflection_get(err, method)
-    if method.nargs ~= 0 then
-        return nil -- NYI
-    end
-    if method.rtype == ffi.C.CTYPE_INT then
-        return tonumber(ffi.C.exception_get_int(err, method))
-    elseif method.rtype == ffi.C.CTYPE_CONST_CHAR_PTR then
-        local str = ffi.C.exception_get_string(err, method)
-        if str == nil then
-            return nil
-        end
-        return ffi.string(str)
-    end
-end
+const struct error_field *
+error_find_field(const struct error *e, const char *name);
+]]
 
 local function error_base_type(err)
     return ffi.string(err._type.name)
@@ -175,11 +139,11 @@ local function error_unpack(err)
     for key, getter in pairs(error_fields)  do
         result[key] = getter(err)
     end
-    for key, getter in pairs(reflection_enumerate(err)) do
-        local value = reflection_get(err, getter)
-        if value ~= nil then
-            result[key] = value
-        end
+    local payload = err._payload
+    local fields = payload._fields
+    for i = 0, payload._count - 1 do
+        local f = fields[i]
+        result[ffi.string(f._name)] = mp_decode(f._data)
     end
     return result
 end
@@ -216,12 +180,9 @@ local function error_index(err, key)
     if getter ~= nil then
         return getter(err)
     end
-    getter = reflection_enumerate(err)[key]
-    if getter ~= nil and getter.nargs == 0 then
-        local val = reflection_get(err, getter)
-        if val ~= nil then
-            return val
-        end
+    local f = ffi.C.error_find_field(err, key)
+    if f ~= nil then
+        return mp_decode(f._data)
     end
     return error_methods[key]
 end
diff --git a/src/lua/init.lua b/src/lua/init.lua
index 9e3c813c3..f808a5c32 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -3,37 +3,13 @@
 
 local ffi = require('ffi')
 ffi.cdef[[
-struct type_info;
 struct method_info;
 
-enum ctype {
-    CTYPE_VOID = 0,
-    CTYPE_INT,
-    CTYPE_CONST_CHAR_PTR
-};
-
 struct type_info {
     const char *name;
     const struct type_info *parent;
     const struct method_info *methods;
 };
-
-enum { METHOD_ARG_MAX = 8 };
-
-struct method_info {
-    const struct type_info *owner;
-    const char *name;
-    enum ctype rtype;
-    enum ctype atype[METHOD_ARG_MAX];
-    int nargs;
-    bool isconst;
-
-    union {
-        /* Add extra space to get proper struct size in C */
-        void *_spacer[2];
-    };
-};
-
 double
 tarantool_uptime(void);
 typedef int32_t pid_t;
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 08/11] luatest: copy config in cluster:build_server()
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (8 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 07/11] error: use error_payload in Lua Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 09/11] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

It takes box.cfg config as an argument. And changes the argument
by adding a new key 'command'. If the caller wants to pass the
same box.cfg or slightly modified to several build_server() calls,
it won't work - all options will be the same on all instances.

For example:

    local cfg = {...}
    cfg.replication = {url1}
    cluster:build_server(cfg)
    cfg.replication = {url2}
    cluster:build_server(cfg)

It will not work. Both servers will get the same 'command' and the
same 'replication'.
---
 test/luatest_helpers/cluster.lua | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/luatest_helpers/cluster.lua b/test/luatest_helpers/cluster.lua
index 01291b43c..43e3479f7 100644
--- a/test/luatest_helpers/cluster.lua
+++ b/test/luatest_helpers/cluster.lua
@@ -93,6 +93,7 @@ end
 
 function Cluster:build_server(server_config, instance_file)
     instance_file = instance_file or 'default.lua'
+    server_config = table.deepcopy(server_config)
     server_config.command = fio.pathjoin(root, 'test/instances/', instance_file)
     assert(server_config.alias, 'Either replicaset.alias or server.alias must be given')
     local server = Server:new(server_config)
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 09/11] luatest: add new helpers for 'server' object
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (9 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 08/11] luatest: copy config in cluster:build_server() Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-11 23:54 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-12 23:25 ` [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason Vladislav Shpilevoy via Tarantool-patches
  2021-11-16 22:08 ` [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:54 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

-- Wait until the instance becomes an elected leader.
server:wait_election_leader()

-- Wait until an election leader is found.
server:wait_election_leader_found()

-- Get numeric ID of the instance like in box.info.id.
server:instance_id()

-- Get UUID of the instance like in box.info.uuid.
server:instance_uuid()

These are going to be used in a new test in a next commit.
---
 test/luatest_helpers/server.lua | 66 +++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/test/luatest_helpers/server.lua b/test/luatest_helpers/server.lua
index b6d6f1400..900db2243 100644
--- a/test/luatest_helpers/server.lua
+++ b/test/luatest_helpers/server.lua
@@ -75,28 +75,45 @@ function Server:build_env()
     return res
 end
 
-function Server:wait_for_readiness()
-    local alias = self.alias
-    local id = self.id
-    local pid = self.process.pid
+local function wait_cond(cond_name, server, func, ...)
+    local alias = server.alias
+    local id = server.id
+    local pid = server.process.pid
 
     local deadline = clock.time() + WAIT_TIMEOUT
     while true do
-        local ok, is_ready = pcall(function()
-            self:connect_net_box()
-            return self.net_box:eval('return _G.ready') == true
-        end)
-        if ok and is_ready then
-            break
+        if func(...) then
+            return
         end
         if clock.time() > deadline then
-            error(('Starting of server %s-%s (PID %d) was timed out'):format(
-                alias, id, pid))
+            error(('Waiting for "%s" on server %s-%s (PID %d) timed out')
+                  :format(cond_name, alias, id, pid))
         end
         fiber.sleep(WAIT_DELAY)
     end
 end
 
+function Server:wait_for_readiness()
+    return wait_cond('readiness', self, function()
+        local ok, is_ready = pcall(function()
+            self:connect_net_box()
+            return self.net_box:eval('return _G.ready') == true
+        end)
+        return ok and is_ready
+    end)
+end
+
+function Server:wait_election_leader()
+    return wait_cond('election leader', self, self.exec, self, function()
+        return box.info.election.state == 'leader'
+    end)
+end
+
+function Server:wait_election_leader_found()
+    return wait_cond('election leader is found', self, self.exec, self,
+                     function() return box.info.election.leader ~= 0 end)
+end
+
 -- Unlike the original luatest.Server function it waits for
 -- starting the server.
 function Server:start(opts)
@@ -116,6 +133,29 @@ function Server:start(opts)
     end
 end
 
+function Server:instance_id()
+    -- Cache the value when found it first time.
+    if self.instance_id_value then
+        return self.instance_id_value
+    end
+    local id = self:exec(function() return box.info.id end)
+    -- But do not cache 0 - it is an anon instance, its ID might change.
+    if id ~= 0 then
+        self.instance_id_value = id
+    end
+    return id
+end
+
+function Server:instance_uuid()
+    -- Cache the value when found it first time.
+    if self.instance_uuid_value then
+        return self.instance_uuid_value
+    end
+    local uuid = self:exec(function() return box.info.uuid end)
+    self.instance_uuid_value = uuid
+    return uuid
+end
+
 -- TODO: Add the 'wait_for_readiness' parameter for the restart()
 -- method.
 
@@ -146,6 +186,8 @@ function Server:cleanup()
     for _, pattern in ipairs(DEFAULT_CHECKPOINT_PATTERNS) do
         fio.rmtree(('%s/%s'):format(self.workdir, pattern))
     end
+    self.instance_id_value = nil
+    self.instance_uuid_value = nil
 end
 
 function Server:drop()
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-12  7:30   ` Serge Petrenko via Tarantool-patches
  2021-11-12 23:24     ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 21+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-12  7:30 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches



12.11.2021 02:54, Vladislav Shpilevoy пишет:
> ER_READONLY used not to have any details about the exact reason
> why the instance is read-only. The patch changes that by adding
> new fields into the error which explain why the error happened and
> even help to avoid it for next requests.
>

Thanks for the changes!

Please, find two comments below.

Sorry for coming up late with this one, but I think it'd be good
to report ro reason in box.info. Maybe box.info.ro_reason or
something similar. Only when box.info.ro is true, of course.

Otherwise we help the user only partially. He sees what's wrong when 
receives
an error, but has to check every parameter manually when checks box.info.ro

> ---
>   .../unreleased/gh-5568-readonly-reason.md     |   4 +
>   src/box/box.cc                                |  54 +++-
>   .../gh_5568_read_only_reason_test.lua         | 285 ++++++++++++++++++
>   3 files changed, 335 insertions(+), 8 deletions(-)
>   create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
>   create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
>
> diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
> new file mode 100644
> index 000000000..f3a2db986
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5568-readonly-reason.md
> @@ -0,0 +1,4 @@
> +## feature/core
> +
> +* Error objects with the code `box.error.READONLY` now have additional fields
> +  explaining why the error happened (gh-5568).
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1ed1ce3f8..323982969 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -177,16 +177,54 @@ box_update_ro_summary(void)
>   static int
>   box_check_writable(void)
>   {
> -	if (is_ro_summary) {
> +	if (!is_ro_summary)
> +		return 0;
> +	struct error *e = diag_set(ClientError, ER_READONLY);
> +	struct raft *raft = box_raft();
> +	/*
> +	 * In case of multiple reasons at the same time only one is reported.
> +	 * But the order is important. For example, if the instance has election
> +	 * enabled, for the client it is better to see that it is a 'follower'
> +	 * and who is the leader than just see cfg 'read_only' is true.
> +	 */
> +	if (raft_is_ro(raft)) {
> +		error_set_str(e, "reason", "election");
> +		error_set_str(e, "state", raft_state_str(raft->state));
> +		error_set_uint(e, "term", raft->volatile_term);
> +		uint32_t id = raft->leader;
> +		if (id != REPLICA_ID_NIL) {
> +			error_set_uint(e, "leader_id", id);
> +			struct replica *r = replica_by_id(id);
> +			/*
> +			 * XXX: when the leader is dropped from _cluster, it
> +			 * is not reported to Raft.
> +			 */
> +			if (r != NULL)
> +				error_set_uuid(e, "leader_uuid", &r->uuid);
> +		}
> +	} else if (txn_limbo_is_ro(&txn_limbo)) {
> +		error_set_str(e, "reason", "synchro");
> +		uint32_t id = txn_limbo.owner_id;
> +		error_set_uint(e, "queue_owner_id", id);
> +		error_set_uint(e, "term", raft->volatile_term);

I just noticed, we should report txn_limbo_greatest_term here, probably.

This instance (which received ER_READONLY) is the leader, but hasn't claimed
the limbo yet.
This only makes sense when limbo term is behind raft's one.

> +		struct replica *r = replica_by_id(id);
>   		/*
> -		 * XXX: return a special error when the node is not a leader to
> -		 * reroute to the leader node.
> +		 * XXX: when an instance is deleted from _cluster, its limbo's
> +		 * ownership is not cleared.
>   		 */
> -		diag_set(ClientError, ER_READONLY);
> -		diag_log();
> -		return -1;
> -	}
> -	return 0;
> +		if (r != NULL)
> +			error_set_uuid(e, "queue_owner_uuid", &r->uuid);
> +	} else {
> +		error_set_str(e, "reason", "state");
> +		if (is_ro)
> +			error_set_str(e, "state", "read_only");
> +		else if (is_orphan)
> +			error_set_str(e, "state", "orphan");
> +		else
> +			assert(false);
> +	}
> +	diag_log();
> +	return -1;
>   }
>   
>   static void
>
--
Serge Petrenko


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-12  7:30   ` Serge Petrenko via Tarantool-patches
@ 2021-11-12 23:24     ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-15  6:51       ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-12 23:24 UTC (permalink / raw)
  To: Serge Petrenko, tarantool-patches

Hi! Thanks for the review!

> 12.11.2021 02:54, Vladislav Shpilevoy пишет:
>> ER_READONLY used not to have any details about the exact reason
>> why the instance is read-only. The patch changes that by adding
>> new fields into the error which explain why the error happened and
>> even help to avoid it for next requests.
>>
> 
> Thanks for the changes!
> 
> Please, find two comments below.
> 
> Sorry for coming up late with this one, but I think it'd be good
> to report ro reason in box.info. Maybe box.info.ro_reason or
> something similar. Only when box.info.ro is true, of course.
> 
> Otherwise we help the user only partially. He sees what's wrong when receives
> an error, but has to check every parameter manually when checks box.info.ro

Sounds useful, I added it in a new commit on top of the branch. See a new
email in the same thread.

>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 1ed1ce3f8..323982969 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -177,16 +177,54 @@ box_update_ro_summary(void)
>>   static int
>>   box_check_writable(void)
>>   {
>> -    if (is_ro_summary) {
>> +    if (!is_ro_summary)
>> +        return 0;
>> +    struct error *e = diag_set(ClientError, ER_READONLY);
>> +    struct raft *raft = box_raft();
>> +    /*
>> +     * In case of multiple reasons at the same time only one is reported.
>> +     * But the order is important. For example, if the instance has election
>> +     * enabled, for the client it is better to see that it is a 'follower'
>> +     * and who is the leader than just see cfg 'read_only' is true.
>> +     */
>> +    if (raft_is_ro(raft)) {
>> +        error_set_str(e, "reason", "election");
>> +        error_set_str(e, "state", raft_state_str(raft->state));
>> +        error_set_uint(e, "term", raft->volatile_term);
>> +        uint32_t id = raft->leader;
>> +        if (id != REPLICA_ID_NIL) {
>> +            error_set_uint(e, "leader_id", id);
>> +            struct replica *r = replica_by_id(id);
>> +            /*
>> +             * XXX: when the leader is dropped from _cluster, it
>> +             * is not reported to Raft.
>> +             */
>> +            if (r != NULL)
>> +                error_set_uuid(e, "leader_uuid", &r->uuid);
>> +        }
>> +    } else if (txn_limbo_is_ro(&txn_limbo)) {
>> +        error_set_str(e, "reason", "synchro");
>> +        uint32_t id = txn_limbo.owner_id;
>> +        error_set_uint(e, "queue_owner_id", id);
>> +        error_set_uint(e, "term", raft->volatile_term);
> 
> I just noticed, we should report txn_limbo_greatest_term here, probably.
> 
> This instance (which received ER_READONLY) is the leader, but hasn't claimed
> the limbo yet.
> This only makes sense when limbo term is behind raft's one.

Hmm, you are probably right. Thanks for noticing! Fixed in the
previous commit.

====================
@@ -214,7 +214,7 @@ box_check_writable(void)
 	} else if (txn_limbo_is_ro(&txn_limbo)) {
 		error_set_str(e, "reason", "synchro");
 		uint32_t id = txn_limbo.owner_id;
-		uint64_t term = raft->volatile_term;
+		uint64_t term = txn_limbo.promote_greatest_term;
 		error_set_uint(e, "queue_owner_id", id);
 		error_set_uint(e, "term", term);
====================

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (10 preceding siblings ...)
  2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 09/11] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-12 23:25 ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-15  6:53   ` Serge Petrenko via Tarantool-patches
  2021-11-16 22:08 ` [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
  12 siblings, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-12 23:25 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

    Follow-up #5568
    
    @TarantoolBot document
    Title: box.info.ro_reason
    
    The new `box.info` field - `ro_reason` - is `nil` on a writable
    instance. On a read-only instance `box.info.ro == true` it reports
    an error reason. Currently the list is
    - `'election'` - `box.cfg.election_mode` is not `'off'` and this
      instance is not a leader. See `box.info.election` for details.
    - `'synchro'` - the synchro queue is owned by some other instance.
      For details see `box.info.synchro`.
    - `'config'` - `box.cfg.read_only` is true;
    - `'orphan'` - the instance is in orphan state.
---
diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
index f3a2db986..d2985eede 100644
--- a/changelogs/unreleased/gh-5568-readonly-reason.md
+++ b/changelogs/unreleased/gh-5568-readonly-reason.md
@@ -1,4 +1,6 @@
 ## feature/core
 
 * Error objects with the code `box.error.READONLY` now have additional fields
-  explaining why the error happened (gh-5568).
+  explaining why the error happened.
+  Also there is a new field `box.info.ro_reason`. It is `nil` on a writable
+  instance, but reports a reason when `box.info.ro` is true (gh-5568).
diff --git a/src/box/box.cc b/src/box/box.cc
index f776c82c1..c3bdff2f5 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -174,6 +174,20 @@ box_update_ro_summary(void)
 	fiber_cond_broadcast(&ro_cond);
 }
 
+const char *
+box_ro_reason(void)
+{
+	if (raft_is_ro(box_raft()))
+		return "election";
+	if (txn_limbo_is_ro(&txn_limbo))
+		return "synchro";
+	if (is_ro)
+		return "config";
+	if (is_orphan)
+		return "orphan";
+	assert(false);
+}
+
 static int
 box_check_writable(void)
 {
diff --git a/src/box/box.h b/src/box/box.h
index 14278a294..83fdba332 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -152,6 +152,13 @@ box_do_set_orphan(bool orphan);
 void
 box_update_ro_summary(void);
 
+/**
+ * Get the reason why the instance is read only if it is. Can't be called on a
+ * writable instance.
+ */
+const char *
+box_ro_reason(void);
+
 /**
  * Iterate over all spaces and save them to the
  * snapshot file.
diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index 6e13041b2..cc0790077 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -321,6 +321,16 @@ lbox_info_ro(struct lua_State *L)
 	return 1;
 }
 
+static int
+lbox_info_ro_reason(struct lua_State *L)
+{
+	if (box_is_ro())
+		lua_pushstring(L, box_ro_reason());
+	else
+		lua_pushnil(L);
+	return 1;
+}
+
 /*
  * Tarantool 1.6.x compat
  */
@@ -635,6 +645,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"signature", lbox_info_signature},
 	{"vclock", lbox_info_vclock},
 	{"ro", lbox_info_ro},
+	{"ro_reason", lbox_info_ro_reason},
 	{"replication", lbox_info_replication},
 	{"replication_anon", lbox_info_replication_anon},
 	{"status", lbox_info_status},
diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
index 5e67029c0..13682cbba 100644
--- a/test/replication-luatest/gh_5568_read_only_reason_test.lua
+++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
@@ -53,6 +53,10 @@ local read_only_msg = "Can't modify data on a read-only instance - "
 -- Read-only because of box.cfg{read_only = true}.
 --
 g.test_read_only_reason_cfg = function(g)
+    t.assert_equals(g.master:exec(function()
+        return box.info.ro_reason
+    end), nil, "no ro reason");
+
     local ok, err = g.master:exec(function()
         box.cfg{read_only = true}
         local ok, err = pcall(box.schema.create_space, 'test')
@@ -61,6 +65,9 @@ g.test_read_only_reason_cfg = function(g)
     t.assert(not ok, 'fail ddl')
     t.assert_str_contains(err.message, read_only_msg..
                           'box.cfg.read_only is true')
+    t.assert_equals(g.master:exec(function()
+        return box.info.ro_reason
+    end), "config", "ro reason config");
     t.assert_covers(err, {
         reason = 'state',
         state = 'read_only',
@@ -93,6 +100,9 @@ g.test_read_only_reason_orphan = function(g)
     end, {fake_uri})
     t.assert(not ok, 'fail ddl')
     t.assert_str_contains(err.message, read_only_msg..'it is an orphan')
+    t.assert_equals(g.master:exec(function()
+        return box.info.ro_reason
+    end), "orphan", "ro reason orphan");
     t.assert_covers(err, {
         reason = 'state',
         state = 'orphan',
@@ -124,6 +134,9 @@ g.test_read_only_reason_election_no_leader = function(g)
     t.assert(err.term, 'has term')
     t.assert_str_contains(err.message, read_only_msg..('state is election '..
                           '%s with term %s'):format(err.state, err.term))
+    t.assert_equals(g.master:exec(function()
+        return box.info.ro_reason
+    end), "election", "ro reason election");
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -158,6 +171,9 @@ g.test_read_only_reason_election_has_leader = function(g)
     t.assert_str_contains(err.message, read_only_msg..('state is election '..
                           '%s with term %s, leader is %s (%s)'):format(
                           err.state, err.term, err.leader_id, err.leader_uuid))
+    t.assert_equals(g.replica:exec(function()
+        return box.info.ro_reason
+    end), "election", "ro reason election");
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -200,6 +216,9 @@ g.test_read_only_reason_synchro = function(g)
     t.assert_str_contains(err.message, read_only_msg..('synchro queue with '..
                           'term %s belongs to %s (%s)'):format(err.term,
                           err.queue_owner_id, err.queue_owner_uuid))
+    t.assert_equals(g.replica:exec(function()
+        return box.info.ro_reason
+    end), "synchro", "ro reason synchro");
     t.assert_covers(err, {
         reason = 'synchro',
         queue_owner_id = g.master:instance_id(),
@@ -258,6 +277,9 @@ g.test_read_only_reason_election_has_leader_no_uuid = function(g)
     t.assert_str_contains(err.message, read_only_msg..('state is election %s '..
                           'with term %s, leader is %s'):format(err.state,
                           err.term, err.leader_id))
+    t.assert_equals(g.replica:exec(function()
+        return box.info.ro_reason
+    end), "election", "ro reason election");
     t.assert_covers(err, {
         reason = 'election',
         state = 'follower',
@@ -295,6 +317,9 @@ g.test_read_only_reason_synchro_no_uuid = function(g)
     t.assert_str_contains(err.message, read_only_msg..('synchro queue with '..
                           'term %s belongs to %s'):format(err.term,
                           err.queue_owner_id))
+    t.assert_equals(g.replica:exec(function()
+        return box.info.ro_reason
+    end), "synchro", "ro reason synchro");
     t.assert_covers(err, {
         reason = 'synchro',
         queue_owner_id = leader_id,

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-12 23:24     ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-15  6:51       ` Serge Petrenko via Tarantool-patches
  2021-11-15 21:56         ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 21+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-15  6:51 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches



13.11.2021 02:24, Vladislav Shpilevoy пишет:
> Hi! Thanks for the review!
>
>> 12.11.2021 02:54, Vladislav Shpilevoy пишет:
>>> ER_READONLY used not to have any details about the exact reason
>>> why the instance is read-only. The patch changes that by adding
>>> new fields into the error which explain why the error happened and
>>> even help to avoid it for next requests.
>>>
>> Thanks for the changes!
>>
>> Please, find two comments below.
>>
>> Sorry for coming up late with this one, but I think it'd be good
>> to report ro reason in box.info. Maybe box.info.ro_reason or
>> something similar. Only when box.info.ro is true, of course.
>>
>> Otherwise we help the user only partially. He sees what's wrong when receives
>> an error, but has to check every parameter manually when checks box.info.ro
> Sounds useful, I added it in a new commit on top of the branch. See a new
> email in the same thread.
>
>>> diff --git a/src/box/box.cc b/src/box/box.cc
>>> index 1ed1ce3f8..323982969 100644
>>> --- a/src/box/box.cc
>>> +++ b/src/box/box.cc
>>> @@ -177,16 +177,54 @@ box_update_ro_summary(void)
>>>    static int
>>>    box_check_writable(void)
>>>    {
>>> -    if (is_ro_summary) {
>>> +    if (!is_ro_summary)
>>> +        return 0;
>>> +    struct error *e = diag_set(ClientError, ER_READONLY);
>>> +    struct raft *raft = box_raft();
>>> +    /*
>>> +     * In case of multiple reasons at the same time only one is reported.
>>> +     * But the order is important. For example, if the instance has election
>>> +     * enabled, for the client it is better to see that it is a 'follower'
>>> +     * and who is the leader than just see cfg 'read_only' is true.
>>> +     */
>>> +    if (raft_is_ro(raft)) {
>>> +        error_set_str(e, "reason", "election");
>>> +        error_set_str(e, "state", raft_state_str(raft->state));
>>> +        error_set_uint(e, "term", raft->volatile_term);
>>> +        uint32_t id = raft->leader;
>>> +        if (id != REPLICA_ID_NIL) {
>>> +            error_set_uint(e, "leader_id", id);
>>> +            struct replica *r = replica_by_id(id);
>>> +            /*
>>> +             * XXX: when the leader is dropped from _cluster, it
>>> +             * is not reported to Raft.
>>> +             */
>>> +            if (r != NULL)
>>> +                error_set_uuid(e, "leader_uuid", &r->uuid);
>>> +        }
>>> +    } else if (txn_limbo_is_ro(&txn_limbo)) {
>>> +        error_set_str(e, "reason", "synchro");
>>> +        uint32_t id = txn_limbo.owner_id;
>>> +        error_set_uint(e, "queue_owner_id", id);
>>> +        error_set_uint(e, "term", raft->volatile_term);
>> I just noticed, we should report txn_limbo_greatest_term here, probably.
>>
>> This instance (which received ER_READONLY) is the leader, but hasn't claimed
>> the limbo yet.
>> This only makes sense when limbo term is behind raft's one.
> Hmm, you are probably right. Thanks for noticing! Fixed in the
> previous commit.
>
> ====================
> @@ -214,7 +214,7 @@ box_check_writable(void)
>   	} else if (txn_limbo_is_ro(&txn_limbo)) {
>   		error_set_str(e, "reason", "synchro");
>   		uint32_t id = txn_limbo.owner_id;
> -		uint64_t term = raft->volatile_term;
> +		uint64_t term = txn_limbo.promote_greatest_term;
>   		error_set_uint(e, "queue_owner_id", id);
>   		error_set_uint(e, "term", term);
> ====================

Thanks for the changes!

One final note:
I propose to move box_ro_reason() from the last commit here and use it
when setting error reason, so that error.reason would be the same as
box.info.ro_reason.


LGTM otherwise.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason
  2021-11-12 23:25 ` [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-15  6:53   ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 21+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-15  6:53 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches

13.11.2021 02:25, Vladislav Shpilevoy пишет:
>      Follow-up #5568
>      
>      @TarantoolBot document
>      Title: box.info.ro_reason
>      
>      The new `box.info` field - `ro_reason` - is `nil` on a writable
>      instance. On a read-only instance `box.info.ro == true` it reports
>      an error reason. Currently the list is
>      - `'election'` - `box.cfg.election_mode` is not `'off'` and this
>        instance is not a leader. See `box.info.election` for details.
>      - `'synchro'` - the synchro queue is owned by some other instance.
>        For details see `box.info.synchro`.
>      - `'config'` - `box.cfg.read_only` is true;
>      - `'orphan'` - the instance is in orphan state.
> ---

Thanks for the patch!
LGTM.

> diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
> index f3a2db986..d2985eede 100644
> --- a/changelogs/unreleased/gh-5568-readonly-reason.md
> +++ b/changelogs/unreleased/gh-5568-readonly-reason.md
...

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-15  6:51       ` Serge Petrenko via Tarantool-patches
@ 2021-11-15 21:56         ` Vladislav Shpilevoy via Tarantool-patches
  2021-11-16  9:53           ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-15 21:56 UTC (permalink / raw)
  To: Serge Petrenko, tarantool-patches

> Thanks for the changes!
> 
> One final note:
> I propose to move box_ro_reason() from the last commit here and use it
> when setting error reason, so that error.reason would be the same as
> box.info.ro_reason.

I added more details to error message than to ro_reason because when you
see ro_reason, you can go and check other fields of box.info for details.
When you have only an error object, and what is worse - it is cast to a
string during packing, you don't have a place to look up the details
already. Thus I wanted the error object to be self-sufficient. Even if
it degrades to a simple string during marshaling.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-15 21:56         ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-16  9:53           ` Serge Petrenko via Tarantool-patches
  2021-11-16 22:08             ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 21+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-16  9:53 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches



16.11.2021 00:56, Vladislav Shpilevoy пишет:
>> Thanks for the changes!
>>
>> One final note:
>> I propose to move box_ro_reason() from the last commit here and use it
>> when setting error reason, so that error.reason would be the same as
>> box.info.ro_reason.
> I added more details to error message than to ro_reason because when you
> see ro_reason, you can go and check other fields of box.info for details.
> When you have only an error object, and what is worse - it is cast to a
> string during packing, you don't have a place to look up the details
> already. Thus I wanted the error object to be self-sufficient. Even if
> it degrades to a simple string during marshaling.
I suppose you misunderstood me. I don't want to remove anything from the
error object or the message, I just want to unify error.reason and 
ro_reason.

There are 4 error reasons now, and error_object.reason / box.info.ro_reason
vary in their representation:
There are elections and reasons "election" and "election",
There is limbo being someone else's and the reasons being "synchro" and 
"synchro"

And then there are `is_ro` and `is_orphan`, both having error.reason == 
"state".
But box.cfg.ro_reason differs for them. It's "config" for `is_ro` and 
"orphan" for `is_orphan`.

My proposal was to make error.reason match ro_reason (reuse 
box_ro_reason() when setting
error.reason).

Feel free to ignore this comment.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details
  2021-11-16  9:53           ` Serge Petrenko via Tarantool-patches
@ 2021-11-16 22:08             ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-16 22:08 UTC (permalink / raw)
  To: Serge Petrenko, tarantool-patches

>>> One final note:
>>> I propose to move box_ro_reason() from the last commit here and use it
>>> when setting error reason, so that error.reason would be the same as
>>> box.info.ro_reason.
>> I added more details to error message than to ro_reason because when you
>> see ro_reason, you can go and check other fields of box.info for details.
>> When you have only an error object, and what is worse - it is cast to a
>> string during packing, you don't have a place to look up the details
>> already. Thus I wanted the error object to be self-sufficient. Even if
>> it degrades to a simple string during marshaling.
> I suppose you misunderstood me. I don't want to remove anything from the
> error object or the message, I just want to unify error.reason and ro_reason.
> 
> There are 4 error reasons now, and error_object.reason / box.info.ro_reason
> vary in their representation:
> There are elections and reasons "election" and "election",
> There is limbo being someone else's and the reasons being "synchro" and "synchro"
> 
> And then there are `is_ro` and `is_orphan`, both having error.reason == "state".
> But box.cfg.ro_reason differs for them. It's "config" for `is_ro` and "orphan" for `is_orphan`.
> 
> My proposal was to make error.reason match ro_reason (reuse box_ro_reason() when setting
> error.reason).
> 
> Feel free to ignore this comment.
I like the idea. I applied your suggestion. Now error.reason can be 'config' and
'orphan'.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason
  2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
                   ` (11 preceding siblings ...)
  2021-11-12 23:25 ` [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-16 22:08 ` Vladislav Shpilevoy via Tarantool-patches
  12 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-16 22:08 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko

Pushed to master.

On 12.11.2021 00:54, Vladislav Shpilevoy via Tarantool-patches wrote:
> Changes in v2:
> - A lot of minor fixes in all the commits;
> - libuuid is moved into libcore;
> - ER_READONLY message got the same details as in the fields.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5568-err-readonly-reason
> Issue: https://github.com/tarantool/tarantool/issues/5568
> 
> Vladislav Shpilevoy (11):
>   diag: return created error from diag_set()
>   uuid: move into libcore
>   error: introduce error_payload
>   error: move code to struct error from ClientError
>   error: use error_payload to store optional members
>   error: use error_payload in MessagePack codecs
>   error: use error_payload in Lua
>   luatest: copy config in cluster:build_server()
>   luatest: add new helpers for 'server' object
>   box: enrich ER_READONLY with new details
>   error: report ER_READONLY reason in message
> 
>  .../unreleased/gh-5568-readonly-reason.md     |   4 +
>  extra/exports                                 |   3 +-
>  src/CMakeLists.txt                            |   3 +-
>  src/box/applier.h                             |   2 +-
>  src/box/bind.h                                |   2 +-
>  src/box/box.cc                                |  69 ++-
>  src/box/errcode.h                             |   2 +-
>  src/box/error.cc                              |  57 +--
>  src/box/error.h                               |  60 +--
>  src/box/field_def.c                           |   4 +-
>  src/box/index.cc                              |   4 +-
>  src/box/lua/serialize_lua.c                   |   2 +-
>  src/box/mp_error.cc                           | 197 +++-----
>  src/box/msgpack.c                             |   2 +-
>  src/box/replication.h                         |   2 +-
>  src/box/sql/mem.c                             |   2 +-
>  src/box/sql/mem.h                             |   2 +-
>  src/box/tuple.h                               |   2 +-
>  src/box/tuple_compare.cc                      |   8 +-
>  src/box/xlog.h                                |   2 +-
>  src/box/xrow.h                                |   2 +-
>  src/lib/CMakeLists.txt                        |   1 -
>  src/lib/core/CMakeLists.txt                   |   3 +
>  src/lib/core/crash.c                          |   2 +-
>  src/lib/core/diag.c                           |  37 +-
>  src/lib/core/diag.h                           | 109 +++-
>  src/lib/core/error_payload.c                  | 301 +++++++++++
>  src/lib/core/error_payload.h                  | 190 +++++++
>  src/lib/core/exception.cc                     |   8 +-
>  src/lib/core/exception.h                      |  66 +++
>  src/lib/{uuid => core}/mp_uuid.c              |   0
>  src/lib/{uuid => core}/mp_uuid.h              |   0
>  src/lib/{uuid => core}/tt_uuid.c              |   0
>  src/lib/{uuid => core}/tt_uuid.h              |   0
>  src/lib/mpstream/CMakeLists.txt               |   2 +-
>  src/lib/mpstream/mpstream.c                   |   2 +-
>  src/lib/swim/CMakeLists.txt                   |   2 +-
>  src/lib/swim/swim_io.h                        |   2 +-
>  src/lib/swim/swim_proto.h                     |   2 +-
>  src/lib/uuid/CMakeLists.txt                   |   2 -
>  src/lua/error.lua                             |  84 ++--
>  src/lua/init.lua                              |  24 -
>  src/lua/msgpack.c                             |   6 +-
>  src/lua/tnt_msgpuck.h                         |   2 +-
>  src/lua/utils.c                               |   2 +-
>  test/box/error.result                         |   4 +-
>  test/box/error.test.lua                       |   2 +-
>  test/engine/func_index.result                 |   3 +-
>  test/luatest_helpers/cluster.lua              |   1 +
>  test/luatest_helpers/server.lua               |  66 ++-
>  .../gh_5568_read_only_reason_test.lua         | 304 ++++++++++++
>  test/replication/anon.result                  |   6 +-
>  test/replication/catch.result                 |   4 +-
>  test/replication/election_qsync.result        |  11 +-
>  test/replication/election_qsync.test.lua      |   4 +-
>  .../gh-6034-qsync-limbo-ownership.result      |  22 +-
>  .../gh-6034-qsync-limbo-ownership.test.lua    |   8 +-
>  test/sql-tap/CMakeLists.txt                   |   2 +-
>  test/sql-tap/gh-6024-funcs-return-bin.c       |   2 +-
>  test/sql-tap/sql_uuid.c                       |   2 +-
>  test/unit/CMakeLists.txt                      |   4 +-
>  test/unit/error.c                             | 466 ++++++++++++++++++
>  test/unit/error.result                        | 161 ++++++
>  test/unit/mp_error.cc                         |  76 ++-
>  test/unit/mp_error.result                     |  27 +-
>  test/unit/swim_proto.c                        |   2 +-
>  test/unit/swim_test_utils.c                   |   2 +-
>  test/unit/swim_test_utils.h                   |   2 +-
>  test/unit/uuid.c                              |   4 +-
>  test/unit/vy_iterators_helper.c               |   2 +-
>  test/unit/xrow.cc                             |   2 +-
>  test/vinyl/misc.result                        |   2 +-
>  third_party/lua-cjson/lua_cjson.c             |   2 +-
>  third_party/lua-yaml/lyaml.cc                 |   2 +-
>  74 files changed, 2069 insertions(+), 404 deletions(-)
>  create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
>  create mode 100644 src/lib/core/error_payload.c
>  create mode 100644 src/lib/core/error_payload.h
>  rename src/lib/{uuid => core}/mp_uuid.c (100%)
>  rename src/lib/{uuid => core}/mp_uuid.h (100%)
>  rename src/lib/{uuid => core}/tt_uuid.c (100%)
>  rename src/lib/{uuid => core}/tt_uuid.h (100%)
>  delete mode 100644 src/lib/uuid/CMakeLists.txt
>  create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
>  create mode 100644 test/unit/error.c
>  create mode 100644 test/unit/error.result
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-11-16 22:09 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 23:54 [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 01/11] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
2021-11-12  7:30   ` Serge Petrenko via Tarantool-patches
2021-11-12 23:24     ` Vladislav Shpilevoy via Tarantool-patches
2021-11-15  6:51       ` Serge Petrenko via Tarantool-patches
2021-11-15 21:56         ` Vladislav Shpilevoy via Tarantool-patches
2021-11-16  9:53           ` Serge Petrenko via Tarantool-patches
2021-11-16 22:08             ` Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 11/11] error: report ER_READONLY reason in message Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 02/11] uuid: move into libcore Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 03/11] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 04/11] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 05/11] error: use error_payload to store optional members Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 06/11] error: use error_payload in MessagePack codecs Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 07/11] error: use error_payload in Lua Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 08/11] luatest: copy config in cluster:build_server() Vladislav Shpilevoy via Tarantool-patches
2021-11-11 23:54 ` [Tarantool-patches] [PATCH v2 09/11] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
2021-11-12 23:25 ` [Tarantool-patches] [PATCH v2 12/11] error: introduce box.info.ro_reason Vladislav Shpilevoy via Tarantool-patches
2021-11-15  6:53   ` Serge Petrenko via Tarantool-patches
2021-11-16 22:08 ` [Tarantool-patches] [PATCH v2 00/11] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox