Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Mergen Imeev]

Hello Kirill!

Below you can see my idea of solving the problem. Also,
after that I will answer your questions.

In the last discussion, we decided to solve the problem more
globally: we are going to create a way to change session settings
that can be used from any interface. So, here are the requirements
for this method:
1) All settings must be in the system space or system view.
2) We should be able to SELECT from this space.
3) We should be able to UPDATE data in this space. INSERT, DELETE
and REPLACE cannot be used on this space.
4) This space should not affect performance.
5) This space should not be involved in transactions.
6) We should be able to change the settings on master and on
replica.
7) Space does not have to be persistent.

To fulfill these requirements, I decided to create and use a new
engine. This engine will only be used for this space. The format
of the space will be {{name = 'name', type = 'string'},
{name = 'value', type = 'any'}}. The space will be temporary.

When the user selects a tuple from space, the space will create a
new tuple on the fly. The space itself will be empty, so do not
worry about performance degradation. On UPDATE, the space will
change the session settings.

After we create the space, we can simply remove the control
pragmas. To change a setting from SQL we just need to
update value in the space using UPDATE statement.


On Fri, Dec 06, 2019 at 02:37:11PM +0300, Kirill Yukhin wrote:
> Hello Mergen!
> 
> On 07 ноя 13:36, imeevma@tarantool.org wrote:
> > The main goal of this set of patches is to replace control pragmas
> > with the SET operator. Control pragmas are those that change SQL
> > settings. Along with this, we will allow to see SQL session-local
> > settings in unified way.
> > 
> > https://github.com/tarantool/tarantool/issues/4511
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-pragma-replaced-by-set
> 
> We have at least two mail threads with discussions of how
> this will be working (PRAGMA and SET keywords).
> 
> We have couple of verbal discussions, could you please
> post approach we developed?
> 
> This is what I can remember: we're introducing new view (say
> _vsettings), which is specific for each sessions.
In the current idea, this is not entirely true. The space
is the same for all sessions. Simply, the tuples it creates
contain the settings for the current session.

> This view should contain all settings which user allowed to
> read and write. Inserts are prohibited to that view. Updates
> are alowed only if user has enough privilages to the given
> setting. E.g. user cannot changes a way FK are checked or
> max nesting depth of a SELECT statement.
Since the user will change the setting of current session,
there won't be any interactions between users/sessions.
> 
> I guess, we should create this view lazily in order not to harm
> performance. Please, make sure when you'll be working on the
> patchset.
In a sense, space will work lazily - it will create a tuple
only when asked about it. The only thing we have to do when
starting Tarantool (not a session!) Is to initialize an
array of settings. Also, we must release it upon exiting
Tarantool.

> 
> Having such a view will alow us to:
>   1. get rid of nasty pragma/set statements at all;
>   2. will simplify connectors programming;
>   3. improve security by allowing to *bind* values we
>      wish to update, instead of concatenating big string
>      like other broken DBMSes.
> 
> AFAIR, we decided to set {'string', 'any'} format for that
> new view.
Yes.

> 
> Could you please summarize overall design and post it here
> (or in discussions, whatever).
> 
> --
> Regards, Kirill Yukhin

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Sergey Ostanevich]

Hi!

I believe it can be a missing point: the system space described actually
contains no data at no time. On SELECT from this space a tuple is filled
from the session settings and on UPDATE corresponding setting will be
set in session structure and no data will be present in the space 
thereafter.

Given that - the space will be the same for all sessions, but since it 
contains no data there will be no cross-session interaction.

Regards,
Sergos

On 06 Dec 16:50, Mergen Imeev wrote:
> 
> Hello Kirill!
> 
> Below you can see my idea of solving the problem. Also,
> after that I will answer your questions.
> 
> In the last discussion, we decided to solve the problem more
> globally: we are going to create a way to change session settings
> that can be used from any interface. So, here are the requirements
> for this method:
> 1) All settings must be in the system space or system view.
> 2) We should be able to SELECT from this space.
> 3) We should be able to UPDATE data in this space. INSERT, DELETE
> and REPLACE cannot be used on this space.
> 4) This space should not affect performance.
> 5) This space should not be involved in transactions.
> 6) We should be able to change the settings on master and on
> replica.
> 7) Space does not have to be persistent.
> 
> To fulfill these requirements, I decided to create and use a new
> engine. This engine will only be used for this space. The format
> of the space will be {{name = 'name', type = 'string'},
> {name = 'value', type = 'any'}}. The space will be temporary.
> 
> When the user selects a tuple from space, the space will create a
> new tuple on the fly. The space itself will be empty, so do not
> worry about performance degradation. On UPDATE, the space will
> change the session settings.
> 
> After we create the space, we can simply remove the control
> pragmas. To change a setting from SQL we just need to
> update value in the space using UPDATE statement.
> 
> 
> On Fri, Dec 06, 2019 at 02:37:11PM +0300, Kirill Yukhin wrote:
> > Hello Mergen!
> > 
> > On 07 ноя 13:36, imeevma@tarantool.org wrote:
> > > The main goal of this set of patches is to replace control pragmas
> > > with the SET operator. Control pragmas are those that change SQL
> > > settings. Along with this, we will allow to see SQL session-local
> > > settings in unified way.
> > > 
> > > https://github.com/tarantool/tarantool/issues/4511
> > > https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-pragma-replaced-by-set
> > 
> > We have at least two mail threads with discussions of how
> > this will be working (PRAGMA and SET keywords).
> > 
> > We have couple of verbal discussions, could you please
> > post approach we developed?
> > 
> > This is what I can remember: we're introducing new view (say
> > _vsettings), which is specific for each sessions.
> In the current idea, this is not entirely true. The space
> is the same for all sessions. Simply, the tuples it creates
> contain the settings for the current session.
> 
> > This view should contain all settings which user allowed to
> > read and write. Inserts are prohibited to that view. Updates
> > are alowed only if user has enough privilages to the given
> > setting. E.g. user cannot changes a way FK are checked or
> > max nesting depth of a SELECT statement.
> Since the user will change the setting of current session,
> there won't be any interactions between users/sessions.
> > 
> > I guess, we should create this view lazily in order not to harm
> > performance. Please, make sure when you'll be working on the
> > patchset.
> In a sense, space will work lazily - it will create a tuple
> only when asked about it. The only thing we have to do when
> starting Tarantool (not a session!) Is to initialize an
> array of settings. Also, we must release it upon exiting
> Tarantool.
> 
> > 
> > Having such a view will alow us to:
> >   1. get rid of nasty pragma/set statements at all;
> >   2. will simplify connectors programming;
> >   3. improve security by allowing to *bind* values we
> >      wish to update, instead of concatenating big string
> >      like other broken DBMSes.
> > 
> > AFAIR, we decided to set {'string', 'any'} format for that
> > new view.
> Yes.
> 
> > 
> > Could you please summarize overall design and post it here
> > (or in discussions, whatever).
> > 
> > --
> > Regards, Kirill Yukhin

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Alexander Turenko]

Mergen,

I don't have enough context here, but have questions about this
proposal. Sorry if they were already discussed and I missed it.

Most important question is that I don't understand why we don't want to
provide language specific APIs for sessions settings. It looks both more
convenient and more performant.

See this and other questions and notes below.

(CCed Peter, because he may have some opinion how SQL API should look.)

WBR, Alexader Turenko.

----

AFAIR discussions around a space / view for session settings arose from
Kostya O. proposal to move toward support of standard information schema
views (please, correct me, if I remember it wrongly). Then it becomes
out of scope somehow. But okay, let it being out.

(BTW, I looked over SQL/Schemata 2011 and don't found anything like
MySQL's GLOBAL_VARIABLES and SESSION_VARIABLES tables. It seems there is
no standard table for session variables. I don't sure however.)

----

We have two basic variants:

* Implement an API for session settings for each supported language
  (C, Lua, SQL) and a protocol for connectors.
* Provide a system view / space (this is proposed by Mergen).

First, a space / view is not most convenient way to operate on session
settings from a language. Let's compare.

Lua:

 | box.space._vsession_settings:get({'sql_default_engine'}).value
 | box.space._vsession_settings:update({'sql_default_engine'},
 |                                     {{'=', 'value', 'vinyl'}})
 |
 | box.session.settings:get('sql_default_engine')
 | box.session.settings:set('sql_default_engine', 'vinyl')

SQL:

 | SELECT "value" FROM "_vsession_settings" WHERE "name" = 'sql_default_engine'
 | UPDATE "_vsession_settings" SET "value" = 'vinyl' \
 |                             WHERE "name" = 'sql_default_engine'
 |
 | SESSION GET 'sql_default_engine'
 | SESSION SET 'sql_default_engine' = 'vinyl'

C (sketchy):

 | /* Read from a _vsession_settings. */
 |
 | enum {
 |         BOX_VSESSION_SETTINGS_VALUE_ID = 2
 | };
 |
 | char key[32];
 | char *key_end = key;
 | key_end = mp_encode_array(key_end, 1);
 | key_end = mp_encode_str(key_end, "sql_default_engine",
 |                         sizeof("sql_default_engine") - 1);
 |
 | box_tuple_t *tuple;
 | box_iterator_t *it = box_index_iterator(BOX_VSESSION_SETTINGS_ID, 0, ITER_EQ,
 |                                         key, key_end);
 | box_iterator_next(it, &tuple);
 | const char *buf = box_tuple_field(tuple, BOX_VSESSION_SETTINGS_VALUE_ID);
 |
 | uint32_t engine_len;
 | const char *engine = mp_decode_str(&buf, &engine_len);
 |
 | box_iterator_free(it);
 |
 | /* Update a value in _vsession_settings. */
 |
 | <I'll skip it.>
 |
 | /* Get and set with a language aware API. */
 |
 | uint32_t engine_len;
 | const char *engine = box_session_get_str(SESSION_SQL_DEFAULT_ENGINE,
 |                                          &engine_len);
 |
 | box_session_set_str(SESSION_SQL_DEFAULT_ENGINE, "vinyl",
 |                     sizeof("vinyl") - 1);

Languare-aware APIs above are just examples. I propose to implement such
APIs, but not how they should look exactly.

To sum the examples up: it seems for me that language aware APIs are a
way more simple for a user.

Second, all tuples are msgpack encoded (at least now). So any get/set
operation on _vspace_settings will require to encode and decode msgpack
(yep, both encode and decode at once). It will be surely less performant
then a hashmap lookup (session id -> struct session_settings) plus a
field access.

So, language aware API can be implemented in more performant way then
general space-like one.

It seems that we anyway need an API for connectors. So we can provide
the proposed view, but don't use it internally to implement language
specific APIs (for performance).

----

Re SET / SHOW, GET / SELECT -- I don't think it really matters so much.
Maybe we should look for SQL/PSM and catch some syntax to make things
look consistent in a future. Maybe we should add a keyword SESSION (as
in my SQL API examples) to avoid possible future incompatibilities (say,
with SQL/PSM).

----

Console session settings (like statements delimiter, input language,
output format) are out of scope here?

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Peter Gulutzan]

Hi,

On 2019-12-17 3:11 p.m., Alexander Turenko wrote:
 > Mergen,
 >
 > I don't have enough context here, but have questions about this
 > proposal. Sorry if they were already discussed and I missed it.
 >
 > Most important question is that I don't understand why we don't want to
 > provide language specific APIs for sessions settings. It looks both more
 > convenient and more performant.
 >
 > See this and other questions and notes below.
 >
 > (CCed Peter, because he may have some opinion how SQL API should look.)
 >
 > WBR, Alexader Turenko.
 >
 > ----
 >
 > AFAIR discussions around a space / view for session settings arose from
 > Kostya O. proposal to move toward support of standard information schema
 > views (please, correct me, if I remember it wrongly). Then it becomes
 > out of scope somehow. But okay, let it being out.
 >

