From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2D01946971A for ; Fri, 13 Dec 2019 16:55:43 +0300 (MSK) Date: Fri, 13 Dec 2019 16:55:42 +0300 From: Nikita Pettik Message-ID: <20191213135542.GA52107@tarantool.org> References: <10842a572a8b14dd8c96c258ffe372dda45416e5.1574277369.git.korablev@tarantool.org> <527d98cf-be26-edcc-f615-c5b9a2f7dd7c@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <527d98cf-be26-edcc-f615-c5b9a2f7dd7c@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 ... : 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; > > }