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

[Mergen Imeev <imeevma@tarantool.org>]

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
>