I don't know whether you are wrong, but I have looked at the dev thread
that Imeev Mergen started on September 12,
"[dev] Replacing control pragmas by SET".
It seemed to me that Imeev Mergen + Nikita Pettik + Konstantin Osipov
all collaborated about having a system space.


 > (BTW, I looked over SQL/Schemata 2011 and don't found anything like
 > MySQL's GLOBAL_VARIABLES and SESSION_VARIABLES tables. It seems there is
 > no standard table for session variables. I don't sure however.)
 >

You are right, there is no standard table.

 > ----
 >
 > We have two basic variants:
 >
 > * Implement an API for session settings for each supported language
 >   (C, Lua, SQL) and a protocol for connectors.
 > * Provide a system view / space (this is proposed by Mergen).
 >
 > First, a space / view is not most convenient way to operate on session
 > settings from a language. Let's compare.
 >
 > Lua:
 >
 >  | box.space._vsession_settings:get({'sql_default_engine'}).value
 >  | box.space._vsession_settings:update({'sql_default_engine'},
 >  |                                     {{'=', 'value', 'vinyl'}})
 >  |
 >  | box.session.settings:get('sql_default_engine')
 >  | box.session.settings:set('sql_default_engine', 'vinyl')
 >
 > SQL:
 >
 >  | SELECT "value" FROM "_vsession_settings" WHERE "name" = 
'sql_default_engine'
 >  | UPDATE "_vsession_settings" SET "value" = 'vinyl' \
 >  |                             WHERE "name" = 'sql_default_engine'
 >  |
 >  | SESSION GET 'sql_default_engine'
 >  | SESSION SET 'sql_default_engine' = 'vinyl'

 >

 From Issue#4511 "sql: replace PRAGMA by SET for some pragmas"
I gather that Imeev Mergen is already working on something very close to
your UPDATE "_vsession_settings" example.
But Imeev Mergen says I can't SELECT from this space, I don't know why not.
Anyway, if a space exists, SESSION GET or SESSION SET would be redundant.

 > C (sketchy):
 >
 >  | /* Read from a _vsession_settings. */
 >  |
 >  | enum {
 >  |         BOX_VSESSION_SETTINGS_VALUE_ID = 2
 >  | };
 >  |
 >  | char key[32];
 >  | char *key_end = key;
 >  | key_end = mp_encode_array(key_end, 1);
 >  | key_end = mp_encode_str(key_end, "sql_default_engine",
 >  |                         sizeof("sql_default_engine") - 1);
 >  |
 >  | box_tuple_t *tuple;
 >  | box_iterator_t *it = box_index_iterator(BOX_VSESSION_SETTINGS_ID, 
0, ITER_EQ,
 >  |                                         key, key_end);
 >  | box_iterator_next(it, &tuple);
 >  | const char *buf = box_tuple_field(tuple, 
BOX_VSESSION_SETTINGS_VALUE_ID);
 >  |
 >  | uint32_t engine_len;
 >  | const char *engine = mp_decode_str(&buf, &engine_len);
 >  |
 >  | box_iterator_free(it);
 >  |
 >  | /* Update a value in _vsession_settings. */
 >  |
 >  | <I'll skip it.>
 >  |
 >  | /* Get and set with a language aware API. */
 >  |
 >  | uint32_t engine_len;
 >  | const char *engine = box_session_get_str(SESSION_SQL_DEFAULT_ENGINE,
 >  |                                          &engine_len);
 >  |
 >  | box_session_set_str(SESSION_SQL_DEFAULT_ENGINE, "vinyl",
 >  |                     sizeof("vinyl") - 1);
 >
 > Languare-aware APIs above are just examples. I propose to implement such
 > APIs, but not how they should look exactly.
 >
 > To sum the examples up: it seems for me that language aware APIs are a
 > way more simple for a user.

To me, the SQL looks way more simple. But I admit I am prejudiced.

 >
 > Second, all tuples are msgpack encoded (at least now). So any get/set
 > operation on _vspace_settings will require to encode and decode msgpack
 > (yep, both encode and decode at once). It will be surely less performant
 > then a hashmap lookup (session id -> struct session_settings) plus a
 > field access.
 >
 > So, language aware API can be implemented in more performant way then
 > general space-like one.
 >
 > It seems that we anyway need an API for connectors. So we can provide
 > the proposed view, but don't use it internally to implement language
 > specific APIs (for performance).
 > ----
 >
 > Re SET / SHOW, GET / SELECT -- I don't think it really matters so much.
 > Maybe we should look for SQL/PSM and catch some syntax to make things
 > look consistent in a future. Maybe we should add a keyword SESSION (as
 > in my SQL API examples) to avoid possible future incompatibilities (say,
 > with SQL/PSM).
 >
 > ----

Oracle has a variety. For example changing the encryption wallet is done
with special statements (ALTER SYSTEM etc.) and reading is done
with special tables (V$WALLET etc.).
https://www.morganslibrary.org/reference/wallet.html
Here I believe we're getting into an area where trying for compatibility
would be difficult and not rewarding. But I also believe that by
using a system table we'd be slightly more compatible than we are now.

 >
 > Console session settings (like statements delimiter, input language,
 > output format) are out of scope here?

I have documented PRAGMA in the version-2.3 manual.
If you believe it should be removed before this becomes the master manual,
please discuss with Roman Khabibov.

Peter Gulutzan

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Peter Gulutzan]

Hi,

I think that there still might be a few small SQL-specific issues.
I pulled today from branch imeevma/gh-4511-pragma-replaced-by-set.

1.

The type of "value" is 'any', which is shown in metadata as 'string':
"
tarantool> box.execute([[SELECT typeof("value") FROM 
"_vsession_settings" LIMIT 1;]])
---
- metadata:
   - name: typeof("value")
     type: string
   rows:
   - ['any']
...
"

But that means that searches of "value" will usually fail, thus:
"
tarantool> box.execute([[SELECT "name", "value" FROM 
"_vsession_settings" where "value" = 'vinyl';]])
---
- null
- 'Type mismatch: can not convert text to boolean'
...
"

Why not scalar?

2.

