Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the restriction on create/delete, enable/disable the user indexes in system tables [CORE5746] #2171

Closed
firebird-automations opened this issue Feb 11, 2018 · 18 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: @abzalov

Assigned to: @abzalov

Jira_subtask_inward CORE5612

Attachments:
test.7z
core_4731.fbt

Proceeding from the fact that it is necessary to apply adaptive mechanics, and from the fact that in the source code is not used anywhere specifying the plan (perhaps there are reasons for it) - there is hardly a chance that you implement the use of "correct" system indexes in the right places.
Therefore, please remove the restriction on creating indexes for system tables, and disabling/enabling system indexes.
We ourselves will manipulate them at the right time.

In the end, what can be a bad from modification of system indexes, given that DB developers understand what they are doing?
For example, in Oracle there are no restrictions in modifying the system tables - even insert into dual table, not to mention the creation of indexes.

Of course, we are can find a way around this restriction (check_gbak_cheating_insupd, check_gbak_cheating_delete), but why can not you provide a regular way?

Commits: ec86419 bf5614b acb1c28 191a3b6 2bc844e FirebirdSQL/fbt-repository@a4ad37c

====== Test Details ======

todo: update test for CORE4731 ("Prohibit an ability to issue DML or DDL statements on RDB$ tables") - no more restriction on creating user indices for system tables.

@firebird-automations
Copy link
Collaborator Author

Modified by: @abzalov

description: Proceeding from the fact that it is necessary to apply adaptive mechanics, and from the fact that in the source code is not used anywhere specifying the plan (perhaps there are reasons for it) - there is hardly a chance that you implement the use of "correct" system indexes in the right places.
Therefore, please remove the restriction on creating indexes for system tables, and disabling/enabling system indexes.
We ourselves will manipulate them at the right time.

In the end, what can be a bad from modification of system indexes, given that DB developers understand what they are doing?
For example, in Oracle there are no restrictions in modifying the system tables - even insert into dual table, not to mention the creation of indexes.

Of course, you can find a way around this restriction (check_gbak_cheating_insupd, check_gbak_cheating_delete), but why can not you provide a regular way?

=>

Proceeding from the fact that it is necessary to apply adaptive mechanics, and from the fact that in the source code is not used anywhere specifying the plan (perhaps there are reasons for it) - there is hardly a chance that you implement the use of "correct" system indexes in the right places.
Therefore, please remove the restriction on creating indexes for system tables, and disabling/enabling system indexes.
We ourselves will manipulate them at the right time.

In the end, what can be a bad from modification of system indexes, given that DB developers understand what they are doing?
For example, in Oracle there are no restrictions in modifying the system tables - even insert into dual table, not to mention the creation of indexes.

Of course, we are can find a way around this restriction (check_gbak_cheating_insupd, check_gbak_cheating_delete), but why can not you provide a regular way?

@firebird-automations
Copy link
Collaborator Author

Commented by: Sean Leyne (seanleyne)

I don't believe that allowing developers to make changes to system indexes is necessary or advisable.

IMO, what you are seeing is the result some core FB deficiencies (1) the lack of some keys indexes for your use case (which would likely benefit all FB users and (2) some potential optimizations/changes that should be made to selected engine routines*

* Vlad Khorsun posted to the Firebird-Dev mailing list.

I could be wrong, but: to make it properly, one should look as SCL_check_relation(..., SCL_control) calls at VIO_erase\VIO_modify\VIO_store near the "case rel_indices" and pass "false" into last parameter
(protectSys) for user indices. The same should be done at checkPermission() method of CreateIndexNode, AlterIndexNode, DropIndexNode, and SetStatisticsNode classes.

@firebird-automations
Copy link
Collaborator Author

Commented by: @abzalov

Test case with database for 2.5 and it's backup. The database contains 300 existed views.
The bat-file creates 100 views in two ways (in a copy of the source database):
   1) with the disabled system indexes in RDB$DEPENDENCIES
   2) with the enabled system indexes in RDB$DEPENDENCIES
Disabled indices are RDB$INDEX_27 (RDB$DEPENDENT_NAME) and RDB$INDEX_28 (RDB$DEPENDED_ON_NAME).

@firebird-automations
Copy link
Collaborator Author

Modified by: @abzalov

Attachment: test.7z [ 13220 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @abzalov

The test case is not correct because it was tested on version 2.5.5.
On version 2.5.8 it not reproduced - the execution time is equally fast.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

The pull requests implementing "enable to manage user indices on system tables" is merged, see
#156 and
#155

I offer to change this ticket description accordingly (user indices, not system!) and close it.

@firebird-automations
Copy link
Collaborator Author

Modified by: @abzalov

summary: Remove the restriction on create/delete, enable/disable the system indexes in system tables => Remove the restriction on create/delete, enable/disable the user indexes in system tables

@firebird-automations
Copy link
Collaborator Author

Modified by: @hvlad

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 4.0 Beta 1 [ 10750 ]

Fix Version: 3.0.4 [ 10863 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

assignee: Rashid Abzalov [ rashid ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Done successfully

Test Details: todo: update test for CORE4731 ("Prohibit an ability to issue DML or DDL statements on RDB$ tables") - no more restriction on creating user indices for system tables.

@firebird-automations
Copy link
Collaborator Author

Commented by: @artyom-smirnov

I noticed after those changes test bugs.core_4731 from fbt now hanging server (both 3 and 4). It quite complex test and I have not yet extracted exact testcase but it hang on query like "create descending index RDB_PROCEDURES on RDB$PROCEDURES(RDB$PROCEDURE_NAME);" which appears to be related to this ticket.

@firebird-automations
Copy link
Collaborator Author

Commented by: @abzalov

Where can I see the contents of this test?

@firebird-automations
Copy link
Collaborator Author

@firebird-automations
Copy link
Collaborator Author

Commented by: @abzalov

New test file with fixed hang is attached.

But probably in the test file still need to add a check to remove/disable the system and user indexes

@firebird-automations
Copy link
Collaborator Author

Modified by: @abzalov

Attachment: core_4731.fbt [ 13236 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

The test is fixed, thanks !

@firebird-automations
Copy link
Collaborator Author

Commented by: @livius2

Is this really fixed?
About what system tables is this ticket?

i try to create index on MON$ tables and it fail WI-V3.0.4.32954 Firebird 3.0
SQL Message : -607
This operation is not defined for system tables.

Engine Code : 335544351
Engine Message :
unsuccessful metadata update
feature is not supported

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Karol,

yes, it was really fixed, i believe.

It is about "real" tables, not virtual. I.e. it is about RDB$ tables and not about MON$\SEC$.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment