postgres - add indexes to foreign keys

[lynzt]

Description

We don't have indexes on foreign keys in our postgres db. This adds indexes to our foreign keys.

Reviewer Notes

I think I caught them all, but worth a double check?

Setup

make db_dev_e2e_populate
make db_test_e2e_populate

Ensure no errors and check for indexes on table columns.

Code Review Verification Steps

• Any new migrations/schema changes:
  • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
  • Have been communicated to #dp3-engineering
  • Secure migrations have been tested using bin/run-prod-migrations
• Request review from a member of a different team.
• Have the Pivotal acceptance criteria been met for this change?

References

• Pivotal story for this change
• this article explains more about the approach used.

[chrisgilmerproj]

So this would be separate from the blackout_dates_index? The existing one covers a combination of:

• transportation_service_provider_id
• start_blackout_date
• end_blackout_date
• market
• source_gbloc

Are you just trying to target foreign keys on their own or do you want to capture the most common query pattern?

[lynzt]

for now, I was adding according to foreign keys, but I agree, I'd like to add them by query pattern eventually. This was a first pass at trying to make things faster. At some point, seeing what keys are actually hit and how they're used would be great... (I'm sure some queries would do well w/ composite indexes)

[lynzt]

Also, I see that I could drop the transportation_service_provider_id since, as you mention there's already a composite index w/ the first column being transportation_service_provider_id.

An index on transportation_service_provider_id only isn't needed as it's redundant as it can use the existing index. (yes?)

[chrisgilmerproj]

My experience is no, composite indexes will only be used if you're also querying for the other things as well. But I'd be happy to be proven wrong there. I'm also not against being completionist here just so we are explicit that we did the work and others don't come along with questions later. You can do some research but it's probably fine to have both.

[codecov]

Codecov Report

Merging #1908 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1908   +/-   ##
=======================================
  Coverage   50.56%   50.56%           
=======================================
  Files         442      442           
  Lines       19283    19283           
  Branches     1673     1673           
=======================================
  Hits         9751     9751           
  Misses       8673     8673           
  Partials      859      859

[chrisgilmerproj]

This is a great start! If you're going for just adding default indexes then I'd stick to adding *_id (foreign key) and *email* columns as indexes. I'm sure that will immediately improve our query performance.

I'd also like to have, either in this PR or a follow up, a look at common columns queried together to make more specific indexes. I've pointed out a few that exist in my comments. It would be nice to target things that are commonly queried. Also adding unique indexes where appropriate would be a nice thing to have.

Two things I'd like to see as a follow up after you make changes. Let's change the instructions for testing to also include running prod migrations with bin/run-prod-migrations to make sure it works with our other migrations in the cloud. Secondly, before we merge we should test this out on the Experimental environment. I'm curious how the DB will perform when we add all these indexes at once. I'm betting we'll be ok but we might run into some table locking during this that could cause us headaches on the app. Would be good to have an infra engineer join you for that deploy so we can watch it together.

[lynzt]

Added the additional indexes you requested...
Added indexes on any email fields I saw...
I did not add single field indexes on tables that had a composite index as we can use the existing index.

[chrisgilmerproj]

When you're ready to try this on experimental let me know and I'm happy to walk you through the process.

[chrisgilmerproj]

🎸 🌟 !