From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 8A00D46971A for ; Fri, 13 Dec 2019 16:53:44 +0300 (MSK) Date: Fri, 13 Dec 2019 16:53:43 +0300 From: Nikita Pettik Message-ID: <20191213135343.GA75855@tarantool.org> References: <10842a572a8b14dd8c96c258ffe372dda45416e5.1574277369.git.korablev@tarantool.org> <20191204115244.GI6592@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191204115244.GI6592@atlas> 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: Konstantin Osipov , tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 04 Dec 14:52, Konstantin Osipov wrote: > * Nikita Pettik [19/11/21 10:00]: > > Dump formats of DQL and DML queries are different: the last one contains > > Ergh, this is not dump, but result set/serialization format. Dump > in SQL is usually used for logical or physical database > backup. Okay, let's rename to sql_serialization_format > > 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. > > > > > 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. > > enum sizeof in C and older C++ is implementation dependent. > > what do you mean here? To store uint8 numbers instead of enums (due to their implementation dependent sizes). > > + struct port_sql *port_sql = (struct port_sql *) base; > > + if (port_sql->request == PREPARE_AND_EXECUTE) > > + sql_finalize(((struct port_sql *)base)->stmt); > > Does this work with the statement object only or with the cache as > well? Only with prepared statement objects. > I suggest we introduce a clear naming for API calls: > > sql_stmt_* - for statement object api > sql_stmt_cache_* - for prepared statement api > sql_finalize, sql_prepare, sql_execute - for SQL high level API > which manipulates both statements and the cache. > > What do you think? Ok, will move renaming sql_finalize -> sql_stmt_finalize etc to a separate patch. I'm okay with suggested naming and I'm going to use it in the next patches. > > -port_sql_create(struct port *port, struct sql_stmt *stmt) > > +port_sql_create(struct port *port, struct sql_stmt *stmt, > > + enum sql_dump_format dump_format, enum sql_request_type request) > > { > > port_tuple_create(port); > > - ((struct port_sql *)port)->stmt = stmt; > > port->vtab = &port_sql_vtab; > > + struct port_sql *port_sql = (struct port_sql *) port; > > + port_sql->stmt = stmt; > > + port_sql->dump_format = dump_format; > > Let's use sql_result_set_format? Do you have to introduce this > enum ? This information can be derived from sql_request_type, no + > statement type, no? If yes, I suggest to have a function, which > returns it, rather than store. To save 1 byte? :) Doesn't seem to be reasonable tho. Current implementation looks quite suitable in code. > > +/** > > + * One of possible formats used to dump msgpack/Lua. > > + * For details see port_sql_dump_msgpack() and port_sql_dump_lua(). > > > > + */ > > +enum sql_dump_format { > > + DQL_EXECUTE = 0, > > + DML_EXECUTE = 1, > > + DQL_PREPARE = 2, > > + DML_PREPARE = 3, > > +}; > > Neither the names of the members nor comments convey what is going > on here - looks like a simple matrix dml/dql * prepare/execute. That's it. > Having a couple of if statments is not such a big deal IMHO. With this enum code looks way much better. > > + /** > > + * Dump format depends on type of SQL query: DML or DQL; > > + * and on type of SQL request: execute or prepare. > > + */ > > + uint8_t dump_format; > > + /** enum sql_request_type */ > > + uint8_t request; > > If I had to add these constants, I would not use type-erasing > uint8 here, but use max value for enum in C/C++, which is 64 bits > and the original type. Now one is replaced with do_finalize as Vlad suggested.