You wrote this comment on issue#4511:
"
@pgulutzan @kostja It seems that we are going to solve the problem in a 
different way: we will create a special space that will contain the 
settings. This space will only allow updates. So, to change the setting 
value, you should do something like this:
box.execute([[UPDATE "_session_settings" SET "value" = 'vinyl' WHERE 
"name" = 'sql_default_engine']])
or
box.space._session_settings:update('sql_default_engine', {{'=', 'value', 
'vinyl'}})
This will allow us to use the same method to change any settings in any 
front-end. Also after that we can simply remove the control p
"
But at this moment _session_settings does not exist.
The only thing that works is
box.execute([[SET sql_default_engine = 'vinyl';]])
which is what I and Konstantin Osipov were raising questions about.
In fact I can do it without 'write' privileges on anything.

This is just temporary, right?

3.

VALUE happens to be a reserved word in DB2 and Oracle.
Although we have no plans to reserve it, well, it's a micro-issue for 
compatibility.
You might of course dismiss this because our column name is "value" not 
value.
But suppose that some user agrees totally with Konstantin Osipov (alas),
and decides to use a method that allows access to the data without quote 
marks:
"
tarantool> box.execute([[CREATE VIEW vsession_settings AS SELECT "value" 
AS value, "name" AS name FROM "_vsession_settings";]])
---
- row_count: 1
...

tarantool> box.execute([[SELECT * FROM vsession_settings WHERE name = 
'sql_default_engine';]])
---
- metadata:
   - name: VALUE
     type: any
   - name: NAME
     type: string
   rows:
   - ['vinyl', 'sql_default_engine']
...
"
I think this means that some users will have columns named VALUE,
so we will cause trouble if someday we reserve it.

4.

This suggestion was accepted according to Issue#4511:
_session_settings has columns 'name' and 'value', so:
UPDATE "session_settings" SET "value" = 'vinyl' WHERE "name" = 
'sql_default_engine';
This suggestion (made by N. Pettik in the thread) was rejected:
_session_settings has many columns including 'sql_default_engine' and 
'value', so
UPDATE "session_settings" SET "sql_default_engine" = 'vinyl';

Okay, but ...

If we someday support GRANT, and sql_default_engine was a column, we 
could say:
GRANT UPDATE ON "_session_settings" ("sql_default_engine");
that is, we could be very specific about what you can update.
But there is no equivalent GRANT statement, with #4511,
unless one adds a non-standard extension like
GRANT UPDATE on "_session_settings" ("value") WHERE "name" = 
'sql_default_engine';

I think that a few other things are simpler if sql_default_engine is a 
column:
CHECK ("sql_default_engine" IN ('memtx','vinyl')) versus
CHECK ("name" <> 'sql_default_engine' OR "value" IN ('memtx','vinyl'))
and
SELECT "sql_default_engine", "parser_trace" FROM "_vsession_settings"; 
versus
SELECT "value" AS sql_default_engine FROM "_vsession_settings" WHERE 
"name" = 'sql_default_engine'
UNION
SELECT "value" AS parser_trace FROM "_vsession_settings" WHERE "name" = 
'parser_trace';
(I say "I think" and admit that there may be better solutions I didn't 
think about.)

5.

The name _session_settings hints that, if I change the setting, I only 
affect my own session.
However, as you know, if I change sql_compound_select_limit, I affect 
all sessions.
So perhaps it should be in a new different space, _vglobal_settings?

Peter Gulutzan

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Mergen Imeev]

Hi,

Thank you for your suggestions! I will try to answer your
questions.

On Wed, Dec 18, 2019 at 10:39:58AM -0700, Peter Gulutzan wrote:
> Hi,
> 
> I think that there still might be a few small SQL-specific issues.
> I pulled today from branch imeevma/gh-4511-pragma-replaced-by-set.
>
This branch is out of date. I have not deleted it since
the problem is still not resolved.

New branch: imeevma/gh-4511-new-engine

> 1.
> 
> The type of "value" is 'any', which is shown in metadata as 'string':
> "
> tarantool> box.execute([[SELECT typeof("value") FROM "_vsession_settings"
> LIMIT 1;]])
> ---
> - metadata:
>   - name: typeof("value")
>     type: string
>   rows:
>   - ['any']
> ...
> "
> 
> But that means that searches of "value" will usually fail, thus:
> "
> tarantool> box.execute([[SELECT "name", "value" FROM "_vsession_settings"
> where "value" = 'vinyl';]])
> ---
> - null
> - 'Type mismatch: can not convert text to boolean'
> ...
> "
> 
> Why not scalar?
> 
True, I did not think about this problem before. I think
that I will change the type of the field to SCALAR if
there are no objections.

> 2.
> 
> You wrote this comment on issue#4511:
> "
> @pgulutzan @kostja It seems that we are going to solve the problem in a
> different way: we will create a special space that will contain the
> settings. This space will only allow updates. So, to change the setting
> value, you should do something like this:
> box.execute([[UPDATE "_session_settings" SET "value" = 'vinyl' WHERE "name"
> = 'sql_default_engine']])
> or
> box.space._session_settings:update('sql_default_engine', {{'=', 'value',
> 'vinyl'}})
> This will allow us to use the same method to change any settings in any
> front-end. Also after that we can simply remove the control p
> "
> But at this moment _session_settings does not exist.
> The only thing that works is
> box.execute([[SET sql_default_engine = 'vinyl';]])
> which is what I and Konstantin Osipov were raising questions about.
> In fact I can do it without 'write' privileges on anything.
> 
> This is just temporary, right?
> 
Not exactly, the system space with allowed UPDATE can be
seen on new branch.

> 3.
> 
> VALUE happens to be a reserved word in DB2 and Oracle.
> Although we have no plans to reserve it, well, it's a micro-issue for
> compatibility.
> You might of course dismiss this because our column name is "value" not
> value.
> But suppose that some user agrees totally with Konstantin Osipov (alas),
> and decides to use a method that allows access to the data without quote
> marks:
> "
> tarantool> box.execute([[CREATE VIEW vsession_settings AS SELECT "value" AS
> value, "name" AS name FROM "_vsession_settings";]])
> ---
> - row_count: 1
> ...
> 
> tarantool> box.execute([[SELECT * FROM vsession_settings WHERE name =
> 'sql_default_engine';]])
> ---
> - metadata:
>   - name: VALUE
>     type: any
>   - name: NAME
>     type: string
>   rows:
>   - ['vinyl', 'sql_default_engine']
> ...
> "
> I think this means that some users will have columns named VALUE,
> so we will cause trouble if someday we reserve it.
> 
We can change the name of the field before the patch is
pushed. Do you have any suggestions on how we should name
the field?

