[tarantool-patches] Re: [PATCH v2 5/7] sql: create interface vstream
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Nov 23 00:49:15 MSK 2018
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 at 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 <v.shpilevoy at tarantool.org>
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 <assert.h>
#include <stdint.h>
#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
More information about the Tarantool-patches
mailing list