From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 6737226C72 for ; Tue, 20 Aug 2019 12:06:07 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tDUPMaifqpe5 for ; Tue, 20 Aug 2019 12:06:07 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AEFDA25DB5 for ; Tue, 20 Aug 2019 12:06:06 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v2 7/8] sql: get rid of FuncDef function hash From: "n.pettik" In-Reply-To: <00293576-fea9-151f-11cf-85afe76b065e@tarantool.org> Date: Tue, 20 Aug 2019 19:06:03 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <29c44d7790720584498ca1763cb28ff98e35c71d.1565275470.git.kshcherbatov@tarantool.org> <00293576-fea9-151f-11cf-85afe76b065e@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov >>> + /** A call method for a function. */ >>=20 >> Add explanation why we don=E2=80=99t use basic (i.e. call >> method of base class) =E2=80=9Ccall=E2=80=9D 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=E2=80=99t -> don't > * method to gain the best possible performance for SQL > * requests. Can=E2=80=99t 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); >=20 >>> +/** >>> + * Memory cell pMem contains the context of an aggregate function. >>=20 >> Nit: pMem -> mem. > Fixed. It=E2=80=99s not. >>> assert(func !=3D NULL); >>=20 >> Instead of asserting func existence let=E2=80=99s raise an error: now >> user can remove built-in function from hash - there=E2=80=99s 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=E2=80=99t drop it: tarantool> box.schema.func.create('WAITFOR', {language =3D = 'SQL_BUILTIN', param_list =3D {'integer'}, returns =3D = 'integer',exports =3D {'SQL'}}) --- =E2=80=A6 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=E2=80=99t 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=E2=80=99s consider = removing it. > Removed user_data at all. I=E2=80=99d better move it in a separate commit.