> 4.
> 
> This suggestion was accepted according to Issue#4511:
> _session_settings has columns 'name' and 'value', so:
> UPDATE "session_settings" SET "value" = 'vinyl' WHERE "name" =
> 'sql_default_engine';
> This suggestion (made by N. Pettik in the thread) was rejected:
> _session_settings has many columns including 'sql_default_engine' and
> 'value', so
> UPDATE "session_settings" SET "sql_default_engine" = 'vinyl';
> 
> Okay, but ...
> 
> If we someday support GRANT, and sql_default_engine was a column, we could
> say:
> GRANT UPDATE ON "_session_settings" ("sql_default_engine");
> that is, we could be very specific about what you can update.
> But there is no equivalent GRANT statement, with #4511,
> unless one adds a non-standard extension like
> GRANT UPDATE on "_session_settings" ("value") WHERE "name" =
> 'sql_default_engine';
> 
> I think that a few other things are simpler if sql_default_engine is a
> column:
> CHECK ("sql_default_engine" IN ('memtx','vinyl')) versus
> CHECK ("name" <> 'sql_default_engine' OR "value" IN ('memtx','vinyl'))
> and
> SELECT "sql_default_engine", "parser_trace" FROM "_vsession_settings";
> versus
> SELECT "value" AS sql_default_engine FROM "_vsession_settings" WHERE "name"
> = 'sql_default_engine'
> UNION
> SELECT "value" AS parser_trace FROM "_vsession_settings" WHERE "name" =
> 'parser_trace';
> (I say "I think" and admit that there may be better solutions I didn't think
> about.)
> 
That is a bit problematic, since if we want to
add/change/remove a setting, we have to rebuild bootstrap.

At the same time, I think we can add privilege checking.
This should not be a big problem for this space, since
it has custom methods.

> 5.
> 
> The name _session_settings hints that, if I change the setting, I only
> affect my own session.
> However, as you know, if I change sql_compound_select_limit, I affect all
> sessions.
You are a bit wrong: both _vsession_settings and
_session_settings do not contain sql_compound_select_limit
setting.

> So perhaps it should be in a new different space, _vglobal_settings?
> 
There is definitely should be a different system space for
global settings. But, we have to decide, should it be
persistent or not. If the space should be persistent, than
we can create usual system space with memtx engine, that
should contain the settings. In the other case we can use
the same approach as for _session_settings system space.

> Peter Gulutzan
> 

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Peter Gulutzan]

Hi,

On 2019-12-19 2:59 a.m., Mergen Imeev wrote:
 > Hi,
 >
 > Thank you for your suggestions! I will try to answer your
 > questions.
 >
 > On Wed, Dec 18, 2019 at 10:39:58AM -0700, Peter Gulutzan wrote:
 >> Hi,
 >>
 >> I think that there still might be a few small SQL-specific issues.
 >> I pulled today from branch imeevma/gh-4511-pragma-replaced-by-set.
 >>
 > This branch is out of date. I have not deleted it since
 > the problem is still not resolved.
 >
 > New branch: imeevma/gh-4511-new-engine
 >
 >> 1.
 >>
 >> The type of "value" is 'any', which is shown in metadata as 'string':
 >> "
 >> tarantool> box.execute([[SELECT typeof("value") FROM 
"_vsession_settings"
 >> LIMIT 1;]])
 >> ---
 >> - metadata:
 >>   - name: typeof("value")
 >>     type: string
 >>   rows:
 >>   - ['any']
 >> ...
 >> "
 >>
 >> But that means that searches of "value" will usually fail, thus:
 >> "
 >> tarantool> box.execute([[SELECT "name", "value" FROM 
"_vsession_settings"
 >> where "value" = 'vinyl';]])
 >> ---
 >> - null
 >> - 'Type mismatch: can not convert text to boolean'
 >> ...
 >> "
 >>
 >> Why not scalar?
 >>
 > True, I did not think about this problem before. I think
 > that I will change the type of the field to SCALAR if
 > there are no objections.
 >

Thanks, although I see now that my suggestion had a flaw.
Even if you change to SCALAR, the problem will still exist.
I vaguely remember that this has been discussed before,
but I cannot find an email that mentions it.
Anyway: it is bad and I think it should be regarded as a bug.
However, I certainly don't object to changing the data type
to a data type that is known to SQL.

 >> 2.
 >>
 >> You wrote this comment on issue#4511:
 >> "
 >> @pgulutzan @kostja It seems that we are going to solve the problem in a
 >> different way: we will create a special space that will contain the
 >> settings. This space will only allow updates. So, to change the setting
 >> value, you should do something like this:
 >> box.execute([[UPDATE "_session_settings" SET "value" = 'vinyl' WHERE 
"name"
 >> = 'sql_default_engine']])
 >> or
 >> box.space._session_settings:update('sql_default_engine', {{'=', 'value',
 >> 'vinyl'}})
 >> This will allow us to use the same method to change any settings in any
 >> front-end. Also after that we can simply remove the control p
 >> "
 >> But at this moment _session_settings does not exist.
 >> The only thing that works is
 >> box.execute([[SET sql_default_engine = 'vinyl';]])
 >> which is what I and Konstantin Osipov were raising questions about.
 >> In fact I can do it without 'write' privileges on anything.
 >>
 >> This is just temporary, right?
 >>
 > Not exactly, the system space with allowed UPDATE can be
 > seen on new branch.
 >

