Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 07/20] refactoring: remove exceptions from index_def_new_from_tuple

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 07/20] refactoring: remove exceptions from index_def_new_from_tuple

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]


Hi!

Thanks for your review.
We are using diag_raise here as far as index_def_check_sequence is already refactored and we need to process the error it returns, while sequence_field_from_tuple itself is not being refactored in this patch, so we can't return an error here, only raise an exception.

Sincerely,
Ilya Kosarev

>Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
>
>Hi!
>
>LGTM, just a few nits in comments below
>
>> index 8d565e189..10a5f8d64 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,
>>  }
>> 
>>  /**
>> - * Throw an exception if the given index definition
>> + * Return an error if the given index definition
>>   * is incompatible with a sequence.
>>   */
>> -static void
>> +static int
>>  index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>  			 const char *sequence_path, uint32_t sequence_path_len,
>>  			 const char *space_name)
>> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>  		}
>>  	}
>>  	if (sequence_part == NULL) {
>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>> -			  space_name, "sequence field must be a part of "
>> -			  "the index");
>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>> +			 space_name, "sequence field must be a part of "
>> +				     "the index");
>> +		return -1;
>>  	}
>>  	enum field_type type = sequence_part->type;
>>  	if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {
>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>> -			  space_name, "sequence cannot be used with "
>> -			  "a non-integer key");
>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>> +			 space_name, "sequence cannot be used with "
>> +				     "a non-integer key");
>> +		return -1;
>>  	}
>> +	return 0;
>>  }
>> 
>>  /**
>>   * Support function for index_def_new_from_tuple(..)
>> - * Checks tuple (of _index space) and throws a nice error if it is invalid
>> + * Checks tuple (of _index space) and sets a nice diag if it is invalid
>Should be 'Return an error', same as for the case above.
>
>> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,
>>  		if (path_len == 0)
>>  			path_raw = NULL;
>>  	}
>> -	index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>> -				 space_name(space));
>> +	if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>> +				 space_name(space)) != 0)
>> +		diag_raise();
>
>I wonder why we leave some diag_raise() and not diag_set() plus return
>-1 or NULL?
>
>
>regards,
>Sergos
>


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 4130 bytes --]

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 08/20] refactoring: remove exceptions from func_def_new_from_tuple

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]


Hi!

Thanks for your review.
I guess it is debatable. On the one hand, it will be easier to understand which line did the error happened in, on the other hand, it looks better, when such blocks are combined. "Better layout" might seem minorish, however, if we need to debug this code, we can alway place a breakpoint and look at old_def & new_def intently to discover which one is actually NULL. Also Georgy recommended the opposite thing in his review for patch 5 ( https://www.freelists.org/post/tarantool-patches/PATCH-v4-0520-refactoring-clear-privilege-managing-triggers-from-exceptions,1 ), so i guess we can discuss it verbally.
Sincerely,
Ilya Kosarev


>Среда, 30 октября 2019, 13:34 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
>
>On 23 Sep 18:56, Ilya Kosarev wrote:
>
>Hi! 
>
>Just one nit below, LGTM with follow-up.
>
>Sergos
>
>
>> func_def_new_from_tuple is used in on_replace_dd_func therefore
>> it has to be cleared from exceptions. Now it doesn't throw any
>> more. It means we also need to clear from exceptions it's
>> subsidiary function func_def_get_ids_from_tuple.
>> Their usages are updated.
>> 
>> Part of #4247
>> ---
>>  src/box/alter.cc | 149 ++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 97 insertions(+), 52 deletions(-)
>> 
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 10a5f8d64..3d1c59189 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -3132,6 +3175,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
>>  		});
>>  		old_def = func_def_new_from_tuple(old_tuple);
>Shall we return error here, rather than possibly mask it with another one at
>the next line?
>
>>  		new_def = func_def_new_from_tuple(new_tuple);
>> +		if (old_def == NULL || new_def == NULL)
>> +			return -1;
>>  		if (func_def_cmp(new_def, old_def) != 0) {
>>  			diag_set(ClientError, ER_UNSUPPORTED, "function",
>>  				  "alter");
>> -- 
>> 2.17.1
>> 
>> 


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 3120 bytes --]

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 07/20] refactoring: remove exceptions from index_def_new_from_tuple

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 3034 bytes --]


Hi!

Thanks for explanation! 
The patch is still LGTM.

Sergos 



Sent from Mail.ru app for iOS


Wednesday, 30 October 2019, 20:08 +0300 from Ilya Kosarev  <i.kosarev@tarantool.org>:
>Hi!
>
>Thanks for your review.
>We are using diag_raise here as far as index_def_check_sequence is already refactored and we need to process the error it returns, while sequence_field_from_tuple itself is not being refactored in this patch, so we can't return an error here, only raise an exception.
>
>Sincerely,
>Ilya Kosarev
>
>>Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich < sergos@tarantool.org >:
>>
>>Hi!
>>
>>LGTM, just a few nits in comments below
>>
>>> index 8d565e189..10a5f8d64 100644
>>> --- a/src/box/alter.cc
>>> +++ b/src/box/alter.cc
>>> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,
>>>  }
>>> 
>>>  /**
>>> - * Throw an exception if the given index definition
>>> + * Return an error if the given index definition
>>>   * is incompatible with a sequence.
>>>   */
>>> -static void
>>> +static int
>>>  index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>>  			 const char *sequence_path, uint32_t sequence_path_len,
>>>  			 const char *space_name)
>>> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>>  		}
>>>  	}
>>>  	if (sequence_part == NULL) {
>>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> -			  space_name, "sequence field must be a part of "
>>> -			  "the index");
>>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> +			 space_name, "sequence field must be a part of "
>>> +				     "the index");
>>> +		return -1;
>>>  	}
>>>  	enum field_type type = sequence_part->type;
>>>  	if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {
>>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> -			  space_name, "sequence cannot be used with "
>>> -			  "a non-integer key");
>>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> +			 space_name, "sequence cannot be used with "
>>> +				     "a non-integer key");
>>> +		return -1;
>>>  	}
>>> +	return 0;
>>>  }
>>> 
>>>  /**
>>>   * Support function for index_def_new_from_tuple(..)
>>> - * Checks tuple (of _index space) and throws a nice error if it is invalid
>>> + * Checks tuple (of _index space) and sets a nice diag if it is invalid
>>Should be 'Return an error', same as for the case above.
>>
>>> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,
>>>  		if (path_len == 0)
>>>  			path_raw = NULL;
>>>  	}
>>> -	index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>>> -				 space_name(space));
>>> +	if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>>> +				 space_name(space)) != 0)
>>> +		diag_raise();
>>
>>I wonder why we leave some diag_raise() and not diag_set() plus return
>>-1 or NULL?
>>
>>
>>regards,
>>Sergos
>>
>
>
>-- 
>Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 8664 bytes --]

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 11/20] refactoring: use non _xc version of functions in triggers

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]


Hi!

Thanks for your review.
Yes, i definitely missed diag_set with sequence_by_id. Will be fixed in v5. 

Also i will reconsider combining child & parent space search.
Sincerely,
Ilya Kosarev


>Суббота,  9 ноября 2019, 23:01 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
>
>Hi!
>
>LGTM, just couple of nits below you can add to a follow-up.
>
>Sergos
>
>>diff --git a/src/box/alter.cc b/src/box/alter.cc
>>index e56d7c6cc..69a749326 100644
>>--- a/src/box/alter.cc
>>+++ b/src/box/alter.cc
>>@@ -4117,14 +4167,18 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
>> 	struct tuple *old_tuple = stmt->old_tuple;
>> 	struct tuple *new_tuple = stmt->new_tuple;
>>
>>-	uint32_t id = tuple_field_u32_xc(old_tuple ?: new_tuple,
>>-					 BOX_SEQUENCE_DATA_FIELD_ID);
>>-	struct sequence *seq = sequence_cache_find(id);
>>+	uint32_t id;
>>+	if (tuple_field_u32(old_tuple ?: new_tuple, BOX_SEQUENCE_DATA_FIELD_ID,
>>+			    &id) != 0)
>>+		return -1;
>>+	struct sequence *seq = sequence_by_id(id);
>> 	if (seq == NULL)
>> 		return -1;
>Note, that here we will miss the diag_set for non-zero value. 
>Perhaps a question of further patches for sequence_by_id() - but I didn't find it in log
>
>>@@ -4958,10 +5034,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>> 			fk_constraint_def_new_from_tuple(old_tuple,
>> 						ER_DROP_FK_CONSTRAINT);
>> 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
>>-		struct space *child_space =
>>-			space_cache_find_xc(fk_def->child_id);
>>-		struct space *parent_space =
>>-			space_cache_find_xc(fk_def->parent_id);
>>+		struct space *child_space = space_cache_find(fk_def->child_id);
>>+		struct space *parent_space = space_cache_find(fk_def->parent_id);
>>+		if (child_space == NULL or parent_space == NULL)
>>+			return -1; 
>Shall we return after the first error? Will they affect each other?
>
>>
>     


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 3858 bytes --]

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 08/20] refactoring: remove exceptions from func_def_new_from_tuple

[Sergey Ostanevich]

Hi!

No need to discuss - I believe your and Georgy argument is valid. Let it
be like this.

LGTM
Sergos


On 30 Oct 14:02, Ilya Kosarev wrote:
> 
> Hi!
> 
> Thanks for your review.
> I guess it is debatable. On the one hand, it will be easier to understand which line did the error happened in, on the other hand, it looks better, when such blocks are combined. "Better layout" might seem minorish, however, if we need to debug this code, we can alway place a breakpoint and look at old_def & new_def intently to discover which one is actually NULL. Also Georgy recommended the opposite thing in his review for patch 5 ( https://www.freelists.org/post/tarantool-patches/PATCH-v4-0520-refactoring-clear-privilege-managing-triggers-from-exceptions,1 ), so i guess we can discuss it verbally.
> Sincerely,
> Ilya Kosarev
> 
> 
> >Среда, 30 октября 2019, 13:34 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
> >
> >On 23 Sep 18:56, Ilya Kosarev wrote:
> >
> >Hi! 
> >
> >Just one nit below, LGTM with follow-up.
> >
> >Sergos
> >
> >
> >> func_def_new_from_tuple is used in on_replace_dd_func therefore
> >> it has to be cleared from exceptions. Now it doesn't throw any
> >> more. It means we also need to clear from exceptions it's
> >> subsidiary function func_def_get_ids_from_tuple.
> >> Their usages are updated.
> >> 
> >> Part of #4247
> >> ---
> >>  src/box/alter.cc | 149 ++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 97 insertions(+), 52 deletions(-)
> >> 
> >> diff --git a/src/box/alter.cc b/src/box/alter.cc
> >> index 10a5f8d64..3d1c59189 100644
> >> --- a/src/box/alter.cc
> >> +++ b/src/box/alter.cc
> >> @@ -3132,6 +3175,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
> >>  		});
> >>  		old_def = func_def_new_from_tuple(old_tuple);
> >Shall we return error here, rather than possibly mask it with another one at
> >the next line?
> >
> >>  		new_def = func_def_new_from_tuple(new_tuple);
> >> +		if (old_def == NULL || new_def == NULL)
> >> +			return -1;
> >>  		if (func_def_cmp(new_def, old_def) != 0) {
> >>  			diag_set(ClientError, ER_UNSUPPORTED, "function",
> >>  				  "alter");
> >> -- 
> >> 2.17.1
> >> 
> >> 
> 
> 
> -- 
> Ilya Kosarev

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 11/20] refactoring: use non _xc version of functions in triggers

[Sergey Ostanevich]

Hi!

Great, LGTM.

Sergos

On 10 Nov 01:52, Ilya Kosarev wrote:
> 
> Hi!
> 
> Thanks for your review.
> Yes, i definitely missed diag_set with sequence_by_id. Will be fixed in v5. 
> 
> Also i will reconsider combining child & parent space search.
> Sincerely,
> Ilya Kosarev
> 
> 
> >Суббота,  9 ноября 2019, 23:01 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
> >
> >Hi!
> >
> >LGTM, just couple of nits below you can add to a follow-up.
> >
> >Sergos
> >
> >>diff --git a/src/box/alter.cc b/src/box/alter.cc
> >>index e56d7c6cc..69a749326 100644
> >>--- a/src/box/alter.cc
> >>+++ b/src/box/alter.cc
> >>@@ -4117,14 +4167,18 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
> >> 	struct tuple *old_tuple = stmt->old_tuple;
> >> 	struct tuple *new_tuple = stmt->new_tuple;
> >>
> >>-	uint32_t id = tuple_field_u32_xc(old_tuple ?: new_tuple,
> >>-					 BOX_SEQUENCE_DATA_FIELD_ID);
> >>-	struct sequence *seq = sequence_cache_find(id);
> >>+	uint32_t id;
> >>+	if (tuple_field_u32(old_tuple ?: new_tuple, BOX_SEQUENCE_DATA_FIELD_ID,
> >>+			    &id) != 0)
> >>+		return -1;
> >>+	struct sequence *seq = sequence_by_id(id);
> >> 	if (seq == NULL)
> >> 		return -1;
> >Note, that here we will miss the diag_set for non-zero value. 
> >Perhaps a question of further patches for sequence_by_id() - but I didn't find it in log
> >
> >>@@ -4958,10 +5034,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
> >> 			fk_constraint_def_new_from_tuple(old_tuple,
> >> 						ER_DROP_FK_CONSTRAINT);
> >> 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
> >>-		struct space *child_space =
> >>-			space_cache_find_xc(fk_def->child_id);
> >>-		struct space *parent_space =
> >>-			space_cache_find_xc(fk_def->parent_id);
> >>+		struct space *child_space = space_cache_find(fk_def->child_id);
> >>+		struct space *parent_space = space_cache_find(fk_def->parent_id);
> >>+		if (child_space == NULL or parent_space == NULL)
> >>+			return -1; 
> >Shall we return after the first error? Will they affect each other?
> >
> >>
> >     
> 
> 
> -- 
> Ilya Kosarev

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 12/20] refactoring: remove exceptions from space_def_new_from_tuple

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]


Hi!

Thanks for your review!
Guess we might need to confirm it with Pettik 
or someone else who interacts with sql a lot,
but i am quite sure sqlRunParser doesn't need
to set diagnostics even in case of returning -1.
So far, i think we need to set diagnostics here,
in field_def_decode, like we do it in
ck_constraint_new.
Although it is not something i broke within
this patchset, i will add it to follow-up.

Sincerely,
Ilya Kosarev


>Вторник, 12 ноября 2019, 14:00 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
>
>Hi!
>
>Thanks for the patch. One follow-up below, LGTM .
>
>
>Sergos
>
>>  	const char *dv = field->default_value;
>> @@ -496,8 +507,9 @@ field_def_decode(struct field_def *field, const char **data,
>>  		field->default_value_expr = sql_expr_compile(sql_get(), dv,
>>  							     strlen(dv));
>>  		if (field->default_value_expr == NULL)
>> -			diag_raise();
>> +			return -1;
>
>The sql_expr_compile() calls for sqlRunParser() that does not set diag
>in all exit paths. Please, put into followup.
>
>


-- 
Ilya Kosarev
field_def_decode

[-- Attachment #2: Type: text/html, Size: 2609 bytes --]

[Tarantool-patches] [PATCH] refactoring: remove exceptions from fk_constraint_def_new_from_tuple

[Ilya Kosarev]

fk_constraint_def_new_from_tuple is used in
on_replace_dd_fk_constraint therefore it has to be cleared from
exceptions. Now it doesn't throw any more. It means we also need
to clear from exceptions it's subsidiary function: decode_fk_links.
Their usages are updated. Some _xc functions, not needed any more,
are removed.

Part of #4247
---

 src/box/alter.cc | 105 ++++++++++++++++++++++++++++++-----------------
 src/box/tuple.h  |  21 ----------
 2 files changed, 67 insertions(+), 59 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index de2ded79d..32f84ded7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4748,38 +4748,45 @@ decode_fk_links(struct tuple *tuple, uint32_t *out_count,
 		const char *constraint_name, uint32_t constraint_len,
 		uint32_t errcode)
 {
-	const char *parent_cols =
-		tuple_field_with_type_xc(tuple,
-					 BOX_FK_CONSTRAINT_FIELD_PARENT_COLS,
-					 MP_ARRAY);
+	const char *parent_cols = tuple_field_with_type(tuple,
+		BOX_FK_CONSTRAINT_FIELD_PARENT_COLS, MP_ARRAY);
+	if (parent_cols == NULL)
+		return NULL;
 	uint32_t count = mp_decode_array(&parent_cols);
 	if (count == 0) {
-		tnt_raise(ClientError, errcode,
+		diag_set(ClientError, errcode,
 			  tt_cstr(constraint_name, constraint_len),
 			  "at least one link must be specified");
+		return NULL;
 	}
-	const char *child_cols =
-		tuple_field_with_type_xc(tuple,
-					 BOX_FK_CONSTRAINT_FIELD_CHILD_COLS,
-					 MP_ARRAY);
+	const char *child_cols = tuple_field_with_type(tuple,
+		BOX_FK_CONSTRAINT_FIELD_CHILD_COLS, MP_ARRAY);
+	if (child_cols == NULL)
+		return NULL;
 	if (mp_decode_array(&child_cols) != count) {
-		tnt_raise(ClientError, errcode,
+		diag_set(ClientError, errcode,
 			  tt_cstr(constraint_name, constraint_len),
 			  "number of referenced and referencing fields "
 			  "must be the same");
+		return NULL;
 	}
 	*out_count = count;
 	size_t size = count * sizeof(struct field_link);
 	struct field_link *region_links =
-		(struct field_link *) region_alloc_xc(&fiber()->gc, size);
+		(struct field_link *)region_alloc(&fiber()->gc, size);
+	if (region_links == NULL) {
+		diag_set(OutOfMemory, size, "region", "new slab");
+		return NULL;
+	}
 	memset(region_links, 0, size);
 	for (uint32_t i = 0; i < count; ++i) {
 		if (mp_typeof(*parent_cols) != MP_UINT ||
 		    mp_typeof(*child_cols) != MP_UINT) {
-			tnt_raise(ClientError, errcode,
+			diag_set(ClientError, errcode,
 				  tt_cstr(constraint_name, constraint_len),
 				  tt_sprintf("value of %d link is not unsigned",
 					     i));
+			return NULL;
 		}
 		region_links[i].parent_field = mp_decode_uint(&parent_cols);
 		region_links[i].child_field = mp_decode_uint(&child_cols);
@@ -4792,24 +4799,30 @@ static struct fk_constraint_def *
 fk_constraint_def_new_from_tuple(struct tuple *tuple, uint32_t errcode)
 {
 	uint32_t name_len;
-	const char *name =
-		tuple_field_str_xc(tuple, BOX_FK_CONSTRAINT_FIELD_NAME,
-				   &name_len);
+	const char *name = tuple_field_str(tuple,
+		BOX_FK_CONSTRAINT_FIELD_NAME, &name_len);
+	if (name == NULL)
+		return NULL;
 	if (name_len > BOX_NAME_MAX) {
-		tnt_raise(ClientError, errcode,
+		diag_set(ClientError, errcode,
 			  tt_cstr(name, BOX_INVALID_NAME_MAX),
 			  "constraint name is too long");
+		return NULL;
 	}
-	identifier_check_xc(name, name_len);
+	if (identifier_check(name, name_len) != 0)
+		return NULL;
 	uint32_t link_count;
 	struct field_link *links = decode_fk_links(tuple, &link_count, name,
 						   name_len, errcode);
+	if (links == NULL)
+		return NULL;
 	size_t fk_def_sz = fk_constraint_def_sizeof(link_count, name_len);
 	struct fk_constraint_def *fk_def =
 		(struct fk_constraint_def *) malloc(fk_def_sz);
 	if (fk_def == NULL) {
-		tnt_raise(OutOfMemory, fk_def_sz, "malloc",
+		diag_set(OutOfMemory, fk_def_sz, "malloc",
 			  "struct fk_constraint_def");
+		return NULL;
 	}
 	auto def_guard = make_scoped_guard([=] { free(fk_def); });
 	memcpy(fk_def->name, name, name_len);
@@ -4818,37 +4831,46 @@ fk_constraint_def_new_from_tuple(struct tuple *tuple, uint32_t errcode)
 					      name_len + 1);
 	memcpy(fk_def->links, links, link_count * sizeof(struct field_link));
 	fk_def->field_count = link_count;
-	fk_def->child_id = tuple_field_u32_xc(tuple,
-					      BOX_FK_CONSTRAINT_FIELD_CHILD_ID);
-	fk_def->parent_id =
-		tuple_field_u32_xc(tuple, BOX_FK_CONSTRAINT_FIELD_PARENT_ID);
-	fk_def->is_deferred =
-		tuple_field_bool_xc(tuple, BOX_FK_CONSTRAINT_FIELD_DEFERRED);
-	const char *match = tuple_field_str_xc(tuple,
-					       BOX_FK_CONSTRAINT_FIELD_MATCH,
-					       &name_len);
+	if (tuple_field_u32(tuple, BOX_FK_CONSTRAINT_FIELD_CHILD_ID,
+			    &(fk_def->child_id )) != 0)
+		return NULL;
+	if (tuple_field_u32(tuple, BOX_FK_CONSTRAINT_FIELD_PARENT_ID,
+			    &(fk_def->parent_id)) != 0)
+		return NULL;
+	if (tuple_field_bool(tuple, BOX_FK_CONSTRAINT_FIELD_DEFERRED,
+			     &(fk_def->is_deferred)) != 0)
+		return NULL;
+	const char *match = tuple_field_str(tuple,
+		BOX_FK_CONSTRAINT_FIELD_MATCH, &name_len);
+	if (match == NULL)
+		return NULL;
 	fk_def->match = STRN2ENUM(fk_constraint_match, match, name_len);
 	if (fk_def->match == fk_constraint_match_MAX) {
-		tnt_raise(ClientError, errcode, fk_def->name,
+		diag_set(ClientError, errcode, fk_def->name,
 			  "unknown MATCH clause");
+		return NULL;
 	}
-	const char *on_delete_action =
-		tuple_field_str_xc(tuple, BOX_FK_CONSTRAINT_FIELD_ON_DELETE,
-				   &name_len);
+	const char *on_delete_action = tuple_field_str(tuple,
+		BOX_FK_CONSTRAINT_FIELD_ON_DELETE, &name_len);
+	if (on_delete_action == NULL)
+		return NULL;
 	fk_def->on_delete = STRN2ENUM(fk_constraint_action,
 				      on_delete_action, name_len);
 	if (fk_def->on_delete == fk_constraint_action_MAX) {
-		tnt_raise(ClientError, errcode, fk_def->name,
+		diag_set(ClientError, errcode, fk_def->name,
 			  "unknown ON DELETE action");
+		return NULL;
 	}
-	const char *on_update_action =
-		tuple_field_str_xc(tuple, BOX_FK_CONSTRAINT_FIELD_ON_UPDATE,
-				   &name_len);
+	const char *on_update_action = tuple_field_str(tuple,
+		BOX_FK_CONSTRAINT_FIELD_ON_UPDATE, &name_len);
+	if (on_update_action == NULL)
+		return NULL;
 	fk_def->on_update = STRN2ENUM(fk_constraint_action,
 				      on_update_action, name_len);
 	if (fk_def->on_update == fk_constraint_action_MAX) {
-		tnt_raise(ClientError, errcode, fk_def->name,
+		diag_set(ClientError, errcode, fk_def->name,
 			  "unknown ON UPDATE action");
+		return NULL;
 	}
 	def_guard.is_active = false;
 	return fk_def;
@@ -5032,6 +5054,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 		struct fk_constraint_def *fk_def =
 			fk_constraint_def_new_from_tuple(new_tuple,
 							 ER_CREATE_FK_CONSTRAINT);
+		if (fk_def == NULL)
+			return -1;
 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
 		struct space *child_space = space_cache_find(fk_def->child_id);
 		if (child_space == NULL)
@@ -5185,6 +5209,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 		struct fk_constraint_def *fk_def =
 			fk_constraint_def_new_from_tuple(old_tuple,
 						ER_DROP_FK_CONSTRAINT);
+		if (fk_def == NULL)
+			return -1;
 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
 		struct space *child_space = space_cache_find(fk_def->child_id);
 		struct space *parent_space = space_cache_find(fk_def->parent_id);
@@ -5239,9 +5265,12 @@ ck_constraint_def_new_from_tuple(struct tuple *tuple)
 	const char *expr_str =
 		tuple_field_str_xc(tuple, BOX_CK_CONSTRAINT_FIELD_CODE,
 				   &expr_str_len);
+	bool out;
+	if (tuple_field_bool(tuple, BOX_CK_CONSTRAINT_FIELD_IS_ENABLED,
+			     &out) != 0)
+		diag_raise();
 	bool is_enabled =
-		tuple_field_count(tuple) <= BOX_CK_CONSTRAINT_FIELD_IS_ENABLED ||
-		tuple_field_bool_xc(tuple, BOX_CK_CONSTRAINT_FIELD_IS_ENABLED);
+		tuple_field_count(tuple) <= BOX_CK_CONSTRAINT_FIELD_IS_ENABLED || out;
 	struct ck_constraint_def *ck_def =
 		ck_constraint_def_new(name, name_len, expr_str, expr_str_len,
 				      space_id, language, is_enabled);
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 9f636a421..43078bd59 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -1118,27 +1118,6 @@ tuple_to_buf(struct tuple *tuple, char *buf, size_t size);
 #include "xrow_update.h"
 #include "errinj.h"
 
-/* @copydoc tuple_field_with_type() */
-static inline const char *
-tuple_field_with_type_xc(struct tuple *tuple, uint32_t fieldno,
-		         enum mp_type type)
-{
-	const char *out = tuple_field_with_type(tuple, fieldno, type);
-	if (out == NULL)
-		diag_raise();
-	return out;
-}
-
-/* @copydoc tuple_field_bool() */
-static inline bool
-tuple_field_bool_xc(struct tuple *tuple, uint32_t fieldno)
-{
-	bool out;
-	if (tuple_field_bool(tuple, fieldno, &out) != 0)
-		diag_raise();
-	return out;
-}
-
 /* @copydoc tuple_field_u32() */
 static inline uint32_t
 tuple_field_u32_xc(struct tuple *tuple, uint32_t fieldno)
-- 
2.17.1

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 17/20] refactoring: remove exceptions from fk_constraint_def_new_from_tuple

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]


Hi!

Yes, it happened due to changes in master. Fixed the patch. Sent it in-reply to your letter.

Sincerely,
Ilya Kosarev

>Среда, 13 ноября 2019, 16:34 +03:00 от Kirill Yukhin <kyukhin@tarantool.org>:
>
>Hello,
>
>On 23 сен 18:57, Ilya Kosarev wrote:
>> fk_constraint_def_new_from_tuple is used in
>> on_replace_dd_fk_constraint therefore it has to be cleared from
>> exceptions. Now it doesn't throw any more. It means we also need
>> to clear from exceptions it's subsidiary function: decode_fk_links.
>> Their usages are updated. Some _xc functions, not needed any more,
>> are removed.
>> 
>> Part of #4247
>
>Your patch gives compilation error on recent master.
>
>--
>Regards, Kirill Yukhin
>


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 1606 bytes --]

Re: [Tarantool-patches] [PATCH] refactoring: remove exceptions from fk_constraint_def_new_from_tuple

[Kirill Yukhin]

Hello,

On 13 ноя 16:49, Ilya Kosarev wrote:
> fk_constraint_def_new_from_tuple is used in
> on_replace_dd_fk_constraint therefore it has to be cleared from
> exceptions. Now it doesn't throw any more. It means we also need
> to clear from exceptions it's subsidiary function: decode_fk_links.
> Their usages are updated. Some _xc functions, not needed any more,
> are removed.
> 
> Part of #4247

