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

[Oleg Babin <olegrok@tarantool.org>]

Thanks for your review!
I've sent the second version of this patch. Seems similar function 
existed untill 
https://github.com/tarantool/tarantool/commit/3ff1f1e36e14381c0ebb5862943d4da281254767

I've change an error type to ClientError and modified tests a bit.

On 06/03/2020 15:55, Nikita Pettik wrote:
> 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
>>