Summary:
Ref T7149. When users give us dumpfiles for import, they will almost inevitably use the `phabricator` namespace. They need to be renamed to use an instance namespace.
We can do this either by:
- importing the data first, then renaming; or
- renaming first, then importing.
This implements the second one, basically `storage renamespace --in dump.sql --from phabricator --to instancename > instance.sql`.
Renaming first is a little hackier since we have to `preg_match()` a SQL dump file, but I think it's better overall:
- With only one database, it lets you dump/import without downtime.
- If you have development stuff in a development environment in the `phabricator` namespace, you don't have to move it aside to do an import.
- No possibility that two people doing an import at the same time on the same box will collide with each other.
- You can do the rename once and then repeat the import process with the renamed dump more easily.
- No tricky stuff with modern Phabricator running against an old dump and the database names not matching up.
None of this is super important, but it just makes large dumps a bit easier to work with, and the dumpfile format is regular enough that this seems unlikely to ever really not work.
Test Plan: Renamespaced a dump, did a `diff -u`, saw all the relevant parts changed (and only those parts changed).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12105
Summary:
Ref T7522. This seems like the least-bad approach to a messy issue:
- When backfilling accounts from an imported instance, I need to write ExternalAccount rows to the instance to link instance accounts with upstream accounts.
- We do this in the daemons in some other cases, which lets us run all the code in the context of the instance. However, I really want to do this in-process here because it's way way simpler and we need to do writes to //both// the instance and the upstream, and they're interleaved, and they depend on one another.
- I can hard-code the query with `qsprintf()` but that feels like 100x worse than this.
This allows me to do this:
```
id(new PhabricatorExternalAccount())
->setForcedConnnection($instance_conn)
->...
->save();
```
...and get a write to the instance database, which is at least not completely a minefield.
Test Plan: Backfilled instance accounts and got interleaved instance and upstream writes as expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12098
Summary:
Fixes T7422. After the recent fix for "sort" columns, we can end up with invalid SQL in some cases when running quickstart.
In particular, we do "COLLATE binary CHARACTER SET utf8_general_ci" (which is invalid).
Preprocess these so we get "COLLATE utf8 CHARACTER SET utf8_general_ci" (which is valid and correct).
Test Plan: Ran `bin/storage upgrade -f --namespace blahblhbaba` with and without `--disable-utf8mb4`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7422
Differential Revision: https://secure.phabricator.com/D11929
Summary:
Fixes T7287. This trades off 4-byte character support for case insensitivity in these columns, which is a much better trade on the balance.
Also adds more warnings about old MySQL. Note that we already issue a warning when you run "storage adjust" (which I've made stronger) and already "strongly recommend" MySQL 5.5 or newer in the install documentation.
Test Plan:
- Ran `storage adjust --disable-utf8mb4` to go to old definitions, then ran `storage adjust` to get back to the new ones. Everything seemed OK in both cases.
- Verified that utf8mb4 data can be migrated out of these colums with `--unsafe` (which will truncate).
- Verified that manual explains this.
- Faked my way into the setup warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7287
Differential Revision: https://secure.phabricator.com/D11893
Summary:
Ref T6840. This feels a little dirty; open to alternate suggestions.
We currently have a race condition where multiple daemons may load a commit and then save it at the same time, when processing "reverts X" text. Prior to this feature, two daemons would never load a commit at the same time.
The "reverts X" load/save has no effect (doesn't change any object properties), but it will set the state back to the loaded state on save(). This overwrites any flag updates made to the commit in the meantime, and can produce the race in T6840.
In other cases (triggers, harbormaster, repositories) we deal with this kind of problem with "append-only-updates + single-consumer", or a bunch of locking. There isn't really a good place to add a single consumer for commits, since a lot of daemons need to access them. We could move the flags column to a separate table, but this feels pretty complicated. And locking is messy, also mostly because we have so many consumers.
Just exempting this column (which has unusual behavior) from `save()` feels OK-ish? I don't know if we'll have other use cases for this, and I like it even less if we never do, but this patch is pretty small and feels fairly understandable (that said, I also don't like that it can make some properties just silently not update if you aren't on the lookout).
So, this is //a// fix, and feels simplest/least-bad for the moment to me, I thiiink.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6840
Differential Revision: https://secure.phabricator.com/D11822
Summary: mysqldump output can end up having weird encoding issues when raw BLOBs are in the output, preventing the backup restoration from succeeding. This hex-encodes blobs in the dump from the backup workflow causing the output file to only contain ASCII and ensure imports are successful.
Test Plan: Had issues restoring a backup from the original `mysqldump` command issued by this workflow. Ran the same command with this flag added and I was able to restore the backup.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11704
Summary: Fixes T7078. Adds a `./bin/storage shell` command which passes through to a MySQL shell. This is slightly more convenient than running `mysql` manually.
Test Plan: Ran `./bin/storage shell` and got a MySQL shell.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7078
Differential Revision: https://secure.phabricator.com/D11548
Summary: Fixes T7050. I got the regexp slightly wrong and didn't catch it because it works fine on modern MySQL.
Test Plan: `arc unit --everything` still passes.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7050
Differential Revision: https://secure.phabricator.com/D11522
Summary:
One advantage I wanted to get out of T1191 is automated rebuilds of `quickstart.sql`. If they don't actually work, I'd like to know sooner rather than later. We haven't rebuilt in a couple months, so give it a shot.
Ran into two issues:
- Some very old patches specify overlong keys which don't work if your default charsets are utf8mb4. Shorten these. No real users have applied these in a very long time.
- Some gymnastics around `corpus` for the new Conpherence search index.
Test Plan:
- Ran `arc unit --everything`, got clean results.
- Cost to do a storage upgrade on an empty namespace dropped from ~4s to ~3s.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11454
Summary:
We have to do some garbage nonsense to write database backups right now, see T6996.
When storage isn't initialized, we previously ended up with this message gzipped in a file and an empty error. Make the behavior slightly more tolerable.
Test Plan: Saw a meaningful error after trying to back up an uninitialized database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11449
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `LiskDAO::establishConnection()`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11412
Summary:
Ref T6881. This is part 1 of my 35-step plan to support subscriptions that bill monthly.
Expanding the capabilities of counters will let me use them to create a logical clock on time-based event updates, build a daemon on top of that, and eventually get time-based triggers.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11395
Summary:
Fixes T6548.
- This workflow doesn't work under reasonable configurations and isn't trivial to fix (see T6548).
- We don't need it; this just makes things a little bit faster if you have to migrate everything (e.g., immediately after T1191) and the installs we know about have generally upgraded by now.
- This keeps kicking PKCS8 keys out of cache which is a pain.
Test Plan: Ran `bin/storage adjust` without it doing an implicit cache purge.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6548
Differential Revision: https://secure.phabricator.com/D11377
Summary: Ref T6822. There are a bunch of places where we call `$something->generatePHID(...)` externally (outside of the class). Therefore, these methods need to be `public`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11363
Summary: Ref T6822.
Test Plan: Visual inspection. This method is only called from within the `PhabricatorTestCase` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11245
Summary:
Ref T3165. Builds a dedicated index for Conpherence to avoid scale/policy filtering concerns.
- This is pretty one-off but I think it's generally OK.
- There's no UI for it.
- `ConpherenceFulltextQuery` is very low-level. You would need to do another query on the PHIDs it returns to actually show anything to the user.
- The `previousTransactionPHID` is so you can load chat context efficiently. Specifically, if you want to show results like this:
> previous line of context
> **line of chat that matches the query**
> next line of context
...you can read the previous lines out of `previousTransactionPHID` directly, and the next lines by issuing one query with `WHERE previousTransactionPHID IN (...)`.
I'm not 100% sure this is useful, but it seemed like a reasonable thing to provide, since there's no way to query this efficiently otherwise and I figure a lot of chat might make way more sense with a couple of lines of context.
Test Plan:
- Indexed a thread manually (whole thing indexed).
- Indexed a thread by updating it (just the new comment indexed).
- Wrote a hacky test script and got reasonable-looking query results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D11234
Summary: Fixes T6795. Fixes T6813. We can give more tailored instructions for surplus schemata than we currently do, and provide more information on resolving them.
Test Plan:
- Ran `storage adjust` with just surplus schemata (friendly warning).
- Ran `storage adjust` with surplus schemata and other serious errors (more severe error).
- Read document.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6795, T6813
Differential Revision: https://secure.phabricator.com/D11054
Summary:
Ref T6238. I'm building the instance management application now, but not putting it in the upstream -- I think the only use case for it is to build SAAS. If someone comes up with a use case (maybe a college course that wants to create an instance per-class or something?) we could open it up eventually, but it seems cleaner to keep it out of the upstream until we have such a use case.
I need to add schema patches. Make it easier for a subclass to just "add all the patches in this directory", like "autopatches/" works.
Test Plan:
- Ran `bin/storage status`, saw all normal patches still valid.
- In some future diff, the instances application will use this to apply patches.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6238
Differential Revision: https://secure.phabricator.com/D10848
Summary:
Fixes T1191. I'll write up the changelog with notes about this and open a feedback task for followups.
When you run `storage upgrade`, automatically run `storage adjust` afterward. Provide a flag to disable this.
This brings everyone into the utf8mb4 world.
Test Plan: Ran `bin/storage upgrade` with various flags. Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10800
Summary:
Ref T1191. Use `storage quickstart` to regenerate `quickstart.sql` using modern schema construction statements.
This puts new installs into utf8mb4 mode immediately without requiring storage adjustment.
Test Plan:
- Ran `arc unit --everything`, which uses quickstart.
- Ran `bin/storage upgrade --namespace temp`, to quickstart a new namespace.
- Ran `bin/storage upgrade --namespace temp --disable-utf8mb4`, to quickstart a new namespace without utf8mb4 support.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10797
Summary:
Ref T1191. Currently if a developer forgot to specify a column type, `storage adjust` aborts explosively mid-stream. Instead:
- Make this a formal error with an unambiugous name/description instead of something you sort of infer by seeing "<unknown>".
- Make this error prevent generation of adjustment warnings, so we don't try to `ALTER TABLE t CHANGE COLUMN c <unknown>`, which is nonsense.
- When schemata errors exist, surface them prominiently in `storage adjust`.
Overall:
- Once `storage upgrade` runs `storage adjust` automatically (soon), this will make it relatively difficult to miss these errors.
- Letting these errors slip through no longer escalates into a more severe issue.
Test Plan:
Commented out the recent `mailKey` spec and ran `storage adjust`:
```
$ ./bin/storage adjust --force
Verifying database schemata...
Found no adjustments for schemata.
Target Error
phabricator2_phriction.phriction_document.mailKey Column Has No Specification
SCHEMATA ERRORS
The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.
If you are not developing Phabricator itself, report this issue to the
upstream.
If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10771
Summary:
Ref T1191. Notable stuff:
- Adds `--disable-utf8mb4` to `bin/storage` to make it easier to test what things will (approximately) do on old MySQL. This isn't 100% perfect but should catch all the major stuff. It basically makes us pretend the server is an old server.
- Require utf8mb4 to dump a quickstart.
- Fix some issues with quickstart generation, notably special casing the FULLTEXT handling.
- Add an `--unsafe` flag to `bin/storage adjust` to let it truncate data to fix schemata.
- Fix some old patches which don't work if the default table charset is utf8mb4.
Test Plan:
- Dumped a quickstart.
- Loaded the quickstart with utf8mb4.
- Loaded the quickstart with `--disable-utf8mb4` (verified that we get binary columns, etc).
- Adjusted schema with `--disable-utf8mb4` (got a long adjustment with binary columns, some truncation stuff with weird edge case test data).
- Adjusted schema with `--disable-utf8mb4 --unsafe` (got truncations and clean adjust).
- Adjusted schema back without `--disable-utf8mb4` (got a long adjustment with utf8mb4 columns, some invalid data on truncated utf8).
- Adjusted schema without `--disable-utf8mb4`, but with `--unsafe` (got truncations on the invalid data).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10757
Summary: without escapage here, creating databases fails. Fixes T6251.
Test Plan: ran the command CREATE DATABASE foo COLLATION binary and it failed; ran the command CREATE DATABASE foo2 COLLATION "binary" and it worked; trusting that the %T still works as advertised.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6251
Differential Revision: https://secure.phabricator.com/D10641
Summary:
Ref T2787. Phortune currently stores a bunch of stuff as `...inUSDCents`. This ends up being pretty cumbersome and I worry it will create a huge headache down the road (and possibly not that far off if we do Coinbase/Bitcoin soon). Even now, it's more of a pain than I figured it would be.
Instead:
- Provide an application-level serialization mechanism.
- Provide currency serialization.
- Store currency in an abstract way (currently, as "1.23 USD") that can handle currencies in the future.
- Change all `...inUSDCents` to `..asCurrency`.
- This generally simplifies all the application code.
- Also remove some columns which don't make sense or don't make sense anymore. Notably, `Product` is going to get more abstract and mostly be provided by applications.
Test Plan:
- Created a new product.
- Purchased a product.
- Backed an initiative.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10633
Summary: Ref T1191. We don't create new databases with appropriate collation yet.
Test Plan:
Created a new database and saw it issue:
```
>>> [10] <query> CREATE DATABASE IF NOT EXISTS `phabricator2_testo` COLLATE utf8mb4_bin
```
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10632
Summary:
Ref T4209. This creates storage for public keys against authorized hosts, such that servers can be authorized to make Conduit calls as the omnipotent user.
Servers are registered into this system by running the following command once:
```
bin/almanac register
```
NOTE: This doesn't implement authorization between servers, just the storage of public keys.
Placing this against Almanac seemed like the most sensible place, since I'm imagining in future that the `register` command will accept more information (like the hostname of the server so it can be found in the service directory).
Test Plan: Ran `bin/almanac register` and saw the host (and public key information) appear in the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4209
Differential Revision: https://secure.phabricator.com/D10400
Summary: Ref T1191. The bulk of the slowness in T1191 is copying tables. In some cases, we can't avoid this, but we have various readthrough caches which may be very large and are safe to drop, and dropping them is very quick (much less than 1 second). In particular, dropping the `changeset_parse_cache` made the process at least ~8 minutes faster on `secure.phabricator.com` (I killed it after 8 minutes, so I'm not sure what the real number is).
Test Plan: Ran `bin/storage adjust` and saw it drop caches before applying adjustments.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10616
Summary: Ref T1191. I renamed the phases but missed these two since I didn't have any more key issues locally.
Test Plan: Ran `bin/storage adjust` in production with key issues.
Reviewers: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10612
Summary:
Ref T1191. When changing the column type of an AUTO_INCREMENT column, we currently may lose the autoincrement attribute.
Instead, support it. This is a bit messy because AUTO_INCREMENT columns interact with PRIMARY KEY columns (tables may only have one AUTO_INCREMENT column, and it must be a primary key). We need to migrate in more phases to avoid this issue.
Introduce new `auto` and `auto64` types to represent autoincrement IDs.
Test Plan:
- Saw autoincrement show up correctly in web UI.
- Fixed an autoincrement issue on the XHProf storage table with `bin/storage adjust` safely.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10607
Summary:
Ref T1191. Currently, the `quickstart.sql` gets generated in a pretty manual fashion. This is a pain, and will become more of a pain in the world of utf8mb4.
Provide a workflow which does upgrade + adjust + dump + destroy, then massages the output to produce a workable `quickstart.sql`.
Test Plan: Inspected output; I'll test this more throughly before actually generating a new quickstart, but that's some ways away.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10603
Summary:
Ref T1191. These are a bit tricky because keys can interact with column changes, so basically we do three phases:
1. Nuke all bad keys.
2. Make all column (and database/table) changes.
3. Fix all nuked keys.
Test Plan: Ran migration locally. See note for remaining issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10599
Summary:
Ref T1191. Adds a new workflow which can apply schema adjustments.
For now, it only performs database and table collation/charset adjustments. I believe these are extremely safe/minor, because they only affect the default values for newly created columns.
Test Plan:
- Ran migration on various database states, database/table changes went through cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10595
Summary:
Ref T1191. Nothing too notable here:
- Allow a Lisk object to specify that there's no expectation that a table exists. We have one Harbormaster object and one Token object like this.
- Removed BuildPlanTransactionComment because it's currently unused.
Test Plan:
- Saw ~200 fewer warnings; just ~800 left.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10583
Summary: See <https://github.com/phacility/phabricator/issues/665>. From reading documentation, this seems dramatically better for InnoDB tables than the default behavior.
Test Plan: Ran `bin/storage dump`, got a reasonable-looking dump.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10606
Summary: Ref T1191. Handful of minor things here (T6150, T6149, T6148, T6147, T6146) but nothing very noteworthy.
Test Plan: Viewed web UI, saw fewer errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10527
Summary:
Ref T1191. Three parts:
- The old way of getting key information only got primary / unique / foreign keys, not all keys. Use `SHOW INDEXES` to get all keys instead.
- Track key uniqueness and raise warnings about it.
- Add a new "all issues" view to show an expanded, flat view of all issues. This is just an easier way to get a list so you don't have to dig around in the hierarchical view.
Test Plan:
{F206351}
{F206352}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10525
Summary: Ref T1191. Nothing too exciting in these.
Test Plan: Saw more blue in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10521
Summary:
Ref T1191. Notable:
- Allowed objects to remove default columns (some feed tables have no `id`).
- Added a "note" severity and moved all the charset stuff down to that to make progress more clear.
Test Plan:
Trying to make the whole thing blue...
{F205970}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10519
Summary: Ref T1191. Fills in some more of the databases. Nothing very notable here. I didn't encounter any issues or overlong keys.
Test Plan: Used web UI to click around and verify expected schemata match up against actual schemata well.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10516
