[tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement

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

Thanks for the fixes! See my answers, 8 comments and a
commit on the branch.

On 25/07/2018 13:03, n.pettik wrote:
> 
>> Also see other fixes on the branch in a separate commit.
> 
> Thx for fixes. I have squashed them all.
> 
> Except fixes mentioned below, I disabled (temporary) sql-tap/alter2.test.lua
> (it checks work of ALTER TABLE ADD CONSTRAINT) for vinyl engine.
> Since in previous patch we prohibited creation of FK constraints on
> non-empty spaces and as condition used ‘index_size()’, some tests turn out
> to be flaky. (I don’t think that we should disable these tests for vinyl, but didn’t
> come up with satisfactory solution.)

Vinyl indexes has method 'compact' available in Lua. If a space is
logically empty, you can trigger compaction for each index to clean
the space garbage.

>>> +		    !fkey_child_is_modified(fk_def, aChange))
>>>   			continue;
>>> -		}
>>> -
>>> @@ -977,100 +686,74 @@ sqlite3FkCheck(Parse * pParse,	/* Parse context */
>>>   				 * might be set incorrectly if any OP_FkCounter related scans are
>>>   				 * omitted.
>>>   				 */
>>> -				if (!pFKey->isDeferred && eAction != OE_Cascade
>>> -				    && eAction != OE_SetNull) {
>>> +				if (!fk_def->is_deferred &&
>>> +				    action != FKEY_ACTION_CASCADE &&
>>> +				    action != FKEY_ACTION_SET_NULL) {
>>>   					sqlite3MayAbort(pParse);
>>>   				}
>>>   			}
>>> -			pItem->zName = 0;
>>>   			sqlite3SrcListDelete(db, pSrc);
>>>   		}
>>> -		sqlite3DbFree(db, aiCol);
>>>   	}
>>>   }
>>>     #define COLUMN_MASK(x) (((x)>31) ? 0xffffffff : ((u32)1<<(x)))
>>
>> 24. Lets use 64 bitmask and utilities from column_mask.h.
> 
> It is not so easy to do: the same mask is used for triggers as well.
> So if we want to change format of mast for FK, we should do it
> almost everywhere in SQL source code. I may do it as separate
> follow-up patch, if you wish.

No, I opened an issue:
https://github.com/tarantool/tarantool/issues/3571
Lets do it later.

> 
>>>   -/*
>>> - * This function is called before generating code to update or delete a
>>> - * row contained in table pTab.
>>> - */
>>> -u32
>>> -sqlite3FkOldmask(Parse * pParse,	/* Parse context */
>>> -		 Table * pTab	/* Table being modified */
>>> -    )
>>> +uint32_t
>>> +fkey_old_mask(uint32_t space_id)
>>
>> 25. I think we should calculate this mask once on fk creation
>> like it is done for key_def.columnm_mask.
> 
> In fact, this mask is calculated for whole space (i.e. all of its FK constraints),
> not for particular FK. So basically, we need to add this mask to space_def/space
> and update on each FK creation. Is this OK?

I think it is worth the cost. Lets add it to struct space where
fkeys are stored.

> diff --git a/extra/mkopcodeh.sh b/extra/mkopcodeh.sh
> index 63ad0d56a..9e97a50f0 100755
> --- a/extra/mkopcodeh.sh
> +++ b/extra/mkopcodeh.sh
> @@ -220,8 +224,12 @@ while [ "$i" -lt "$nOp" ]; do
>       i=$((i + 1))
>   done
>   max="$cnt"
> +echo "//*************** $max $nOp $mxTk"

1. This echo seems to be debug print.

> @@ -283,11 +303,16 @@ printf '%s\n' "#define OPFLG_OUT2        0x10  /* out2:  P2 is an output */"
>   printf '%s\n' "#define OPFLG_OUT3        0x20  /* out3:  P3 is an output */"
>   printf '%s\n' "#define OPFLG_INITIALIZER {\\"
>   i=0
> -while [ "$i" -le "$max" ]; do
> +while [ "$i" -le "$mxTk" ]; do
>       if [ "$((i % 8))" -eq 0 ]; then
>           printf '/* %3d */' "$i"
>       fi
> -    eval "bv=\$ARRAY_bv_$i"
> +    eval "is_exists=\${ARRAY_bv_$i:-}"

2. 'is_exists'? I have refactored this changes
slightly on the branch.

