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

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

Thanks for the patch! See 6 comments below and a separate
commit on the branch.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 5b55bfd7a..6b9e29470 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3660,6 +3660,50 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name)
>   	return NULL;
>   }
>   
> +static void

1. No comment.

> +fkey_set_mask(const struct fkey *fk, uint64_t *parent_mask,
> +	      uint64_t *child_mask)
> +{
> +	for (uint32_t i = 0; i < fk->def->field_count; ++i) {
> +		*parent_mask |= FKEY_MASK(fk->def->links[i].parent_field);
> +		*child_mask |= FKEY_MASK(fk->def->links[i].child_field);
> +	}
> +}
> +
> @@ -3673,6 +3717,7 @@ on_create_fkey_rollback(struct trigger *trigger, void *event)
>   	rlist_del_entry(fk, parent_link);
>   	rlist_del_entry(fk, child_link);
>   	fkey_delete(fk);
> +	fkey_update_mask(fk);

2. Use after free.

>   }
>   
>   /** Return old FK and release memory for the new one. */
> @@ -3712,6 +3759,7 @@ on_drop_or_replace_fkey_commit(struct trigger *trigger, void *event)
>   {
>   	(void) event;
>   	struct fkey *fk = (struct fkey *)trigger->data;
> +	fkey_update_mask(fk);

3. You should not update mask on commit. The mask should be updated
before the commit yielded. Else, during yield, some new requests
can arrive which will not match the mask despite of presence of the
fkey in the space.

>   	fkey_delete(fk);
>   }
>   
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 9795ad2ac..46a0c3472 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1489,6 +1393,23 @@ tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
>   	return p - buf;
>   }
>   
> +int
> +fkey_encode_links(uint32_t *links, uint32_t link_count, char *buf)
> +{
> +	const struct Enc *enc = get_enc(buf);
> +	char *p = enc->encode_array(buf, link_count);
> +	for (uint32_t i = 0; i < link_count; ++i) {
> +		/*
> +		 * field_link consists of two uin32_t members,
> +		 * so if we calculate proper offset, we will
> +		 * get next parent/child member.
> +		 */
> +		size_t offset = sizeof(struct field_link) * i;
> +		p = enc->encode_uint(p, *((char *) links + offset));

4. Encode_uint takes a second argument of type uint64_t. But you
cast here links to char * and then unreference it so it becomes
just char - 8 bits.

> +	}
> +	return p - buf;
> +}
> +
>   /*
>    * Format "parts" array for _index entry.
>    * Returns result size.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 819c2626a..13013ee5a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1982,6 +2165,15 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>   		sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
>   		VdbeComment((v, "Delete entry from _sequence"));
>   	}
> +	/* Delete all child FK constraints. */
> +	struct fkey *child_fk;
> +	rlist_foreach_entry (child_fk, &space->child_fkey, child_link) {
> +		const char *fk_name_dup = sqlite3DbStrDup(v->db,
> +							  child_fk->def->name);
> +		if (fk_name_dup == NULL)
> +			return;
> +		vdbe_emit_fkey_drop(parse_context, fk_name_dup, space_id);

5. Can we use P4_STATIC and do not duplicate? I want to say, that if we
do not duplicate the memory here, then only 2 situations are possible:
1) Until the VDBE is executed nothing is happened, the memory is ok and
can be used.
2) Such alter occurred before VDBE execution so the memory is dirty and
we can not use it. But in such a case schema_version is updated and the
VDBE is expired entirely that leads to its regeneration (will lead).

> +	}
>   	/*
>   	 * Drop all _space and _index entries that refer to the
>   	 * table.
> @@ -2106,177 +2299,281 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
> +void
> +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)
> +{
> -
> -	/* Link the foreign key to the table as the last step.
> +	memcpy(fk->name, constraint_name, name_len);
> +	fk->name[name_len] = '\0';
> +	sqlite3NormalizeName(fk->name);

6. You do not need to normalize it here since the name was either
auto-generated or got from sqlite3NameFromToken(). The latter already
calls normalize(). The former should generate a well-formed name.

What is more, if you call normalize only here, then all the errors
above with auto-generated names appears in lowercase, but below -
in uppercase, that leads to curious test results: somewhere
errors show fk_constraint_1_C, in another places: FK_CONSTRAINT_1_C.

> +	/*
> +	 * In case of CREATE TABLE processing, all foreign keys
> +	 * constraints must be created after space itself, so
> +	 * lets delay it until sqlite3EndTable() call and simply
> +	 * maintain list of all FK constraints inside parser.
>   	 */
> -	p->pFKey = pFKey;
> -	pFKey = 0;
> +	if (!is_alter) {
> +		struct fkey_parse *parse_fk =
> +			rlist_first_entry(&parse_context->new_fkey,
> +					  struct fkey_parse, link);
> +		parse_fk->fkey = fk;
> +	} else {
> +		vdbe_emit_fkey_create(parse_context, fk);
> +	}
>