[tarantool-patches] Re: [PATCH v2 5/7] sql: create interface vstream

[Vladislav Shpilevoy <v.shpilevoy@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 <v.shpilevoy@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