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

[Nikita Pettik <korablev@tarantool.org>]

On 14 May 23:32, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> On 14/05/2020 04:21, Nikita Pettik wrote:
> > On 01 May 02:31, Vladislav Shpilevoy wrote:
> >> Hi! Thanks for the patch!
> >>
> >> Firstly, Kostja left some comments here. Would be cool to address them.
> > 
> > Done (sorry, I did not ignore them, just had to work on other more vital bugs).
> >  
> >> Secondly, here is my personal opinion. I don't like just skipping things
> >> a user committed without any error appearing in the application. IMO, we
> >> should apply only the first commit. And let a user see this error so as he
> >> could notice the problem. To fix reads he could do delete() of the bad key.
> > 
> > The problem with delete it leaves user no way to restore the rest
> > of upsert history. Moreover, these upserts will get stuck until
> > user finds in logs corresponding error (I guess we can't abort
> > compaction due to invalid upserts).
> > 
> >> However, how a user will be able to find the exact broken key - I don't
> >> know. Maybe the ignore + logging is better.
> > 
> > Why can't we just log broken key? E.g. see logs in vy_apply_upsert().
> 
> We can log it. This is what you do in this patchset.
> 
> I also noticed, that you skip the failed upsert in case of any error.
> Even if it is an OOM, not related to format problems. I think it is safer
> to check error type, and skip it only in case of a ClientError.

Personally I don't like relying on error type, but this pattern is
already in the source code (e.g. upsert_do_ops), so here's diff:

diff --git a/src/box/vy_history.c b/src/box/vy_history.c
index 8c4719a85..2232348f9 100644
--- a/src/box/vy_history.c
+++ b/src/box/vy_history.c
@@ -109,6 +109,9 @@ vy_history_apply(struct vy_history *history, struct key_def *cmp_def,
                         * 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;
                } else {
                        ++*upserts_applied;
                        if (curr_stmt != NULL)
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index aefad5942..1a2ea43d9 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -1050,6 +1050,8 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
                if (applied == NULL) {
                        struct error *e = diag_last_error(diag_get());
                        assert(e != NULL);
+                       if (e->type != &type_ClientError)
+                               return -1;
                        error_log(e);
                } else {
                        lsm->stat.upsert.applied++;
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 13d5bbede..4d34d96c8 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -857,6 +857,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
                         */
                        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);
@@ -878,6 +880,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
                if (applied == NULL) {
                        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);
                } else {