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

[Peter Gulutzan <pgulutzan@ocelot.ca>]

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