Skip to content

New indexes for states and recording_runs tables #6688

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.

Sign up for GitHub

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

Merged
balloob merged 10 commits into from
Mar 24, 2017
Merged

New indexes for states and recording_runs tables #6688

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
894699b
New indexes for states table
m00dawg Mar 19, 2017
f9a0f23
Added recorder_runs indexes
m00dawg Mar 19, 2017
77c007d
Created a new function for compound indexes.
m00dawg Mar 19, 2017
7d7c4e4
Merge branch 'dev' of https://github.com/home-assistant/home-assistan…
m00dawg Mar 19, 2017
38aa348
Merge branch 'dev' of https://github.com/home-assistant/home-assistan…
m00dawg Mar 21, 2017
d397ccb
Adjusted indexes, POC for ref indexes by name.
m00dawg Mar 21, 2017
c8e37e9
Corrected lint errors
m00dawg Mar 21, 2017
b3b1360
Fixed pydocstyle error
m00dawg Mar 21, 2017
37e3e5d
Moved create_index function outside apply_update
m00dawg Mar 22, 2017
7cd55c9
Moved to single line (just barely)
m00dawg Mar 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions homeassistant/components/recorder/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ def run(self):
migration.migrate_schema(self)
self._setup_run()
connected = True
_LOGGER.debug("Connected to recorder database")
except Exception as err: # pylint: disable=broad-except
_LOGGER.error("Error during connection setup: %s (retrying "
"in %s seconds)", err, CONNECT_RETRY_WAIT)
Expand Down
43 changes: 31 additions & 12 deletions homeassistant/components/recorder/migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,39 @@ def _apply_update(engine, new_version):
from sqlalchemy import Table
from . import models

if new_version == 1:
def create_index(table_name, column_name):
"""Create an index for the specified table and column."""
table = Table(table_name, models.Base.metadata)
name = "_".join(("ix", table_name, column_name))
# Look up the index object that was created from the models
index = next(idx for idx in table.indexes if idx.name == name)
_LOGGER.debug("Creating index for table %s column %s",
table_name, column_name)
index.create(engine)
_LOGGER.debug("Index creation done for table %s column %s",
table_name, column_name)
def create_index(table_name, column_name):

@balloob balloob Mar 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you move these methods out of the _apply_update method?

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd defer to @armills on that one since my PR uses mostly what was already there. I would imagine these functions would only be useful during a migration unless things were getting very complex (say by creating a temporary table and creating an index on that for later use) since, otherwise, the real places index definitions live is in the models?

The HA approach to migrations is pretty new to me so I'm still wrapping my head around it, to be fair.

"""Create an index for the specified table and column."""
table = Table(table_name, models.Base.metadata)
name = "_".join(("ix", table_name, column_name))

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about something like this to combine these into one function? The pattern is usually called *args if you need to google it. You can replace the debug messages with the one you wrote for compound index so the individual column names don't need to be used.

def create_index(table_name, *column_names):
   name = "_".join("_".join("ix", table_name), column_names)
    ...

Then it could be used for both index types like this

create_index("recorder_runs", "start", "end")
create_index("states", "last_updated")

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The index = next(...) bits are a bit new to me. Does that effectively mean that any arguments passed from the function end up getting passed to an Index constructor? Asking because if we can only specify columns, we may still have an issue if we wanted to, for instance, create a unique index where we would have to pass "Unique=true". If it's just blindly passing all the arguments though, that shouldn't be a problem? If it's only column names, that would be a problem (granted that I haven't solved in my code updates to this point yet either way).

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So what happens when we load an old database with new models objects, is that the actual Index sqlalchemy metadata objects get initialized with all the parameters we used in models.py, but the index isn't created in the underlying database engine. These indexes are present in the table.indexes list.

next is a python built-in that returns the next item from an iterator. Since we create a new iterator by searching through table.indexes for idx.name == name, there's only going to be one index in that iterator. What that means is that we're getting the metadata object for the index that was created by models.py. Once we have the reference all we have to do is call index.create.

# Look up the index object that was created from the models
index = next(idx for idx in table.indexes if idx.name == name)
_LOGGER.debug("Creating index for table %s column %s",
table_name, column_name)
index.create(engine)
_LOGGER.debug("Index creation done for table %s column %s",
table_name, column_name)

def create_compound_index(table_name, column_names):
"""Create an index for the specified table and columns."""
table = Table(table_name, models.Base.metadata)
index_name = "ix_" + table_name + "_" + "_".join(column_names)

@balloob balloob Mar 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prefer string formatting over concatenation.

index_name = "ix_{}_{}".format(table_name, "_".join(…))

Although, actually I prefer @armills approach better. Extract a function:

def _generate_name(*args):
    """Generate a name based on arguments."""
    return "_".join(args)

@m00dawg m00dawg Mar 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Per my comment about migration.py I think in order to do that we'd have to split out column names from extra arguments (e.g. unique=True). I generally prefer using something like field1_field2_idx for non-unique, field1_field2_uq for unique and field1_fk for indexes required for foreign keys (I think that may be MySQL dependent though).

