[Tarantool-patches] [PATCH 0/2] Validate result of upserts squash & skip invalid upserts which can't be applied

[Nikita Pettik]

On 26 May 23:33, Vladislav Shpilevoy wrote:
> Thanks for the patch, nice bug you found!
> 
> > Author: Nikita Pettik <korablev at tarantool.org>
> > Date:   Fri May 22 04:38:45 2020 +0300
> > 
> >     vinyl: handle invalid upserts in background squash process
> >     
> >     When L0 collects more than certain number of upserts (to be more precise
> >     VY_UPSERT_THRESHOLD == 128), background process is launched to squash
> >     them. During this squash, new statements could be inserted into L0.
> >     Until committed prepared statements feature lsn = MAX_LSN as a special
> >     marker.
> 
> Did you mean 'until' -> 'while'?
> 
> >     It is assumed that all upserts will be successfully squashed. As
> >     a result statements with given key in L0 will be able to have only
> >     MAX_LSN lsn (except for the squash result).
> 
> Honestly, I didn't understand anything from this. I got more info from
> reading squashing code source. Probably this needs some rewording. For
> example, in the second sentence say 'After squash it is expected all
> newer statements on top of the squashed tuple have MAX_LSN, i.e. they
> are prepared but not committed. Otherwise they would end up being squashed
> too.'

Ok, sorry, probably it was really messy one. Look at this version:

vinyl: handle invalid upserts in background squash process

When L0 collects more than certain number of upserts (to be more precise
VY_UPSERT_THRESHOLD == 128), background process is launched to squash
them. Squash process does not prevent from handling new statements
being inserted in L0. Prepared (but still not committed) statements
feature lsn >= MAX_LSN as a special marker.
It is assumed (by mistake) that all upserts will be successfully
squashed. According to this assumption, the previous statement to squash
result is considered to have lsn >= MAX_LSN (as it is prepared but
still not committed; if it has been already committed then it is
affected by squash process). Obviously, it is not so if squashing fails.
In this case upserts which are not squashed remain in L0 but have normal
(i.e. < MAX_LSN) lsn. So the assertion which verifies that previous
statements are prepared but not committed is wrong.
Moreover, if we execute enough (more than threshold limit) invalid upserts,
they will prevent from squashing valid upserts. So let's skip invalid
upserts and don't account them while calculating number of upserts
residing in L0.

Follow-up #1622

> 
> >     However, it is not so if at
> >     least one upsert is failed to be applied: it will get stack in L0 until
> 
> stack -> stuck? The same in the code comments.

Fixed.

> > diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> > index 55a4c8fb7..b39e3b9d8 100644
> > --- a/src/box/vinyl.c
> > +++ b/src/box/vinyl.c
> > @@ -3539,10 +3539,21 @@ vy_squash_process(struct vy_squash *squash)
> >                 if (vy_tuple_compare(result, mem_stmt, lsm->cmp_def) != 0 ||
> >                     vy_stmt_type(mem_stmt) != IPROTO_UPSERT)
> >                         break;
> > -               assert(vy_stmt_lsn(mem_stmt) >= MAX_LSN);
> > -               vy_stmt_set_n_upserts((struct tuple *)mem_stmt, n_upserts);
> > -               if (n_upserts <= VY_UPSERT_THRESHOLD)
> > -                       ++n_upserts;
> > +               /*
> > +                * Upsert was not applied in vy_history_apply(),
> > +                * but it still resides in tree memory.
> 
> Could we remove these upserts from memory right away? The code below was not
> simple beforehand, and now looks even more complex. I am looking for a way
> to simplify it anyhow.
> 
> Could vy_tx_write() fix help? If you would skip them there instead of writing
> them to the tree anyway. Or is it something totally not related?

These invalid upserts are already committed and residing in tree.
The only way to delete them is to generate DELETE statement and
insert it to the tree. I guess I can try this approach (I considered
it while was preparing this patch) but code definitely won't become
simpler (that's why I've choosen current one).