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

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

Thanks for the patch! I have pushed my review fixes in
a separate commit.

Also see 2 comments below.

On 12/08/2018 17:13, Nikita Pettik wrote:
> After struggling with SQLite legacy ON CONFLICT actions, finally we
> decided to remove them. Still, it is allowed to specify action on NULL
> conflict and on whole query action (e.g. INSERT OR IGNORE).
> 
> This patch also fixes behaviour of INSERT/UPDATE OR REPLACE statement.
> Now we are looking at each secondary index to find an entry with given
> key in order to delete it (since original space:replace() provides real
> replace only in PK). Almost the same happens for UPDATE OR IGNORE: as
> far as UPDATE causes implicit deletion of old entry and insertion of
> new one, in case of duplicates in secondary indexes we should omit
> removal of old entry.
> 
> Finally, removed extra cursors allocations from UPDATE and INSERT
> processing and moved the rest closer to their real usage.
> 
> Closes #3566
> Closes #3565
> Part of #3561
> ---
>   src/box/sql.c                            |   7 -
>   src/box/sql/build.c                      |  90 +---
>   src/box/sql/insert.c                     | 827 ++++++++-----------------------
>   src/box/sql/main.c                       |  45 +-
>   src/box/sql/parse.y                      |  30 +-
>   src/box/sql/sqliteInt.h                  | 131 +++--
>   src/box/sql/update.c                     | 248 +++------
>   src/box/sql/vdbe.c                       |   9 +-
>   src/box/sql/vdbeaux.c                    |  11 +-
>   src/box/sql/where.c                      |   5 -
>   test/sql-tap/conflict3.test.lua          | 402 ---------------
>   test/sql-tap/gh-2931-savepoints.test.lua |   2 +-
>   test/sql-tap/gh2140-trans.test.lua       |  54 +-
>   test/sql-tap/gh2964-abort.test.lua       |  11 +-
>   test/sql-tap/index1.test.lua             | 111 +----
>   test/sql-tap/null.test.lua               |   6 +-
>   test/sql-tap/tkt-4a03edc4c8.test.lua     |   6 +-
>   test/sql-tap/triggerC.test.lua           |   2 +-
>   test/sql/on-conflict.result              | 134 +++--
>   test/sql/on-conflict.test.lua            |  79 +--
>   20 files changed, 550 insertions(+), 1660 deletions(-)
>   delete mode 100755 test/sql-tap/conflict3.test.lua
> 
> 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).

> +		/* ABORT is a default error action. */
> +		if (on_conflict_nullable == ON_CONFLICT_ACTION_DEFAULT)
> +			on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
> +		struct Expr *dflt = space_column_default_expr(tab->def->id, i);
> +		if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE &&
> +		    dflt == NULL)
> +			on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
> +		switch (on_conflict_nullable) {
>   		case ON_CONFLICT_ACTION_ABORT:
> -			sqlite3MayAbort(pParse);
> -			/* Fall through */
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 3fdf5a9af..51cc5cef3 100644
> --- 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?

> +	/* Allocate cursor on primary index. */
> +	int pk_cursor = pParse->nTab++;
> +	pTabList->a[0].iCursor = pk_cursor;
> +	pPk = is_view ? NULL : sqlite3PrimaryKeyIndex(pTab);
> +
> +	aXRef = sqlite3DbMallocRawNN(db, sizeof(int) * def->field_count);
> +	if (aXRef == NULL)
>   		goto update_cleanup;
> -	aRegIdx = aXRef + def->field_count;
> -	aToOpen = (u8 *) (aRegIdx + nIdx);
> -	memset(aToOpen, 1, nIdx + 1);
> -	aToOpen[nIdx + 1] = 0;
>   	for (i = 0; i < (int)def->field_count; i++)
>   		aXRef[i] = -1;
>