From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6C4BC46970F for ; Fri, 29 Nov 2019 01:41:53 +0300 (MSK) References: <2a81f02865168030c1632b4b4000ea331c84a016.1574846892.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 28 Nov 2019 23:41:49 +0100 MIME-Version: 1.0 In-Reply-To: <2a81f02865168030c1632b4b4000ea331c84a016.1574846892.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/6] sql: refactor resulting set metadata List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! How is your life, kids, wife, job? I hope everything is good! Lets consider what can we do better here. Below is just my own opinion and some of advice, nothing obligatory or toxic! See 6 tiny friendly comments below :) (no dot in the end = no toxic) On 27/11/2019 13:15, Nikita Pettik wrote: > Move names and types of resulting set to a separate structure. Simplify > their storage by introducing separate members for name and type > (previously names and types were stored in one char * array). It will > allow us to add new metadata properties with ease. > > Needed for #4407 > --- > src/box/sql/delete.c | 6 ++-- > src/box/sql/insert.c | 5 ++-- > src/box/sql/legacy.c | 2 +- > src/box/sql/pragma.c | 14 ++++----- > src/box/sql/prepare.c | 9 +++--- > src/box/sql/select.c | 60 ++++++++++++++++++-------------------- > src/box/sql/update.c | 6 ++-- > src/box/sql/vdbe.h | 28 ++++++++++-------- > src/box/sql/vdbeInt.h | 8 ++++- > src/box/sql/vdbeapi.c | 81 +++++++++------------------------------------------ > src/box/sql/vdbeaux.c | 81 +++++++++++++++++++++++++++------------------------ > 11 files changed, 124 insertions(+), 176 deletions(-) > > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 0ecc676e2..2d3466cc7 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -146,11 +146,10 @@ sqlPrepare(sql * db, /* Database handle. */ > sqlVdbeSetNumCols(sParse.pVdbe, name_count); > for (int i = 0; i < name_count; i++) { > int name_index = 2 * i + name_first; > - sqlVdbeSetColName(sParse.pVdbe, i, COLNAME_NAME, > - azColName[name_index], SQL_STATIC); > - sqlVdbeSetColName(sParse.pVdbe, i, COLNAME_DECLTYPE, > - azColName[name_index + 1], > - SQL_STATIC); > + vdbe_set_metadata_col_name(sParse.pVdbe, i, > + azColName[name_index]); > + vdbe_set_metadata_col_type(sParse.pVdbe, i, > + azColName[name_index + 1]); 1. Lets fix this indentation. > } > } > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 8f93edd16..d6b8a158f 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2828,7 +2824,7 @@ multiSelect(Parse * pParse, /* Parsing context */ > Select *pFirst = p; > while (pFirst->pPrior) > pFirst = pFirst->pPrior; > - generateColumnNames(pParse, > + generate_column_metadata(pParse, > pFirst->pSrc, > pFirst->pEList); 2. And this too. > } > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index 582d48a1f..4142fb6ba 100644 > --- a/src/box/sql/vdbe.h > +++ b/src/box/sql/vdbe.h > @@ -248,7 +241,18 @@ void sqlVdbeResetStepResult(Vdbe *); > void sqlVdbeRewind(Vdbe *); > int sqlVdbeReset(Vdbe *); > void sqlVdbeSetNumCols(Vdbe *, int); > -int sqlVdbeSetColName(Vdbe *, int, int, const char *, void (*)(void *)); > + > +/** > + * Set the name of the idx'th column to be returned by the SQL > + * statement. @name must be a pointer to a nul terminated string. > + * This call must be made after a call to sqlVdbeSetNumCols(). > + */ > +int > +vdbe_set_metadata_col_name(struct Vdbe *v, int col_idx, const char *name); > + > +int > +vdbe_set_metadata_col_type(struct Vdbe *v, int col_idx, const char *type); 3. I think, that we should use one prefix for metadata methods. You used 'vdbe_metadata_delete()' above, so lets make it 'vdbe_metadata_*' everywhere. And these functions will be vdbe_metadata_set_col_name/type... > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index 0f32b4cd6..9ab3753cb 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -394,7 +399,8 @@ struct Vdbe { > Op *aOp; /* Space to hold the virtual machine's program */ > Mem *aMem; /* The memory locations */ > Mem **apArg; /* Arguments to currently executing user function */ > - Mem *aColName; /* Column names to return */ > + /** SQL metadata for SELECT queries. */ 4. Looks like it can be emitted on DML too. I see vdbe_set_metadata_col_name(v, 0, "rows updated") in sqlUpdate(), and vdbe_set_metadata_col_name(v, 0, "rows deleted") in sql_table_delete_from(). > + struct sql_column_metadata *metadata; > Mem *pResultSet; /* Pointer to an array of results */ > VdbeCursor **apCsr; /* One element of this array for each open cursor */ > Mem *aVar; /* Values for the OP_Variable opcode. */ > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index a1d658648..db11fbf33 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -1827,6 +1827,18 @@ Cleanup(Vdbe * p) > p->pResultSet = 0; > } > > +void > +vdbe_metadata_delete(struct Vdbe *v) > +{ > + if (v->metadata != NULL) { > + for (int i = 0; i < v->nResColumn; ++i) { > + free((void *)v->metadata[i].name); > + free((void *)v->metadata[i].type); 5. The necessity to do that cast basically makes 'const' in the struct useless. Could you please make them just char *, not const char *? > + } > + free(v->metadata); > + } > +} > + > /* > * Set the number of result columns that will be returned by this SQL > * statement. This is now set at compile time, rather than during > @@ -1836,50 +1848,44 @@ Cleanup(Vdbe * p) > +vdbe_set_metadata_col_name(struct Vdbe *p, int idx, const char *name) > +{ > assert(idx < p->nResColumn); > - assert(var < COLNAME_N); > - if (p->db->mallocFailed) { > - assert(!zName || xDel != SQL_DYNAMIC); > + if (p->metadata[idx].name != NULL) > + free((void *)p->metadata[idx].name); > + p->metadata[idx].name = strdup(name); > + if (p->metadata[idx].name == NULL) { > + diag_set(OutOfMemory, strlen(name), "strdup", "name"); 6. Len + 1. Dup allocates terminating zero too. In the next patches too. > return -1; > } > - assert(p->aColName != 0); > - assert(var == COLNAME_NAME || var == COLNAME_DECLTYPE); > - pColName = &(p->aColName[idx + var * p->nResColumn]); > - rc = sqlVdbeMemSetStr(pColName, zName, -1, 1, xDel); > - assert(rc != 0 || !zName || (pColName->flags & MEM_Term) != 0); > - return rc; > + return 0; > +} > +