Re: [Tarantool-patches] [PATCH 0/6] Extending error functionality

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for summarization!

I'll add my two coins.

CCed all participants.

WBR, Alexander Turenko.

On Sat, Mar 28, 2020 at 02:11:46AM +0300, lvasiliev wrote:
> Summary of discussion with Alexander Turenko, Vlad, Mons, Sergos and Kirill
> Yukhin:
> 1) Session parameters
> Move negotiations to UPDATE (as for _vsession_settings).

To be clear: use UPDATE of _vsession_settings instead of the negotiation
protocol. It is less intruisive. Also it looks logical to have all
session settings in _vsession_settings.

> As further optimization session settings can be moved to auth message for to
> remove an additional RTT

However I don't sure whether it is really worth, because we can send two
packets in a batch (send the second w/o waiting for answer of the first
one). A client should handle four cases: {success, failure} x {success,
failure} in the case: it does not look difficult.

> 
> 2) Backtrace
> Leave adding to the error only Lua backtrace (which is added to the error at
> luaT_pusherror()).
> We must have a per server option for enable/disable adding backtrace.
> In addition a bactrace enable/disable adding can be forced by the creation
> function parametr.

Ok.

> The backtrace should be combined with the message if tostring is
> used.(@knazarov )

We didn't discuss it on our meeting and, to be honest, I unsure here.

AFAIU, Kostya want to see a backtrace in logs when an error is not
catched anywhere (and maybe in some other cases?). We should ask for
expected behaviour rather that for certain implementation.

We have __tostring(), which is used when casting to a string explicitly
or implicitly via concatenation. I would expect just error message here.

We have __serialize() for repl. It is logical to have backtrace and
other fields here.

Note: __serialize() returns an error message and on the first glance it
seems that we should keep it here for the old way marshaling over the
binary protocol. However, no, we'll handle errors in serializers
explicitly and will not use __serialize().

What method is in use when an error is written to logs (when not
catched, when diag_log() is used, when written by log.info() from Lua)?
I think it should be __serialize(), but it is possible that it is not so
now.

Hm. Maybe Kostya is about case like:

 | local res, err = my_func()
 | if res == nil then
 |     log.info('My message, reason: ' .. err)
 |     return
 | end

What we'll propose for this case? Something like so?

 | log.info('My message, reason: ' .. yaml.encode(err))

Need to think.

> Notes:
> To add a C bt is possible when error is created (for example) but unwind the
> Lua stack in diag.c is difficult (reason: the lua functions like
> lua_getstack, lua_getinfo are unavailable where)

To be clear: C stack trace itself maybe not so valuabe, when it is not
intermixed with Lua frames information. The case is using of a trigger.
So we agreed to not add them, at least for now.

> 
> 3) About returning(return box.error.new(...)) an error through IPROTO
> Now the error is serialized to string for marshaling through connection,
> which makes it difficult to add various error parameters.
> In the first iteration format was changed to table. But after discussion was
> decided to add a new msgpack type for the errors transmitting(by analogy
> with MP_DECIMAL)

Note: The new mp_exp subtype should have extensible format. I propose to
keep key-value pairs like in mp_map or even contain mp_map inside. Keys
should be number constants, not strings (to shrink packet size).

> An intermediate type mp_error will be introduced for this purposes (reason:
> it is impossible to  create an object of the ClientError class (or some
> other classes) at the time of decoding)

Seems ok.

However I would note that rewriting all error into C is also possible
way to solve this.

> Due to the need to use different serialization methods, depending on the
> session settings, the serializator context (containing fickle options) was
> added.

Seems ok. Maybe.

However I still have some doubt about dividing options to 'rarely
changed' and 'often changed'. It looks highly dependent of use case and
does not looks as property of an option.

And it seems that it is necessary only to save copying of ~60-80 bytes
of `struct serializer` for each serialization process.

Should not this context allow to store any serializer option? Or maybe
we should provide some common way to speed up options 'copying'. Or
maybe this <100 bytes copy is not so heavy?

Dividing options to rarely/often changed feels as not general solution
of the problem for me and that is why I complain.

Maybe we can use caching here? Let's calculate hashsum of per-call
serializer options and store full set of serializer options in a
hashmap. When global options are changed, this hashmap should be flushed
or updated.

This looks as general solution: it works for any option, it speeds up
(at least in theory) per-call options usage.

TL;DR: Idea is to cache full serializer options using per-call
serializer options (subset of all options).

> 
> 4) Payload
> For storage the payload at struct error will be added ptr to data with data
> size.
> For using a payload from Lua set/get_payload (worked with table as
> argument/return type) will be used. It is possible to use msgpack encode
> when set and decode on get, but not is rationally.
> To reduce overhead is offered to save a link to the table and use it as
> response output.
> For these purposes is proposed to use inheritance from the struct error in
> the C style with adding additional attributes like ptr to the Lua table. In
> the case of a marshaling through net the payload will be encoded/decoded
> with msgpack using.

Note: We should move all additional fields from error C++ classes to
struct error (use union) and remove all C++ virtual methods from those
classes. After this we can add more fields to a `struct error` child w/o
padding.

Note: We should return a pointer to base (`struct error`) to Lua to keep
cdata type being `struct error&`. It seems that iserror() is not
documented and users likely use ffi.istype().

We agreed on serializing Lua payload into msgpack at encoding of an
error (rather than doing it at an error creation). A table of virtual
methods should be added so to serialize base error / lua error into
msgpack (like it is done in `struct port`).

The reason to postpone serialization of payload to an error
serialization is that we don't necessarily will serialize an error at
all: maybe it'll be processed within a tarantool instance.

WBR, Alexander Turenko.