[Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql
Nikita Pettik
korablev at tarantool.org
Fri Dec 13 16:55:42 MSK 2019
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;
> > }
More information about the Tarantool-patches
mailing list