django icon indicating copy to clipboard operation
django copied to clipboard

Documented Meta.indexes as a better option than Field.db_index.

Open adamchainz opened this issue 4 years ago • 6 comments

Copy and adapt the Meta.index_together admonition here, to help guide users to controlling their indexes better. I am not sure if "may be deprecated" is valid, but it's been around on index_together for many versions now and probably helps guide people to the "better" way of doing things.

Inspired because @hannseman on the mailing list is looking at PostgreSQL extra indexes when an db_index / unique is set for CharFields: https://groups.google.com/forum/#!msg/django-developers/D7qhJzYgow8/DKQ0kEIVBAAJ

adamchainz avatar Apr 14 '20 12:04 adamchainz

@adamchainz Do we want to make the same considerations for spatial_index? It is True by default compared to db_index which is False by default, but probably the same thing applies that using Meta.indexes gives more flexibility and allows for actually customising the type of index used, as well as other parameters such as opclasses.

ngnpope avatar Apr 14 '20 14:04 ngnpope

Not really used GeoDjango so hadn't considered it. It looks like we don't currently support spatial indexes through an Index class, nor any options for them through spatial_index, so I don't think we can make that recommendation.

adamchainz avatar Apr 14 '20 15:04 adamchainz

Ah, I guess I had my PostgreSQL hat on where we can support other index types. Currently for spatial_index we could set it to False and then use GistIndex in Meta.indexes instead. Obviously this doesn't help for the other backends which have no equivalent implementation in Django yet.

ngnpope avatar Apr 14 '20 16:04 ngnpope

Not really used GeoDjango so hadn't considered it. It looks like we don't currently support spatial indexes through an Index class, nor any options for them through spatial_index, so I don't think we can make that recommendation.

That's not entirely true, see tests/gis_tests/geoapp/test_indexes.py. Nevertheless I agree that it's too early to recommend them because they're not supported by all back-ends (e.g. ticket-31252).

felixxm avatar Apr 15 '20 05:04 felixxm

Looks like this tweak deprecating the db_index field stalled out about a year ago over questions about spacial indexes in Oracle DBs.

The PR has a small change that says:

Use the Options.indexes option instead. The :attr:Meta.indexes <Options.indexes> option provides more functionality than db_index. db_index may be deprecated in the future.

Since there's some trickiness around the spacial indexes on Oracle, could we just soften this a bit and get this tweak in place? It's been years that db_index has been discouraged, but with no update to the docs.

Maybe this would work:

Where possible use the Options.indexes option instead. In nearly all cases, the :attr:Meta.indexes <Options.indexes> option provides more functionality than db_index. db_index may be deprecated in the future.

I know this would have helped me a few times over the years.

mlissner avatar May 24 '21 18:05 mlissner

Thanks for the suggestion @mlissner . I've just updated to use it.

I concur that this should not be stalled on rare use case of spatial indexes on Oracle. For nearly every use case, Meta.indexes is superior.

adamchainz avatar Nov 02 '21 17:11 adamchainz

I'm managing a developer that just used db_index=True, and I was hoping to have some documentation to point them to that says that db_index=True is discouraged. Alas, this PR is the best thing I've got.

I think this is ready for a merge and uncontroversial. Is there anybody out there that disagrees? And is there a kind soul that's willing and able to merge it?

mlissner avatar Jan 13 '23 23:01 mlissner

@adamchainz Thanks :+1:

felixxm avatar Feb 24 '23 08:02 felixxm

Test failure is not related with this patch.

felixxm avatar Feb 24 '23 08:02 felixxm