Otherwise, the column names could get kinda wonky. The other weird bit I ran into I'm not sure how to solve is the column name had to match what was in models.py. if it didn't, HA would get confused and fail (I'm not sure exactly why though). So I wonder if there was a way to either grab the index name from the models.py or have a function that generated the name in both places?

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I described the procedure in the comment above, but basically the only thing this function is doing is looking up the index object by name. It isn't creating it or specifying any arguments such as unique that will be used by the index.

The reason we are generating the name here is so that it will match the name generated by sqlalchemy for index=True columns.

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

💡

Aha ok that helps explain things quite a bit! If it's ok, I'll probably wrap the above explanation around some in-line comments since that might be helpful to future folks trying to figure out how that works.

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think expanding the documentation never hurts. Obviously we should leave out next since that's a built-in that people can search, but describing how the objects are pre-created would be helpful.

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm in terms of the index naming, would it make more sense to use literals here? If we have to manually create the index name on models.py, it seems like it might be easier to do that here? So something like create_index('recorder_runs', 'ix_recorder_runs_start_end') given what is in models.py is __table_args__ = (Index('ix_recorder_runs_start_end', 'start', 'end'),)

I'm normally not a fan of literals like that, but unless there is a way to auto-generate an index name in models.py, it might keep things a bit less error-prone for folks that don't know about the auto-naming bits going on in migration.py?

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm OK with that myself. We should have one function that takes the table and index name as it's inputs. We can then have a separate function like balloob described that can generate index names for single column indexes, which gets passed into the first function.

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, we could. Actually I wonder if we can reuse whatever function SQLAlchemy uses for that (I pulled up the docs to figure out how it names its indexes when using Index=True but it just provided the naming convention).

For more clarity, here's what I've done based on the above:

    def create_index(table_name, index_name):
        """Create an index for the specified table.

        The index name should match the name given for the index
        within the table definition described in the models"""
        table = Table(table_name, models.Base.metadata)
        _LOGGER.debug("Looking up index for table %s", table_name)
        # Look up the index object by name that was created in the models
        index = next(idx for idx in table.indexes if idx.name == index_name)
        _LOGGER.debug("Creating %s index", index_name)
        index.create(engine)
        _LOGGER.debug("Finished creating %s", index_name)

    if new_version == 1:
        #create_index("events", "time_fired")
        create_index("events", "ix_events_time_fired")
    elif new_version == 2:
        # Create compound start/end index for recorder_runs
        create_index("recorder_runs", "ix_recorder_runs_start_end")
        # Create indexes for states
        create_index("states", "ix_states_last_updated")
        create_index("states", "ix_states_created")

# Look up the index object that was created from the models
_LOGGER.debug("Looking up index for table %s", table_name)
index = next(idx for idx in table.indexes if idx.name == index_name)
_LOGGER.debug("Creating %s index for table %s",
index_name, table_name)
index.create(engine)
_LOGGER.debug("Done creating %s index for table %s",
index_name, table_name)

if new_version == 1:
create_index("events", "time_fired")
elif new_version == 2:
# Create compound start/end index for recorder_runs
create_compound_index("recorder_runs", ("start", "end"))
# Create indexes for states
create_index("states", "last_updated")
create_index("states", "created")

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I managed to get a compound index in here for recorder_runs. It's not a huge table (at least in my own DB) relative to states, but was causing a non-indexed table-scan. I really wanted to convert the create_index function to handle single or multiple indexes, but given how the Index constructor works, I was having a heck of a time. In either case, not sure how pruned indexes (e.g. ALTER TABLE ... ADD INDEX ix_string (string(8))) works but I believe it's possible.

I mention that because some of the types within tables ideally should be an ENUM for performance, but from the standpoint of flexibility are varchars. A reasonable compromise for that is to use a small pruned index which saves space but still gives decent cardinality. That's outside the scope of this PR, but sort of explaining my thought process here.

@emlove emlove Mar 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only thing create_index does that doesn't apply to a multi-column index is it generates the name of the index based on the table and column name. If you separate the logic out below the name = line, you should be able to use it for a multi-column index.

@m00dawg m00dawg Mar 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Took me a while to figure it out, but you're right that does work. I'll be submitting an update to the PR shortly.

else:
raise ValueError("No schema migration defined for version {}"
.format(new_version))
Expand Down
12 changes: 8 additions & 4 deletions homeassistant/components/recorder/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# pylint: disable=invalid-name
Base = declarative_base()

SCHEMA_VERSION = 1
SCHEMA_VERSION = 2

_LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -66,13 +66,15 @@ class States(Base): # type: ignore
attributes = Column(Text)
event_id = Column(Integer, ForeignKey('events.event_id'))
last_changed = Column(DateTime(timezone=True), default=datetime.utcnow)
last_updated = Column(DateTime(timezone=True), default=datetime.utcnow)
created = Column(DateTime(timezone=True), default=datetime.utcnow)
last_updated = Column(DateTime(timezone=True), default=datetime.utcnow,
index=True)
created = Column(DateTime(timezone=True), default=datetime.utcnow,
index=True)

__table_args__ = (Index('states__state_changes',
'last_changed', 'last_updated', 'entity_id'),
Index('states__significant_changes',
'domain', 'last_updated', 'entity_id'), )
'domain', 'last_updated', 'entity_id'))

@staticmethod
def from_event(event):
Expand Down Expand Up @@ -124,6 +126,8 @@ class RecorderRuns(Base): # type: ignore
closed_incorrect = Column(Boolean, default=False)
created = Column(DateTime(timezone=True), default=datetime.utcnow)

__table_args__ = (Index('ix_recorder_runs_start_end', 'start', 'end'),)

def entity_ids(self, point_in_time=None):
"""Return the entity ids that existed in this run.

Expand Down