fix: skip repair RemoveBrokenProperties if already run

[kyteinsky]

• Resolves: #

Summary

The repair step removes null characters from the properties table. The PR which added this also made sure no further entries are inserted with null characters so it should be safe to run once and then skip it for the later upgrades.
The need for this arises from the fact that valuetype column is compared which does not have an index and should not have it since it's not used anywhere else, but it results in a long scan of the DB whenever the repair step runs.

The step is marked expensive but expensive repair steps still run in maintenance mode and cause a long downtime.

It was added in #49528

TODO

• ...

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI

[ChristophWurst]

I'm wondering if this one-time migration should have just been a background job. Downside of moving it there is that it runs outside the maintenance window. Not sure if that is problematic.

[SebastianKrupinski]

I'm wondering if this one-time migration should have just been a background job. Downside of moving it there is that it runs outside the maintenance window. Not sure if that is problematic.

TBH, I think that might be a better solution, If anyone ever needs to rerun this, it would be easier to remember just to trigger the migration again, which creates a new job.

[kyteinsky]

Downside of moving it there is that it runs outside the maintenance window. Not sure if that is problematic.

maybe the initial query that reads the complete oc_properties table could be a little heavy when the instance is live but looks good otherwise to me.

for the re-run possibility, it can be run again with occ maintenance:repair --include-expensive after setting the config key to false.

[SebastianKrupinski]

for the re-run possibility, it can be run again with occ maintenance:repair --include-expensive after setting the config key to false.

Yes. But and there is nothing wrong with that but without looking at the code, there is no way for a user or one of our engineers, to know that this repair works differently. It a hidden gotcha. This would be confusing an frustrating to find a hidden off switch.

With @ChristophWurst recommendation, there is no hidden gotcha, and we still get the same logic of a one time run.

[kyteinsky]

My understanding was that it wouldn't be needed in normal circumstances since the source of the issue has been fixed.
What would be the cases when it would be needed to be run again?
We can think about documenting it in the admin manual perhaps.

Also, I'm not sure of the structure of the background job, if it's a queued job (not a timed job), it would just run once and then be removed from the job list. It would then be not possible to run it?
occ background-job:execute [--force-execute] [--] <job-id> expects a job ID but the job isn't in the job list.

I'm sorry to be a little conservative on the change since then we would have to test and make sure it make the instance act funny. With the current change, it would be just two occ commands to re-run it (one to change the config and the other to run the repairs), everything else remains the same.