Sergei, sorry, I forgot to add that implementation with @@force_fields_visible is cleaner and shorter. Also that helps to deprecate sysvers_show and MDEV-16587 which I would greatly appreciate.
On Tue, Apr 27, 2021 at 4:07 PM Aleksey Midenkov <mide...@gmail.com> wrote: > > Hi Sergei! > > On Tue, Apr 6, 2021 at 3:29 PM Sergei Golubchik <s...@mariadb.org> wrote: > > > > Hi, Aleksey! > > > > On Apr 06, Aleksey Midenkov wrote: > > > > > But, since we need to specify implicit system fields we cannot > > > > > avoid adding one more session variable. In my current iteration I > > > > > made @@force_fields_visible which is more straightforward in what > > > > > it does: > > > > > > > > I'm sorry, I don't understand. > > > > > > > > First, visibility is pretty much unrelated concept. > > > > row_start/row_end can be visible or invisible, and they can be > > > > writable or not writable - those are orthogonal concepts. > > > > > > To be able to specify them in INSERT command they must be at least > > > user-invisible. System-invisible fields are ignored. > > > > Sure, that's what @@system_versioning_insert_history would do. > > > > That's not about permission of inserting history which can be > controlled by @@secure_timestamp setting. But rather that's about > allowing to put system-invisible fields into INSERT command. You can > suggest a better name for it. > > > > > And second, the name is wrong, there are no "fields" row_start and > > > > row_end unless the user creates then explicitly. They are pieces of > > > > metadata that every row has, something that Oracle, for example, > > > > calls "pseudocolumns". Something like > > > > @@system_versioning_row_start_row_end_visible would be more correct, > > > > but ugly. In fact, I'd say that @@system_versioning_insert_history > > > > was the best one. > > > > > > I think you are complicating things where complication is not needed. > > > Pseudo- or not they are fields. > > > > No, this is internal implementation detail that can change and it should > > not leak into the UI. Like, sql_select.cc has a function called > > sub_select(), but we never tell users that what it does is "subselect", > > not in the documentation, not in error messages, never. Because it > > doesn't (it performs one step in a nested-loop join). > > Every concept should have a proper application. I don't think "double > design" in hiding system versioning fields adds any value. I can not > be sure about "subselect" design, but I suspect it will never be > replaced by anything else. In any case each and every solution should > be judged by whether it adds usability (i.e. ease of use) or not. I > have a suspicion that the current design of hidden row_start/row_end > adds more obstacles to a user than helps him. > > > > > The fact that some internal enum value is named INVISIBLE_SYSTEM > > is not something that should affect the user visible behavior. > > > > > Besides, the variable is not about system versioning. It can make any > > > system-invisible fields visible. > > > > Which is incorrect. ROWNUM (MDEV-24089) is a pseudocolumn, and it cannot > > be "visible" in the sense of @force_fields_visible. We don't support > > ROWID, but if we would — it cannot be visible in the sense of > > @force_fields_visible either. Basically this variable can only apply to > > row_start/row_end pseudocolumns, despite its generic name. > > That is just the subject for a new feature request, isn't it? In any > case, please suggest a different name. I can think of > @@system_versioning_show_row_timestamps but this looks a bit long to > me. > > > > > Regards, > > Sergei > > VP of MariaDB Server Engineering > > and secur...@mariadb.org > > > > -- > All the best, > > Aleksey Midenkov > @midenok -- All the best, Aleksey Midenkov @midenok _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp