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

["n.pettik" <korablev@tarantool.org>]

I squashed your fixes. However, personally I don’t understand why you
decided to refactor endTable() routine, for instance. Ofc refactoring
is always appreciated but I would prefer separating functional part of patch and
cosmetic/codestyle changes. Initial version of this patch AFAIR included 1k +- lines.
Now (due to codestyle fixes) it is about 1.8k + and 1.6k -.. It would be quite
complicated to find smth in such giant diff.

>> 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.

Comment for static inline two-lines function? Seriously?
Ok, I will add it, but I think we should consider making an exception
in our developing guide for instance for static functions less than
10 lines or sort of.

+++ b/src/box/alter.cc
@@ -3665,6 +3665,15 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name)
  */
 #define FKEY_MASK(x) (((x)>31) ? 0xffffffff : ((uint64_t)1<<(x)))
 
+/**
+ * Set bits of @mask which correspond to fields involved in
+ * given foreign key constraint.
+ *
+ * @param fk Links of this FK constraint are used to update mask.
+ * @param[out] mask Mask to be updated.
+ * @param type Type of links to be used to update mask:
+ *             parent or child.
+ */
 static inline void
 fkey_set_mask(const struct fkey *fk, uint64_t *mask, int type)

> 
>> +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.

Skipped since you already fixed on branch.

>>  }
>>    /** 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.

Yep, it is definitely mistake. Also skipped.

>>  	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.

The same (I realised my mistake and skipped).

> 
>> +	}
>> +	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?

No, unfortunately we can’t. P4_STATIC attribute is usable only when the same
memory is used twice during VDBE execution. STATIC attribute prevents from
memory releasing (even at the end of execution AFAIK). If we released memory
right after generating opcode (or at the end of parsing, it doesn’t matter), we will
probably get corrupted memory.

> 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.

Can be used, but wouldn’t be freed (or I misunderstand what you initially meant).

> 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.

Ok, I forgot about normalization in sqlite3NameFromToken(). Thx for fix.