Opened 5 years ago

Closed 2 weeks ago

#16220 closed Cleanup/optimization (fixed)

Add introspection for multicolumn indexes

Reported by: Jeffrey Gelens Owned by: Jeffrey Gelens
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: dceu2011 db-indexes
Cc: jeffrey@…, hvdklauw@…, gert.vangool@…, charette.s@…, Ivan Virabyan, aksheshdoshi@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The current database introspection methods don't support multi column indexes.

It's difficult to add this functionality to the current get_indexes methods as it's difficult to include it the current returned data d, which is:

{u'group_id': {'primary_key': False, 'unique': False},
 u'id': {'primary_key': True, 'unique': True}}

What hvdklauw, jgelens and charstring suggest is a (*cough*) "get_real_indexes" method.

Attachments (1)

get_indexes.diff (4.2 KB) - added by Harro 5 years ago.
Quick implementation for PostgreSQL, MySQL and SQLite

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by Harro

Cc: Harro added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Also to note, the current mysql implementation isn't even correct compared to the other implementations. (dunno about oracle, don't know it).
It get's all the indexes and then loops through them and adds them to the dict. Meaning the last mention of a fieldname is used to define the indexes is has.

comment:2 Changed 5 years ago by Jeffrey Gelens

As this is a non public API the result can be updated to include the multicolumn fields. South is using this introspection function too, it needs to be patched accordingly.

Last edited 5 years ago by Jeffrey Gelens (previous) (diff)

comment:3 Changed 5 years ago by Jeffrey Gelens

Cc: jeffrey@… added

comment:4 Changed 5 years ago by Jeffrey Gelens

Suggestion:

Old output:

{u'group_id': {'primary_key': False, 'unique': False},
 u'id': {'primary_key': True, 'unique': True}}

Possible new output:

{
    u'table_name_pkey': {
        'primary_key': False, 
        'unique': False, 
        'fields': ('id',)
    },
    u'table_name_somefield': {
        'primary_key': False, 
        'unique': False, 
        'fields': ('somefield',)
    },
    u'table_name_355fd33': {
        'primary_key': False, 
        'unique': False, 
        'fields': ('somefield1', 'somefield2')  # a multicolumn index
    },
}

Where the key of the primary hash is the index name like how it's represented in the DB.

Last edited 5 years ago by Jeffrey Gelens (previous) (diff)

comment:5 Changed 5 years ago by Harro

Triage Stage: UnreviewedDesign decision needed

comment:6 Changed 5 years ago by Harro

Cc: Harro removed

comment:7 Changed 5 years ago by Harro

Cc: hvdklauw@… added

comment:8 Changed 5 years ago by Ramiro Morales

Keywords: inspectdb added

comment:9 Changed 5 years ago by Harro

#5805 depends on this one

comment:10 Changed 5 years ago by Gert Van Gool

Cc: gert.vangool@… added

comment:11 Changed 5 years ago by Jeffrey Gelens

Got this working now:

{
    u'id': {
        'primary_key': False, 
        'unique': False, 
        'fields': ('id',)
    },
    u'somefield': {
        'primary_key': False, 
        'unique': True, 
        'fields': ('somefield',)
    },
    u'somefield1': {
        'primary_key': False, 
        'unique': False, 
        'fields': ('somefield1', 'somefield2')  # a multicolumn index
    },
}

Btw, I didn't find any usage of get_indexes in south :-/

comment:12 Changed 5 years ago by Jeffrey Gelens

Owner: changed from nobody to Jeffrey Gelens

Changed 5 years ago by Harro

Attachment: get_indexes.diff added

Quick implementation for PostgreSQL, MySQL and SQLite

comment:13 Changed 5 years ago by Harro

I did note some inconsistencies between the different backends. (SQLite didn't output unique indexes)

Now they do the same, still not sure about the key values, you can't use the field name as there might be multiple.

I don't have access or knowledge of Oracle, so someone who does should fix that; also haven't written tests because django by itself doesn't support adding indexes on multiple fields (need #5805 for that, which in turn needs this to do tests)

comment:14 Changed 5 years ago by Simon Charette

Cc: charette.s@… added

comment:15 Changed 4 years ago by Ivan Virabyan

Cc: Ivan Virabyan added

comment:16 Changed 4 years ago by Dan Loewenherz

Has patch: set
Needs tests: set

This blocks #5805 (or at least blocks writing tests that ensure #5805 is working).

I see there's a design decision needed. IMO this is a good addition--there's a solid use case for only needing to index fields in the context of others. Some queries always reference the same fields when filtering, e.g. first_name + last_name.

comment:17 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

What design decision is expected?

comment:18 Changed 4 years ago by Jeffrey Gelens

I think the design decision was for the suggested output of get_indexes().

comment:19 Changed 6 weeks ago by Akshesh Doshi

The get_constraints method of the introspection currently returns something similar to this for all databases.

{  
    u'introspection_article_headline_pub_date_c749bad3_idx':{  
        'index':True,
        'primary_key':False,
        'orders':['ASC', 'ASC'],
        'foreign_key':False,
        'unique':False,
        'check':False,
        'columns':[u'headline', u'pub_date']
    },
    '__primary__':{  
        'index':False,
        'primary_key':True,
        'foreign_key':False,
        'unique':False,
        'check':False,
        'columns':[u'id']
    },
    u'introspection_article_reporter_id_c7562f75':{  
        'index':True,
        'primary_key':False,
        'orders':['ASC'],
        'foreign_key':False,
        'unique':False,
        'check':False,
        'columns':[u'reporter_id']
    },
}

If I understand correctly, that means this ticket got resolved somewhere down the line.

---

I also found an introspection test for get_indexes method with the following docstring

"""
Test that multicolumn indexes are not included in the introspection
results.
"""

which I found something contradicting to this ticket.

Last edited 6 weeks ago by Akshesh Doshi (previous) (diff)

comment:20 Changed 6 weeks ago by Akshesh Doshi

Keywords: db-indexes added; inspectdb removed
Version: 1.3master

comment:21 Changed 6 weeks ago by Claude Paroz

AFAIK, the fact that get_indexes only return single column indexes was due to its "limited" utility in inspectdb which was just to determine the primary key or unique property of individual fields.
If we can make inspectdb use the get_constraints result to determine primary key and unique properties of individual fields (should be doable), we might as well deprecate and eventually remove get_indexes.

comment:22 Changed 6 weeks ago by Akshesh Doshi

Cc: aksheshdoshi@… added

comment:23 Changed 2 weeks ago by Claude Paroz

Resolution: fixed
Status: newclosed

get_indexes has been deprecated in #27098.

As get_constraints is introspecting multicolumn indexes, we can consider this ticket resolved.

See #27060 about adding multicolumn indexes support in inspectdb.

Note: See TracTickets for help on using tickets.
Back to Top