[tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces

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

On 21/05/2018 13:25, Kirill Yukhin wrote:
> On 21 мая 12:47, Vladislav Shpilevoy wrote:
>> Hello. Thanks for the fixes!
>>
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index df75186..68f81e8 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse,	/* The parsing context */
>>> -int
>>> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
>>> +bool
>>
>> In Tarantool code style we do not return boolean on non-check functions.
>> 0 or -1, please.
> Done.

No(( Tests are failing. See 5 comments below, why.

> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index df75186..f59087b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
>   int
> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
> +sql_view_column_names(struct Parse *parse, struct Table *table)
>   {
> -	Table *pSelTab;		/* A fake table from which we get the result set */
> -	Select *pSel;		/* Copy of the SELECT that implements the view */
> -	int nErr = 0;		/* Number of errors encountered */
> -	int n;			/* Temporarily holds the number of cursors assigned */
> -	sqlite3 *db = pParse->db;	/* Database connection for malloc errors */
> -
> -	assert(pTable);
> -
> -#ifndef SQLITE_OMIT_VIEW
> -	/* A positive nCol means the columns names for this view are
> -	 * already known.
> +	assert(table != NULL);
> +	assert(space_is_view(table));
> +	/* A positive nCol means the columns names for this view
> +	 * are already known.
>   	 */
> -	if (pTable->nCol > 0)
> -		return 0;
> +	if (table->nCol > 0)
> +		return true;

1. Here you return 'true', but returned type is int. It is converted to 1,
and this function is treated as failed.

>   
> -	/* A negative nCol is a special marker meaning that we are currently
> -	 * trying to compute the column names.  If we enter this routine with
> -	 * a negative nCol, it means two or more views form a loop, like this:
> +	/* A negative nCol is a special marker meaning that we are
> +	 * currently trying to compute the column names.  If we
> +	 * enter this routine with a negative nCol, it means two
> +	 * or more views form a loop, like this:
>   	 *
>   	 *     CREATE VIEW one AS SELECT * FROM two;
>   	 *     CREATE VIEW two AS SELECT * FROM one;
>   	 *
> -	 * Actually, the error above is now caught prior to reaching this point.
> -	 * But the following test is still important as it does come up
> -	 * in the following:
> +	 * Actually, the error above is now caught prior to
> +	 * reaching this point. But the following test is still
> +	 * important as it does come up in the following:
>   	 *
>   	 *     CREATE TABLE main.ex1(a);
>   	 *     CREATE TEMP VIEW ex1 AS SELECT a FROM ex1;
>   	 *     SELECT * FROM temp.ex1;
>   	 */
> -	if (pTable->nCol < 0) {
> -		sqlite3ErrorMsg(pParse, "view %s is circularly defined",
> -				pTable->zName);
> -		return 1;
> -	}
> -	assert(pTable->nCol >= 0);
> -
> -	/* If we get this far, it means we need to compute the table names.
> -	 * Note that the call to sqlite3ResultSetOfSelect() will expand any
> -	 * "*" elements in the results set of the view and will assign cursors
> -	 * to the elements of the FROM clause.  But we do not want these changes
> -	 * to be permanent.  So the computation is done on a copy of the SELECT
> -	 * statement that defines the view.
> +	if (table->nCol < 0) {
> +		sqlite3ErrorMsg(parse, "view %s is circularly defined",
> +				table->zName);
> +		return false;

2. Same.

> +	}
> +	assert(table->nCol >= 0);
> +
> +	/* If we get this far, it means we need to compute the
> +	 * table names. Note that the call to
> +	 * sqlite3ResultSetOfSelect() will expand any "*" elements
> +	 * in the results set of the view and will assign cursors
> +	 * to the elements of the FROM clause.  But we do not want
> +	 * these changes to be permanent.  So the computation is
> +	 * done on a copy of the SELECT statement that defines the
> +	 * view.
>   	 */
> -	assert(pTable->pSelect);
> -	pSel = sqlite3SelectDup(db, pTable->pSelect, 0);
> -	if (pSel) {
> -		n = pParse->nTab;
> -		sqlite3SrcListAssignCursors(pParse, pSel->pSrc);
> -		pTable->nCol = -1;
> -		db->lookaside.bDisable++;
> -		pSelTab = sqlite3ResultSetOfSelect(pParse, pSel);
> -		pParse->nTab = n;
> -		if (pTable->pCheck) {
> -			/* CREATE VIEW name(arglist) AS ...
> -			 * The names of the columns in the table are taken from
> -			 * arglist which is stored in pTable->pCheck.  The pCheck field
> -			 * normally holds CHECK constraints on an ordinary table, but for
> -			 * a VIEW it holds the list of column names.
> -			 */
> -			sqlite3ColumnsFromExprList(pParse, pTable->pCheck,
> -						   &pTable->nCol,
> -						   &pTable->aCol);
> -			if (db->mallocFailed == 0 && pParse->nErr == 0
> -			    && pTable->nCol == pSel->pEList->nExpr) {
> -				sqlite3SelectAddColumnTypeAndCollation(pParse,
> -								       pTable,
> -								       pSel);
> -			}
> -		} else if (pSelTab) {
> -			/* CREATE VIEW name AS...  without an argument list.  Construct
> -			 * the column names from the SELECT statement that defines the view.
> -			 */
> -			assert(pTable->aCol == 0);
> -			pTable->nCol = pSelTab->nCol;
> -			pTable->aCol = pSelTab->aCol;
> -			pSelTab->nCol = 0;
> -			pSelTab->aCol = 0;
> -		} else {
> -			pTable->nCol = 0;
> -			nErr++;
> +	assert(table->pSelect != NULL);
> +	sqlite3 *db = parse->db;
> +	struct Select *select = sqlite3SelectDup(db, table->pSelect, 0);
> +	if (select == NULL)
> +		return false;

3. Same.

> +	int n = parse->nTab;
> +	sqlite3SrcListAssignCursors(parse, select->pSrc);
> +	table->nCol = -1;
> +	db->lookaside.bDisable++;
> +	struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select);
> +	parse->nTab = n;
> +	bool is_ok = true;

4. is_ok -> rc.

> +	if (table->pCheck != NULL) {
> +		/* CREATE VIEW name(arglist) AS ...
> +		 * The names of the columns in the table are taken
> +		 * from arglist which is stored in table->pCheck.
> +		 * The pCheck field normally holds CHECK
> +		 * constraints on an ordinary table, but for a
> +		 * VIEW it holds the list of column names.
> +		 */
> +		sqlite3ColumnsFromExprList(parse, table->pCheck,
> +					   &table->nCol,
> +					   &table->aCol);
> +		if (db->mallocFailed == 0 && parse->nErr == 0
> +		    && table->nCol == select->pEList->nExpr) {
> +			sqlite3SelectAddColumnTypeAndCollation(parse, table,
> +							       select);
>   		}
> -		sqlite3DeleteTable(db, pSelTab);
> -		sqlite3SelectDelete(db, pSel);
> -		db->lookaside.bDisable--;
> +	} else if (sel_tab != NULL) {
> +		/* CREATE VIEW name AS...  without an argument
> +		 * list.  Construct the column names from the
> +		 * SELECT statement that defines the view.
> +		 */
> +		assert(table->aCol == NULL);
> +		table->nCol = sel_tab->nCol;
> +		table->aCol = sel_tab->aCol;
> +		sel_tab->nCol = 0;
> +		sel_tab->aCol = NULL;
>   	} else {
> -		nErr++;
> +		table->nCol = 0;
> +		is_ok = false;
>   	}
> -#endif				/* SQLITE_OMIT_VIEW */
> -	return nErr;
> +	sqlite3DeleteTable(db, sel_tab);
> +	sqlite3SelectDelete(db, select);
> +	db->lookaside.bDisable--;
> +
> +	return is_ok;

5. Same as 1 - 3.