search

nextcloud / polls

🗳️ Polls app for Nextcloud
https://apps.nextcloud.com/apps/polls
GNU Affero General Public License v3.0
257 stars 73 forks source link

Avoid expensive index recreation #3694

Closed dartcafe closed 2 months ago

dartcafe commented 2 months ago

This is a hotfix to address a particular problem with heavy and expensive index recreation on large databases.

relates #3679 but not final fix backlink to #3689 for transparancy

Problem description: At least one known instance with a large amount of polls and votes, etc. suffers from the recreation of indices, because it slows down the database after every update. Oviously the database discards existing execution plans and in the result, the database slows down until the execution plans are optimized again.

This is the problem how I understood it. Correction are welcome.

A quick analysis of the current code reveals, that it is safe to keep the index creation, since existing indices seem to stay untouched. This affects unique and other indices (check for existing indices already exists) and foreign key constrains as well. So it seems, that the removal of the pre-migration repair step alone will prevent an index recreation.

Currently this is safe for most of the installations, since the last table change, where a prior index removal was neccessary, is long time ago. I will check this later in detail, how long ago.

If neccesary I plan to add outdated migrations to the latest existing migration, to ensure that this is only applies for updates from Versions before Polls 6.1.

dartcafe commented 2 months ago

Shall we also delete lib/Migration/RepairSteps/RemoveIndices.php?

Could be, but I want to leave it for the final fixes. This is just an emergency fix.

dartcafe commented 2 months ago

/backport to next

hamza221 commented 2 months ago

Let's wait for @Altahrim and @ArtificialOwl to have a look before merging 🙏

ArtificialOwl commented 2 months ago

i am down for any cleaning of pre/post migration steps !

dartcafe commented 2 months ago

i am down for any cleaning of pre/post migration steps !

I will tidy these things up later. But I found another problems, the foreign key constraints don't get created, if removed once. Have to get into that.

AndyScherzinger commented 2 months ago

I suppose the same conceptual logic applies for the FKs? Don't recreate if already there (smart manager bla, sicne I personally lack Nc apps dev knowhow beyond the mobile apps...)

dartcafe commented 2 months ago

I suppose the same conceptual logic applies for the FKs? Don't recreate if already there (smart manager bla, sicne I personally lack Nc apps dev knowhow beyond the mobile apps...)

The problem is, I have no idea how to check, if the FK already exists or not.

I plan to get away from it, but the logic of removing dependent rows from other tables has to be moved into the app in this case. At least into the janitor cron.

hamza221 commented 2 months ago

I'm sorry I'm just trying to further understand, what the problem is so maybe I can try help:

dartcafe commented 2 months ago
  • The foreign keys and indices used to both be dropped the RemoveIndicesthe step which is not used anymore so why is it needed to recreate them.

They don't need to be recreated. Just in case they are not there.

? If so can't listTableForeignKeys() be used ?

🤣 This is what I missed. Thanks.

But the situation is, if the FK is requested to be created and is already present the commend gets ignored and the FK index stays untouched.

But I tricked myself or I am too stupid to recognize, that the FK indices get created. False alarm.

dartcafe commented 2 months ago

Summary:

But I found another problems, the foreign key constraints don't get created, if removed once. Have to get into that.

This was the false alarm

@AndyScherzinger I believe that should work for your use case.

I would merge with your OK and build a release 7.2.2. So I can try to bring it up to the appstore tomorrow.

dartcafe commented 2 months ago

I now inspected the changes until 16 months ago. Since then no change to the database was made, which could be a problem to the indices (and makes it neccessary to rebuild them).

If some one with an old version (prior v5) runs into an update issue, I will take care of it.

dartcafe commented 2 months ago

and a final question, Do you know why they can't be created? (a log or an error message)

@hamza221 I just confused myself, beacuase I was checking the migrations with a remote server and check the database with phpMyAdmin. And I expected the fk_* indices to appear together with the other indices.

Until I realized, the poll_id was linked to the polls table and remembered that the KF indices are visible inside the relation view.

Layer 8 problem. 😁

I think it is time to install a nextcloud test and dev instance locally.

AndyScherzinger commented 2 months ago

@dartcafe sounds good to me (7.2.2 release and publishing) folks are already out but I see to have them test an update 7.1.4 to 7.2.2 on Monday then as a final step before suggesting the upgrade.

@hamza221 could you do a final test with sql logging on db to check no index nor fk gets (re) created? Thanks in advance.

AndyScherzinger commented 2 months ago

@dartcafe just to be safe here since I can't tell if you left out the last step (publishing to the store) on purpose. So for automated publishing you'll need to push the tag 7.2.2 of this repository to https://github.com/nextcloud-releases/polls where you'd also create a release based on it.

The automation should then take care of the app store publishing based on that tag/release.

I just wanted to check since I am not sure if you wanted to wait until folk had another testing round on Monday.

dartcafe commented 2 months ago

@AndyScherzinger I an just not sure how to do that. Is it really meant by doing the steps manually?

dartcafe commented 2 months ago

Nope. This did not work.

What exatctly is meant by "push the tag 7.2.2 of this repository to https://github.com/nextcloud-releases/polls"? Nerver pushed a tag from one repo to another.

git push https://github.com/nextcloud-releases/polls v7.2.2?

dartcafe commented 2 months ago

Yeah ok. it was git push https://github.com/nextcloud-releases/polls v7.2.2

AndyScherzinger commented 2 months ago

Would have had to. Do it myself to tell you the git command since I use GUI git clients only 🙂

AndyScherzinger commented 2 months ago

Also just checked the app store and 7.2.2 as well as 8.0.0-alpha4 are showing 👍