From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id DF8C82EB67 for ; Thu, 22 Nov 2018 16:49:22 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hIcE2zMphvNt for ; Thu, 22 Nov 2018 16:49:22 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6F8F82EAF9 for ; Thu, 22 Nov 2018 16:49:22 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 5/7] sql: create interface vstream References: <574b8d1d8e7fdf12afd9141c98b119b0786de711.1542910674.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 23 Nov 2018 00:49:15 +0300 MIME-Version: 1.0 In-Reply-To: <574b8d1d8e7fdf12afd9141c98b119b0786de711.1542910674.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org Thanks for the fixes! See my 4 comments below, fix at the end of the email and on the branch. On 22/11/2018 22:11, imeevma@tarantool.org wrote: > If we want to use functions from execute.h not only in IPROTO we > should create special interface. This interface will allow us to > create different implementations for mpstream and lua_State and > use functions from execute.c without changing them. This patch > creates such interface and its implementation for mpstream and > replaces mpstream functions in execute.c by methods of this > interface. > > Needed for #3505 > --- > src/box/execute.c | 69 ++++++++++++----------- > src/box/execute.h | 6 +- > src/box/iproto.cc | 11 ++-- > src/box/vstream.h | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/mpstream.c | 78 ++++++++++++++++++++++++++ > 5 files changed, 291 insertions(+), 37 deletions(-) > create mode 100644 src/box/vstream.h > > diff --git a/src/box/execute.h b/src/box/execute.h > index 940f3a3..5a11a8a 100644 > --- a/src/box/execute.h > +++ b/src/box/execute.h > @@ -75,6 +75,8 @@ struct sql_response { > struct port port; > /** Prepared SQL statement with metadata. */ > void *prep_stmt; > + /** Result should be flatten if true. */ > + bool is_flatten; 1. What does it mean 'result should be flatten'? Are all tuples merged into a single flattened one? Or are all metafields merged into a single array? Please, be more specific. It is far from obvious now what is it 'flattened result'. Also, I guess, 'flatten' is a verb, so you can not say 'is flatten'. Only 'is flattened'. 2. I do not see where do you initialize this field for iproto. So it is now initialized with stack garbage. (I've fixed all these things since we hurry.) > }; > > /** > diff --git a/src/box/vstream.h b/src/box/vstream.h > new file mode 100644 > index 0000000..a8dcfc2 > --- /dev/null > +++ b/src/box/vstream.h > @@ -0,0 +1,164 @@ > +#ifndef TARANTOOL_VSTREAM_H_INCLUDED > +#define TARANTOOL_VSTREAM_H_INCLUDED > +/* > + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include "diag.h" > +#include "mpstream.h" 3. vstream should not depend on mpstream, but vice versa. Please, look at how struct port is done. It uses padding into which its descendants can lay anything. > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +struct vstream; > +struct lua_State; > +struct port; > + > diff --git a/src/mpstream.c b/src/mpstream.c > index e4f7950..f9943e4 100644 > --- a/src/mpstream.c > +++ b/src/mpstream.c > @@ -175,3 +180,76 @@ mpstream_encode_bool(struct mpstream *stream, bool val) > char *pos = mp_encode_bool(data, val); > mpstream_advance(stream, pos - data); > } > + > +typedef void (*encode_array_f)(struct vstream *stream, uint32_t size); > +typedef void (*encode_map_f)(struct vstream *stream, uint32_t size); > +typedef void (*encode_uint_f)(struct vstream *stream, uint64_t num); > +typedef void (*encode_int_f)(struct vstream *stream, int64_t num); > +typedef void (*encode_float_f)(struct vstream *stream, float num); > +typedef void (*encode_double_f)(struct vstream *stream, double num); > +typedef void (*encode_strn_f)(struct vstream *stream, const char *str, > + uint32_t len); > +typedef void (*encode_nil_f)(struct vstream *stream); > +typedef void (*encode_bool_f)(struct vstream *stream, bool val); > + 4. I think, it should be part of vstream.h. And struct vstream members should be defined with these types. ==================================================================== commit a75e03dd1edf9c106e78f3d10618b4bfa80b84d5 Author: Vladislav Shpilevoy Date: Fri Nov 23 00:01:56 2018 +0300 Review fixes diff --git a/src/box/execute.c b/src/box/execute.c index 8093d9c99..18133b1e7 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -648,7 +648,7 @@ err: stailq_foreach_entry(id_entry, autoinc_id_list, link) id_count++; } - if (!response->is_flatten) { + if (!response->is_info_flattened) { vstream_encode_enum(stream, IPROTO_SQL_INFO, "info"); vstream_encode_map(stream, map_size); } @@ -671,7 +671,7 @@ err: } vstream_encode_map_commit(stream); } - if (!response->is_flatten) + if (!response->is_info_flattened) vstream_encode_map_commit(stream); } finish: diff --git a/src/box/execute.h b/src/box/execute.h index 5a11a8a2b..42bbe63de 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -75,8 +75,20 @@ struct sql_response { struct port port; /** Prepared SQL statement with metadata. */ void *prep_stmt; - /** Result should be flatten if true. */ - bool is_flatten; + /** + * SQL response can be dumped into msgpack to be sent via + * iproto or onto Lua stack to be returned into an + * application. In the first case response body has + * explicit field IPROTO_SQL_INFO: {rowcount = ..., + * autoids = ...}. But in case of Lua this field is + * flattened. A result never has 'info' field, it has + * inlined 'rowcount' and 'autoids'. In iproto + * IPROTO_SQL_INFO field is sent mostly to explicitly + * distinguish two response types: DML/DDL vs DQL, + * IPROTO_SQL_INFO vs IPROTO_METADATA. So this flag is set + * by Lua and allows to flatten SQL_INFO fields. + */ + bool is_info_flattened; }; /** diff --git a/src/box/iproto.cc b/src/box/iproto.cc index 380a6eec0..1c4c65176 100644 --- a/src/box/iproto.cc +++ b/src/box/iproto.cc @@ -1588,6 +1588,7 @@ tx_process_sql(struct cmsg *m) struct iproto_msg *msg = tx_accept_msg(m); struct obuf *out; struct sql_response response; + memset(&response, 0, sizeof(response)); bool is_error = false; tx_fiber_init(msg->connection->session, msg->header.sync); diff --git a/src/box/vstream.h b/src/box/vstream.h index a8dcfc289..9b52acd3d 100644 --- a/src/box/vstream.h +++ b/src/box/vstream.h @@ -30,10 +30,6 @@ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. */ - -#include "diag.h" -#include "mpstream.h" - #if defined(__cplusplus) extern "C" { #endif /* defined(__cplusplus) */ @@ -42,28 +38,43 @@ struct vstream; struct lua_State; struct port; +typedef void (*encode_array_f)(struct vstream *stream, uint32_t size); +typedef void (*encode_map_f)(struct vstream *stream, uint32_t size); +typedef void (*encode_uint_f)(struct vstream *stream, uint64_t num); +typedef void (*encode_int_f)(struct vstream *stream, int64_t num); +typedef void (*encode_float_f)(struct vstream *stream, float num); +typedef void (*encode_double_f)(struct vstream *stream, double num); +typedef void (*encode_strn_f)(struct vstream *stream, const char *str, + uint32_t len); +typedef void (*encode_nil_f)(struct vstream *stream); +typedef void (*encode_bool_f)(struct vstream *stream, bool val); +typedef void (*encode_enum_f)(struct vstream *stream, int64_t num, + const char *str); +typedef int (*encode_port_f)(struct vstream *stream, struct port *port); +typedef void (*encode_array_commit_f)(struct vstream *stream, uint32_t id); +typedef void (*encode_map_commit_f)(struct vstream *stream); + struct vstream_vtab { - void (*encode_array)(struct vstream *stream, uint32_t size); - void (*encode_map)(struct vstream *stream, uint32_t size); - void (*encode_uint)(struct vstream *stream, uint64_t num); - void (*encode_int)(struct vstream *stream, int64_t num); - void (*encode_float)(struct vstream *stream, float num); - void (*encode_double)(struct vstream *stream, double num); - void (*encode_strn)(struct vstream *stream, const char *str, - uint32_t len); - void (*encode_nil)(struct vstream *stream); - void (*encode_bool)(struct vstream *stream, bool val); - void (*encode_enum)(struct vstream *stream, int64_t num, - const char *str); - int (*encode_port)(struct vstream *stream, struct port *port); - void (*encode_array_commit)(struct vstream *stream, uint32_t id); - void (*encode_map_commit)(struct vstream *stream); + encode_array_f encode_array; + encode_map_f encode_map; + encode_uint_f encode_uint; + encode_int_f encode_int; + encode_float_f encode_float; + encode_double_f encode_double; + encode_strn_f encode_strn; + encode_nil_f encode_nil; + encode_bool_f encode_bool; + encode_enum_f encode_enum; + encode_port_f encode_port; + encode_array_commit_f encode_array_commit; + encode_map_commit_f encode_map_commit; }; struct vstream { /** TODO: Write comment. */ union { - struct mpstream mpstream; + /** Here struct mpstream lives under the hood. */ + char inheritance_padding[64]; struct lua_State *L; }; /** Virtual function table. */ diff --git a/src/mpstream.c b/src/mpstream.c index f9943e493..23b20892c 100644 --- a/src/mpstream.c +++ b/src/mpstream.c @@ -33,11 +33,8 @@ #include #include #include "msgpuck.h" - #include "box/vstream.h" -#include "box/iproto_constants.h" #include "box/port.h" -#include "box/xrow.h" void mpstream_reserve_slow(struct mpstream *stream, size_t size) @@ -181,17 +178,6 @@ mpstream_encode_bool(struct mpstream *stream, bool val) mpstream_advance(stream, pos - data); } -typedef void (*encode_array_f)(struct vstream *stream, uint32_t size); -typedef void (*encode_map_f)(struct vstream *stream, uint32_t size); -typedef void (*encode_uint_f)(struct vstream *stream, uint64_t num); -typedef void (*encode_int_f)(struct vstream *stream, int64_t num); -typedef void (*encode_float_f)(struct vstream *stream, float num); -typedef void (*encode_double_f)(struct vstream *stream, double num); -typedef void (*encode_strn_f)(struct vstream *stream, const char *str, - uint32_t len); -typedef void (*encode_nil_f)(struct vstream *stream); -typedef void (*encode_bool_f)(struct vstream *stream, bool val); - int mp_vstream_encode_port(struct vstream *stream, struct port *port) { @@ -220,16 +206,9 @@ mp_vstream_encode_enum(struct vstream *stream, int64_t num, const char *str) } void -mp_vstream_encode_map_commit(struct vstream *stream) -{ - (void)stream; -} - -void -mp_vstream_encode_array_commit(struct vstream *stream, uint32_t id) +mp_vstream_noop(struct vstream *stream, ...) { - (void)stream; - (void)id; + (void) stream; } const struct vstream_vtab mp_vstream_vtab = { @@ -244,8 +223,8 @@ const struct vstream_vtab mp_vstream_vtab = { /** encode_bool = */ (encode_bool_f)mpstream_encode_bool, /** encode_enum = */ mp_vstream_encode_enum, /** encode_port = */ mp_vstream_encode_port, - /** encode_array_commit = */ mp_vstream_encode_array_commit, - /** encode_map_commit = */ mp_vstream_encode_map_commit, + /** encode_array_commit = */ (encode_array_commit_f)mp_vstream_noop, + /** encode_map_commit = */ (encode_map_commit_f)mp_vstream_noop, }; void