[OpenSER-Devel] RFC: memory management in database modules

[Bogdan-Andrei Iancu]

Hi Henning,

Henning Westerholt wrote:
> On Tuesday 05 February 2008, Bogdan-Andrei Iancu wrote:
>   
>> Hi Henning,
>>
>> I agree with you. I see here two arguments for this:
>>     1) if mysql is not duplicating the date returned from driver, it
>> means all the modules using DB are already safe from this point of view
>> - they do their one copy and they do not count on the persistence of
>> data returned from DB.
>>     
>
> Hi Bogdan,
>
> yes, this was the point i was trying to convey.
>   
It sounds natural as it is already present and working ;)
>   
>>     2) copying data too many times may have a performance impact, but
>> not because of alloc/copy/free ops, but mainly because of memory
>> fragmentation - the size of data operated with DB vary a lot (like size
>> of chucks), so the impact may be huge.
>>     
>
> I know that you're receptive for performance arguments.. ;-)
>   
Well ....do not take me as a performance maniac :D...As I said, it is 
not about performance but about functionality - memory fragmentation is 
something serious and we should try to avoid it as much as possible.
>   
>> But I have here a note: it may not be possible in all case to pass the
>> pointer returned by driver to the upper layer (module) as the data
>> returned by driver may need some pre-processing. Like the postgres
>> module does for string and blobs (if I'm not wrong) to do escape and
>> unescape. 
>>     
>
> In the postgres module its not a problem for STRINGs and STRs, but for BLOBs. 
> The escaping function allocates new memory that don't belong to the request 
> that is build. Its also not valid to assign the escaped string to the old 
> string pointer (to free it on free_result), because it could be bigger, and 
> according to the documentation this is also not allowed. Thus its necessary 
> to free this memory in the free_row function.
>   
yes, I know this is a tricky thing - I got into this code after Norman 
did some changes there, and the logic for handling the BLOB case was 
quite complex and tricky. Most probably because the DB API did not 
provide any support (as extension) to keep additional information about 
the memory status of the field (if statics, allocated, etc)
>   
>> So, the DB module may hide (totally transparent) that certain 
>> fields are re-allocated due some pre-processing. This extra mem must be
>> also freed (also transparent) by the DB module when the result is freed.
>> This will not break the the overall behaviour, but I just mentioned
>> because the no suppositions should made on the data returned by the DB
>> module -  it may be allocated by underlaying driver, may be in openser
>> pkg mem or in heap. If a module needs the data, it must make a copy!
>>     
>
> I don't want to change this assumption. So i'll introduce a flag in the 
> db_val_t structure to track the memory status of a value. Then we've the 
> flexibility to support different types of modules, and we can also keep or 
> improve the performance. 
>   
Perfect!
> For the VAL_NAMES->s exists the same issue, i'll evaluate if it make sense to 
> introduce a flag for them too.
>   
well...is it needed any altering of the column names? so far I think all 
are static (as returned by driver)


BTW, do you have any estimation (as time) about the start and end of 
these changes? I plan also some major changes in the DB interaction and 
preferable will be not to overlap ;)

Regards,
Bogdan