Re: [Tarantool-patches] [PATCH v4 01/16] sql: remove MEM_Zero flag from struct MEM

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sorry, I wrote wrong answer. Fixed, see below.

On Tue, Oct 05, 2021 at 11:46:57AM +0300, Mergen Imeev via Tarantool-patches wrote:
> Hi! Thank you for the review! my answers, diff and new patch below.
> 
> On Mon, Oct 04, 2021 at 11:51:42PM +0200, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patch!
> > 
> > See 2 comments below.
> > 
> > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > > index 29d713fd0..ff0c461ce 100644
> > > --- a/src/box/sql/func.c
> > > +++ b/src/box/sql/func.c
> > > @@ -1272,11 +1272,19 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
> > >  	n = mem_get_int_unsafe(argv[0]);
> > >  	if (n < 0)
> > >  		n = 0;
> > > -	if (sql_result_zeroblob64(context, n) != 0) {
> > > +	if (n > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
> > >  		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
> > >  			 "is too big");
> > >  		context->is_aborted = true;
> > > +		return;
> > > +	}
> > > +	char *str = sqlDbMallocRawNN(sql_get(), n);
> > > +	memset(str, 0, n);
> > 
> > 1. There is sqlDbMallocZero().
> > 
> Thanks, fixed.
> 
> > > +	if (str == NULL) {
> > > +		context->is_aborted = true;
> > > +		return;
> > >  	}
> > > +	mem_set_bin_allocated(context->pOut, str, n);
> > >  }
> > >  
> > >  /*
> > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > > index 5e23c901c..24d6d7dbf 100644
> > > --- a/src/box/sql/mem.c
> > > +++ b/src/box/sql/mem.c
> > > @@ -1869,8 +1850,6 @@ mem_get_bin(const struct Mem *mem, const char **s)
> > >  		*s = mem->n > 0 ? mem->z : NULL;
> > >  		return 0;
> > >  	}
> > > -	if (mem->type != MEM_TYPE_BIN || (mem->flags & MEM_Zero) != 0)
> > > -		return -1;
> > 
> > 2. If the type is not MEM_TYPE_BIN, you will return garbage. Maybe
> > keep return -1 for other types?
> > 
Thanks, fixed.

> True, fixed. There is a case where the behavior will be different - I
> mean the case where new_size < szMalloc and new_size + n > szMalloc,
> but that doesn't seem to be a very common case.
> 
> > >  	*s = mem->z;
> > >  	return 0;
> > >  }
> 
> 
> Diff:
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index ff0c461ce..187456d2c 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1278,8 +1278,7 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
>  		context->is_aborted = true;
>  		return;
>  	}
> -	char *str = sqlDbMallocRawNN(sql_get(), n);
> -	memset(str, 0, n);
> +	char *str = sqlDbMallocZero(sql_get(), n);
>  	if (str == NULL) {
>  		context->is_aborted = true;
>  		return;
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 24d6d7dbf..117a44314 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1850,6 +1850,8 @@ mem_get_bin(const struct Mem *mem, const char **s)
>  		*s = mem->n > 0 ? mem->z : NULL;
>  		return 0;
>  	}
> +	if (mem->type != MEM_TYPE_BIN)
> +		return -1;
>  	*s = mem->z;
>  	return 0;
>  }
> 
> 
> New patch:
> 
> commit 37ccef0873f7d62271d0c46352f8f9cbfb73c22f
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Oct 1 14:05:23 2021 +0300
> 
>     sql: remove MEM_Zero flag from struct MEM
>     
>     This patch removes zeroblob optimization from SQL code. This
>     optimization complicates the code, and there is almost no profit from
>     it.
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 29d713fd0..187456d2c 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1272,11 +1272,18 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
>  	n = mem_get_int_unsafe(argv[0]);
>  	if (n < 0)
>  		n = 0;
> -	if (sql_result_zeroblob64(context, n) != 0) {
> +	if (n > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
>  		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
>  			 "is too big");
>  		context->is_aborted = true;
> +		return;
> +	}
> +	char *str = sqlDbMallocZero(sql_get(), n);
> +	if (str == NULL) {
> +		context->is_aborted = true;
> +		return;
>  	}
> +	mem_set_bin_allocated(context->pOut, str, n);
>  }
>  
>  /*
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 5e23c901c..117a44314 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -503,19 +503,6 @@ mem_copy_bin(struct Mem *mem, const char *value, uint32_t size)
>  	return 0;
>  }
>  
> -void
> -mem_set_zerobin(struct Mem *mem, int n)
> -{
> -	mem_destroy(mem);
> -	if (n < 0)
> -		n = 0;
> -	mem->u.nZero = n;
> -	mem->z = NULL;
> -	mem->n = 0;
> -	mem->type = MEM_TYPE_BIN;
> -	mem->flags = MEM_Zero;
> -}
> -
>  static inline void
>  set_msgpack_value(struct Mem *mem, char *value, uint32_t size, int alloc_type,
>  		  enum mem_type type)
> @@ -806,8 +793,6 @@ static inline int
>  bin_to_str(struct Mem *mem)
>  {
>  	assert(mem->type == MEM_TYPE_BIN);
> -	if (ExpandBlob(mem) != 0)
> -		return -1;
>  	mem->type = MEM_TYPE_STR;
>  	mem->flags &= ~MEM_Scalar;
>  	return 0;
> @@ -817,8 +802,6 @@ static inline int
>  bin_to_str0(struct Mem *mem)
>  {
>  	assert(mem->type == MEM_TYPE_BIN);
> -	if (ExpandBlob(mem) != 0)
> -		return -1;
>  	if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
>  		return -1;
>  	mem->z[mem->n] = '\0';
> @@ -831,8 +814,6 @@ static inline int
>  bin_to_uuid(struct Mem *mem)
>  {
>  	assert(mem->type == MEM_TYPE_BIN);
> -	if (ExpandBlob(mem) != 0)
> -		return -1;
>  	if (mem->n != UUID_LEN ||
>  	    tt_uuid_validate((struct tt_uuid *)mem->z) != 0)
>  		return -1;
> @@ -1869,7 +1850,7 @@ mem_get_bin(const struct Mem *mem, const char **s)
>  		*s = mem->n > 0 ? mem->z : NULL;
>  		return 0;
>  	}
> -	if (mem->type != MEM_TYPE_BIN || (mem->flags & MEM_Zero) != 0)
> +	if (mem->type != MEM_TYPE_BIN)
>  		return -1;
>  	*s = mem->z;
>  	return 0;
> @@ -1880,11 +1861,7 @@ mem_len(const struct Mem *mem, uint32_t *len)
>  {
>  	if (!mem_is_bytes(mem))
>  		return -1;
> -	assert((mem->flags & MEM_Zero) == 0 || mem->type == MEM_TYPE_BIN);
> -	if ((mem->flags & MEM_Zero) != 0)
> -		*len = mem->n + mem->u.nZero;
> -	else
> -		*len = mem->n;
> +	*len = mem->n;
>  	return 0;
>  }
>  
> @@ -1910,9 +1887,6 @@ mem_copy(struct Mem *to, const struct Mem *from)
>  		return 0;
>  	if ((to->flags & MEM_Static) != 0)
>  		return 0;
> -	assert((to->flags & MEM_Zero) == 0 || to->type == MEM_TYPE_BIN);
> -	if ((to->flags & MEM_Zero) != 0)
> -		return sqlVdbeMemExpandBlob(to);
>  	to->zMalloc = sqlDbRealloc(to->db, to->zMalloc, MAX(32, to->n));
>  	assert(to->zMalloc != NULL || sql_get()->mallocFailed != 0);
>  	if (to->zMalloc == NULL)
> @@ -1937,7 +1911,7 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
>  		return;
>  	if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
>  		return;
> -	to->flags &= MEM_Term | MEM_Zero;
> +	to->flags &= MEM_Term;
>  	to->flags |= MEM_Ephem;
>  	return;
>  }
> @@ -1954,7 +1928,7 @@ mem_move(struct Mem *to, struct Mem *from)
>  }
>  
>  int
> -mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
> +mem_concat(const struct Mem *a, const struct Mem *b, struct Mem *result)
>  {
>  	if (mem_is_any_null(a, b)) {
>  		mem_set_null(result);
> @@ -1981,9 +1955,6 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
>  		return -1;
>  	}
>  
> -	if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
> -		return -1;
> -
>  	uint32_t size = a->n + b->n;
>  	if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
>  		diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
> @@ -2330,37 +2301,8 @@ static int
>  mem_cmp_bin(const struct Mem *a, const struct Mem *b)
>  {
>  	assert((a->type & b->type & MEM_TYPE_BIN) != 0);
> -	int an = a->n;
> -	int bn = b->n;
> -	int minlen = MIN(an, bn);
> -
> -	/*
> -	 * It is possible to have a Blob value that has some non-zero content
> -	 * followed by zero content.  But that only comes up for Blobs formed
> -	 * by the OP_MakeRecord opcode, and such Blobs never get passed into
> -	 * mem_compare().
> -	 */
> -	assert((a->flags & MEM_Zero) == 0 || an == 0);
> -	assert((b->flags & MEM_Zero) == 0 || bn == 0);
> -
> -	if ((a->flags & b->flags & MEM_Zero) != 0)
> -		return a->u.nZero - b->u.nZero;
> -	if ((a->flags & MEM_Zero) != 0) {
> -		for (int i = 0; i < minlen; ++i) {
> -			if (b->z[i] != 0)
> -				return -1;
> -		}
> -		return a->u.nZero - bn;
> -	}
> -	if ((b->flags & MEM_Zero) != 0) {
> -		for (int i = 0; i < minlen; ++i) {
> -			if (a->z[i] != 0)
> -				return 1;
> -		}
> -		return b->u.nZero - an;
> -	}
> -	int res = memcmp(a->z, b->z, minlen);
> -	return res != 0 ? res : an - bn;
> +	int res = memcmp(a->z, b->z, MIN(a->n, b->n));
> +	return res != 0 ? res : a->n - b->n;
>  }
>  
>  static int
> @@ -2797,10 +2739,6 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
>  		}
>  		sql_snprintf(100, zCsr, "]%s", "(8)");
>  		zCsr += sqlStrlen30(zCsr);
> -		if (f & MEM_Zero) {
> -			sql_snprintf(100, zCsr,"+%dz",pMem->u.nZero);
> -			zCsr += sqlStrlen30(zCsr);
> -		}
>  		*zCsr = '\0';
>  	} else if (pMem->type == MEM_TYPE_STR) {
>  		int j, k;
> @@ -2886,32 +2824,6 @@ registerTrace(int iReg, Mem *p) {
>  }
>  #endif
>  
> -/*
> - * If the given Mem* has a zero-filled tail, turn it into an ordinary
> - * blob stored in dynamically allocated space.
> - */
> -int
> -sqlVdbeMemExpandBlob(Mem * pMem)
> -{
> -	int nByte;
> -	assert(pMem->flags & MEM_Zero);
> -	assert(pMem->type == MEM_TYPE_BIN);
> -
> -	/* Set nByte to the number of bytes required to store the expanded blob. */
> -	nByte = pMem->n + pMem->u.nZero;
> -	if (nByte <= 0) {
> -		nByte = 1;
> -	}
> -	if (sqlVdbeMemGrow(pMem, nByte, 1)) {
> -		return -1;
> -	}
> -
> -	memset(&pMem->z[pMem->n], 0, pMem->u.nZero);
> -	pMem->n += pMem->u.nZero;
> -	pMem->flags &= ~(MEM_Zero | MEM_Term);
> -	return 0;
> -}
> -
>  static int
>  sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve)
>  {
> @@ -3036,13 +2948,8 @@ int
>  sqlVdbeMemTooBig(Mem * p)
>  {
>  	assert(p->db != 0);
> -	if (mem_is_bytes(p)) {
> -		int n = p->n;
> -		if (p->flags & MEM_Zero) {
> -			n += p->u.nZero;
> -		}
> -		return n > p->db->aLimit[SQL_LIMIT_LENGTH];
> -	}
> +	if (mem_is_bytes(p))
> +		return p->n > p->db->aLimit[SQL_LIMIT_LENGTH];
>  	return 0;
>  }
>  
> @@ -3251,14 +3158,8 @@ mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var)
>  		mpstream_encode_double(stream, var->u.r);
>  		return;
>  	case MEM_TYPE_BIN:
> -		if ((var->flags & MEM_Zero) != 0) {
> -			mpstream_encode_binl(stream, var->n + var->u.nZero);
> -			mpstream_memcpy(stream, var->z, var->n);
> -			mpstream_memset(stream, 0, var->u.nZero);
> -		} else {
> -			mpstream_encode_binl(stream, var->n);
> -			mpstream_memcpy(stream, var->z, var->n);
> -		}
> +		mpstream_encode_binl(stream, var->n);
> +		mpstream_memcpy(stream, var->z, var->n);
>  		return;
>  	case MEM_TYPE_ARRAY:
>  	case MEM_TYPE_MAP:
> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> index 0da45b8af..1574da22d 100644
> --- a/src/box/sql/mem.h
> +++ b/src/box/sql/mem.h
> @@ -68,7 +68,6 @@ struct Mem {
>  		i64 i;		/* Integer value used when MEM_Int is set in flags */
>  		uint64_t u;	/* Unsigned integer used when MEM_UInt is set. */
>  		bool b;         /* Boolean value used when MEM_Bool is set in flags */
> -		int nZero;	/* Used when bit MEM_Zero is set in flags */
>  		void *p;	/* Generic pointer */
>  		/**
>  		 * A pointer to function implementation.
> @@ -111,7 +110,6 @@ struct Mem {
>  #define MEM_Dyn       0x0800	/* Need to call Mem.xDel() on Mem.z */
>  #define MEM_Static    0x1000	/* Mem.z points to a static string */
>  #define MEM_Ephem     0x2000	/* Mem.z points to an ephemeral string */
> -#define MEM_Zero      0x8000	/* Mem.i contains count of 0s appended to blob */
>  
>  static inline bool
>  mem_is_null(const struct Mem *mem)
> @@ -246,13 +244,6 @@ mem_is_cleared(const struct Mem *mem)
>  	return (mem->flags & MEM_Cleared) != 0;
>  }
>  
> -static inline bool
> -mem_is_zerobin(const struct Mem *mem)
> -{
> -	assert((mem->flags & MEM_Zero) == 0 || mem->type == MEM_TYPE_BIN);
> -	return (mem->flags & MEM_Zero) != 0;
> -}
> -
>  static inline bool
>  mem_is_same_type(const struct Mem *mem1, const struct Mem *mem2)
>  {
> @@ -474,12 +465,6 @@ mem_set_binl(struct Mem *mem, char *value, uint32_t size,
>  		return mem_set_bin_dynamic(mem, value, size);
>  }
>  
> -/**
> - * Clear MEM and set it to VARBINARY. The binary value consist of n zero bytes.
> - */
> -void
> -mem_set_zerobin(struct Mem *mem, int n);
> -
>  /**
>   * Copy binary value to a newly allocated memory. The MEM type becomes
>   * VARBINARY.
> @@ -603,7 +588,7 @@ mem_move(struct Mem *to, struct Mem *from);
>   * result MEM is set to NULL even if the result MEM is actually the first MEM.
>   */
>  int
> -mem_concat(struct Mem *left, struct Mem *right, struct Mem *result);
> +mem_concat(const struct Mem *a, const struct Mem *b, struct Mem *result);
>  
>  /**
>   * Add the first MEM to the second MEM and write the result to the third MEM.
> @@ -935,11 +920,6 @@ registerTrace(int iReg, Mem *p);
>  #define memIsValid(M)  ((M)->type != MEM_TYPE_INVALID)
>  #endif
>  
> -int sqlVdbeMemExpandBlob(struct Mem *);
> -#define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0)
> -
> -/** Setters = Change MEM value. */
> -
>  int sqlVdbeMemClearAndResize(struct Mem * pMem, int n);
>  
>  void sqlValueFree(struct Mem *);
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2e250dc29..eac6bcec8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -394,13 +394,6 @@ void
>  sql_result_value(sql_context *,
>  		     sql_value *);
>  
> -void
> -sql_result_zeroblob(sql_context *, int n);
> -
> -int
> -sql_result_zeroblob64(sql_context *,
> -			  sql_uint64 n);
> -
>  char *
>  sql_mprintf(const char *, ...);
>  char *
> @@ -631,13 +624,6 @@ int
>  sql_bind_text64(sql_stmt *, int, const char *,
>  		    sql_uint64, void (*)(void *));
>  
> -int
> -sql_bind_zeroblob(sql_stmt *, int, int n);
> -
> -int
> -sql_bind_zeroblob64(sql_stmt *, int,
> -			sql_uint64);
> -
>  int
>  sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
>  
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 44533fb3e..86550541b 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1462,8 +1462,6 @@ case OP_MustBeInt: {            /* jump, in1 */
>   */
>  case OP_Cast: {                  /* in1 */
>  	pIn1 = &aMem[pOp->p1];
> -	if (ExpandBlob(pIn1) != 0)
> -		goto abort_due_to_error;
>  	rc = mem_cast_explicit(pIn1, pOp->p2);
>  	UPDATE_MAX_BLOBSIZE(pIn1);
>  	if (rc == 0)
> @@ -2772,8 +2770,6 @@ case OP_Found: {        /* jump, in3 */
>  #ifdef SQL_DEBUG
>  		for(ii=0; ii<r.nField; ii++) {
>  			assert(memIsValid(&r.aMem[ii]));
> -			assert(!mem_is_zerobin(&r.aMem[ii]) ||
> -			       r.aMem[ii].n == 0);
>  			if (ii != 0)
>  				REGISTER_TRACE(p, pOp->p3+ii, &r.aMem[ii]);
>  		}
> @@ -2784,7 +2780,6 @@ case OP_Found: {        /* jump, in3 */
>  		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
>  		if (pIdxKey==0) goto no_mem;
>  		assert(mem_is_bin(pIn3));
> -		(void)ExpandBlob(pIn3);
>  		sqlVdbeRecordUnpackMsgpack(pC->key_def,
>  					       pIn3->z, pIdxKey);
>  	}
> @@ -3411,8 +3406,7 @@ case OP_SorterInsert: {      /* in2 */
>  	assert(isSorter(cursor));
>  	pIn2 = &aMem[pOp->p2];
>  	assert(mem_is_bin(pIn2));
> -	if (ExpandBlob(pIn2) != 0 ||
> -	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
> +	if (sqlVdbeSorterWrite(cursor, pIn2) != 0)
>  		goto abort_due_to_error;
>  	break;
>  }
> @@ -3445,8 +3439,6 @@ case OP_IdxReplace:
>  case OP_IdxInsert: {
>  	pIn2 = &aMem[pOp->p1];
>  	assert(mem_is_bin(pIn2));
> -	if (ExpandBlob(pIn2) != 0)
> -		goto abort_due_to_error;
>  	struct space *space;
>  	if (pOp->p4type == P4_SPACEPTR)
>  		space = pOp->p4.space;
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 115940227..97bd19863 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -236,25 +236,6 @@ sql_result_value(sql_context * pCtx, sql_value * pValue)
>  		pCtx->is_aborted = true;
>  }
>  
> -void
> -sql_result_zeroblob(sql_context * pCtx, int n)
> -{
> -	mem_set_zerobin(pCtx->pOut, n);
> -}
> -
> -int
> -sql_result_zeroblob64(sql_context * pCtx, u64 n)
> -{
> -	Mem *pOut = pCtx->pOut;
> -	if (n > (u64) pOut->db->aLimit[SQL_LIMIT_LENGTH]) {
> -		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
> -			 "is too big");
> -		return -1;
> -	}
> -	mem_set_zerobin(pCtx->pOut, (int)n);
> -	return 0;
> -}
> -
>  /*
>   * Execute the statement pStmt, either until a row of data is ready, the
>   * statement is completely executed or an error occurs.
> @@ -818,29 +799,6 @@ sql_bind_text64(sql_stmt * pStmt,
>  	}
>  }
>  
> -int
> -sql_bind_zeroblob(sql_stmt * pStmt, int i, int n)
> -{
> -	Vdbe *p = (Vdbe *) pStmt;
> -	if (vdbeUnbind(p, i) != 0)
> -		return -1;
> -	mem_set_zerobin(&p->aVar[i - 1], n);
> -	return 0;
> -}
> -
> -int
> -sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
> -{
> -	Vdbe *p = (Vdbe *) pStmt;
> -	if (n > (u64) p->db->aLimit[SQL_LIMIT_LENGTH]) {
> -		diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
> -			 "is too big");
> -		return -1;
> -	}
> -	assert((n & 0x7FFFFFFF) == n);
> -	return sql_bind_zeroblob(pStmt, i, n);
> -}
> -
>  int
>  sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
>  {