[tarantool-patches] Re: [PATCH 1/2] sql: allow key_def to be NULL for ephemeral create

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

Now LGTM.

On 18/05/2018 13:48, Kirill Yukhin wrote:
> On 18 мая 13:33, Vladislav Shpilevoy wrote:
>> Hello. Thanks for fixes!
>>
>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>>> index 3455f52..2c1ce44 100644
>>> --- a/src/box/sql/delete.c
>>> +++ b/src/box/sql/delete.c
>>> @@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
>>>    			iPk = pParse->nMem + 1;
>>>    			pParse->nMem += nPk;
>>>    			iEphCur = pParse->nTab++;
>>> -			struct key_def *def = key_def_new(nPk);
>>> -			if (def == NULL) {
>>> -				sqlite3OomFault(db);
>>> -				goto delete_from_cleanup;
>>> -			}
>>>    			addrEphOpen =
>>>    				sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur,
>>> -						  nPk, 0, (char*)def, P4_KEYDEF);
>>> +						  nPk);
>>
>> Compilation error.
> Whoops, fixed.
> 
> --
> Regards, Kirill Yukhin
> 
> commit 8640d51f1fb60022b4b61b84fd5c6111c5dbe916
> Author: Kirill Yukhin <kyukhin@tarantool.org>
> Date:   Wed May 16 09:47:02 2018 +0300
> 
>      sql: allow key_def to be NULL for ephemeral create
>      
>      There're many cases in SQL FE, where exact key def passed to
>      ephemeral table doesn't matter. It is used to store data only.
>      Only field count makes sense in such a case. However it is
>      passed separately (in P2). So, allow P4 field to be NULL for
>      OP_OpenTEphemeral. Update delete from routine from sql/delete.c
>      accordingly.
>      
>      Part of #3235
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 195fdfb..6d2e0b9 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -370,8 +370,9 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
>    *
>    * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
>    */
> -int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> -				    struct key_def *def)
> +int
> +tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> +				struct key_def *def)
>   {
>   	assert(pCur);
>   	assert(pCur->curFlags & BTCF_TEphemCursor);
> @@ -381,7 +382,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
>   		return SQL_TARANTOOL_ERROR;
>   	for (uint32_t part = 0; part < field_count; ++part) {
>   		struct coll *coll;
> -		if (part < def->part_count)
> +		if (def != NULL && part < def->part_count)
>   			coll = def->parts[part].coll;
>   		else
>   			coll = NULL;
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 3455f52..1a2f3d0 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
>   			iPk = pParse->nMem + 1;
>   			pParse->nMem += nPk;
>   			iEphCur = pParse->nTab++;
> -			struct key_def *def = key_def_new(nPk);
> -			if (def == NULL) {
> -				sqlite3OomFault(db);
> -				goto delete_from_cleanup;
> -			}
>   			addrEphOpen =
> -				sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur,
> -						  nPk, 0, (char*)def, P4_KEYDEF);
> +				sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEphCur,
> +						  nPk);
>   		} else {
>   			pPk = sqlite3PrimaryKeyIndex(pTab);
>   			assert(pPk != 0);
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 5ada87e..54a7e4a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -566,13 +566,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>   			regRec = sqlite3GetTempReg(pParse);
>   			regCopy = sqlite3GetTempRange(pParse, nColumn);
>   			regTempId = sqlite3GetTempReg(pParse);
> -			struct key_def *def = key_def_new(nColumn + 1);
> -			if (def == NULL) {
> -				sqlite3OomFault(db);
> -				goto insert_cleanup;
> -			}
> -			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1,
> -					  0, (char*)def, P4_KEYDEF);
> +			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, srcTab, nColumn+1);
>   			/* Create counter for rowid */
>   			sqlite3VdbeAddOp4Dup8(v, OP_Int64,
>   					      0 /* unused */,
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index bd895bc..de111d3 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2215,13 +2215,7 @@ generateWithRecursiveQuery(Parse * pParse,	/* Parsing context */
>   		VdbeComment((v, "Orderby table"));
>   		destQueue.pOrderBy = pOrderBy;
>   	} else {
> -		struct key_def *def = key_def_new(nCol + 1);
> -		if (def == NULL) {
> -			sqlite3OomFault(pParse->db);
> -			goto end_of_recursive_query;
> -		}
> -		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iQueue, nCol + 1, 0,
> -				  (char*)def, P4_KEYDEF);
> +		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iQueue, nCol + 1);
>   		VdbeComment((v, "Queue table"));
>   	}
>   	if (iDistinct) {
> @@ -2422,13 +2416,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   	if (dest.eDest == SRT_EphemTab) {
>   		assert(p->pEList);
>   		int nCols = p->pEList->nExpr;
> -		struct key_def *def = key_def_new(nCols + 1);
> -		if (def == NULL) {
> -			sqlite3OomFault(db);
> -			goto multi_select_end;
> -		}
> -		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1,
> -				  0, (char*)def, P4_KEYDEF);
> +		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1);
>   		VdbeComment((v, "Destination temp"));
>   		dest.eDest = SRT_Table;
>   	}
> @@ -5608,11 +5596,8 @@ sqlite3Select(Parse * pParse,		/* The parser context */
>   	/* If the output is destined for a temporary table, open that table.
>   	 */
>   	if (pDest->eDest == SRT_EphemTab) {
> -		struct key_def *def = key_def_new(pEList->nExpr + 1);
> -		if (def == NULL)
> -			sqlite3OomFault(db);
> -		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, pDest->iSDParm,
> -				  pEList->nExpr + 1, 0, (char*)def, P4_KEYDEF);
> +		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, pDest->iSDParm,
> +				  pEList->nExpr + 1);
>   
>   		VdbeComment((v, "Output table"));
>   	}
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index f3db92c..204adc2 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -358,13 +358,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>   	sqlite3VdbeAddOp2(v, OP_Null, 0, iPk);
>   
>   	if (isView) {
> -		struct key_def *def = key_def_new(nKey);
> -		if (def == NULL) {
> -			sqlite3OomFault(db);
> -			goto update_cleanup;
> -		}
> -		addrOpen = sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEph,
> -					     nKey, 0, (char*)def, P4_KEYDEF);
> +		addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph,
> +					     nKey);
>   	} else {
>   		addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nPk);
>   		sql_vdbe_set_p4_key_def(pParse, pPk);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index e27fe2f..455198d 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3225,9 +3225,9 @@ open_cursor_set_hints:
>   /**
>    * Opcode: OpenTEphemeral P1 P2 * P4 *
>    * Synopsis:
> - * @param P1 index of new cursor to be created
> - * @param P2 number of columns in a new table
> - * @param P4 key def for new table
> + * @param P1 index of new cursor to be created.
> + * @param P2 number of columns in a new table.
> + * @param P4 key def for new table, NULL is allowed.
>    *
>    * This opcode creates Tarantool's ephemeral table and sets cursor P1 to it.
>    */
> @@ -3236,21 +3236,20 @@ case OP_OpenTEphemeral: {
>   	BtCursor *pBtCur;
>   	assert(pOp->p1 >= 0);
>   	assert(pOp->p2 > 0);
> -	assert(pOp->p4.key_def != NULL);
> -	assert(pOp->p4type == P4_KEYDEF);
> +	assert(pOp->p4type != P4_KEYDEF || pOp->p4.key_def != NULL);
>   
>   	pCx = allocateCursor(p, pOp->p1, pOp->p2, CURTYPE_TARANTOOL);
>   	if (pCx == 0) goto no_mem;
>   	pCx->nullRow = 1;
>   
> -	pCx->key_def  = pOp->p4.key_def;
>   	pBtCur = pCx->uc.pCursor;
>   	/* Ephemeral spaces don't have space_id */
>   	pBtCur->eState = CURSOR_INVALID;
>   	pBtCur->curFlags = BTCF_TEphemCursor;
>   
>   	rc = tarantoolSqlite3EphemeralCreate(pCx->uc.pCursor, pOp->p2,
> -					     pCx->key_def);
> +					     pOp->p4.key_def);
> +	pCx->key_def = pCx->uc.pCursor->index->def->key_def;
>   	if (rc) goto abort_due_to_error;
>   	break;
>   }
>