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

[Alexander Turenko]

On Thu, Aug 08, 2019 at 10:46:29PM +0200, Vladislav Shpilevoy wrote:
> > I had the objection against a list of errors in the diagnostic area: a
> > user may want to save an error, perform some new commands, which can
> > rewrite a last diagnostic (replace the diagnostic list) and then (s)he
> > may want to look at the stack of reasons of the saved error. But now it
> > seems you discarded the idea. (BTW, it would be good to briefly mention
> > changes you made for a new revision of your document: the text looks
> > similar and it is hard to find differences.)
> 
> Not sure if I understood the problem. What is wrong the list of errors?
> And how does it correlate with an error accidental removal? If I got
> the problem right, it exists even now, and does not depend on structure
> of an error. Even errno has that problem, and the only way to solve it -
> save the error to a local variable, perform needed preparations, and look
> at the saved value. You can do it now. Just save 'box.error.last()', do
> your stuff, and deal with the saved value.

I meant a difference between the approach when struct error does not
have a pointer to its reason (but a diagnostics area is expanded to
store a list) and the approach when struct error stores a pointer to a
reason. The former requires to copy the entire list to push it to Lua
(or save to in C to handle later), the latter requires just save a
pointer (and implement proper reference counting). Anyway, it seems we
stick now with the latter way and all agreed that this is better. Hope
we now understood each other.

> 
> > 
> > I think that struct error should have a pointer to its cause, which is
> > another error.
> 
> But it does exactly this now. It is called 'reason', which IMO is
> easier to understand. My first association with 'cause' is an informal
> writing of 'because', and it confuses a bit.

Maybe it is a kind of my baby duck syndrome. I understood wrap/unwrap
now, but was a bit confused when meet those terms at the first time. I
have no strong objections against this terms. Want to collect more
thoughts from the developers and maintainers.

> 
> > I like Java terminology here: cause is exactly what we want to save for
> > an error. Wrap / unwrap (Go terminology) look too abstract and maybe
> > even unclear for me. Maybe this terms appears from merry / xerror
> > packages, which were inspired to wrap built-in errors with additional
> > information (say, stack traces). It is up to maintainers, but my vote is
> > for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
> > some prefix, because it is C).
> 
> Strangely, but I understood wrap/unwrap easily. So I vote to keep them.
> But if there are more people who doesn't understand, then here is my
> humble proposal:
> 
>     C API:
>     box_error_reason(const struct error *) // To get a reason error.
>     box_error_set_reason(struct error *, struct error *) // To set a reason.
> 
>     Lua API:
>     error:reason() -- Get a reason error.
>     error:set_reason(reason) -- Set a reason error.

Just to compare, now we have:

 | C API:
 | struct error *
 | box_error_unwrap(box_error_t *error); // Get a reason and detach from a
 |                                       // passed error.
 | struct error *
 | box_error_wrap(box_error_t *error, box_error_t *reason); // Set a reason.

 | Lua API:
 | error:unwrap() -> reason    -- Get a reason and detach it from a passed
 |                             -- error.
 | error:wrap(reason) -> error -- Set a reason.

I doubt whether a function to get a reason should modify the original
error. The terms need to be discussed with others.

> 
> > 'Is' method (Go terminology, here it is named 'has') also don't look
> > self-descriptive enough for me. I would name it 'find_cause()'.
> 
> I vote for just 'find'. I don't think 'cause' or 'reason' should be a
> part of that function's name, because a real reason of the final result
> is only one - the first error. This function on the contrary is able
> to find any error in stack.

It seems here we agreed on terms 'find'. Neat.

> 
> > 
> > I doubt a bit that iproto change you proposed would be
> > backward-compatible: imagine that you connect from an old tarantool to a
> > new tarantool using net.box or handle errors using another existing
> > connector. I would send those messages as before, but add a field with
> > IPROTO_ERROR_LIST key in addition to existing IPROTO_ERROR (0x31) to a
> > body map. The value can be a list of maps like {code = <mp_uint>,
> > message = <mp_str>} or a list of list [<code>, <message>]. The
> > IPROTO_ERROR_LIST should start from a last (top) error I think.
> 
> I agree. I forgot about backward compatibility. Indeed, we can send both
> new single error, and a list of errors. The list can even contain the
> last error second time, because anyway error packets are rare and not
> normal. We may not optimize them too much in terms of space.

Agreed.

> 
> > 
> > Maybe we should use 'warnings' term here if this feature is intended to
> > be used for SQL warnings. If we want to use the proposed mechanics for
> > warnings, then my proposal re using 'cause' term looks doubtful. Don't
> > sure whether we should introduce some kind of warnings list for the
> > diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.

This is most confusing part for me. Say, we want to set a warning re
precision loss during execution a SQL query. The response will be
successful. There will not be an error to wrap this warning. How to
store the warning (it looks as a query-local object) and how to show it
in the response (in the binary protocol)?

Another case: we emit a warning re precission loss and an error occurs
afterwards during the query execution (say, a constraint violation). The
warning is not a reason / cause / parent for the error and it is not
logical to using our current terms for this case.

We want to support SQL stacked diagnostics and it seems that the current
proposal does not move us forward to them. I had read mysql docs on
that, but I hope the standard described quite same thing:
https://dev.mysql.com/doc/refman/5.6/en/diagnostics-area.html

I think we need at least keep SQL stacked diagnostics in a mind and
explicitly decide whether we'll going (a bit?) forward to support them
within this issue / proposal / discussion.

> > 
> > I didn't look at your patch: I want to agree on terms and overall
> > approach first. At least one of maintainers should approve it.
> 
> Most of you objections are about names, so I guess you already can take
> a look at the functionality and tests.

Okay. I gave a glance on the patches and don't find anything that would
confuse me outside of questions we discussing here: terms, binary
protocol support (+net.box), SQL warnings.