[Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c

Timur Safin tsafin at tarantool.org
Tue Feb 9 12:51:14 MSK 2021



-----Original Message-----
From: Timur Safin <tsafin at tarantool.org> 
Sent: Tuesday, February 9, 2021 12:50 PM
To: 'imeevma at tarantool.org' <imeevma at tarantool.org>
Subject: RE: [PATCH v1 09/10] sql: refactor vdbeaux.c

There are similar notes about neg/pos vs signed/unsigned in 
function names as I've mentioned earlier.

But also some extra comments, please see below...

: From: imeevma at tarantool.org <imeevma at tarantool.org>
: Subject: [PATCH v1 09/10] sql: refactor vdbeaux.c
: 
: ---
:  src/box/sql/vdbeaux.c | 266 +++++++++++++++++++-----------------------
:  1 file changed, 122 insertions(+), 144 deletions(-)
: 
: diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
: index 7b8a1e1d8..4ffb34a4e 100644
: --- a/src/box/sql/vdbeaux.c
: +++ b/src/box/sql/vdbeaux.c
: @@ -1382,13 +1376,25 @@ sqlVdbeList(Vdbe * p)
:  					if (apSub[j] == pOp->p4.pProgram)
:  						break;
:  				}
: -				if (j == nSub &&
: -				    sqlVdbeMemGrow(pSub, nByte,
: -						   nSub != 0) == 0) {
: -					apSub = (SubProgram **) pSub->z;
: -					apSub[nSub++] = pOp->p4.pProgram;
: -					pSub->flags |= MEM_Blob;
: -					pSub->n = nSub * sizeof(SubProgram *);
: +				if (j == nSub) {
: +					size_t svp = region_used(&fiber()->gc);
: +					struct SubProgram **buf = (SubProgram **)
: +						region_aligned_alloc(&fiber()->gc,
: +								     nByte,
: +								     alignof(struct
: SubProgram));
: +					if (buf == NULL) {
: +						diag_set(OutOfMemory, nByte,
: +							 "region_aligned_alloc",
: +							 "buf");
: +						p->is_aborted = true;
: +						return -1;
: +					}
: +					if (nSub > 0)
: +						memcpy(buf, pSub->z, pSub->n);
: +					buf[nSub++] = pOp->p4.pProgram;
: +					mem_set_bin(pSub, (char *)buf, nByte, 0,
: +						    false);
: +					region_truncate(&fiber()->gc, svp);
:  				}
:  			}
:  		}

This was quite unexpected. I'd rather expect that refactoring would 
reduce number of code used inside of functions, not make them
verbose. Could you please explain me why this extra code would 
become necessary? (And why not wrap them elsewhere?)

: @@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p)
:  		mem_set_i64(pMem, pOp->p3);
:  		pMem++;
: 
: -		if (sqlVdbeMemClearAndResize(pMem, 256)) {
: -			assert(p->db->mallocFailed);
: -			return -1;
: -		}
: -		pMem->flags = MEM_Str | MEM_Term;
: -		zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
: -
: -		if (zP4 != pMem->z) {
: -			pMem->n = 0;
: -			sqlVdbeMemSetStr(pMem, zP4, -1, 1, 0);
: -		} else {
: -			assert(pMem->z != 0);
: -			pMem->n = sqlStrlen30(pMem->z);
: -		}
: +		size_t size = 256;
: +		char *tmp_buf = (char *) static_alloc(size);
: +		assert(tmp_buf != NULL);
: +		zP4 = displayP4(pOp, tmp_buf, size);
: +		mem_set_str(pMem, zP4, strlen(zP4), 0, true);
:  		pMem++;

Please, please, not introduce any newer usage of static_alloc,
it's very fragile and might work only inside of controlled context of 
single module (i.e. swim). If used by multiple modules -
results are unpredictable. Any other allocator is fine if used 
by multiple fibers, but not static_alloc.

Regards,
Timur



More information about the Tarantool-patches mailing list