(6/5) Collapse "sled_resource" into "sled_resource_vmm" type #7471
Conversation
| // Using the connection we got from the connection pool prior to applying | ||
| // the schema migrations, attempt to insert a sled resource. This involves | ||
| // the `sled_resource_kind` enum, whose OID was changed by the schema | ||
| // migration in version 53.0.0 (by virtue of the enum being dropped and | ||
| // added back with a different set of variants). If the diesel OID cache was | ||
| // populated when we acquired the connection from the pool, this will fail | ||
| // with a `type with ID $NUM does not exist` error. |
There was a problem hiding this comment.
IIRC, this was intended to guard against a problematic OID-caching bug. Is it safe to replace this with "any other old enum?"
(this needs to change in this PR because the enum-under-test no longer exists)
There was a problem hiding this comment.
IIUC it should be possible to use any enum that didn't exist in the first version.
There was a problem hiding this comment.
This is something I wanted to confirm - tagging @jgallagher , since I think you implemented this originally.
I thought this test needed to check for:
- Any enum that exists in "version 1" of the schema...
- Which was modified in a later schema upgrade...
- ... and which still exists in the most recent schema, so it can be queried?
There was a problem hiding this comment.
mmm it might be even more specific than that - I think this type was actually "dropped" and re-created - so the name is shared between "v1" and "the current version", but the type is wholly distinct
There was a problem hiding this comment.
I think instance_auto_restart has also been DROPPED and re-created, but it did not exist in the initial version. I don't think any other enum exists with the same characteristics of:
- Existed in v1
- Exists in current revision
- Was dropped + recreated (so the name is shared, but the OID is altered)
There was a problem hiding this comment.
mmm it might be even more specific than that - I think this type was actually "dropped" and re-created - so the name is shared between "v1" and "the current version", but the type is wholly distinct
I believe this is correct (this test came out of #5561, for a very large amount of context).
I think instance_auto_restart has also been DROPPED and re-created, but it did not exist in the initial version. I don't think any other enum exists with the same characteristics of: ...
I think you could adjust this test to use instance_auto_restart by tweaking how many schema migrations are applied before we pause and grab a connection. Right now we .take(1), grab a connection, then .skip(1), then use the connection after all migrations are applied. We could change the 1 to whatever migration initially added instance_auto_restart, I believe?
There was a problem hiding this comment.
Okay, I've implemented this in fb8976d
I decided to move it into the validate_data_migration test, and to expand the "test context" of that function. We already had hooks for "before/after" schema migrations there, and I figured this made it a little more explicit about "use state from right before update XXX, and then check it later".
I've also added some documentation to hopefully make it easier to find other cases of these enums, in case this data shifts again in the future (which assuredly it will, at some point)
This specializes the table for VMM usage specifically, rather than leaving it generic for speculative usage by other services.