From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 202EE26A01 for ; Thu, 8 Aug 2019 19:29:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5C0QSiW0wMm0 for ; Thu, 8 Aug 2019 19:29:17 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AB85226944 for ; Thu, 8 Aug 2019 19:29:17 -0400 (EDT) Date: Fri, 9 Aug 2019 02:29:01 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Message-ID: <20190808232900.bf6wj2g6of4cf6ti@tkn_work_nb> References: <101e2b28c29ebfc4cf58b45e534207fd4f6d9b3a.1564657285.git.kshcherbatov@tarantool.org> <20190807232755.s7gruhxeja75hob7@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org, Kirill Shcherbatov , kostja@tarantool.org 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 = , > > message = } or a list of list [, ]. 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.