[Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function
Nikita Pettik
korablev at tarantool.org
Fri Dec 13 16:56:26 MSK 2019
On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> 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.
TBO it seems to be too long. I'd better use Konstantin's version:
sql_stmt_est_size().
> > /**
> > * 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.
Fixed.
> > +{
> > + 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.
It is exactly what is accounted here. Fixed a bit comment:
- /* Resulting set */
+ /* Names and types of result set columns */
Memory cells containing values of result set are already accounted
in v->nMem.
> > + /* 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.
Thanks, refactord:
@@ -825,33 +825,33 @@ sql_stmt_sizeof(const sql_stmt *stmt)
for (int i = 0; i < v->nOp; ++i) {
/* Estimate size of p4 operand. */
- if (v->aOp[i].p4type != P4_NOTUSED) {
- 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;
- }
+ if (v->aOp[i].p4type == P4_NOTUSED)
+ continue;
+ 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);
> > + 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;
> > + }
> > + }
More information about the Tarantool-patches
mailing list