Re: [Tarantool-patches] [PATCH 1/2] box: introduce internal sequence error

[Nikita Pettik <korablev@tarantool.org>]

On 06 Mar 14:46, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> This patch introduces SequenceError. It is thrown
> in case when we try to get current value of sequence
> before it was initialized ("next" was called at least once)

Why do you need separate type class for this error?
All sequence errors are of ClientError class, so why
don't you simply use it? Otherwise, it would be nice
to make all sequence related errors be of SequenceError class.
 
> Need for #4752
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4752
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current
>  src/box/sequence.c        | 12 ++++++++----
>  src/box/sequence.h        |  7 ++++---
>  src/box/sql/vdbe.c        |  5 ++++-
>  src/lib/core/diag.h       |  2 ++
>  src/lib/core/exception.cc | 25 +++++++++++++++++++++++++
>  src/lib/core/exception.h  |  6 ++++++
>  6 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/sequence.c b/src/box/sequence.c
> index 5ebfa2747..617b7ca38 100644
> --- a/src/box/sequence.c
> +++ b/src/box/sequence.c
> @@ -367,14 +367,18 @@ sequence_data_iterator_create(void)
>  	return &iter->base;
>  }
>  
> -int64_t
> -sequence_get_value(struct sequence *seq)
> +int
> +sequence_get_value(struct sequence *seq, int64_t *result)
>  {
>  	uint32_t key = seq->def->id;
>  	uint32_t hash = sequence_hash(key);
>  	uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
> -	assert(pos != light_sequence_end);
> +	if (pos == light_sequence_end) {
> +		diag_set(SequenceError, "Sequence is not initialized yet");
> +		return -1;
> +	}
>  	struct sequence_data data = light_sequence_get(&sequence_data_index,
>  						       pos);
> -	return data.value;
> +	*result = data.value;
> +	return 0;
>  }
> diff --git a/src/box/sequence.h b/src/box/sequence.h
> index a164da9af..8c442872a 100644
> --- a/src/box/sequence.h
> +++ b/src/box/sequence.h
> @@ -171,10 +171,11 @@ sequence_data_iterator_create(void);
>   * Get last element of given sequence.
>   *
>   * @param seq sequence to get value from.
> - * @retval last element of sequence.
> + * @result value of sequence.
> + * Return 0 on success, -1 if sequence is not initialized.
>   */
> -int64_t
> -sequence_get_value(struct sequence *seq);
> +int
> +sequence_get_value(struct sequence *seq, int64_t *result);
>  
>  #if defined(__cplusplus)
>  } /* extern "C" */
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 912a10153..0de07ca1d 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4328,7 +4328,10 @@ case OP_IdxInsert: {
>  		p->nChange++;
>  	if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
>  		assert(space->sequence != NULL);
> -		int64_t value = sequence_get_value(space->sequence);
> +		int64_t value;
> +		if (sequence_get_value(space->sequence, &value) != 0) {
> +			goto abort_due_to_error;
> +		}
>  		if (vdbe_add_new_autoinc_id(p, value) != 0)
>  			goto abort_due_to_error;
>  	}
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index f763957c2..e19b6b824 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -259,6 +259,8 @@ struct error *
>  BuildSwimError(const char *file, unsigned line, const char *format, ...);
>  struct error *
>  BuildCryptoError(const char *file, unsigned line, const char *format, ...);
> +struct error *
> +BuildSequenceError(const char *file, unsigned line, const char *format, ...);
>  
>  struct index_def;
>  
> diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
> index 76dcea553..53b86291f 100644
> --- a/src/lib/core/exception.cc
> +++ b/src/lib/core/exception.cc
> @@ -287,6 +287,19 @@ CryptoError::CryptoError(const char *file, unsigned line,
>  	va_end(ap);
>  }
>  
> +const struct type_info type_SequenceError =
> +		make_type("SequenceError", &type_Exception);
> +
> +SequenceError::SequenceError(const char *file, unsigned line,
> +						 const char *format, ...)
> +		: Exception(&type_SequenceError, file, line)
> +{
> +	va_list ap;
> +	va_start(ap, format);
> +	error_vformat_msg(this, format, ap);
> +	va_end(ap);
> +}
> +
>  #define BuildAlloc(type)				\
>  	void *p = malloc(sizeof(type));			\
>  	if (p == NULL)					\
> @@ -390,6 +403,18 @@ BuildCryptoError(const char *file, unsigned line, const char *format, ...)
>  	return e;
>  }
>  
> +struct error *
> +BuildSequenceError(const char *file, unsigned line, const char *format, ...)
> +{
> +	BuildAlloc(CryptoError);
> +	SequenceError *e =  new (p) SequenceError(file, line, "");
> +	va_list ap;
> +	va_start(ap, format);
> +	error_vformat_msg(e, format, ap);
> +	va_end(ap);
> +	return e;
> +}
> +
>  struct error *
>  BuildSocketError(const char *file, unsigned line, const char *socketname,
>  		 const char *format, ...)
> diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
> index d6154eb32..d9c4542da 100644
> --- a/src/lib/core/exception.h
> +++ b/src/lib/core/exception.h
> @@ -168,6 +168,12 @@ public:
>  	virtual void raise() { throw this; }
>  };
>  
> +class SequenceError: public Exception {
> +public:
> +	SequenceError(const char *file, unsigned line, const char *format, ...);
> +	virtual void raise() { throw this; }
> +};
> +
>  /**
>   * Initialize the exception subsystem.
>   */
> -- 
> 2.23.0
>