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

Improve the filtering query, and indexes needed #1504

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

joshk
Copy link
Collaborator

@joshk joshk commented Sep 10, 2024

A few things came up while looking into #1501

  • The devices query included lots of nested joins, and none of this data was being used in the device list or filters
  • Most of the filters didn't have corresponding indexes to go with them, including tags
  • The new double wildcard identifier change means the current index can't be used

This PR simplifies the filter query, adds indexes, including some pg_trgm goodness.

The pg_trgm bits have been inspired by:

@lawik
Copy link
Contributor

lawik commented Sep 10, 2024

Looks very nice!
My only question is whether we have enough test coverage on devices LiveView to know if we break any of the filters :)

@joshk
Copy link
Collaborator Author

joshk commented Sep 10, 2024

Thats a fair question. I've tested it locally, and the filters work as expected, but the test suite should be expanded.

@joshk joshk merged commit 5108e29 into main Sep 10, 2024
2 checks passed
@joshk joshk deleted the better-indexes-and-a-better-filter-query branch September 10, 2024 08:10
@oestrich
Copy link
Contributor

Leaving a note post close because I'm a little concerned with adding in 6 indexes on a table that gets hammered with heartbeat updates. Something to look out for once this gets deployed at smartrent @jjcarstens

@lawik
Copy link
Contributor

lawik commented Sep 10, 2024

Yeah, that's a valid concern. I would imagine it won't have to touch indexes unless the indexed value actually changes?

@@ -163,7 +176,7 @@ defmodule NervesHub.Devices do
where(
query,
[d],
fragment("array_to_string(?, ',') ILIKE ?", d.tags, ^"%#{tag}%")
fragment("array_to_string(?, ' ', ' ') ILIKE ?", d.tags, ^"%#{tag}%")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does adding a trailing space do here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this isn't using the string_array_to_string function that we're creating in the migration it's probably not going to use that index.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get it. A third value added 🤦🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this @esvinson


def change do
create index("devices", [:product_id], concurrently: true)
create index("devices", [:updates_enabled], concurrently: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indexes on boolean fields probably won't be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general thought was "index more stuff, see what is used, remove indexes as we see fit"

This was some general advice from some Postgres contributors at Heroku that I was friendly with.

@disable_ddl_transaction true

def change do
create index("devices", [:product_id], concurrently: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional note, if we create the indexes using create_if_not_exists we can create some of the indexes in advance to help with performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea!

@jjcarstens jjcarstens mentioned this pull request Sep 10, 2024
@joshk
Copy link
Collaborator Author

joshk commented Sep 10, 2024

Leaving a note post close because I'm a little concerned with adding in 6 indexes on a table that gets hammered with heartbeat updates. Something to look out for once this gets deployed at smartrent @jjcarstens

As indexes are separate, this should be fine. It would only be a concern if the indexed data is updated often.

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

Successfully merging this pull request may close these issues.

6 participants