Tarantool development patches archive
 help / color / mirror / Atom feed
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
To: tarantool-patches@freelists.org,
	Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 0/4] sql: store regular identifiers in case-normal form
Date: Fri, 22 Feb 2019 15:38:56 +0300	[thread overview]
Message-ID: <f2329866-7dcc-50d1-e98d-4c5b995b1a87@tarantool.org> (raw)
In-Reply-To: <d1588da2-f1ef-fe84-597d-090a2987f68e@tarantool.org>

> Talking about the patchset - I do not understand a point of
> the first 3 patches. I see, that the only reason why you
> pass the parser is a wish to set nErr. Why instead not to just
> do diag_set, return -1, and check for that in all upper functions,
> until parser is already in one of them? This how all box functions
> work with SQL right now - they do not take Vdbe, nor Parse. They
> just set diag and return -1.

The problem is that when the family of functions calling sql_normalize_name return NULL,
it is assumed that a memory allocation error has occurred, although this is not the case.
The API for working with expressions is heterogeneous, and in some cases already accepts Parser -
for example sqlPExpr, sqlPExprAddSelect.
Moreover, in the same way, to set a third-party error it is used in sql Expr: see sqlExprCheckHeight
that set 
sqlErrorMsg(pParse, "Expression tree is too large (maximum depth %d)", mxHeight);
Thus, I do not introduce new practices to the API, but on the contrary, bring it to a more consistent state.

The alternative that you propose to use the forced setting SQL_TARANTOOL_ERROR (in some cases) three functions.
the stack if the function returned NULL and !db.mallocFailed. This will work, however, for the same sqlPExpr analogy
will be incorrect: it is not true that if the expression construction function returned NULL, a Tarantool error occurred.

This will confuse the already conflicting SQL code.

  reply	other threads:[~2019-02-22 12:38 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-15 13:30 [tarantool-patches] " Kirill Shcherbatov
2019-02-15 13:30 ` [tarantool-patches] [PATCH v1 1/4] sql: patch sql_name_from_token to use Parser Kirill Shcherbatov
2019-02-15 13:30 ` [tarantool-patches] [PATCH v1 2/4] sql: patch sql_trigger_step_allocate " Kirill Shcherbatov
2019-02-15 13:30 ` [tarantool-patches] [PATCH v1 3/4] sql: patch sql_expr_create routine " Kirill Shcherbatov
2019-02-15 13:30 ` [tarantool-patches] [PATCH v1 4/4] sql: store regular identifiers in case-normal form Kirill Shcherbatov
2019-02-22 12:20 ` [tarantool-patches] Re: [PATCH v1 0/4] " Vladislav Shpilevoy
2019-02-22 12:38   ` Kirill Shcherbatov [this message]
2019-02-22 12:43     ` Vladislav Shpilevoy

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=f2329866-7dcc-50d1-e98d-4c5b995b1a87@tarantool.org \
    --to=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH v1 0/4] sql: store regular identifiers in case-normal form' \
    /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