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

[Mergen Imeev <imeevma@tarantool.org>]

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
>