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 107E12106C for ; Mon, 9 Sep 2019 15:40:39 -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 cG1_qnNBDek2 for ; Mon, 9 Sep 2019 15:40:38 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 C2F99204B7 for ; Mon, 9 Sep 2019 15:40:38 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool References: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 9 Sep 2019 21:44:18 +0200 MIME-Version: 1.0 In-Reply-To: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, Kirill Shcherbatov , georgy@tarantool.org Cc: alexander.turenko@tarantool.org, kostja@tarantool.org Hi! First of all, great RFC, this is getting better and better! > +### Binary protocol > +Currently errors are sent as `(IPROTO_ERROR | errcode)` messages with an error's message string payload. > + > +We must to design a backward-compatible error transfer specification. Imagine a net.box old-new Tarantool connection, same as existing connector: they mustn't broken. > + > +Let's extend existent binary protocol with a new key IPROTO_ERROR_V2: I would rather rename the old one to IPROTO_ERROR_16, just like we did with IPROTO_CALL_16. And the new one will be IPROTO_ERROR. > +{ > + // backward compatibility > + IPROTO_ERROR: "the most recent error message", > + // modern error message > + IPROTO_ERROR_V2: { > + { > + // the most recent error object > + "code": error_code_number, Please, don't use strings as map keys, you will pad out the protocol. It is a binary proto, not text. Just use numeric constants: IPROTO_ERROR_CODE = 0x01, IPROTO_ERROR_REASON = 0x02, etc. > + "reason": error_reason_string, > + "file": file_name_string, > + "line": line_number, > + }, > + ... > + { > + // the oldest (reason) error object > + }, > + } > +} > + > +### SQL Warnings > +SQL Standard defines ["WARNINGS"](https://docs.microsoft.com/en-us/sql/t-sql/statements/set-ansi-warnings-transact-sql?view=sql-server-2017) term: > +- When set to ON, if null values appear in aggregate functions, such as SUM, AVG, MAX, MIN, STDEV, STDEVP, VAR, VARP, or COUNT, a warning message is generated. When set to OFF, no warning is issued. > +- When set to ON, the divide-by-zero and arithmetic overflow errors cause the statement to be rolled back and an error message is generated. When set to OFF, the divide-by-zero and arithmetic overflow errors cause null values to be returned. The behavior in which a divide-by-zero or arithmetic overflow error causes null values to be returned occurs if an INSERT or UPDATE is tried on a character, Unicode, or binary column in which the length of a new value exceeds the maximum size of the column. If SET ANSI_WARNINGS is ON, the INSERT or UPDATE is canceled as specified by the ISO standard. Trailing blanks are ignored for character columns and trailing nulls are ignored for binary columns. When OFF, data is truncated to the size of the column and the statement succeeds. > + > +According to the current convention, all Tarantool's function use "return not null error code on error" convention. Iff the global diagnostic area is valid (actually, Unix errno uses the same semantics). So we need an additional `Parser`/`VDBE` marker to indicate that the warning diagnostics > +is set. Say, in addition to `bool is_aborted` field we may introduce and additional field `bool is_warning_set`. > + > +To avoid a mess between SQL warnings and real errors let's better introduce an > +"diagnostics"-semantics area in Parser/VDBE classes where all warnings are > +organized in a list. Internal warning representation needn't follow error > +specification and may use other representation when it is reasonable. > I agree, warnings have nothing to do with non-SQL methods, and should be kept for SQL queries only if possible.