> +    if [ ! -n "$is_exists" ] ; then
> +        bv=0
> +    else
> +        eval "bv=\$ARRAY_bv_$i"
> +    fi
>       printf ' 0x%02x,' "$bv"
>       if [ "$((i % 8))" -eq 7 ]; then
>           printf '%s\n' "\\"
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 8c1c36b9b..0e770272e 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -190,12 +189,6 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>   		sqlite3ErrorMsg(pParse, "Cannot add a UNIQUE column");
>   		return;
>   	}
> -	if ((user_session->sql_flags & SQLITE_ForeignKeys) && pNew->pFKey

3. After we've found that defer fkey flag was checked in the parser I
checked other flags of fkeys. And it emerged that SQLITE_ForeignKeys is
still used in the parser in such places as sqlite3FkCheck, fkey_old_mask,
fkey_is_required, sqlite3FkActions, sqlite3GenerateConstraintChecks,
xferOptimization. I think, that we should check it during execution, not
parsing. It is not?

Actually it is not the only flag checked during parsing: I've found also
SQLITE_CountRows, SQLITE_IgnoreChecks (btw what is the difference between
ignore_checks and !foreign_keys?), SQLITE_RecTriggers, SQLITE_FullColNames,
SQLITE_ShortColNames, SQLITE_EnableTrigger, SQLITE_ReverseOrder.

And we have 3 options as I understand:

* somehow merge these things into VDBE;

* when we will have PREPARE API, rebuild prepared statements of
the user session on change of any of these flags;

* we announce these flags as parsing stage only and if you changed them,
the already prepared statements would not be affected.

> -	    && pDflt) {
> -		sqlite3ErrorMsg(pParse,
> -				"Cannot add a REFERENCES column with non-NULL default value");
> -		return;
> -	}
>   	assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable ==
>   	       action_is_nullable(pNew->def->fields[
>   		pNew->def->field_count - 1].nullable_action));
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 789a628d6..fc097a319 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> +
> +static int
> +resolve_link(struct Parse *parse_context, const struct space_def *def,
> +	     const char *field_name, uint32_t *link)
> +{
> +	assert(link != NULL);
> +	for (uint32_t j = 0; j < def->field_count; ++j) {
> +		if (strcmp(field_name, def->fields[j].name) == 0) {
> +			*link = j;
> +			return 0;
> +		}
> +	}
> +	sqlite3ErrorMsg(parse_context, "unknown column %s in foreign key "
> +		        "definition", field_name);

4. ER_CREATE_FK_CONSTRAINT ?

> +	return -1;
> +}
> +
>   /*
>    * This routine is called to report the final ")" that terminates
>    * a CREATE TABLE statement.
> @@ -1720,6 +1803,43 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>   
>   		/* Reparse everything to update our internal data structures */
>   		parseTableSchemaRecord(pParse, iSpaceId, zStmt);	/* consumes zStmt */
> +
> +		/* Code creation of FK constraints, if any. */
> +		struct fkey_parse *fk_parse;
> +		rlist_foreach_entry(fk_parse, &pParse->new_fkey, link) {
> +			struct fkey_def *fk = fk_parse->fkey;
> +			if (fk_parse->selfref_cols != NULL) {
> +				struct ExprList *cols = fk_parse->selfref_cols;
> +				for (uint32_t i = 0; i < fk->field_count; ++i) {
> +					if (resolve_link(pParse, p->def,
> +							 cols->a[i].zName,
> +							 &fk->links[i].parent_field) != 0)
> +						return;
> +				}
> +				fk->parent_id = iSpaceId;
> +			} else if (fk_parse->is_self_referenced) {
> +				struct Index *pk = sqlite3PrimaryKeyIndex(p);
> +				if (pk->def->key_def->part_count !=
> +				    fk->field_count) {
> +					diag_set(ClientError,
> +						 ER_CREATE_FK_CONSTRAINT,
> +						 fk->name, "number of columns "
> +						 "in foreign key does not "
> +						 "match the number of columns "
> +						 "in the referenced table");

5. However, this message was 3 times duplicated as I said. Now it
is 2: here and in sql_create_foreign_key.

6. It does not match the problem. You check for field count
in primary index, not in the whole table. So the message should be
like "... the number of columns in the referenced table's primary index"
or something.

> +					pParse->rc = SQL_TARANTOOL_ERROR;
> +					pParse->nErr++;
> +					return;
> +				}
> +				for (uint32_t i = 0; i < fk->field_count; ++i) {
> +					fk->links[i].parent_field =
> +						pk->def->key_def->parts[i].fieldno;
> +				}
> +				fk->parent_id = iSpaceId;
> +			}
> +			fk->child_id = iSpaceId;
> +			vdbe_emit_fkey_create(pParse, fk);
> +		}
>   	}
>   
>   	/* Add the table to the in-memory representation of the database.
> @@ -2106,177 +2262,280 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
>   	sqlite3SrcListDelete(db, table_name_list);
>   }
>   
> -/*
> - * This routine is called to create a new foreign key on the table
> - * currently under construction.  pFromCol determines which columns
> - * in the current table point to the foreign key.  If pFromCol==0 then
> - * connect the key to the last column inserted.  pTo is the name of
> - * the table referred to (a.k.a the "parent" table).  pToCol is a list
> - * of tables in the parent pTo table.  flags contains all
> - * information about the conflict resolution algorithms specified
> - * in the ON DELETE, ON UPDATE and ON INSERT clauses.
> +/**
> + * Return ordinal number of column by name. In case of error,
> + * set error message.
>    *
> - * An FKey structure is created and added to the table currently
> - * under construction in the pParse->pNewTable field.
> + * @param parse_context Parsing context.
> + * @param space Space which column belongs to.
> + * @param column_name Name of column to investigate.
> + * @param[out] colno Found name of column.
>    *
> - * The foreign key is set for IMMEDIATE processing.  A subsequent call
> - * to sqlite3DeferForeignKey() might change this to DEFERRED.
> + * @retval 0 on success, -1 on fault.
>    */
> +static int
> +columnno_by_name(struct Parse *parse_context, const struct space *space,
> +		 const char *column_name, uint32_t *colno)
> +{
> +	assert(colno != NULL);
> +	uint32_t column_len = strlen(column_name);
> +	if (tuple_fieldno_by_name(space->def->dict, column_name, column_len,
> +				  field_name_hash(column_name, column_len),
> +				  colno) != 0) {
> +		sqlite3ErrorMsg(parse_context,
> +				"table \"%s\" doesn't feature column %s",
> +				space->def->name, column_name);

7. diag_set?

> +		return -1;
> +	}
> +	return 0;
> +}
> +
>   void
> -sqlite3CreateForeignKey(Parse * pParse,	/* Parsing context */
> -			ExprList * pFromCol,	/* Columns in this table that point to other table */
> -			Token * pTo,	/* Name of the other table */
> -			ExprList * pToCol,	/* Columns in the other table */
> -			int flags	/* Conflict resolution algorithms. */
> -    )
> +sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> +		       struct Token *constraint, struct ExprList *child_cols,
> +		       struct Token *parent, struct ExprList *parent_cols,
> +		       bool is_deferred, int actions)

