Re: [Tarantool-patches] [PATCH v2 00/16] sql: prepared statements

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

I did a quick overview of the patchset and there are some important questions.

1. The statement cache is a great feature but it is completely orthogonal with 
prepared statement feature. If we consider non-prepared statement going
through the cache (I do not see any reason to not to do statements caching) 
then there is no differences  between them and prepared statements except
caching aggressiveness and parameter substitution.

2. There is an interference between statements prepared in different session. A 
session which prepares huge loads of one-time-used prepared statements evicts
prepared statements from all other session even if this statements are reused 
a lot. And we have not got any clear explanation how you will prevent this 
behavior.
As a database user I have strong opinion that if I have prepared a statement 
then this statement should be alive for the statement lifetime. And I 
definitely do not want any unpredictability from a database.

3. If you will insist on statement cache then I will ask you for any 
justification regarding the chosen caching algorithm.


On Thursday, November 21, 2019 12:27:59 AM MSK Nikita Pettik wrote:
> Branch:
> https://github.com/tarantool/tarantool/tree/np/gh-2592-prepared-statemets-v
> 2 Issue: https://github.com/tarantool/tarantool/issues/2592
> 
> Patch v1:
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/01227
> 4.html
> 
> Changes in this iteration:
> 
> - Now cache is global and statements are shared among all session
>   (main reason for this change is to increase performance);
> - Cache follows next eviction policy: when there's no space for
>   another one statement, statement which was prepared earlier than others
>   is removed from cache (strickly speaking it's not LRU policy, but rather
>   FIFO; however, the same approach is already used for tuple cache and it
>   is called LRU anyway);
> - Owing to the previous point, there's no need in 'unprepare' invocation
>   at all (it concerns both manual request and on_disconnect clean-up);
> - Also due to using replacement policy eviction from cache should
>   not be "visible" to user, i.e. statement should still be capable of being
>   executed. Keeping only numeric ids doesn't allow this, so now we use
>   string ids (which consist of original SQL request);
> - Statement is automatically re-prepared (on demand) if it is expired
>   as a result of DDL request;
> - In case statement is busy (is executed right now by other session), it
>   is compiled from scratch and then executed.
> 
> Nikita Pettik (16):
>   sql: remove sql_prepare_v2()
>   sql: refactor sql_prepare() and sqlPrepare()
>   sql: move sql_prepare() declaration to box/execute.h
>   sql: rename sqlPrepare() to sql_compile()
>   sql: move sql_finalize() to execute.h
>   port: increase padding of struct port
>   port: add dump format and request type to port_sql
>   sql: resurrect sql_bind_parameter_count() function
>   sql: resurrect sql_bind_parameter_name()
>   sql: add sql_stmt_schema_version()
>   sql: introduce sql_stmt_sizeof() function
>   box: increment schema_version on ddl operations
>   sql: introduce sql_stmt_query_str() method
>   sql: introduce cache for prepared statemets
>   box: introduce prepared statements
>   netbox: introduce prepared statements
> 
>  src/box/CMakeLists.txt          |   1 +
>  src/box/alter.cc                |   3 +
>  src/box/box.cc                  |  20 ++
>  src/box/box.h                   |   1 +
>  src/box/ck_constraint.c         |   1 +
>  src/box/errcode.h               |   1 +
>  src/box/execute.c               | 213 ++++++++++++++-
>  src/box/execute.h               |  51 ++++
>  src/box/iproto.cc               |  25 +-
>  src/box/iproto_constants.c      |   6 +-
>  src/box/iproto_constants.h      |   4 +
>  src/box/lua/cfg.cc              |  12 +
>  src/box/lua/execute.c           | 161 ++++++++++-
>  src/box/lua/execute.h           |   2 +-
>  src/box/lua/init.c              |   2 +-
>  src/box/lua/load_cfg.lua        |   3 +
>  src/box/lua/net_box.c           |  79 ++++++
>  src/box/lua/net_box.lua         |  13 +
>  src/box/prep_stmt.c             | 186 +++++++++++++
>  src/box/prep_stmt.h             | 112 ++++++++
>  src/box/session.cc              |   1 +
>  src/box/sql.c                   |   3 +
>  src/box/sql/analyze.c           |  16 +-
>  src/box/sql/legacy.c            |   3 +-
>  src/box/sql/prepare.c           |  56 +---
>  src/box/sql/sqlInt.h            |  44 +--
>  src/box/sql/vdbe.h              |   2 +-
>  src/box/sql/vdbeInt.h           |   1 -
>  src/box/sql/vdbeapi.c           |  95 +++++--
>  src/box/sql/vdbeaux.c           |   5 +-
>  src/box/xrow.c                  |   2 +-
>  src/box/xrow.h                  |   2 +-
>  src/lib/core/port.h             |   2 +-
>  test/app-tap/init_script.result |  37 +--
>  test/box/admin.result           |   2 +
>  test/box/cfg.result             |   7 +
>  test/box/cfg.test.lua           |   1 +
>  test/box/misc.result            |   3 +
>  test/sql/engine.cfg             |   4 +
>  test/sql/prepared.result        | 592
> ++++++++++++++++++++++++++++++++++++++++ test/sql/prepared.test.lua      |
> 227 +++++++++++++++
>  41 files changed, 1851 insertions(+), 150 deletions(-)
>  create mode 100644 src/box/prep_stmt.c
>  create mode 100644 src/box/prep_stmt.h
>  create mode 100644 test/sql/prepared.result
>  create mode 100644 test/sql/prepared.test.lua


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]