From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v3 4/7] iproto: replace obuf by mpstream in execute.c References: <33c6e6cbd7d667980212902f6825d3d7e941ec77.1543344471.git.imeevma@gmail.com> <20181129105340.3d2xmkxqsht4wq3r@esperanza> From: Vladislav Shpilevoy Message-ID: <1134ddeb-5769-2f76-c8c2-062576b614a5@tarantool.org> Date: Thu, 29 Nov 2018 17:04:06 +0300 MIME-Version: 1.0 In-Reply-To: <20181129105340.3d2xmkxqsht4wq3r@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov , imeevma@tarantool.org List-ID: 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@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?