• Would it make sense to first delete all rows which have any values set in these columns? Might be a good final cleanup to make sure nothing collides post-removal, and would probably make the index rebuild faster if we are confident we are only left with the (relatively few) server side settings ... also once we remove these columns, it would be more challening to tell the previously-connected-to-a-user (if any left) rows from the server wide ones.

Yes. They would need to be deleted. And the new index built before the previous one gets deleted.

Updated migration to:

• Do a delete of all settings with type/id not null
• Move new index creation to before old index drop

There's a (legit, I think) issue here with an old migration picked up by the historical migrations test, as side effect of using the Setting class in the migration. I suspect it's a serialization thing, but need to look closer. Will update that to either remove the class or inline a definition, whatever is simpler.

Updated old migration to add YAML value wrapper to Setting class, which I think mirrors the behavior expected at that point in time. (Also, this is ~2YO migration, seems low risk to modify, but does keep migration test happy).

One possible issue is that the post-deployment migration is potentially long and complex, which means increased risk of it failing or being interrupted mid-way, and it disabling transactions means re-running it may be an issue.

Yeah ... strong migrations wanted the "concurrently" on the index addition, which also required the disable ddl line.

One workaround there might be to put JUST the addition of that index in it's own (earlier) migration with disable_ddl, and then everything else in a transaction-wrapped migration to follow up after? Would make the "what do I need to re-run" easier to find if anyone hits interruption issues.

Otherwise it looks good to me, but I'd probably wait after 4.3 at that point.

Yes.

One workaround there might be to put JUST the addition of that index in it's own (earlier) migration with disable_ddl, and then everything else in a transaction-wrapped migration to follow up after? Would make the "what do I need to re-run" easier to find if anyone hits interruption issues.

Wouldn't the deletion need to happen before creating the index, though? Otherwise you'd have conflicts for every user variable.

Wouldn't the deletion need to happen before creating the index, though? Otherwise you'd have conflicts for every user variable.

Ah, yes good catch ... you'd need a) transaction-wrapped delete migration, b) transaction-disabled concurrent index addition, c) transaction wrapped the rest. Three migrations seems overkill for the changes.

This pull request has merge conflicts that must be resolved before it can be merged.

This pull request has resolved merge conflicts and is ready for review.

On this one - I know we were in wait until post-4.3 mode, but I dont know if the issues re: migration setup were resolved or not, or if theres still more to do?

This is rather awkward, but…

I think we need to delete the thing_id: nil, thing_type: nil records in a pre-deployment migration, otherwise they would be incorrectly picked up by the new code, before the post-deployment migrations run.

