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 20:51:16 +0300	[thread overview]
Message-ID: <20191219175116.GA7444@tarantool.org> (raw)
In-Reply-To: <e9280487-90fa-635a-08d0-b6674f1698a5@ocelot.ca>

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
> 

  reply	other threads:[~2019-12-19 17:51 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
2019-12-19 17:35             ` Peter Gulutzan
2019-12-19 17:51               ` Mergen Imeev [this message]
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=20191219175116.GA7444@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