Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev <imeevma@tarantool.org>
To: Peter Gulutzan <pgulutzan@ocelot.ca>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org,
	tarantool-discussions@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET
Date: Thu, 19 Dec 2019 12:59:58 +0300	[thread overview]
Message-ID: <20191219095958.GA26922@tarantool.org> (raw)
In-Reply-To: <67f8badf-2c98-83de-b2aa-781953be5a21@ocelot.ca>

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
> 

  reply	other threads:[~2019-12-19  9:59 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-07 10:36 imeevma
2019-11-07 10:36 ` [Tarantool-patches] [PATCH v3 1/5] sysview: make get() and create_iterator() methods virtual imeevma
2019-11-07 10:36 ` [Tarantool-patches] [PATCH v3 2/5] box: introdice _vsession_settings sysview imeevma
2019-11-07 10:36 ` [Tarantool-patches] [PATCH v3 3/5] sql: introduce SET statement imeevma
2019-11-07 12:40   ` Vladislav Shpilevoy
2019-11-07 14:12     ` Mergen Imeev
2019-11-11 21:56       ` Vladislav Shpilevoy
2019-11-15 14:06         ` Mergen Imeev
2019-11-17 17:26           ` Vladislav Shpilevoy
2019-11-17 20:32             ` Vladislav Shpilevoy
2019-11-27 10:33               ` Mergen Imeev
2019-11-27 23:03                 ` Vladislav Shpilevoy
2019-11-27 23:07                   ` Vladislav Shpilevoy
2019-11-27 23:09                     ` Vladislav Shpilevoy
2019-11-28  8:59                     ` Mergen Imeev
2019-11-28  8:56                   ` Mergen Imeev
2019-11-07 10:36 ` [Tarantool-patches] [PATCH v3 4/5] temporary: disable boolean.test.sql imeevma
2019-11-07 10:37 ` [Tarantool-patches] [PATCH v3 5/5] sql: replace control pragmas imeevma
2019-12-06 11:37 ` [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET Kirill Yukhin
2019-12-06 13:50   ` Mergen Imeev
2019-12-06 14:06     ` Sergey Ostanevich
2019-12-17 22:11     ` Alexander Turenko
2019-12-18  2:39       ` Peter Gulutzan
2019-12-18 17:39         ` Peter Gulutzan
2019-12-19  9:59           ` Mergen Imeev [this message]
2019-12-19 17:35             ` Peter Gulutzan
2019-12-19 17:51               ` Mergen Imeev
2019-12-19 21:09           ` Vladislav Shpilevoy
2019-12-18 10:20       ` Kirill Yukhin
2019-12-18 10:53         ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191219095958.GA26922@tarantool.org \
    --to=imeevma@tarantool.org \
    --cc=pgulutzan@ocelot.ca \
    --cc=tarantool-discussions@dev.tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox