Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: imeevma@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0
Date: Tue, 14 Sep 2021 23:21:42 +0200	[thread overview]
Message-ID: <32e96999-c3d0-1b98-6ff6-61bd0ee2cee8@tarantool.org> (raw)
In-Reply-To: <ee9ef756a69049441b67fe59b0f56367b2efd09e.1631289462.git.imeevma@gmail.com>

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;
> +}

  parent reply	other threads:[~2021-09-14 21:21 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 16:01 [Tarantool-patches] [PATCH v1 00/13] sql: reworks aggregate functions Mergen Imeev via Tarantool-patches
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 01/13] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches
2021-09-10 16:27   ` Mergen Imeev via Tarantool-patches
2021-09-14 21:21   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-09-10 16:01 ` [Tarantool-patches] [PATCH v1 03/13] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
2021-09-14 21:22   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:40     ` Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 04/13] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 05/13] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
2021-09-14 21:23   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:47     ` Mergen Imeev via Tarantool-patches
2021-09-22 22:47       ` Vladislav Shpilevoy via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 06/13] sql: rework SUM() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 07/13] sql: rework TOTAL() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 08/13] sql: rework AVG() Mergen Imeev via Tarantool-patches
2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 09/13] sql: rework COUNT() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 10/13] sql: rework MIN() and MAX() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 11/13] sql: rework GROUP_CONCAT() Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 12/13] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
2021-09-14 21:24   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-21 10:49     ` Mergen Imeev via Tarantool-patches
2021-09-10 16:02 ` [Tarantool-patches] [PATCH v1 13/13] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=32e96999-c3d0-1b98-6ff6-61bd0ee2cee8@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox