[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area

[Konstantin Osipov <kostja@tarantool.org>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/23 13:03]:
>  static inline void
> @@ -90,9 +92,11 @@ static inline void
>  error_unref(struct error *e)
>  {
>  	assert(e->refs > 0);
> -	--e->refs;
> -	if (e->refs == 0)
> +	if (--e->refs == 0) {
> +		if (e->reason != NULL)
> +			error_unref(e->reason);
>  		e->destroy(e);
> +	}

This looks fragile, can you rewrite this place without recursion?
> + * Remove all errors set in a given diagnostics area after a
> + * given savepoint.
> + *
> + * Operation removes reason for the error
> + * preceding the savepoint and releases a diagnostic area's
> + * reference on the most recent error (diag::last for the
> + * rollback beginning). This means that if user code have a
> + * pointer and have a reference to an error object from the
> + * rollback zone, this pointer and the following "reason" error
> + * objects are a valid error list.
> + */

BTW, did you check if anyone likes the name 'reason'? I'm not very
fond of it, previous error is not always the reason of the current
one. I would simply call it 'prev' or 'next'.

> +	if (diag->last != begin) {
> +		assert(prev != NULL && prev->reason == svp);
> +		prev->reason = NULL;
> +		error_unref(begin);

OK, this works because you terminate the list with NULL first...

> +		prev->reason = svp;
> +		error_ref(svp);
> +	}
> +}
> +
> +++ b/src/box/key_list.c
> @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (rc != 0) {
>  		/* Can't evaluate function. */
>  		struct space *space = space_by_id(index_def->space_id);
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't evaluate function");
>  		return -1;
>  	}
>  	uint32_t key_data_sz;
> @@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (key_data == NULL) {
>  		struct space *space = space_by_id(index_def->space_id);
>  		/* Can't get a result returned by function . */
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't get a value returned by function");
>  		return -1;
>  	}

Nice. I bet there is a bunch of places like that, forgotten by
now.
With these two comments, LGTM.

-- 
Konstantin Osipov, Moscow, Russia