Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Nikita Pettik <korablev@tarantool.org>]

On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> On 20/11/2019 22:28, Nikita Pettik wrote:
> > Dump formats of DQL and DML queries are different: the last one contains
> > number of affected rows and optionally list of autoincremented ids; the
> > first one comprises all meta-information including column names of
> > resulting set and their types. What is more, dump format is going to be
> > different for execute and prepare requests. So let's introduce separate
> > member to struct port_sql responsible for dump format to be used.
> > 
> > What is more, prepared statement finalization is required only for
> > PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.
> 
> 1. I don't think it is a good idea to rely on certain
> meta being returned by only certain requests. There is
> a ticket, which will remove the border between different
> request types:
> https://github.com/tarantool/tarantool/issues/3649.

It's in the wishlist, so I doubt that it will be implemented
in observed feature.

As an alternative to your proposal below I can suggest to add ref count
to prepared statements. It is assumed to be increased once it is added
to prepared statement cache and before execution; after execution it is
decremented.

> Yes, now this is either sql_info or metadata, but that
> will change.
> 
> > 
> > Note that C standard specifies that enums are integers, but it does not
> > specify the size. Hence, let's use simple uint8 - mentioned enums are
> > small enough to fit into it.
> 
> 2. Try this:
> 
>     enum ... <member> : 8;

Hm, doesn't it allocate one whole enum member anyway? If so,
still undefined size is implied.

> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -100,7 +100,9 @@ static void
> >  port_sql_destroy(struct port *base)
> >  {
> >  	port_tuple_vtab.destroy(base);
> > -	sql_finalize(((struct port_sql *)base)->stmt);
> > +	struct port_sql *port_sql = (struct port_sql *) base;
> > +	if (port_sql->request == PREPARE_AND_EXECUTE)
> 
> 3. You could turn the types into a bitmask to drop the
> dependency on a certain request type:
> 
>     bool do_finalize : 1;

Well, seems to be quite suitable, let's use it:

diff --git a/src/box/execute.c b/src/box/execute.c
index 0e731672c..dfe586334 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -101,7 +101,7 @@ port_sql_destroy(struct port *base)
 {
        port_tuple_vtab.destroy(base);
        struct port_sql *port_sql = (struct port_sql *) base;
-       if (port_sql->request == PREPARE_AND_EXECUTE)
+       if (port_sql->do_finalize)
                sql_stmt_finalize(((struct port_sql *)base)->stmt);
 }
 
@@ -117,15 +117,14 @@ const struct port_vtab port_sql_vtab = {
 
 static void
 port_sql_create(struct port *port, struct sql_stmt *stmt,
-               enum sql_serialization_format format,
-               enum sql_request_type request)
+               enum sql_serialization_format format, bool do_finalize)
 {
        port_tuple_create(port);
        port->vtab = &port_sql_vtab;
        struct port_sql *port_sql = (struct port_sql *) port;
        port_sql->stmt = stmt;
        port_sql->serialization_format = format;
-       port_sql->request = request;
+       port_sql->do_finalize = do_finalize;
 }
 
 /**
@@ -460,7 +459,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
        assert(stmt != NULL);
        enum sql_serialization_format format = sql_column_count(stmt) > 0 ?
                                           DQL_EXECUTE : DML_EXECUTE;
-       port_sql_create(port, stmt, format, PREPARE_AND_EXECUTE);
+       port_sql_create(port, stmt, format, true);
        if (sql_bind(stmt, bind, bind_count) == 0 &&
            sql_execute(stmt, port, region) == 0)
                return 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index e9b74f7c8..c87e765cf 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -57,12 +57,6 @@ enum sql_serialization_format {
        DML_PREPARE = 3,
 };
 
-enum sql_request_type {
-       PREPARE_AND_EXECUTE = 0,
-       PREPARE = 1,
-       EXECUTE_PREPARED = 2
-};
-
 extern const char *sql_info_key_strs[];
 
 struct region;
@@ -107,8 +101,11 @@ struct port_sql {
         * DQL; and on type of SQL request: execute or prepare.
         */
        uint8_t serialization_format;
-       /** enum sql_request_type */
-       uint8_t request;
+       /**
+        * There's no need in clean-up in case of PREPARE request:
+        * statement remains in cache and will be deleted later.
+        */
+       bool do_finalize;
 };

>     bool do_omit_data : 1;

do_omit_data is not enough: serialize formats are different for all
four cases. 
 
> > +		sql_finalize(((struct port_sql *)base)->stmt);
> >  }
> >  
> >  const struct port_vtab port_sql_vtab = {
> > @@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
> >  				      mp_encode_int(buf, id_entry->id);
> >  			}
> >  		}
> > +		break;
> > +	}
> > +	default: unreachable();
> 
> 4. Please, wrap on a new line, and include into {}. I know,
> this is shorter, but this just does not match the code
> style. In the other place too.

Okay:

diff --git a/src/box/execute.c b/src/box/execute.c
index 0e731672c..3bc4988b7 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -407,7 +406,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
                }
                break;
        }
-       default: unreachable();
+       default: {
+               unreachable();
+       }
        }
        return 0;
 }

diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 2cbc5dfab..b164ffcaf 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -82,7 +82,9 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
                }
                break;
        }
-       default: unreachable();
+       default: {
+               unreachable();
+       }
        }
 }

 
> >  	}
> >  	return 0;
> >  }