Ensure MariaDB/MySQL can be purged and handle states being deleted out from under the recorder

[moinmoin-sh]

Proposed change

Relationships within table "states" and between tables states and events prevent the purge from working correctly on some databases.

This proposal sets related indices to NULL or CASCADE to permit deleting of old rows.

Further explanations can be found here #42402

This proposal also allows to purge the tables "events" and "states" in any order.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes MariaDB doesn't purge #42402
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[homeassistant]

Hi @moinmoin-sh,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[Spartan-II-117]

does this require re-creating the DB?

[moinmoin-sh]

@Spartan-II-117 Good point. Yes it does.

[bdraco]

We will also need to handle the old_state_id being deleted out from under the recorder

bdraco@0f4c934

[bdraco]

does this require re-creating the DB?

We will need to bump the database version in homeassistant.components.recorder.models and update the ForeignKeys in homeassistant.components.recorder.migration

See _inspect_schema_version for how to use the inspector.

for column in inspect(States).columns:
    print column.foreign_keys

If we find one of the ones we want to alter we probably need to drop and re-add them.

On a side note, we probably need to move to alembic for database migrations.

[bdraco]

Here is a migration script based on what alembic thinks should happen to get to version 10.

It needs a bit of adjusting as it couldn't work it out automaticlly.

bdraco@7cdd712

[moinmoin-sh]

does this require re-creating the DB?

We will need to bump the database version in homeassistant.components.recorder.models and update the ForeignKeys in homeassistant.components.recorder.migration

See _inspect_schema_version for how to use the inspector.

for column in inspect(States).columns:
    print column.foreign_keys

If we find one of the ones we want to alter we probably need to drop and re-add them.

On a side note, we probably need to move to alembic for database migrations.

I'm sure the "state" table has to be altered to reflect the newly defined CONSTRAINTS (ondelete ...) as described by other users here #42402. Unfotunately I've no clue how to integrate this in migrations.py. Can you take care for that ?

[bdraco]

It works out to manually write out:

    op.drop_constraint(None, 'states', type_='foreignkey')
    op.create_foreign_key(None, 'states', 'events', ['event_id'], ['event_id'], ondelete='CASCADE')
    op.create_foreign_key(None, 'states', 'states', ['old_state_id'], ['state_id'], ondelete='SET NULL')

from sqlalchemy.engine import reflection
from sqlalchemy import MetaData, Table, ForeignKeyConstraint
from sqlalchemy.schema import DropConstraint
from sqlalchemy.schema import AddConstraint

[bdraco]

I'm sure the "state" table has to be altered to reflect the newly defined CONSTRAINTS (ondelete ...) as described by other users here #42402. Unfotunately I've no clue how to integrate this in migrations.py. Can you take care for that ?

Done. Please give it a try.

[moinmoin-sh]

Thanks , will do tomorrow. Too late for today (11:30 pm)

[bdraco]

I'm 11 hours behind you so I might see your response before I go to bed.

I was hoping to write a test for this but we don't have a way to mock MySQL or MariaDB in the test suite so it's probably ok as is.

[moinmoin-sh]

OK, I did some testing with the modified code:

short summary : looks great to me !

And here a little bit more detail:

• updated installed 0.118.4 with the modified code and restarted Homeassistant
• on restart the existing DB was migrated from schema 9 to schema 10 without showing any errors
• table creation shows the expected result

`CREATE TABLE `states` (
  `state_id` int(11) NOT NULL AUTO_INCREMENT,
  `domain` varchar(64) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `entity_id` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `state` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `attributes` text COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `event_id` int(11) DEFAULT NULL,
  `last_changed` datetime DEFAULT NULL,
  `last_updated` datetime DEFAULT NULL,
  `created` datetime DEFAULT NULL,
  `old_state_id` int(11) DEFAULT NULL,
  PRIMARY KEY (`state_id`),
  KEY `ix_states_event_id` (`event_id`),
  KEY `ix_states_last_updated` (`last_updated`),
  KEY `ix_states_entity_id_last_updated` (`entity_id`,`last_updated`),
  KEY `old_state_id` (`old_state_id`),
  CONSTRAINT `states_ibfk_3` FOREIGN KEY (`event_id`) REFERENCES `events` (`event_id`) ON DELETE CASCADE,
  CONSTRAINT `states_ibfk_4` FOREIGN KEY (`old_state_id`) REFERENCES `states` (`state_id`) ON DELETE SET NULL
) ENGINE=InnoDB AUTO_INCREMENT=120 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci`

• db content still looks valid (History and Logbook)
• rows in tables "events" and "states" can be deleted withot error
  I used manual delete commands because too few days to use service recorder.purge

DELETE  FROM homeassistant.states
	where states.last_updated <  '2020-11-27 17:14:00' ;

DELETE FROM homeassistant.events 
	WHERE events.time_fired <  '2020-11-27 17:14:00';

When using the above commands I observed that when running the DELETE from the "events" table first, also the corresponding rows from the "states" table were deleted - thanks to "ON DELETE CASCADE" I assume.
With that being said,
Would it make sense to swap the order of the two delete command within purge.py? or do we only the delete for "events" ?
I would assume that the "combined" delete when deleting "events" is handled by the DB itself and would be faster than doing seperate deletes for "states" first and then "events".

[bdraco]

Thanks for testing. I want to test downgrades before I merge this.

Sadly all of the databases we support implement foreign keys slightly differently which is how this problem crept in. Since sqlite3 won't handle the cascade the same way we should keep deleting the states first.

I also want to test with a fresh SQLite setup just to be sure (almost definitely fine though since we cover this with tests)

[moinmoin-sh]

OK, understood.

Please let me know if I need to do something to get this merged.

Thanks again and enjoy the Black Friday

[bdraco]

• Upgrade on MariaDB
• Downgrade on MariaDB
• Upgrade on sqlite
• Downgrade on sqlite
• Re-upgrade with manual delete for v10 from db on MariaDB

MariaDB [homeassistant]> delete from schema_changes where change_id=2;
Query OK, 1 row affected (0.019 sec)

[bdraco]

I did some more testing this morning and everything looks good 👍