Yes, I see _session_settings now, although I didn't UPDATE successfully.
And box.execute([[SET sql_default_engine = 'vinyl';]]) fails -- hurray.

 >> 3.
 >>
 >> VALUE happens to be a reserved word in DB2 and Oracle.
 >> Although we have no plans to reserve it, well, it's a micro-issue for
 >> compatibility.
 >> You might of course dismiss this because our column name is "value" not
 >> value.
 >> But suppose that some user agrees totally with Konstantin Osipov (alas),
 >> and decides to use a method that allows access to the data without quote
 >> marks:
 >> "
 >> tarantool> box.execute([[CREATE VIEW vsession_settings AS SELECT 
"value" AS
 >> value, "name" AS name FROM "_vsession_settings";]])
 >> ---
 >> - row_count: 1
 >> ...
 >>
 >> tarantool> box.execute([[SELECT * FROM vsession_settings WHERE name =
 >> 'sql_default_engine';]])
 >> ---
 >> - metadata:
 >>   - name: VALUE
 >>     type: any
 >>   - name: NAME
 >>     type: string
 >>   rows:
 >>   - ['vinyl', 'sql_default_engine']
 >> ...
 >> "
 >> I think this means that some users will have columns named VALUE,
 >> so we will cause trouble if someday we reserve it.
 >>
 > We can change the name of the field before the patch is
 > pushed. Do you have any suggestions on how we should name
 > the field?
 >

Thanks for considering a change.
Unfortunately I have tried and failed to think of a better word.
Therefore, if nobody else has an idea, let us forget this complaint.

 >> 4.
 >>
 >> This suggestion was accepted according to Issue#4511:
 >> _session_settings has columns 'name' and 'value', so:
 >> UPDATE "session_settings" SET "value" = 'vinyl' WHERE "name" =
 >> 'sql_default_engine';
 >> This suggestion (made by N. Pettik in the thread) was rejected:
 >> _session_settings has many columns including 'sql_default_engine' and
 >> 'value', so
 >> UPDATE "session_settings" SET "sql_default_engine" = 'vinyl';
 >>
 >> Okay, but ...
 >>
 >> If we someday support GRANT, and sql_default_engine was a column, we 
could
 >> say:
 >> GRANT UPDATE ON "_session_settings" ("sql_default_engine");
 >> that is, we could be very specific about what you can update.
 >> But there is no equivalent GRANT statement, with #4511,
 >> unless one adds a non-standard extension like
 >> GRANT UPDATE on "_session_settings" ("value") WHERE "name" =
 >> 'sql_default_engine';
 >>
 >> I think that a few other things are simpler if sql_default_engine is a
 >> column:
 >> CHECK ("sql_default_engine" IN ('memtx','vinyl')) versus
 >> CHECK ("name" <> 'sql_default_engine' OR "value" IN ('memtx','vinyl'))
 >> and
 >> SELECT "sql_default_engine", "parser_trace" FROM "_vsession_settings";
 >> versus
 >> SELECT "value" AS sql_default_engine FROM "_vsession_settings" WHERE 
"name"
 >> = 'sql_default_engine'
 >> UNION
 >> SELECT "value" AS parser_trace FROM "_vsession_settings" WHERE "name" =
 >> 'parser_trace';
 >> (I say "I think" and admit that there may be better solutions I 
didn't think
 >> about.)
 >>
 > That is a bit problematic, since if we want to
 > add/change/remove a setting, we have to rebuild bootstrap.
 >

Okay. I regret that, but realize that the world is real.

 > At the same time, I think we can add privilege checking.
 > This should not be a big problem for this space, since
 > it has custom methods.
 >

I guess you mean that you can add a non-standard extension for GRANT.

 >> 5.
 >>
 >> The name _session_settings hints that, if I change the setting, I only
 >> affect my own session.
 >> However, as you know, if I change sql_compound_select_limit, I 
affect all
 >> sessions.
 > You are a bit wrong: both _vsession_settings and
 > _session_settings do not contain sql_compound_select_limit
 > setting.
 >

You are a bit right. I didn't say they contained it, but my remark was 
off topic.

 >> So perhaps it should be in a new different space, _vglobal_settings?
 >>
 > There is definitely should be a different system space for
 > global settings. But, we have to decide, should it be
 > persistent or not. If the space should be persistent, than
 > we can create usual system space with memtx engine, that
 > should contain the settings. In the other case we can use
 > the same approach as for _session_settings system space.

I do not know anything about that.
If the differences affect behaviour that users might expect
(for example rollback and logging), maybe the manual will
mention it but maybe we'll decide they're not important.

Peter Gulutzan

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Mergen Imeev]

Hi,

On Thu, Dec 19, 2019 at 10:35:54AM -0700, Peter Gulutzan wrote:
> Hi,
> 
> On 2019-12-19 2:59 a.m., Mergen Imeev wrote:
> > Hi,
> >
> > Thank you for your suggestions! I will try to answer your
> > questions.
> >
> > On Wed, Dec 18, 2019 at 10:39:58AM -0700, Peter Gulutzan wrote:
> >> Hi,
> >>
> >> I think that there still might be a few small SQL-specific issues.
> >> I pulled today from branch imeevma/gh-4511-pragma-replaced-by-set.
> >>
> > This branch is out of date. I have not deleted it since
> > the problem is still not resolved.
> >
> > New branch: imeevma/gh-4511-new-engine
> >
> >> 1.
> >>
> >> The type of "value" is 'any', which is shown in metadata as 'string':
> >> "
> >> tarantool> box.execute([[SELECT typeof("value") FROM "_vsession_settings"
> >> LIMIT 1;]])
> >> ---
> >> - metadata:
> >>   - name: typeof("value")
> >>     type: string
> >>   rows:
> >>   - ['any']
> >> ...
> >> "
> >>
> >> But that means that searches of "value" will usually fail, thus:
> >> "
> >> tarantool> box.execute([[SELECT "name", "value" FROM "_vsession_settings"
> >> where "value" = 'vinyl';]])
> >> ---
> >> - null
> >> - 'Type mismatch: can not convert text to boolean'
> >> ...
> >> "
> >>
> >> Why not scalar?
> >>
> > True, I did not think about this problem before. I think
> > that I will change the type of the field to SCALAR if
> > there are no objections.
> >
> 
> Thanks, although I see now that my suggestion had a flaw.
> Even if you change to SCALAR, the problem will still exist.
> I vaguely remember that this has been discussed before,
> but I cannot find an email that mentions it.
> Anyway: it is bad and I think it should be regarded as a bug.
> However, I certainly don't object to changing the data type
> to a data type that is known to SQL.
> 
> >> 2.
> >>
> >> You wrote this comment on issue#4511:
> >> "
> >> @pgulutzan @kostja It seems that we are going to solve the problem in a
> >> different way: we will create a special space that will contain the
> >> settings. This space will only allow updates. So, to change the setting
> >> value, you should do something like this:
> >> box.execute([[UPDATE "_session_settings" SET "value" = 'vinyl' WHERE
> "name"
> >> = 'sql_default_engine']])
> >> or
> >> box.space._session_settings:update('sql_default_engine', {{'=', 'value',
> >> 'vinyl'}})
> >> This will allow us to use the same method to change any settings in any
> >> front-end. Also after that we can simply remove the control p
> >> "
> >> But at this moment _session_settings does not exist.
> >> The only thing that works is
> >> box.execute([[SET sql_default_engine = 'vinyl';]])
> >> which is what I and Konstantin Osipov were raising questions about.
> >> In fact I can do it without 'write' privileges on anything.
> >>
> >> This is just temporary, right?
> >>
> > Not exactly, the system space with allowed UPDATE can be
> > seen on new branch.
> >
> 
> Yes, I see _session_settings now, although I didn't UPDATE successfully.
> And box.execute([[SET sql_default_engine = 'vinyl';]]) fails -- hurray.
> 
Well, for SQL it is just a usual space. To update it you
have to use something like:

