Repository navigation
Remove persisted introspection sources - #18302
Conversation
57a2c71 to
940f09b
Compare
| log_logging: bool, | ||
| interval: Option<Duration>, | ||
| sources: Option<Vec<(LogVariant, GlobalId)>>, | ||
| views: Option<Vec<(LogView, GlobalId)>>, |
There was a problem hiding this comment.
This is a change to the serialized catalog state, but according to my testing the deserialization ignores unknown fields, which makes a migration unnecessary. We could still write one to proactively clean up the serialized state. What do the surfaces folk think?
There was a problem hiding this comment.
I don't have a very strong opinion on this. I think it's slightly safer to clean up the state and it's not much extra code.
If we remove these fields and don't clean them up, and then later add new fields, will the deserialization still work correctly?
There was a problem hiding this comment.
I think as long as we don't add ones with the same names, we should be fine? Just guessing. But you are right, better safe than sorry. I'll add a migration.
There was a problem hiding this comment.
@jkosh44 Do you know how to write such a migration? I tried:
diff --git a/src/adapter/src/catalog/storage.rs b/src/adapter/src/catalog/storage.rs
index edf681b33d..2beaa21275 100644
--- a/src/adapter/src/catalog/storage.rs
+++ b/src/adapter/src/catalog/storage.rs
@@ -385,6 +385,16 @@ async fn migrate(
)?;
Ok(())
},
+ // The fields relating to persisted introspection sources and views
+ // where removed from the replica config.
+ //
+ // Introduced in v0.48.0
+ //
+ // TODO(jkosh44) Can be cleared (patched to be empty) v0.50.0
+ |txn: &mut Transaction<'_>, _now, _bootstrap_args| {
+ txn.cluster_replicas.update(|_k, v| Some(v.clone()))?;
+ Ok(())
+ }
// Add new migrations above.
//
// Migrations should be preceded with a comment of the following form:But that doesn't work, the JSON stored in stash remains unchanged. AFAICT this is because update inserts a -1 update for the old v and a +1 update for the new v. It then serializes both vs to JSON and adds them to a stash batch. So in the end that batch has updates inserting and retracting the same value, while the actual old JSON value is untouched.
It seems like we need a way to migrate the serialized stash contents, but I don't think that exists.
There was a problem hiding this comment.
I naively thought that would have worked. Did you try deleting and then re-inserting and see if it's any better? Otherwise, let's just skip it for now.
There was a problem hiding this comment.
I tried that, it has the same effect. The underlying issue is that you can't tell stash to "delete the value at key k" but only "delete this (key, JSON value) pair". And since the migration works on deserialized values, it doesn't know how the old JSON value looked like.
940f09b to
050f73d
Compare
|
I'm pretty sure persisted introspection was never enabled on any customer environment, but just to be sure I queried all production catalogs to see if their |
philip-stoev
left a comment
philip-stoev
left a comment
There was a problem hiding this comment.
Thank you . Given the sheer size of the PR, can you please run Nightly against it as well -- I am surprised that there weren't any tests from test/testdrive/*.td that required adjusting.
|
Yes! I started a Nightlies run: https://buildkite.com/materialize/nightlies/builds/2027
We didn't do a good job of testing these sources I guess. Writing robust tests was difficult because you needed to know the replica IDs to reference the persisted introspection relations and you couldn't use SQL to query them because there is now way to dynamically construct relation names (although testdrive might provide a workaround). |
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Thanks, this is great! Let's wait for tests to come back, but I think we should merge this soon.
|
The Nightlies run is complete. A couple jobs failed, but they seem to fail on the current main too, so I guess we are good? |
| log_logging: bool, | ||
| interval: Option<Duration>, | ||
| sources: Option<Vec<(LogVariant, GlobalId)>>, | ||
| views: Option<Vec<(LogView, GlobalId)>>, |
There was a problem hiding this comment.
I don't have a very strong opinion on this. I think it's slightly safer to clean up the state and it's not much extra code.
If we remove these fields and don't clean them up, and then later add new fields, will the deserialization still work correctly?
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
I suggested related changes that would allow the sqllogictests to pass.
050f73d to
53bbd2f
Compare
This commit removes the persisted introspection sources from the codebase. These have been disabled for a long time due to the load they place on persist. In the meantime we have found that the arranged introspection sources serve as a usable alternative for our debugging purposes. Even though persist has meanwhile improved its performance to the point where it can almost support the workload caused by the persisted introspection sources, those sources still don't provide the ergonomics we would like. They require users to pick a per-replica relation by suffixing it with the replica ID, so they are not more convenient to use than the arranged introspection sources. They also litter `mz_sources` with additional relations for every existing replica, which causes a lot of noise. Their only advantage is that they make it possible to query introspection data from a replica that is crashing fast. So far we haven't had much need for this feature though. We are still looking for ways to make introspection ergonomic. The final solution may or may not use persist. If it uses persist, it will likely look different than the current implementation and therefore would require a substantial rewrite anyway. Based on this, there is not much reason to keep the current implementation around. By removing it, we remove the burden of maintaining its code, which it quite convoluted at times.
53bbd2f to
90b4999
Compare
This PR removes the persisted introspection sources from the codebase. These have been disabled for a long time due to the load they place on persist. In the meantime we have found that the arranged introspection sources serve as a usable alternative for our debugging purposes.
Even though persist has meanwhile improved its performance to the point where it can almost support the workload caused by the persisted introspection sources, those sources still don't provide the ergonomics we would like. They require users to pick a per-replica relation by suffixing it with the replica ID, so they are not more convenient to use than the arranged introspection sources. They also litter
mz_sourceswith additional relations for every existing replica, which causes a lot of noise. Their only advantage is that they make it possible to query introspection data from a replica that is crashing fast. So far we haven't had much need for this feature though.We are still looking for ways to make introspection ergonomic. The final solution may or may not use persist. If it uses persist, it will likely look different than the current implementation and therefore would require a substantial rewrite anyway. Based on this, there is not much reason to keep the current implementation around. By removing it, we remove the burden of maintaining its code, which it quite convoluted at times.
Closes MaterializeInc/database-issues#4481.
Motivation
This is immediately motivated by my work on MaterializeInc/database-issues#5100, where keeping the persisted introspection views updated turned out to be quite time-consuming.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.