You forgot to re-push the branch. Anyway, I've checked your
patch into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 12/20] refactoring: remove exceptions from space_def_new_from_tuple

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]


Hi!
After brief discussion with Nikita we found out that
sqlRunParser should set diagnostics for all error cases,
though it doesn't. Thus i submitted an issue:
https://github.com/tarantool/tarantool/issues/4633

Sincerely,
Ilya Kosarev


>Вторник, 12 ноября 2019, 18:32 +03:00 от Ilya Kosarev <i.kosarev@tarantool.org>:
>
>Hi!
>
>Thanks for your review!
>Guess we might need to confirm it with Pettik 
>or someone else who interacts with sql a lot,
>but i am quite sure sqlRunParser doesn't need
>to set diagnostics even in case of returning -1.
>So far, i think we need to set diagnostics here,
>in field_def_decode, like we do it in
>ck_constraint_new.
>Although it is not something i broke within
>this patchset, i will add it to follow-up.
>
>Sincerely,
>Ilya Kosarev
>
>
>>Вторник, 12 ноября 2019, 14:00 +03:00 от Sergey Ostanevich < sergos@tarantool.org >:
>>
>>Hi!
>>
>>Thanks for the patch. One follow-up below, LGTM .
>>
>>
>>Sergos
>>
>>>  	const char *dv = field->default_value;
>>> @@ -496,8 +507,9 @@ field_def_decode(struct field_def *field, const char **data,
>>>  		field->default_value_expr = sql_expr_compile(sql_get(), dv,
>>>  							     strlen(dv));
>>>  		if (field->default_value_expr == NULL)
>>> -			diag_raise();
>>> +			return -1;
>>
>>The sql_expr_compile() calls for sqlRunParser() that does not set diag
>>in all exit paths. Please, put into followup.
>>
>>
>
>
>-- 
>Ilya Kosarev
>field_def_decode

-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 4123 bytes --]

