[tarantool-patches] Re: [PATCH v1 0/4] sql: store regular identifiers in case-normal form

[Vladislav Shpilevoy]

On 22/02/2019 15:38, Kirill Shcherbatov wrote:
>> 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.

Then stop this assumption. Set diag_set(OutOfMemory) inside this function.

> 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.

Again, you rolled back our progress of getting rid of nErr. You should not
insert it deeper into the API. Please, do not.

> 
> The alternative that you propose to use the forced setting SQL_TARANTOOL_ERROR (in some cases) three functions.

sql_normalize_name always should mean tarantool error. Even on failed sqlite3StrDup
you can set diag.

> 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.
>