Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>,
	georgy@tarantool.org
Cc: alexander.turenko@tarantool.org, kostja@tarantool.org
Subject: [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool
Date: Mon, 9 Sep 2019 21:44:18 +0200	[thread overview]
Message-ID: <a0ec9e97-a1f9-26d8-3c1f-c0ad4050bcaf@tarantool.org> (raw)
In-Reply-To: <5eaffc5d92bd910865f3da937f8b2831f968c4b0.1567435674.git.kshcherbatov@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.

  parent reply	other threads:[~2019-09-09 19:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-09-09  8:13   ` [tarantool-patches] " Kirill Shcherbatov
2019-09-09 19:44     ` Vladislav Shpilevoy
2019-09-09 19:44   ` Vladislav Shpilevoy [this message]
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov
2019-09-04 10:53   ` [tarantool-patches] " Konstantin Osipov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors Kirill Shcherbatov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a0ec9e97-a1f9-26d8-3c1f-c0ad4050bcaf@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=georgy@tarantool.org \
    --cc=kostja@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox