Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

On 10.09.2021 18:01, imeevma@tarantool.org wrote:
> This patch moves the initialization of sql_context out of the VDBE. This
> allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
> which work in a rather strange way. Moreover, due to the changes these
> opcodes make to the VDBEs, it is possible that the estimated size of the
> VDBE may change during execution of VDBE, which could lead to various
> problems.

There should no be any problems if VDBE size is estimated right before it
is copied or whatever the size is used for. Because the updated VDBE would
return the updated size as well. Correct?

I must say, the old behaviour was quite inventive. Allowed to create an
object at runtime and yet not have ifs on the main opcodes paths. I think
the reason was to reduce usage of pointers in the opcodes. The goal we used
to have too.

I am fine with the change though.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3b5df5292..6446ef091 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
>  	parser->parsed_ast.expr =
>  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
>  }
> +
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
> +{
> +	uint32_t size = sizeof(struct sql_context);
> +	if (argc > 1)
> +		size += (argc - 1) * sizeof(struct Mem);

Why do you allocate + size of mem? I don't see where it is saved
here or used. ctx->pMem is left not initialized, pOut is zero. So
where is this size of mem used?

> +	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
> +	if (ctx == NULL)
> +		return NULL;
> +	ctx->pOut = NULL;
> +	ctx->func = func;
> +	ctx->is_aborted = false;
> +	ctx->skipFlag = 0;
> +	ctx->pVdbe = vdbe;
> +	ctx->iOp = 0;
> +	return ctx;
> +}