[tarantool-patches] Re: [PATCH v2 7/8] sql: get rid of FuncDef function hash

[n.pettik]

>>> +	/** A call method for a function. */
>> 
>> Add explanation why we don’t use basic (i.e. call
>> method of base class) “call” method. Furthermore,
>> built-ins are called bypassing func_access_check()
>> function. This should be either documented or fixed.
> 	/**
> 	 * A VDBE-memory-compatible call method for a function.
> 	 * SQL Builting functions doesn't use base class call

Builting -> built-in; doesn’t -> don't

> 	 * method to gain the best possible performance for SQL
> 	 * requests.

Can’t parse this sentence at all.

> As builtin functions are predefined and
> 	 * non of them modifie schema, access checks are

God please, use spell checker.

> 	 * redundant, functions have the same execution
> 	 * privileges as SQL.
> 	 */
> 	void (*call)(sql_context *ctx, int argc, sql_value **argv);
> 
>>> +/**
>>> + * Memory cell pMem contains the context of an aggregate function.
>> 
>> Nit: pMem -> mem.
> Fixed.

It’s not.

>>> 	assert(func != NULL);
>> 
>> Instead of asserting func existence let’s raise an error: now
>> user can remove built-in function from hash - there’s no
>> protection to avoid such cases. Note that such opportunity
>> (removing built-ins from cache) has been introduced by moving
>> built-ins to the global func cache. In this case, assertion will fail
>> in debug mode and lead to unpredictable consequences in
>> release mode. Another option is to introduce mentioned protection,
>> i.e. disallow user to delete functions which are declared with
>> FUNC_LANGUAGE_SQL_BUILTIN flag.
> I've introduce remove protection in alter.cc

Well, now we have another problem: user can create
built-in function, but can’t drop it:

tarantool> box.schema.func.create('WAITFOR', {language = 'SQL_BUILTIN',   param_list = {'integer'}, returns = 'integer',exports = {'SQL'}})
---
…

tarantool> box.schema.func.drop('WAITFOR')
---
- error: 'Can''t drop function 1: function is SQL builtin'
...



>> Can aggregate function be non-builtin?
> Not for now, but I don't like to rely on it so I make all
> corresponding checks.

Please, don’t cut the context related to comment.
Add assertion than, instead of absolutely redundant if-check.
If someday ability of declaring non-builtins as aggregates
would be introduced, one can easily find and fix this place.

>> user_data is used only for min/max functions. Let’s consider removing it.
> Removed user_data at all.

I’d better move it in a separate commit.