From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 19 Dec 2019 20:51:16 +0300 From: Mergen Imeev Message-ID: <20191219175116.GA7444@tarantool.org> References: <20191206113711.ctzr6x7sqbpr3xkd@tarantool.org> <20191206135009.GA6394@tarantool.org> <20191217221143.parwzoyb3e327sw5@tkn_work_nb> <77dd60db-ba26-da21-9502-19f089f1559c@ocelot.ca> <67f8badf-2c98-83de-b2aa-781953be5a21@ocelot.ca> <20191219095958.GA26922@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v3 0/5] Replace control pragmas by SET List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Gulutzan Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, tarantool-discussions@dev.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 >