[tarantool-patches] Re: [PATCH v2 3/3] sql: use name instead of function pointer for UDF

[Nikita Pettik <korablev@tarantool.org>]

On 12 Sep 11:06, Kirill Shcherbatov wrote:
> This patch changes OP_Function parameters convention: now a
> function's name is passed instead of pointer to the function
> object. This allows to normally handle the situation, when UDF
> had been deleted to the moment of the VDBE code execution.

Nit: has been deleted.

> In particular case this may happen with CK constraints that
> refer to a persistent function.

-> refer to a deleted persistent function (?).

> ---
>  src/box/sql/expr.c       | 17 ++++++++++++-----
>  src/box/sql/vdbe.c       | 17 +++++++++++------
>  src/box/sql/vdbe.h       |  1 +
>  test/sql/checks.result   | 10 +++++++---
>  test/sql/checks.test.lua |  3 ++-
>  5 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 6d5ad2a27..04c9393be 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4136,11 +4136,18 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
>  						  (char *)coll, P4_COLLSEQ);
>  			}
> -			int op = func->def->language ==
> -				 FUNC_LANGUAGE_SQL_BUILTIN ?
> -				 OP_BuiltinFunction0 : OP_Function;
> -			sqlVdbeAddOp4(v, op, constMask, r1, target,
> -				      (char *)func, P4_FUNC);
> +			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> +				sqlVdbeAddOp4(v, OP_BuiltinFunction0, constMask,
> +					      r1, target, (char *)func,
> +					      P4_FUNC);
> +			} else {
> +				sqlVdbeAddOp4(v, OP_Function,
> +					      func->def->name_len, r1, target,

Why not null-terminate function's name? Then you don't have to deal
with name's length.

> +					      sqlDbStrNDup(sql_get(),

Nit: you can use pParse->db.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 02e16da19..4c6245348 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1795,16 +1795,21 @@ case OP_BuiltinFunction: {
>  	break;
>  }
>  
> -/* Opcode: Function * P2 P3 P4 P5
> +/* Opcode: Function P1 P2 P3 P4 P5
>   * Synopsis: r[P3]=func(r[P2@P5])
>   *
> - * Invoke a user function (P4 is a pointer to a function object
> - * that defines the function) with P5 arguments taken from
> - * register P2 and successors. The result of the function is
> - * stored in register P3.
> + * Invoke a user function (P4 is a name of the function while P1
> + * is a function name length) with P5 arguments taken from register P2 and

Nit: out of 72 :)

> + * successors. The result of the function is stored in
> + * register P3.
>   */
>  case OP_Function: {
> -	struct func *func = pOp->p4.func;
> +	assert(pOp->p4type == P4_DYNAMIC);
> +	struct func *func = func_by_name(pOp->p4.z, pOp->p1);
> +	if (unlikely(func == NULL)) {
> +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, pOp->p4.z);
> +		goto abort_due_to_error;
> +	}
>  	int argc = pOp->p5;
>  	struct Mem *argv = &aMem[pOp->p2];
>  	struct port args, ret;
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index 29ff99867..e57d7f979 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -128,6 +128,7 @@ struct SubProgram {
>  #define P4_COLLSEQ  (-3)	/* P4 is a pointer to a CollSeq structure */
>  /** P4 is a pointer to a func structure. */
>  #define P4_FUNC     (-4)
> +/** P4 is a function identifier. */

Nit: redundant diff.