However, this also means you can't safely upgrade anymore from a pre-4.2.0 version to a version with this zero-downtime migration. This is already a restriction we have since the 4.3.0 release because of the cookies change, and it's not the first time we are hitting such roadblocks, so maybe it's time to formalize such “breakpoints” and have e.g. some additional pre-migration check to ensure you've reached a specific breakpoint before running any more migrations with SKIP_POST_DEPLOYMENT_MIGRATIONS (e.g. #33089).

I think we need to delete the thing_id: nil, thing_type: nil records in a pre-deployment migration, otherwise they would be incorrectly picked up by the new code, before the post-deployment migrations run.

We have ignored_columns added here, which I think means from the perspective of loading records we are safe -- but then we also are removing the default scope here, which means that the moment the code is deployed, if the migration has not run yet, we are potentially loading what should have been a user setting as a site setting (if there were var name overlap)?

but then we also are removing the default scope here, which means that the moment the code is deployed, if the migration has not run yet, we are potentially loading what should have been a user setting as a site setting (if there were var name overlap)?

Yes, that's my concern. There are some variables with overlap between system variables and user variables: theme, trends, noindex, possibly more. If running the new code before deleting the records, random user variables may be picked up instead of system variables.

Hmm, yeah. Looking throught the diff here, I think...

• The addition of ignored_columns could be added any time since we're already not using these?
• Fabricator change is needed only after the schema change, but in theory could go in any time
• Old migration adding setting class is only needed after change, but also could go in any time

It's really only the itneraction between the scope and the migration that has timing/dependency issues.

We currently have a mix of setting records:

• In some, thing_id and thing_type are present and hold values for users -- however none of these are used anywhere. They are still there as side effect of not cleaning up previously
• In some, thing_id and thing_type are null, and do hold currently used and needed sitewide settings

If we pull out the scope before running the migration, we'll hit (old, unused) user settings for some where names collide.

If we run the migration first w/out changing scope, once the migration finishes the scope will be referring to now-nonexistent columns are start erroring.

What if we...

• Add a migration which does nothing but delete the data where thing_id and thing_type are not null, but does not yet modify the indexes or drop columns
• Get that change only (and maybe the other harmless stuff here) into a release
• In some future release, remove the scope and do the index/drop portions ... I think this will be ok because we will be confident that even though the default scope is gone, the user records will have all be removed

The addition of ignored_columns could be added any time since we're already not using these?

We're currently using them in the default scope.

Add a migration which does nothing but delete the data where thing_id and thing_type are not null, but does not yet modify the indexes or drop columns

As I said earlier, this should be pre-deployment migration (otherwise new code will pick up the old values).

But it requires 4.2.0 code to be running otherwise new records with non-nil columns may be created. Therefore I think this first step still needs a breakpoint, such as what I'm proposing in #33089

But such a breakpoint already exists because of the cookies migration, even if it's not enforced by a task check.

We're currently using them in the default scope.

Wont the scope keep including them in the where clause even if they are in ignored_columns?

As I said earlier, this should be pre-deployment migration (otherwise new code will pick up the old values).

Yes, if we pulled out the deletion portion only to stand alone migration that could be normal migration.

But it requires 4.2.0 code to be running otherwise new records with non-nil columns may be created. Therefore I think this first step still needs a breakpoint, such as what I'm proposing in #33089

I'll check that one out ... isn't there already a policy/guideline of "you must have migrated all the way to end of a last minor version before bumping to next one"? Maybe that's not code enforced - but that's the expectation, right?

But such a breakpoint already exists because of the cookies migration, even if it's not enforced by a task check.

👍

Wont the scope keep including them in the where clause even if they are in ignored_columns?

I haven't tried, but I don't think so.

I'll check that one out ... isn't there already a policy/guideline of "you must have migrated all the way to end of a last minor version before bumping to next one"? Maybe that's not code enforced - but that's the expectation, right?

No, that is not. On the contrary, we try to support skipping version updates, if only because sometimes intermediary versions become uninstallable for one reason or another. But I think it's reasonable to not support zero-downtime migrations when skipping versions.

I haven't tried, but I don't think so.

Running with the default scope, but without the ignored columns...

mastodon(dev)> Setting.count
  Setting Count (1.7ms)  SELECT COUNT(*) FROM "settings" WHERE "settings"."thing_type" IS NULL AND "settings"."thing_id" IS NULL
=> 5
mastodon(dev)> Setting.last
  Setting Load (0.3ms)  SELECT "settings".* FROM "settings" WHERE "settings"."thing_type" IS NULL AND "settings"."thing_id" IS NULL ORDER BY "settings"."id" DESC LIMIT $1  [["LIMIT", 1]]

When I add the ignored columns...

mastodon(dev)> Setting.count
  Setting Count (0.9ms)  SELECT COUNT(*) FROM "settings" WHERE "settings"."thing_type" IS NULL AND "settings"."thing_id" IS NULL
=> 5
mastodon(dev)> Setting.last
  Setting Load (0.3ms)  SELECT "settings"."id", "settings"."var", "settings"."value", "settings"."created_at", "settings"."updated_at" FROM "settings" WHERE "settings"."thing_type" IS NULL AND "settings"."thing_id" IS NULL ORDER BY "settings"."id" DESC LIMIT $1  [["LIMIT", 1]]

So - looks like ignored columns changes the selected columns from * to all-but-ignored, but does not interact with the conditions.

But I think it's reasonable to not support zero-downtime migrations when skipping versions.

Fun.