Avoid expensive index recreation

[dartcafe]

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.

[artonge]

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

[dartcafe]

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]

/backport to next

[hamza221]

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

[ArtificialOwl]

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

[dartcafe]

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]

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]

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]

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

• 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.

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

• You mean before creating them here

  polls/lib/Db/IndexManager.php

  Lines 80 to 88 in 78b97c2

  	public function createForeignKeyConstraint(string $parentTableName, string $childTableName): string {
  	$parentTableName = $this->dbPrefix . $parentTableName;
  	$childTableName = $this->dbPrefix . $childTableName;
  	$parentTable = $this->schema->getTable($parentTableName);
  	$childTable = $this->schema->getTable($childTableName);
  	$childTable->addForeignKeyConstraint($parentTable, ['poll_id'], ['id'], ['onDelete' => 'CASCADE']);
  	return 'Added ' . $parentTableName . '[\'poll_id\'] <- ' . $childTableName . '[\'id\']';
  	}

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

[dartcafe]

• 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]

Summary:

• removing indices has been removed from the steps
• index creation is still present as before, but should not affect existing indices

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]

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]

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]

@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]

Looks good to me 👍

Thanks a lot again for taking care of this matter 🎉

[AndyScherzinger]

@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]

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

• Create Tag v7.2.2
• add release manually to the releases by using the dialog?

[dartcafe]

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]

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

[AndyScherzinger]

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

[AndyScherzinger]

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