[tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes

[n.pettik]

>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index cb120384a..271886ec6 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> +vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
>> +			    int new_tuple_reg,
>> +			    enum on_conflict_action on_conflict,
>> +			    int ignore_label, int *upd_cols)
>>  {
>> -	Vdbe *v;		/* VDBE under constrution */
>> -	Index *pIdx;		/* Pointer to one of the indices */
>> -	Index *pPk = 0;		/* The PRIMARY KEY index */
>> -	sqlite3 *db;		/* Database connection */
>> -	int addr1;		/* Address of jump instruction */
>> -	int seenReplace = 0;	/* True if REPLACE is used to resolve INT PK conflict */
>> -	u8 isUpdate;		/* True if this is an UPDATE operation */
>> -	u8 bAffinityDone = 0;	/* True if the OP_Affinity operation has been run */
>>  	struct session *user_session = current_session();
>> -
>> -	isUpdate = regOldData != 0;
>> -	db = pParse->db;
>> -	v = sqlite3GetVdbe(pParse);
>> -	assert(v != 0);
>> -	struct space_def *def = pTab->def;
>> -	/* This table is not a VIEW */
>> +	struct sqlite3 *db = parse_context->db;
>> +	struct Vdbe *v = sqlite3GetVdbe(parse_context);
>> +	assert(v != NULL);
>> +	struct space_def *def = tab->def;
>> +	/* Insertion into VIEW is prohibited. */
>>  	assert(!def->opts.is_view);
>> -
>> -	pPk = sqlite3PrimaryKeyIndex(pTab);
>> -
>> -	/* Record that this module has started */
>> -	VdbeModuleComment((v, "BEGIN: GenCnstCks(%d,%d,%d,%d,%d)",
>> -			   iDataCur, iIdxCur, regNewData, regOldData, pkChng));
>> -
>> -	enum on_conflict_action override_error = on_conflict->override_error;
>> -	enum on_conflict_action on_error;
>> -	/* Test all NOT NULL constraints.
>> -	 */
>> +	bool is_update = upd_cols != NULL;
>> +	/* Test all NOT NULL constraints. */
>>  	for (uint32_t i = 0; i < def->field_count; i++) {
>> -		if (aiChng && aiChng[i] < 0) {
>> -			/* Don't bother checking for NOT NULL on columns that do not change */
>> +		/*
>> +		 * Don't bother checking for NOT NULL on columns
>> +		 * that do not change.
>> +		 */
>> +		if (is_update && upd_cols[i] < 0)
>>  			continue;
>> -		}
>> -		if (def->fields[i].is_nullable || pTab->iAutoIncPKey == (int) i)
>> -			continue;	/* This column is allowed to be NULL */
>> -
>> -		on_error = table_column_nullable_action(pTab, i);
>> -		if (override_error != ON_CONFLICT_ACTION_DEFAULT)
>> -			on_error = override_error;
>> -		/* ABORT is a default error action */
>> -		if (on_error == ON_CONFLICT_ACTION_DEFAULT)
>> -			on_error = ON_CONFLICT_ACTION_ABORT;
>> -
>> -		struct Expr *dflt = NULL;
>> -		dflt = space_column_default_expr(pTab->def->id, i);
>> -		if (on_error == ON_CONFLICT_ACTION_REPLACE && dflt == 0)
>> -			on_error = ON_CONFLICT_ACTION_ABORT;
>> -
>> -		assert(on_error != ON_CONFLICT_ACTION_NONE);
>> -		switch (on_error) {
>> +		/* This column is allowed to be NULL. */
>> +		if (def->fields[i].is_nullable || tab->iAutoIncPKey == (int) i)
>> +			continue;
>> +		enum on_conflict_action on_conflict_nullable =
>> +			on_conflict != ON_CONFLICT_ACTION_DEFAULT ?
>> +			on_conflict : table_column_nullable_action(tab, i);
> 
> 1. Can you remove table_column_nullable_action? It is the single place of its
> usage. You can do space_by_id once at the begin of the function and use
> the space explicitly both here and below for space_checks_expr_list (that
> also would be removed in such a case).

Ok, surely. Space is not even required here, we can fetch nullable action
from table->def:

+++ b/src/box/sql/insert.c
@@ -859,7 +859,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
                        continue;
                enum on_conflict_action on_conflict_nullable =
                        on_conflict != ON_CONFLICT_ACTION_DEFAULT ?
-                       on_conflict : table_column_nullable_action(tab, i);
+                       on_conflict : tab->def->fields[i].nullable_action;

@@ -4796,9 +4858,6 @@ extern int sqlSubProgramsRemaining;
 
 extern int sqlite3InitDatabase(sqlite3 * db);
 
-enum on_conflict_action
-table_column_nullable_action(struct Table *tab, uint32_t column);
-

+++ b/src/box/sql/vdbeaux.c
@@ -3869,30 +3869,3 @@ sqlite3VdbeRecordUnpackMsgpack(struct key_def *key_def,  /* Information about the
                pMem++;
        }
 }
-
-/**
- * Return action on nullable constraint violation of given column in given table.
- * FIXME: This is implemented in expensive way. For each invocation table lookup
- * is performed. In future, first param will be replaced with pointer to struct
- * space.
- *
- * @param tab  pointer to the table
- * @param column column number for which action to be returned
- * @retval return action found for given column
- */
-enum on_conflict_action
-table_column_nullable_action(struct Table *tab, uint32_t column)
-{
-       struct space *space = space_cache_find(tab->def->id);
-
-       assert(space != NULL);
-
-       struct tuple_format *format = space->format;
-
-       assert(format != NULL);
-       assert(format->field_count > column);
-
-       struct tuple_field field = format->fields[column];
-
-       return field.nullable_action;
-}

>> --- a/src/box/sql/update.c
>> +++ b/src/box/sql/update.c
>> @@ -152,34 +144,17 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>>  	}
>>    	struct space_def *def = pTab->def;
>> -
>> -	/* Allocate a cursors for the main database table and for all indices.
>> -	 * The index cursors might not be used, but if they are used they
>> -	 * need to occur right after the database cursor.  So go ahead and
>> -	 * allocate enough space, just in case.
>> -	 */
>> -	pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++;
>> -	iIdxCur = iDataCur + 1;
>> -	pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab);
>> -	for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) {
>> -		if (sql_index_is_primary(pIdx) && pPk != 0) {
>> -			iDataCur = pParse->nTab;
>> -			pTabList->a[0].iCursor = iDataCur;
>> -		}
>> -		pParse->nTab++;
>> -	}
>> -
>> -	/* Allocate space for aXRef[], aRegIdx[], and aToOpen[].
>> -	 * Initialize aXRef[] and aToOpen[] to their default values.
>> -	 */
>> -	aXRef = sqlite3DbMallocRawNN(db, sizeof(int) *
>> -				     (def->field_count + nIdx) + nIdx + 2);
>> -	if (aXRef == 0)
>> +	int nIdx = 0;
>> +	for (struct Index *idx = pTab->pIndex; idx != NULL;
>> +	     idx = idx->pNext, nIdx++);
> 
> 2. In the same function below we do space_by_id from the
> result of which we can get space->index_count. It is not?

Well, it is useless code, actually. I forgot to remove it:

+++ b/src/box/sql/update.c
@@ -144,9 +144,6 @@ sqlite3Update(Parse * pParse,               /* The parser context */
        }
 
        struct space_def *def = pTab->def;
-       int nIdx = 0;
-       for (struct Index *idx = pTab->pIndex; idx != NULL;
-            idx = idx->pNext, nIdx++);