Re: [Tarantool-patches] [PATCH 2/2] vinyl: skip invalid upserts during squash

[Nikita Pettik <korablev@tarantool.org>]

On 26 May 23:33, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/vy_history.c b/src/box/vy_history.c
> > index 0f3b71195..2232348f9 100644
> > --- a/src/box/vy_history.c
> > +++ b/src/box/vy_history.c
> > @@ -102,12 +102,22 @@ vy_history_apply(struct vy_history *history, struct key_def *cmp_def,
> >  	while (node != NULL) {
> >  		struct tuple *stmt = vy_apply_upsert(node->stmt, curr_stmt,
> >  						     cmp_def, format, true);
> > -		++*upserts_applied;
> > -		if (curr_stmt != NULL)
> > -			tuple_unref(curr_stmt);
> > -		if (stmt == NULL)
> > -			return -1;
> > -		curr_stmt = stmt;
> > +		if (stmt == NULL) {
> > +			/*
> > +			 * In case statement hasn't been applied,
> > +			 * simply skip it ignoring errors (otherwise
> > +			 * error will appear during tuple read).
> > +			 */
> > +			assert(diag_last_error(diag_get()) != NULL);
> > +			struct error *e = diag_last_error(diag_get());
> > +			if (e->type != &type_ClientError)
> > +				return -1;
> 
> *. Shouldn't we log here?

No, we shouldn't. vy_history_apply() is called only during reads.
So in order to avoid spam the same error each time on data selection,
we don't log it here.
 
> > diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> > index 04c9926a8..3d630fc01 100644
> > --- a/src/box/vy_lsm.c
> > +++ b/src/box/vy_lsm.c
> > @@ -984,13 +984,20 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem,
> >  		struct tuple *upserted =
> >  			vy_apply_upsert(stmt, older, lsm->cmp_def,
> >  					lsm->mem_format, false);
> > -		lsm->stat.upsert.applied++;
> > -
> >  		if (upserted == NULL) {
> > -			/* OOM */
> > +			/*
> > +			 * OOM or upserted tuple does not fulfill
> > +			 * space format. Hence, upsert can't be
> > +			 * transformed into replace. Log error
> > +			 * and exit.
> > +			 */
> > +			struct error *e = diag_last_error(diag_get());
> > +			assert(e != NULL);
> > +			error_log(e);
> 
> *. These three lines can be replaced with diag_log();.

Ok, let's use diag_log():

diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 3d630fc01..c768d0229 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -991,10 +991,7 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem,
                         * transformed into replace. Log error
                         * and exit.
                         */
-                       struct error *e = diag_last_error(diag_get());
-                       assert(e != NULL);
-                       error_log(e);
-                       diag_clear(diag_get());
+                       diag_log();
                        return;
                }
                lsm->stat.upsert.applied++;
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 1a2ea43d9..0a14478fc 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -516,9 +516,7 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem,
                                tuple_unref(applied);
                                return rc;
                        } else {
-                               struct error *e = diag_last_error(diag_get());
-                               assert(e != NULL);
-                               error_log(e);
+                               diag_log();
                        }
                }
        } else {


> > diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> > index 5029bd8a1..1a2ea43d9 100644
> > --- a/src/box/vy_tx.c
> > +++ b/src/box/vy_tx.c
> > @@ -515,11 +515,11 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem,
> >  						    region_stmt);
> >  				tuple_unref(applied);
> >  				return rc;
> > +			} else {
> > +				struct error *e = diag_last_error(diag_get());
> > +				assert(e != NULL);
> > +				error_log(e);
> 
> *. The same here.

See diff above.
 
> > diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> > index 7a6a20627..4d34d96c8 100644
> > --- a/src/box/vy_write_iterator.c
> > +++ b/src/box/vy_write_iterator.c
> > @@ -850,10 +850,23 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
> >  		       vy_stmt_type(hint) != IPROTO_UPSERT);
> >  		struct tuple *applied = vy_apply_upsert(h->tuple, hint,
> >  				stream->cmp_def, stream->format, false);
> > -		if (applied == NULL)
> > -			return -1;
> > -		vy_stmt_unref_if_possible(h->tuple);
> > -		h->tuple = applied;
> > +		if (applied == NULL) {
> > +			/*
> > +			 * Current upsert can't be applied.
> > +			 * Let's skip it and log error.
> > +			 */
> > +			struct error *e = diag_last_error(diag_get());
> > +			assert(e != NULL);
> > +			if (e->type != &type_ClientError)
> > +				return -1;
> > +			say_info("upsert %s is not applied due to the error: %s",
> > +				 vy_stmt_str(h->tuple), e->errmsg);
> > +			vy_stmt_unref_if_possible(h->tuple);
> 
> *. Why here we use sometimes say_info() and sometimes error_log()? Why
> not something one?

Indeed, let's use here say_error():

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 4d34d96c8..ce700f260 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -859,8 +859,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
                        assert(e != NULL);
                        if (e->type != &type_ClientError)
                                return -1;
-                       say_info("upsert %s is not applied due to the error: %s",
-                                vy_stmt_str(h->tuple), e->errmsg);
+                       say_error("upsert %s is not applied due to the error: %s",
+                                 vy_stmt_str(h->tuple), e->errmsg);
                        vy_stmt_unref_if_possible(h->tuple);
                        h->tuple = hint;
                } else {
@@ -882,8 +882,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
                        assert(e != NULL);
                        if (e->type != &type_ClientError)
                                return -1;
-                       say_info("upsert %s is not applied due to the error: %s",
-                                vy_stmt_str(h->tuple), e->errmsg);
+                       say_error("upsert %s is not applied due to the error: %s",
+                                 vy_stmt_str(h->tuple), e->errmsg);
                } else {
                        vy_stmt_unref_if_possible(result->tuple);
                        result->tuple = applied;