UPDATE "_session_settings" SET "value" = 'vinyl' WHERE "name" = 'sql_default_engine';

or
box.space._session_settings:update('sql_default_engine', {{'=', 2, 'vinyl'}})

Well, not exactly usual. It can be seen if you try to
change type of value of any setting.

> >> 3.
> >>
> >> VALUE happens to be a reserved word in DB2 and Oracle.
> >> Although we have no plans to reserve it, well, it's a micro-issue for
> >> compatibility.
> >> You might of course dismiss this because our column name is "value" not
> >> value.
> >> But suppose that some user agrees totally with Konstantin Osipov (alas),
> >> and decides to use a method that allows access to the data without quote
> >> marks:
> >> "
> >> tarantool> box.execute([[CREATE VIEW vsession_settings AS SELECT "value"
> AS
> >> value, "name" AS name FROM "_vsession_settings";]])
> >> ---
> >> - row_count: 1
> >> ...
> >>
> >> tarantool> box.execute([[SELECT * FROM vsession_settings WHERE name =
> >> 'sql_default_engine';]])
> >> ---
> >> - metadata:
> >>   - name: VALUE
> >>     type: any
> >>   - name: NAME
> >>     type: string
> >>   rows:
> >>   - ['vinyl', 'sql_default_engine']
> >> ...
> >> "
> >> I think this means that some users will have columns named VALUE,
> >> so we will cause trouble if someday we reserve it.
> >>
> > We can change the name of the field before the patch is
> > pushed. Do you have any suggestions on how we should name
> > the field?
> >
> 
> Thanks for considering a change.
> Unfortunately I have tried and failed to think of a better word.
> Therefore, if nobody else has an idea, let us forget this complaint.
> 
> >> 4.
> >>
> >> This suggestion was accepted according to Issue#4511:
> >> _session_settings has columns 'name' and 'value', so:
> >> UPDATE "session_settings" SET "value" = 'vinyl' WHERE "name" =
> >> 'sql_default_engine';
> >> This suggestion (made by N. Pettik in the thread) was rejected:
> >> _session_settings has many columns including 'sql_default_engine' and
> >> 'value', so
> >> UPDATE "session_settings" SET "sql_default_engine" = 'vinyl';
> >>
> >> Okay, but ...
> >>
> >> If we someday support GRANT, and sql_default_engine was a column, we
> could
> >> say:
> >> GRANT UPDATE ON "_session_settings" ("sql_default_engine");
> >> that is, we could be very specific about what you can update.
> >> But there is no equivalent GRANT statement, with #4511,
> >> unless one adds a non-standard extension like
> >> GRANT UPDATE on "_session_settings" ("value") WHERE "name" =
> >> 'sql_default_engine';
> >>
> >> I think that a few other things are simpler if sql_default_engine is a
> >> column:
> >> CHECK ("sql_default_engine" IN ('memtx','vinyl')) versus
> >> CHECK ("name" <> 'sql_default_engine' OR "value" IN ('memtx','vinyl'))
> >> and
> >> SELECT "sql_default_engine", "parser_trace" FROM "_vsession_settings";
> >> versus
> >> SELECT "value" AS sql_default_engine FROM "_vsession_settings" WHERE
> "name"
> >> = 'sql_default_engine'
> >> UNION
> >> SELECT "value" AS parser_trace FROM "_vsession_settings" WHERE "name" =
> >> 'parser_trace';
> >> (I say "I think" and admit that there may be better solutions I didn't
> think
> >> about.)
> >>
> > That is a bit problematic, since if we want to
> > add/change/remove a setting, we have to rebuild bootstrap.
> >
> 
> Okay. I regret that, but realize that the world is real.
> 
> > At the same time, I think we can add privilege checking.
> > This should not be a big problem for this space, since
> > it has custom methods.
> >
> 
> I guess you mean that you can add a non-standard extension for GRANT.
> 
Not sure for now, but I will look at this later.

> >> 5.
> >>
> >> The name _session_settings hints that, if I change the setting, I only
> >> affect my own session.
> >> However, as you know, if I change sql_compound_select_limit, I affect all
> >> sessions.
> > You are a bit wrong: both _vsession_settings and
> > _session_settings do not contain sql_compound_select_limit
> > setting.
> >
> 
> You are a bit right. I didn't say they contained it, but my remark was off
> topic.
> 
> >> So perhaps it should be in a new different space, _vglobal_settings?
> >>
> > There is definitely should be a different system space for
> > global settings. But, we have to decide, should it be
> > persistent or not. If the space should be persistent, than
> > we can create usual system space with memtx engine, that
> > should contain the settings. In the other case we can use
> > the same approach as for _session_settings system space.
> 
> I do not know anything about that.
> If the differences affect behaviour that users might expect
> (for example rollback and logging), maybe the manual will
> mention it but maybe we'll decide they're not important.
> 
> Peter Gulutzan
> 

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Vladislav Shpilevoy]

Hi!

Just one small note below.