Re: [Tarantool-patches] [tarantool-patches] Re[2]: [tarantool-patches] Re: [PATCH v4 12/20] refactoring: remove exceptions from space_def_new_from_tuple

[Sergey Ostanevich]

Hi!

Thanks, Follow-up is fine - the patch is LGTM.

Sergos

On 19 Nov 18:16, Ilya Kosarev wrote:
> 
> Hi!
> After brief discussion with Nikita we found out that
> sqlRunParser should set diagnostics for all error cases,
> though it doesn't. Thus i submitted an issue:
> https://github.com/tarantool/tarantool/issues/4633
> 
> Sincerely,
> Ilya Kosarev
> 
> 
> >Вторник, 12 ноября 2019, 18:32 +03:00 от Ilya Kosarev <i.kosarev@tarantool.org>:
> >
> >Hi!
> >
> >Thanks for your review!
> >Guess we might need to confirm it with Pettik 
> >or someone else who interacts with sql a lot,
> >but i am quite sure sqlRunParser doesn't need
> >to set diagnostics even in case of returning -1.
> >So far, i think we need to set diagnostics here,
> >in field_def_decode, like we do it in
> >ck_constraint_new.
> >Although it is not something i broke within
> >this patchset, i will add it to follow-up.
> >
> >Sincerely,
> >Ilya Kosarev
> >
> >
> >>Вторник, 12 ноября 2019, 14:00 +03:00 от Sergey Ostanevich < sergos@tarantool.org >:
> >>
> >>Hi!
> >>
> >>Thanks for the patch. One follow-up below, LGTM .
> >>
> >>
> >>Sergos
> >>
> >>>  	const char *dv = field->default_value;
> >>> @@ -496,8 +507,9 @@ field_def_decode(struct field_def *field, const char **data,
> >>>  		field->default_value_expr = sql_expr_compile(sql_get(), dv,
> >>>  							     strlen(dv));
> >>>  		if (field->default_value_expr == NULL)
> >>> -			diag_raise();
> >>> +			return -1;
> >>
> >>The sql_expr_compile() calls for sqlRunParser() that does not set diag
> >>in all exit paths. Please, put into followup.
> >>
> >>
> >
> >
> >-- 
> >Ilya Kosarev
> >field_def_decode
> 
> -- 
> Ilya Kosarev