From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7D8C0469719 for ; Fri, 6 Mar 2020 19:37:48 +0300 (MSK) References: <20200306125547.GB12214@tarantool.org> From: Oleg Babin Message-ID: <512041fe-9a7b-6d99-6535-6197efc66ace@tarantool.org> Date: Fri, 6 Mar 2020 19:37:47 +0300 MIME-Version: 1.0 In-Reply-To: <20200306125547.GB12214@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/2] box: introduce internal sequence error List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: Oleg Babin , tarantool-patches@dev.tarantool.org, v.shpilevoy@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 >> >> 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 >>