On 18/12/2019 18:39, Peter Gulutzan wrote:
> Hi,
> 
> I think that there still might be a few small SQL-specific issues.
> I pulled today from branch imeevma/gh-4511-pragma-replaced-by-set.
> 
> 1.
> 
> The type of "value" is 'any', which is shown in metadata as 'string':
> "
> tarantool> box.execute([[SELECT typeof("value") FROM "_vsession_settings" LIMIT 1;]])
> ---
> - metadata:
>   - name: typeof("value")
>     type: string
>   rows:
>   - ['any']
> ...
> "

Well, 'string' is not a type of "value". It is a type of
typeof() function result. So here everything is alright.

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Kirill Yukhin]

Hello,

On 18 дек 01:11, Alexander Turenko wrote:
> Mergen,
> 
> I don't have enough context here, but have questions about this
> proposal. Sorry if they were already discussed and I missed it.
> 
> Most important question is that I don't understand why we don't want to
> provide language specific APIs for sessions settings. It looks both more
> convenient and more performant.
> 
> See this and other questions and notes below.
> 
> (CCed Peter, because he may have some opinion how SQL API should look.)
> 
> WBR, Alexader Turenko.
> 
> ----
> 
> AFAIR discussions around a space / view for session settings arose from
> Kostya O. proposal to move toward support of standard information schema
> views (please, correct me, if I remember it wrongly). Then it becomes
> out of scope somehow. But okay, let it being out.
> 
> (BTW, I looked over SQL/Schemata 2011 and don't found anything like
> MySQL's GLOBAL_VARIABLES and SESSION_VARIABLES tables. It seems there is
> no standard table for session variables. I don't sure however.)
> 
> ----
> 
> We have two basic variants:
> 
> * Implement an API for session settings for each supported language
>   (C, Lua, SQL) and a protocol for connectors.
> * Provide a system view / space (this is proposed by Mergen).
> 
> First, a space / view is not most convenient way to operate on session
> settings from a language. Let's compare.
> 
> Lua:
> 
>  | box.space._vsession_settings:get({'sql_default_engine'}).value
>  | box.space._vsession_settings:update({'sql_default_engine'},
>  |                                     {{'=', 'value', 'vinyl'}})
>  |
>  | box.session.settings:get('sql_default_engine')
>  | box.session.settings:set('sql_default_engine', 'vinyl')

Frankly, to me this is not of big difference. Especially when we are
talking about settings, references to which are rare.

After all, one might want to implement setter to avoid such
updates.

> SQL:
> 
>  | SELECT "value" FROM "_vsession_settings" WHERE "name" = 'sql_default_engine'
>  | UPDATE "_vsession_settings" SET "value" = 'vinyl' \
>  |                             WHERE "name" = 'sql_default_engine'
>  |
>  | SESSION GET 'sql_default_engine'
>  | SESSION SET 'sql_default_engine' = 'vinyl'
> 
> C (sketchy):
> 
>  | /* Read from a _vsession_settings. */
>  |
>  | enum {
>  |         BOX_VSESSION_SETTINGS_VALUE_ID = 2
>  | };
>  |
>  | char key[32];
>  | char *key_end = key;
>  | key_end = mp_encode_array(key_end, 1);
>  | key_end = mp_encode_str(key_end, "sql_default_engine",
>  |                         sizeof("sql_default_engine") - 1);
>  |
>  | box_tuple_t *tuple;
>  | box_iterator_t *it = box_index_iterator(BOX_VSESSION_SETTINGS_ID, 0, ITER_EQ,
>  |                                         key, key_end);
>  | box_iterator_next(it, &tuple);
>  | const char *buf = box_tuple_field(tuple, BOX_VSESSION_SETTINGS_VALUE_ID);
>  |
>  | uint32_t engine_len;
>  | const char *engine = mp_decode_str(&buf, &engine_len);
>  |
>  | box_iterator_free(it);
>  |
>  | /* Update a value in _vsession_settings. */
>  |
>  | <I'll skip it.>
>  |
>  | /* Get and set with a language aware API. */
>  |
>  | uint32_t engine_len;
>  | const char *engine = box_session_get_str(SESSION_SQL_DEFAULT_ENGINE,
>  |                                          &engine_len);
>  |
>  | box_session_set_str(SESSION_SQL_DEFAULT_ENGINE, "vinyl",
>  |                     sizeof("vinyl") - 1);
> 
> Languare-aware APIs above are just examples. I propose to implement such
> APIs, but not how they should look exactly.

This might have sense, but I'd treat it as a follow up activity.

> To sum the examples up: it seems for me that language aware APIs are a
> way more simple for a user.
> 
> Second, all tuples are msgpack encoded (at least now). So any get/set
> operation on _vspace_settings will require to encode and decode msgpack
> (yep, both encode and decode at once). It will be surely less performant
> then a hashmap lookup (session id -> struct session_settings) plus a
> field access.
> 
> So, language aware API can be implemented in more performant way then
> general space-like one.

I think performance out of scope here at all.
 
> It seems that we anyway need an API for connectors. So we can provide
> the proposed view, but don't use it internally to implement language
> specific APIs (for performance).
> 
> Console session settings (like statements delimiter, input language,
> output format) are out of scope here?

Yes.

To sum up. We spent too much time here. I think we can improve approaches
in future. But I see no serious reasons for that:
  1. To make it easier to use we might whant to implement some stored
     routines or something/
  2. Performance of encode/decode is out of intereset here.

I propose you to file a feature request as follow up of the patchset.

--
Regards, Kirill Yukhin

Re: [Tarantool-discussions] [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET

[Alexander Turenko]

> To sum up. We spent too much time here. I think we can improve approaches
> in future. But I see no serious reasons for that:
>   1. To make it easier to use we might whant to implement some stored
>      routines or something/
>   2. Performance of encode/decode is out of intereset here.
> 
> I propose you to file a feature request as follow up of the patchset.

I read this as 'any design / API is okay'. So there is no sense to
discuss it further.

For me access a field of a structure with msgpack encode and then decode
is a kind of `toString(bool_value).length() != 5` check.

It is a bit toxic, I know. Sorry for this.

WBR, Alexander Turenko.