Re: [Tarantool-patches] [PATCH 1/6] sql: refactor resulting set metadata

[Nikita Pettik <korablev@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;