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

Already on GitHub? Sign in to your account

Fix 0017446: Timeline: decrease length of index #210

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

grangeway commented Jun 13, 2014

No description provided.

Member

atrol commented Jun 13, 2014

I am not aware that there is a problem.
Which database does not support the length?

Contributor

grangeway commented Jun 13, 2014

Oracle for instance only allows index strings <30 characters.

To allow for people to have custom prefixes etc, and avoid future issues, we should shorten the length.

For example, if you look at upgrade 187 on the bug monitor table, we use a significantly shorter string:

$upgrade[187] = array( 'CreateIndexSQL', array( 'idx_bug_id', db_get_table( 'bug_monitor' ), 'bug_id' ) );

If you look at upgrade 178, damien already has custom code in master to do specific stuff for oracle.

If you look at the code in the new db layer, it takes a slightly different approach to dealing with the issue.

[I'd probably argue that the name of the index in upgrade step 187 was not really a sensible choice given our previous naming, but that index was added by damien in https://github.com/mantisbt/mantisbt/commit/8ff833820fefc232656ac620a63e101c65017195 and has been in our releases for a while].

In the case of this index, I'd rather shorten the name now, then be stuck with having to work around issues regarding it's length in the future.

Member

rombert commented Jun 13, 2014

I agree with the general principle. However, it looks to me ( without testing in Oracle ) that the index string is 30 characters long, which is exactly the Oracle limit.

In this situation, the current index would be fine. Am I missing something?

Member

atrol commented Jun 13, 2014

Oracle for instance only allows index strings <30 characters.

AFAIK it's <= 30

To allow for people to have custom prefixes

I am not aware that we add the prefixes to index names

If you look at upgrade 178, damien already has custom code in master to do specific stuff for oracle.

This was because the length was 31

Member

atrol commented Jun 13, 2014

is that the index string is 30 characters long

it's 29

Member

rombert commented Jun 13, 2014

is that the index string is 30 characters long

it's 29

$ echo idx_bug_history_date_modified | wc -c
30

Am I counting wrong? But anyway, both 29 and 30 are fine AFAICT.

Member

atrol commented Jun 13, 2014

Am I counting wrong? But anyway, both 29 and 30 are fine AFAICT.

Yes

$ echo 1 | wc -c
2
$ echo -n 1 | wc -c
1
Member

rombert commented Jun 13, 2014

Aha, good point. Thanks.

Contributor

grangeway commented Jun 13, 2014

sigh,

If you look at the new code for the database layer, my plan is currently to allow strings for Y of up to 27 characters to allow "X_Y" where where X is "mantis_" or "idx_" etc- and for oracle the shorter version of M_

Are we seriously going to have a long debate over this? ;/ - given that we currently use shorter strings for other indexes (upgrade 187) that don't include all the details, and that this code got pushed 2 days ago.

Member

atrol commented Jun 13, 2014

Are we seriously going to have a long debate over this?

No.
I think this PR can be closed without changing any code.

If your plan is to introduce clear naming conventions in new database layer it could be idx_Y where Y is up to 26 characters.
This works with all databases.
idx_bug_history_date_modified does not break this rule, thus I see no need for a change.

Contributor

grangeway commented Jun 13, 2014

The index identifier itself would be up to 26 characters.

Contributor

grangeway commented Jun 13, 2014

Seriously though, we should avoid long strings - so I'm not really sure it's a problem to change.

What problem do you foresee with us changing this now to something shorter?

Contributor

grangeway commented Jun 13, 2014

If you consider the line above this change:

$upgrade[191] = array( 'CreateIndexSQL', array('idx_project_hierarchy', db_get_table( 'project_hierarchy' ),'child_id,parent_id',array('UNIQUE')));

it's not like we actually have a standard for index naming we are violating by reducing the length.

I can generate a separate PR to use idx_bug_history_date if you don't like the abbreviation of "bug_history" to "bh" - i.e. if we want to keep table name valid.

Although, the rule of keeping the table name valid is broken by other index naming - for example:

$upgrade[ 9] = array('CreateIndexSQL',array('idx_relationship_source',db_get_table('bug_relationship'),'source_bug_id'));

Member

atrol commented Jun 14, 2014

What problem do you foresee with us changing this now to something shorter?

I have no problem with it.
I just don't understand that you want to change something although there is no need to change it.

@grangeway grangeway closed this Jun 14, 2014

@grangeway grangeway deleted the grangeway:17446 branch Jun 14, 2014

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