Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write()

[Nikita Pettik <korablev@tarantool.org>]

On 21 May 16:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 21/05/2020 03:15, Nikita Pettik wrote:
> > Assert in vy_tx_write() validates that upsert applied on the top of
> > cached statement is always replace. In fact, it is not always so. If
> > vy_apply_upsert() fails due to the fact that PK is modified, it returns
> > old statement (i.e. statement which upsert is applied on). In this
> > regard, if tuple cache contains insert and invalid upsert is executed,
> > vy_apply_upsert() will return insert. As a result, assert will fire.
> > Let's fix it and take into account that vy_apply_upsert() is able to
> > return inserts as well.
> 
> I looked at other places in vinyl, which apply an upsert, and found
> vy_lsm_commit_upsert() in vy_lsm.c:1018. The result is also always
> assumed to be REPLACE. Is it correct?

No, it's not a case. If vy_apply_upsert() fails due to PK modification,
it will return old_stmt. In turn old_stmt has different LSN, so flaw
returns before this assert:

  1030			if (upserted_lsn != lsn) {
  1031				/**
  1032				 * This could only happen if the upsert completely
  1033				 * failed and the old tuple was returned.
  1034				 * In this case we shouldn't insert the same replace
  1035				 * again.
  1036				 */
  1037				assert(older.stmt == NULL ||
  1038				       upserted_lsn == vy_stmt_lsn(older.stmt));
  1039				tuple_unref(upserted.stmt);
  1040				return;
  1041			}

Note that they must have different LSN since they can't come in one
tx set. Otherwise, during apply_upsert() in vy_tx_set_entry() upsert
turns into old_stmt (insert) and simply skipped (due to is_overwritten
flag) during statement commit.