Re: [Tarantool-patches] [PATCH 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Nikita Pettik <korablev@tarantool.org>]

On 11 Apr 19:39, Vladislav Shpilevoy wrote:
> >>> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> >>> index 3d3f41b7a..04c9926a8 100644
> >>> --- a/src/box/vy_lsm.c
> >>> +++ b/src/box/vy_lsm.c
> >>> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
> >>>  {
> >>>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
> >>>  		bsize;
> >>> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> >>> +	if (inj != NULL && inj->iparam >= 0) {
> >>> +		if (inj->iparam-- == 0)
> >>
> >> 1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
> >> which would set it to false instead of decrement? That would make it
> >> clear the injection works only once.
> > 
> > Cause integer allows setting delay of vy_stmt_alloc() failure.
> > For instance, I don't want first invocation to vy_stmt_alloc()
> > fail, but the second, third or tenth one - it may turn out to be
> > vital. This patch fixes bug when first call of vy_stmt_alloc()
> > during compaction fails; the next patch - if tenth call of
> > vy_stmt_alloc() fails.
> 
> Nope, in the next patch you use 0 too. Moreover, when I changed it
> to 10, I got the test hanging in 100% CPU. Regardless of with the
> fix or without.

It should have been 10. Kind of strange since it is exactly this
value that helped me to reveal this bug. Mb it is still unpredictable
consequences of invalid memory access. I will investigate and test on
my mac before next updates. Thx.
 
> >> Also it looks too artificial. The injection basically simulates a tuple
> >> with too big size which was inserted bypassing max_tuple_size check,
> >> and suddenly it was checked here, already after insertion.
> > 
> > Konstantint said, that squashing two upserts of size 'x' may result
> > in new vy_stmt with size > 'x'. Despite the fact that I did not
> > attempt at reproducing this statement, I saw these errors appearing
> > on production machine during compaction. I do not know the exact reason
> > why they revealed, but it is a fact.
> 
> And still this particular test does not use any upserts. So OOM here
> is more likely to happen than max tuple size violation.
> 
> >> Better add an OOM injection for malloc a few lines below, would be more
> >> correct.