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

[Vladislav Shpilevoy <v.shpilevoy@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.