Page MenuHomePhabricator

Allow Owners packages to have arbitrarily long names
ClosedPublic

Authored by epriestley on Apr 27 2017, 10:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 3:28 PM
Unknown Object (File)
Fri, Dec 6, 5:21 PM
Unknown Object (File)
Tue, Dec 3, 10:13 PM
Unknown Object (File)
Tue, Nov 26, 3:25 PM
Unknown Object (File)
Tue, Nov 19, 12:16 PM
Unknown Object (File)
Nov 3 2024, 1:04 AM
Unknown Object (File)
Oct 31 2024, 12:11 PM
Unknown Object (File)
Oct 19 2024, 8:20 AM
Subscribers
None

Details

Summary
  • Change column type from sort128 to sort.
  • Remove originalName. This column is unused. Long ago, we used it to generate a Thread-Topic header for mail, but just use PHIDs now (the value just needs to be stable for a given object, users normally don't see it).
Test Plan
  • Created a package with a beautifully long name. Magnificent!
  • Grepped for originalName / getOriginalName(), found no Owners hits.
  • Verified that there isn't any name-length validation code to remove.

Screen Shot 2017-04-27 at 3.07.27 PM.png (1×1 px, 173 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

We don't need a migration to change the column type on the name column?

bin/storage adjust (which runs automatically after bin/storage upgrade by default, unless you disable it with --no-adjust) can automatically make a class of "safe" schema corrections, which are usually:

  • Adding or removing keys.
  • Changing table engines.
  • Changing collations and charsets.
  • Making columns bigger.
  • (Making columns nullable, I think?)

It will also attempt semi-safe changes, which are basically:

  • Making columns smaller.

I think the behavior there is that it says "couldn't do it since you'd lose some data, re-run with --unsafe if you're okay with that" if any columns won't fit. --unsafe will make the columns smaller and throw away the overflowing text.

Since the sort128 -> sort change is an unambiguously safe one (making a column bigger), adjust will just perform it automatically. I think I lost the local buffer, but it says something in the general vein of:

Some columns can use better types! Luuucky!

owners_package.name [ Upgrade ] Better Column Type Available

Upgrading your database for you now!
...
All done! Have a great day! You're a wonderful human being.

(This is substantially a text fix for an earlier version of the utility which "applied available adjustments"; some users found this ominous.)

For changes in this class, you can still add a migration if you want, but storage adjust will do it for you if you don't (and if you write the migration wrong, storage adjust will normally adjust the schema over it -- the PHP definition is authoritative, and it attempts to move the schema to that definition as long as it can do so by performing only safe operations, regardless of what state .sql migrations -- or, worse, users manually issuing ALTER TABLE statements -- have put the database into).

You can test if storage adjust can do a change automatically by updating the Whatever.php (without writing a migration yet) and then running storage adjust. If it can, it will say "wow amazing time to upgrade". If it can't, it will say "troubling database inconsistency, sorry buddy". Then you can write a migration.

Writing the migration is more work and normally only gives you an opportunity to get something wrong, so I generally won't write migrations for changes that storage adjust can apply. However, there are some rare exceptions: one is that adjust happens after all other migrations apply, so if you have some migrations like this:

  • Migration A, add a new column.
  • Migration Z, run a lot of queries which involve that column.

...and you also have a new key to add on the column, you might want to add that key explicitly in migration B, between A and Z, so that the key exists when migration Z runs. If you do, things will run in "A, B, Z" order (which might be faster), instead of "A, Z, storage adjust adds the key" order (which might be slower, since Z can't take advantage of the key). This is very rare.

This revision is now accepted and ready to land.Apr 27 2017, 11:10 PM

The history on this is that we used to use the default collation (usually latin1) but that ran into issues with UTF8. We manually converted everything to utf8 in a huge patch in D1830.

But, surprise surprise, MySQL's utf8 type is only UTF8 characters with 3 or fewer bytes, so a lot of characters can't be represented (see T1191). And not every version of MySQL had utf8mb4 at the time. storage adjust was the "v2" attempt at this in a way that would let us easily change to utf8mb5 or utf8-plus-plus again in the future without needing to continue churning out huge "mutate every single column every single table" patches, and not require all installs to upgrade to a recent version of MySQL. It also made all of the changes observable and debuggable by introducing introspection in ConfigDatabase Status and ConfigDatabase Issues.

It was a fairly complicated thing to build, but it appears to work pretty well, and has let us solve some other problems in this vein fairly gracefully.

This revision was automatically updated to reflect the committed changes.