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

Fixed #27060: Extended inspectdb so indexes are added to Meta.indexes. #12712

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

drewbrew
Copy link

When inspecting tables, the process creates a constraints dict
that identifies indexes. If we have any such indexes, let's add
them to Meta.indexes using the base Index class.

At this point, it does not support identifying the index type and
substituting in the proper index class, and I don't believe we have
the information at this time to select the right class anyway.

Ref: https://code.djangoproject.com/ticket/27060

@drewbrew
Copy link
Author

Fix for docs test is at #12698.

@felixxm
Copy link
Member

felixxm commented Apr 15, 2020

@drewbrew Thanks for this patch 👍 Please take a look at the previous attempt #7083 and all comments in the ticket. This patch should take into account indexes' types and various parameters (e.g. fillfactor).

@drewbrew
Copy link
Author

Thanks for that pointer, @felixxm.

So far, I've:

  • added in the type check (at least to the level of marking a comment if it's not the default type)
  • skipped single-column rtree indexes (and making a note of it in case the assumption that it's a default spatial index is wrong)
  • added support for index ordering (if available)

(Tests are still wonky, I know)

I'm a little confused by the postgres-specific options. My initial thought is that I'm going to have to do some sort of mapping from Index subclassses' type to that class (in the postgres introspection module) and convert the the options as returned from the db into kwargs accordingly. Does that sound like a reasonable and maintainable approach?

Base automatically changed from master to main March 9, 2021 06:21
@nessita
Copy link
Contributor

nessita commented Apr 28, 2023

Hello @drewbrew, I'm doing some PR cleanup. Do you have time to keep working on this? If yes, would you consider rebasing onto main, resolve conflicts, and flag this PR for review? Thank you!

When inspecting tables, the process creates a constraints dict
that identifies indexes. If we have any such indexes, let's add
them to `Meta.indexes` using the base `Index` class.

At this point, it does not support identifying the index type and
substituting in the proper index class, and I don't believe we have
the information at this time to select the right class anyway.

Ref: https://code.djangoproject.com/ticket/27060
Also add a comment if the type isn't btree
Let's assume they're default spatial indexes
@drewbrew drewbrew force-pushed the 27060-add-indexes-to-inspectdb branch from dc3bda0 to 589b857 Compare April 28, 2023 17:03
@drewbrew
Copy link
Author

Thanks for the prod, @nessita (and congrats on being the new fellow!). I've rebased it (correctly I hope... a lot changes in 3 years!), but how do I flag for review? Just convert it to a draft and back?

@nessita
Copy link
Contributor

nessita commented Apr 28, 2023

Thank you @drewbrew. Could you please revert the changes you made to AUTHORS (other than adding your name to it)?

@nessita nessita changed the title Fix #27060: Add indexes identified in constraints to Meta.indexes Fixed #27060: Extended inspectdb so indexes are added to Meta.indexes. Apr 28, 2023
@nessita
Copy link
Contributor

nessita commented Apr 28, 2023

Also, @drewbrew, would you care commenting in the ticket, specifically replying to the last comment made by @David-Wobrock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants