[tarantool-patches] Re: [PATCH v3 4/7] iproto: replace obuf by mpstream in execute.c

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 29/11/2018 13:53, Vladimir Davydov wrote:
> On Tue, Nov 27, 2018 at 10:25:43PM +0300, imeevma at tarantool.org wrote:
>> @@ -627,81 +610,53 @@ sql_prepare_and_execute(const struct sql_request *request,
>>   }
>>   
>>   int
>> -sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
>> +sql_response_dump(struct sql_response *response, int *keys,
>> +		  struct mpstream *stream)
>>   {
>>   	sqlite3 *db = sql_get();
>>   	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
>> -	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
>>   	int rc = 0, column_count = sqlite3_column_count(stmt);
>>   	if (column_count > 0) {
>> -		if (sql_get_description(stmt, out, column_count) != 0) {
>> +		if (sql_get_description(stmt, stream, column_count) != 0) {
>>   err:
>>   			rc = -1;
>>   			goto finish;
>>   		}
>>   		*keys = 2;
>> -		int size = mp_sizeof_uint(IPROTO_DATA) +
>> -			   mp_sizeof_array(port_tuple->size);
>> -		char *pos = (char *) obuf_alloc(out, size);
>> -		if (pos == NULL) {
>> -			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>> -			goto err;
>> -		}
>> -		pos = mp_encode_uint(pos, IPROTO_DATA);
>> -		pos = mp_encode_array(pos, port_tuple->size);
>> -		/*
>> -		 * Just like SELECT, SQL uses output format compatible
>> -		 * with Tarantool 1.6
>> -		 */
>> -		if (port_dump_msgpack_16(&response->port, out) < 0) {
>> +		mpstream_encode_uint(stream, IPROTO_DATA);
>> +		mpstream_flush(stream);
>> +		if (port_dump_msgpack(&response->port, stream->ctx) < 0) {
> 
> stream->ctx isn't guaranteed to be an obuf
> 
> And when you introduce vstream later, you simply move this code to
> another file. This is confusing. May be we should pass alloc/reserve
> used in mpstream to port_dump instead of obuf?

Good idea, though not sure, if it is worth slowing down port_dump_msgpack
adding a new level of indirection. Since port_dump_msgpack is a hot path
and is used for box.select.

Maybe it is better to just rename port_dump_msgpack to port_dump_obuf
and rename vstream_port_dump to vstream_port_dump_obuf? If we ever will
dump port to not obuf, then we will just add a new method to port_vtab.

Also, it would make port_dump_obuf name consistent with port_dump_lua -
in both cases we not just dump in a specific format, but to a concrete
destination: obuf and lua stack. Now port_dump_msgpack anyway is restricted
by obuf destination.

If you worry about how to call sql_response_dump() to not obuf, then there
is another option. Anyway rename port_dump_msgpack to port_dump_obuf and
introduce a new method: port_dump_mpstream. It will take mpstream and use
its reserve/alloc/error functions. It allows us to do not slow down box.select,
but use the full power of virtual functions in execute.c, which definitely is
not hot.

mpstream implementation of vstream will call port_dump_mpstream, and
luastream implementation of vstream will call port_dump_lua as it does now.
box.select and iproto_call will use port_dump_obuf.

I prefer the second option: introduce port_dump_mpstream. It is ok for you?