8. We have problems with actions. Lets take a look at the parser:

	%type refarg {struct {int value; int mask;}}
	refarg(A) ::= MATCH matcharg(X).     { A.value = X; A.mask = 0xff0000; }
	refarg(A) ::= ON INSERT refact.      { A.value = 0;     A.mask = 0x000000; }
	refarg(A) ::= ON DELETE refact(X).   { A.value = X;     A.mask = 0x0000ff; }
	refarg(A) ::= ON UPDATE refact(X).   { A.value = X<<8;  A.mask = 0x00ff00; }

It builds actions mask. Then lets look at the actions decoding:

	fk->match = (enum fkey_match) ((actions >> 16) & 0xff);
	fk->on_update = (enum fkey_action) ((actions >> 8) & 0xff);
	fk->on_delete = (enum fkey_action) (actions & 0xff);

As you can see, it is expected, that the mask has the layout
{on_delete, on_update, match}, each field is byte.

But the parser stores them as {on_delete/match, on_update} now.

So I've found this test that ignores my MATCH:

	box.cfg{}
	box.sql.execute('CREATE TABLE test (id int primary key, '..
	                'a int unique, b int unique)')
	box.sql.execute('CREATE TABLE test2 (id int primary key'..
	                ', a int references test(a) ON DELETE SET NULL MATCH FULL)')
	box.space._fk_constraint:select{}
	---
	- - ['FK_CONSTRAINT_1_TEST2', 513, 512, false, 'simple', 'cascade', 'no_action', [
	      {'child': 1, 'parent': 1}]]

As you can see, I specified MATCH as FULL, but it turned into SIMPLE. I do
not know what MATCH even means, but it is stored incorrectly.