[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure

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

Thanks for the patch! See 2 comments below.

> commit 5d1010dfcd7d0666774afaaf5934de018f99e181
> Author: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date:   Mon Jul 16 14:21:54 2018 +0300
> 
>     sql: get rid of Column structure
>     
>     Get rid of is_primkey in Column structure as it become
>     redundant. Moved the last member coll with collation pointer
>     to field_def structure. Finally, dropped Column.
> 
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index fe54e5531..c2a211c95 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -146,7 +146,6 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>  	Table *pNew;		/* Copy of pParse->pNewTable */
>  	Table *pTab;		/* Table being altered */
>  	const char *zTab;	/* Table name */
> -	Column *pCol;		/* The new column */
>  	Expr *pDflt;		/* Default value for the new column */
>  	sqlite3 *db;		/* The database connection; */
>  	Vdbe *v = pParse->pVdbe;	/* The prepared statement under construction */
> @@ -161,7 +160,6 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>  
>  	/* Skip the "sqlite_altertab_" prefix on the name. */
>  	zTab = &pNew->def->name[16];
> -	pCol = &pNew->aCol[pNew->def->field_count - 1];
>  	assert(pNew->def != NULL);
>  	pDflt = space_column_default_expr(SQLITE_PAGENO_TO_SPACEID(pNew->tnum),
>  					  pNew->def->field_count - 1);
> @@ -181,7 +179,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>  	 * If there is a NOT NULL constraint, then the default value for the
>  	 * column must not be NULL.
>  	 */
> -	if (pCol->is_primkey) {
> +	struct Index *pk = sqlite3PrimaryKeyIndex(pTab);
> +	if (index_has_column(pk, pNew->def->field_count - 1)) {

1. Please, rebase on the latest 2.0 and use pk->def + key_def_find.

>  		sqlite3ErrorMsg(pParse, "Cannot add a PRIMARY KEY column");
>  		return;
>  	}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ee97ef9ee..ce4012081 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -114,40 +114,70 @@ sql_finish_coding(struct Parse *parse_context)
>  	}
>  }
>  
> +/**
> + * Test if @column_idx is a part of @index.
> + *
> + * @param index to lookup.
> + * @column_idx index of column to test.
> + * @retval true if index contains column_idx.
> + * @retval false else.
> + */
> +bool
> +index_has_column(struct Index *index, uint32_t  column_idx)
> +{
> +	uint32_t parts_count = index->def->key_def->part_count;
> +	struct key_part *parts = index->def->key_def->parts;
> +	for (uint32_t i = 0; i < parts_count; i++) {
> +		if (column_idx == parts[i].fieldno)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  /**
>   * This is a function which should be called during execution
> - * of sqlite3EndTable. It ensures that only PRIMARY KEY
> - * constraint may have ON CONFLICT REPLACE clause.
> + * of sqlite3EndTable. It set defaults for columns having no
> + * separate NULL/NOT NULL specifiers and ensures that only
> + * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause.
>   *
> + * @param parser SQL Parser object.
>   * @param table Space which should be checked.
> - * @retval False, if only primary key constraint has
> - *         ON CONFLICT REPLACE clause or if there are no indexes
> - *         with REPLACE as error action. True otherwise.
> + * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set.
> + * @retval 0 on success.
>   */
> -static bool
> -check_on_conflict_replace_entries(struct Table *table)
> -{
> -	/* Check all NOT NULL constraints. */
> -	for (int i = 0; i < (int)table->def->field_count; i++) {
> -		enum on_conflict_action on_error =
> -			table->def->fields[i].nullable_action;
> -		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
> -		    table->aCol[i].is_primkey == false) {
> -			return true;
> +static int
> +actualize_on_conflict_actions(struct Parse *parser, struct Table *table)
> +{
> +	const char *err_msg = NULL;
> +	struct field_def *field = table->def->fields;
> +	struct Index *pk = sqlite3PrimaryKeyIndex(table);
> +	for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) {
> +		if (field->nullable_action == on_conflict_action_MAX) {
> +			/* Set default. */
> +			field->nullable_action = ON_CONFLICT_ACTION_NONE;
> +			field->is_nullable = true;
>  		}
> +		if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
> +		    !index_has_column(pk, i))

2. As I said verbally, you should not add this redundant scan of primary
index columns. You already have the primary index scan in
convertToWithoutRowidTable, that is called few lines above.

When you merge this scan into convertToWithoutRowidTable, you can
inline the rest of the function into EndTable and remove it together
with index_has_column.

> +			goto non_pk_on_conflict_error;
>  	}
> -	/* Check all UNIQUE constraints. */
> +
>  	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
>  		if (idx->onError == ON_CONFLICT_ACTION_REPLACE &&
> -		    !IsPrimaryKeyIndex(idx)) {
> -			return true;
> -		}
> +		    !IsPrimaryKeyIndex(idx))
> +			goto non_pk_on_conflict_error;
>  	}
> -	/*
> -	 * CHECK constraints are not allowed to have REPLACE as
> -	 * error action and therefore can be skipped.
> -	 */
> -	return false;
> +
> +	return 0;
> +
> +non_pk_on_conflict_error:
> +	err_msg = tt_sprintf("only PRIMARY KEY constraint can have "
> +			     "ON CONFLICT REPLACE clause - %s",
> +			     table->def->name);
> +	diag_set(ClientError, ER_SQL, err_msg);
> +	parser->rc = SQL_TARANTOOL_ERROR;
> +	parser->nErr++;
> +	return -1;
>  }
>