[Tarantool-patches] [PATCH v5 14/52] sql: introduce mem_copy_as_ephemeral()

[Mergen Imeev]

Thank you for the review. My answer below.

On Sun, Apr 11, 2021 at 08:10:19PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> >>> @@ -593,9 +598,12 @@ sqlVdbeMemAboutToChange(Vdbe * pVdbe, Mem * pMem)
> >>>  	int i;
> >>>  	Mem *pX;
> >>>  	for (i = 0, pX = pVdbe->aMem; i < pVdbe->nMem; i++, pX++) {
> >>> -		if (pX->pScopyFrom == pMem) {
> >>> -			pX->flags |= MEM_Undefined;
> >>> -			pX->pScopyFrom = 0;
> >>> +		if ((pX->flags & (MEM_Blob | MEM_Str)) != 0 &&
> >>> +		    (pX->flags & (MEM_Ephem | MEM_Static)) == 0) {
> >>> +			if (pX->pScopyFrom == pMem) {
> >>> +				pX->flags |= MEM_Undefined;
> >>> +				pX->pScopyFrom = 0;
> >>> +			}
> >>
> >> 2. Why did you change that?
> >>
> > This check is only useful for strings and binaries, since they may be lost due
> > to change of another MEM. Also, due to this function it was possible that value
> > of type other than MEM_Blob or MEM_Str will have MEM_Ephem set. This is wrong, I
> > believe.
> 
> Due to which function? sqlVdbeMemAboutToChange() does not set MEM_Ephem.
I'm sorry, I was wrong. Actually it was sqlVdbeMemShallowCopy() fault that it
was possible to MEM to have MEM_Ephem flag even though it was not a string or
varbinary. But, since sqlVdbeMemMakeWriteable() was called in such cases
after sqlVdbeMemShallowCopy(), this flag was removed, I think. About
sqlVdbeMemAboutToChange() and MEM_undefined - I am not sure why I thought so.
Most likely I come to this conclusion during investigation of why memIsValid()
check fails when I removed sqlVdbeMemMakeWriteable() - this was so because MEM
with MEM_Ephem was invalidated in sqlVdbeMemAboutToChange() even though the MEM
wasn't string or varbinary.