From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 6FD1246971A for ; Thu, 5 Dec 2019 14:39:34 +0300 (MSK) Date: Thu, 5 Dec 2019 14:39:33 +0300 From: Nikita Pettik Message-ID: <20191205113933.GB15510@tarantool.org> References: <2a81f02865168030c1632b4b4000ea331c84a016.1574846892.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > > 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. NP: diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 2d3466cc7..9edc59ab5 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -147,9 +147,9 @@ sqlPrepare(sql * db, /* Database handle. */ for (int i = 0; i < name_count; i++) { int name_index = 2 * i + name_first; vdbe_set_metadata_col_name(sParse.pVdbe, i, - azColName[name_index]); + azColName[name_index]); vdbe_set_metadata_col_type(sParse.pVdbe, i, - azColName[name_index + 1]); + azColName[name_index + 1]); } } > > 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/select.c b/src/box/sql/select.c index d6b8a158f..212f33dd7 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -2825,8 +2825,8 @@ multiSelect(Parse * pParse, /* Parsing context */ while (pFirst->pPrior) pFirst = pFirst->pPrior; generate_column_metadata(pParse, - pFirst->pSrc, - pFirst->pEList); + pFirst->pSrc, + pFirst->pEList); } iBreak = sqlVdbeMakeLabel(v); iCont = sqlVdbeMakeLabel(v); > > } > > 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... Ok, renamed here and in other patches. > > 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(). Fair, fixed comment: diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 9ab3753cb..4fabab9d6 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -399,7 +399,7 @@ 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 */ - /** SQL metadata for SELECT queries. */ + /** SQL metadata for DML/DQL queries. */ struct sql_column_metadata *metadata; Mem *pResultSet; /* Pointer to an array of results */ VdbeCursor **apCsr; /* One element of this array for each open cursor */ > > 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 *? Yep: diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 9ab3753cb..64250bee2 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -347,8 +347,8 @@ struct ScanStatus { }; struct sql_column_metadata { - const char *name; - const char *type; + char *name; + char *type; }; diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 8cbe6c368..e5528c0e2 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1832,8 +1832,8 @@ 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); + free(v->metadata[i].name); + free(v->metadata[i].type); } free(v->metadata); } > > > + } > > + 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. diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index db11fbf33..8cbe6c368 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1861,28 +1861,28 @@ sqlVdbeSetNumCols(Vdbe * p, int nResColumn) } int -vdbe_set_metadata_col_name(struct Vdbe *p, int idx, const char *name) +vdbe_metadata_set_col_name(struct Vdbe *p, int idx, const char *name) { assert(idx < p->nResColumn); 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"); + diag_set(OutOfMemory, strlen(name) + 1, "strdup", "name"); return -1; } return 0; } int -vdbe_set_metadata_col_type(struct Vdbe *p, int idx, const char *type) +vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type) { assert(idx < p->nResColumn); if (p->metadata[idx].type != NULL) free((void *)p->metadata[idx].type); p->metadata[idx].type = strdup(type); if (p->metadata[idx].type == NULL) { - diag_set(OutOfMemory, strlen(type), "strdup", "type"); + diag_set(OutOfMemory, strlen(type) + 1, "strdup", "type"); return -1; } return 0;