Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> To implement memory quota of prepared statement cache, we have to
> estimate size of prepared statement. This function attempts at that.
> 
> Part of #2592
> ---
>  src/box/execute.h     |  8 ++++++++
>  src/box/sql/vdbeapi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/src/box/execute.h b/src/box/execute.h
> index 6702a18cc..d5b4d8421 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -116,6 +116,14 @@ extern const struct port_vtab port_sql_vtab;
>  int
>  sql_finalize(struct sql_stmt *stmt);
>  
> +/**
> + * Calculate estimated size of memory occupied by VM.
> + * See sqlVdbeMakeReady() for details concerning allocated
> + * memory.
> + */
> +size_t
> +sql_stmt_sizeof(const struct sql_stmt *stmt);
> +

1. Lets call it 'sql_stmt_sizeof_estimated()'. To emphasize that
the size is not exact.

>  /**
>   * Prepare (compile into VDBE byte-code) statement.
>   *
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index da528a4dc..10135bb68 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -805,6 +805,59 @@ sql_stmt_schema_version(sql_stmt *stmt)
>  	return v->schema_ver;
>  }
>  
> +size_t
> +sql_stmt_sizeof(const sql_stmt *stmt)

2. You have 'struct' in the declaration, but not
here. I think struct should be added there too.

> +{
> +	struct Vdbe *v = (struct Vdbe *) stmt;
> +	size_t size = sizeof(*v);
> +	/* Resulting set */
> +	size += sizeof(struct Mem) * v->nResColumn * COLNAME_N;

3. Could we account column names too? aColName member.

> +	/* Opcodes */
> +	size += sizeof(struct VdbeOp) * v->nOp;
> +	/* Memory cells */
> +	size += sizeof(struct Mem) * v->nMem;
> +	/* Bindings */
> +	size += sizeof(struct Mem) * v->nVar;
> +	/* Bindings included in the result set */
> +	size += sizeof(uint32_t) * v->res_var_count;
> +	/* Cursors */
> +	size += sizeof(struct VdbeCursor *) * v->nCursor;
> +
> +	for (int i = 0; i < v->nOp; ++i) {
> +		/* Estimate size of p4 operand. */
> +		if (v->aOp[i].p4type != P4_NOTUSED) {

4. You can reduce the indentation:

- Try to invert the check and do 'continue' when P4 is
  unused;

- 'Case' should be aligned under 'switch' according to
  the code style.

> +			switch (v->aOp[i].p4type) {
> +				case P4_DYNAMIC:
> +				case P4_STATIC:
> +					if (v->aOp[i].opcode == OP_Blob ||
> +					    v->aOp[i].opcode == OP_String)
> +						size += v->aOp[i].p1;
> +					else if (v->aOp[i].opcode == OP_String8)
> +						size += strlen(v->aOp[i].p4.z);
> +					break;
> +				case P4_BOOL:
> +					size += sizeof(v->aOp[i].p4.b);
> +					break;
> +				case P4_INT32:
> +					size += sizeof(v->aOp[i].p4.i);
> +					break;
> +				case P4_UINT64:
> +				case P4_INT64:
> +					size += sizeof(*v->aOp[i].p4.pI64);
> +					break;
> +				case P4_REAL:
> +					size += sizeof(*v->aOp[i].p4.pReal);
> +					break;
> +				default:
> +					size += sizeof(v->aOp[i].p4.p);
> +					break;
> +			}
> +		}
> +	}
> +	size += strlen(v->zSql);
> +	return size;
> +}
> +
>  /******************************* sql_bind_  **************************
>   *
>   * Routines used to attach values to wildcards in a compiled SQL statement.
>