coord: Parallelize builtin table writes with side effects by ParkMyCar · Pull Request #23551 · MaterializeInc/materialize

ParkMyCar · 2023-11-29T21:28:06Z

This PR improves the latency of creation DDL, e.g. CREATE TABLE, by waiting for builtin table updates to complete concurrently with any side effects, e.g. creating a storage collection.

Part of this change which might be particularly controversial, is refactoring the handling of group commits, and removing the notion of blocking. Now it's up to the caller as to whether or not they want to block on a group commit finishing, by .await-ing a returned Future. This change makes it possible to wait on builtin table updates concurrently with other tasks. Specifically we no longer call self.group_commit_apply(...) directly in group_commit_initiate(...) instead we queue a GroupCommitApply task on the internal command sender. I believe this is okay to do because all internal commands are handled before user commands, so we'll still process the group commit apply before users could observe any intermediate state, e.g. a table being created but not showing up in mz_tables. Briefly I was concerned about the "late" GroupCommitApply message moving the write timestamp backwards, but this invariant is enforced in the implementations of the timestamp oracle.

Open Telemetry Trace

The above is the result of running CREATE TABLE t1 (x int, y text, z timestamp) on staging with this built deployed. You can see that "group_commit_initiate" (builtin table updates) and "insert_without_overwrite" (a side effect), get run in parallel.

Motivation

Improves https://github.com/MaterializeInc/database-issues/issues/7078

Tips for reviewer

Hiding white space is helpful since a number of things either got indented or de-dented a level.

This PR is split into two commits which can be reviewed independently:

Refactoring builtin table updates
Refactoring sequencing so the relevant DDLs run their side effects concurrently with the builtin table updates.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- Internal performance improvement

jkosh44 · 2023-11-29T23:27:35Z

src/adapter/src/coord/appends.rs

+            // Note: while technically we should have `GroupCommitApply` update the notifies, we
+            // know that internal commands get processed before any user commands, so the writes
+            // are still guaranteed to be observable before any user commands.
+            for notify in notifies {
+                // We don't care if the listeners have gone away.
+                let _ = notify.send(());
+            }
+
+            // Trigger a GroupCommitApply, which will run before any user commands since we're
+            // sending it on the internal command sender.


This scares me a bit. It's not obviously correct and seems like something that's easy to accidentally break without realizing it. Why not just send the replies after we apply the timestamp?

We can't send the notifies after we apply the timestamp because that requires waiting for the Coordinator to run the queued GroupCommitApply command, which it won't be able to do because the current command is waiting for the notify to resolve, so we end up in a dead lock.

Joe and I talked in a Huddle, the correctness property here relies on internal commands being run before user commands. We queue a GroupCommitApply on the internal command sender before notifying any waiters, so we know the write will be applied before any user commands could observe the intermediate state.

We don't love relying on this property though, and conveniently some PlatformV2 work will soon make it possible to give an owned handle to the timestmap oracle, to this spawned task. Then we could explicitly apply the write in the task instead of relying on the queued command to get processed before user commands. Waiting to hear back on how soon the shareable timestamp oracle will be ready!

src/adapter/src/coord/appends.rs

jkosh44 · 2023-11-29T23:33:59Z

src/adapter/src/coord/appends.rs

+
+    /// Submit a write to a system table.
+    ///
+    /// This method will block the Coordinator on doing some initial work, and then returns a


on doing some initial work

This seems a bit vague, what exactly does it wait for?

Updated this comment to be more descriptive

jkosh44 · 2023-11-29T23:43:55Z

src/adapter/src/coord/ddl.rs

                })
            })
-            .await;
+            .await?;


Why did we add a ?? Doesn't this always return an error making the rest of this function un-reachable?

Didn't mean to do this, was a cargo culting issue. Fortunately our tests caught the bug! I updated this and made the assertions stronger here.

def- · 2023-11-30T10:34:13Z

The test failures look legitimate. I'll wait until it's green to run nightly/coverage.

jkosh44 · 2023-11-30T16:58:20Z

~~We should also open follow-up issues for the dependency tables.~~

EDIT: Oops I commented this on the wrong PR, please ignore.

jkosh44

LGTM!

jkosh44 · 2023-11-30T16:32:59Z

src/adapter/src/coord/appends.rs

            .metrics
            .append_table_duration_seconds
-            .with_label_values(&[label]);
+            .with_label_values(&["false"]);


Just realizing, is always using false here correct? Whether we block or not is determined by the caller so we don't really know if we're going to block here. Should we just remove the labels from this Histogram?

You're totally right! Removing this label would be best, I'm not sure if we can do that though/if it's forward compatible, let me check

jkosh44 · 2023-11-30T16:42:44Z

src/adapter/src/coord/ddl.rs

+        // Run our side effects concurrently with the table updates.
+        let ((), ()) = futures::future::join(side_effects_fut, table_updates).await;


For my own Rust learnings, how does side_effects_fut and table_updates run concurrently if they both have a mutable reference to the Coordinator?

Or am I wrong about them both needing a mutable reference?

table_updates does not actually have a mutable reference to the Coordinator, which is how they're able to both run concurrently

jkosh44 · 2023-12-01T19:02:26Z

src/adapter/src/coord/appends.rs

+                // Note: while technically we should have `GroupCommitApply` update the notifies, we
+                // know that internal commands get processed before any user commands, so the writes
+                // are still guaranteed to be observable before any user commands, because we
+                // submitted the GroupCommitApply above.


Should we get rid of this commit now or combine it with the one above on line 394?

Sorry I'm a bit confused, what do you mean by this?

I might be misreading, but it seems like the following two comments are trying to say the same thing:

// Trigger a GroupCommitApply, which will run before any user commands since we're
// sending it on the internal command sender.

// Note: while technically we should have GroupCommitApply update the notifies, we
// know that internal commands get processed before any user commands, so the writes
// are still guaranteed to be observable before any user commands, because we
// submitted the GroupCommitApply above.

So it might be a bit easier to maintain if we combined them into a single comment. That's subjective though, so feel free to ignore.

Ah ha! Updated the comment and combined the two

jkosh44 · 2023-12-01T19:04:21Z

src/adapter/src/coord/sequencer/inner.rs

+        let cloud_resource_controller = self
+            .cloud_resource_controller
+            .as_ref()
+            .cloned()
+            .ok_or(AdapterError::Unsupported("AWS PrivateLink connections"))?;


Where did this come from? Or am I missing something obvious?

This was split out from an error case which used to occur after the catalog transaction. But I moved this up so we'd error before the transaction. Turns out this made a bunch of tests fail, so I refactored it again to just emit warnings which is what was suggested in Slack

…rror after creating a connection

shepherdlybot · 2023-12-04T16:52:27Z

Mitigations

Completing required mitigations increases Resilience Coverage.

(Required) Code Review 🔍 Detected
(Required) Feature Flag
(Required) Integration Test
(Required) Observability 🔍 Detected
(Required) QA Review 🔍 Detected
Unit Test

Bug Hotspots:

What's This?

File	Percentile
../sequencer/inner.rs	98
../coord/command_handler.rs	92
../src/coord.rs	100
../coord/ddl.rs	93

ParkMyCar marked this pull request as ready for review November 29, 2023 21:43

ParkMyCar requested review from a team as code owners November 29, 2023 21:43

ParkMyCar requested a review from jkosh44 November 29, 2023 21:43

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from a0d9c6f to 790f656 Compare November 29, 2023 22:03

jkosh44 reviewed Nov 29, 2023

View reviewed changes

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from 790f656 to f72a513 Compare November 30, 2023 16:11

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from 254394b to db89ed7 Compare December 1, 2023 18:40

jkosh44 approved these changes Dec 1, 2023

View reviewed changes

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from db89ed7 to 79845a0 Compare December 4, 2023 13:51

ParkMyCar added 5 commits December 4, 2023 09:37

start, refactor builtin table updates to be async by default

029d411

run create ddl in parallel with builtin table updates

e6594f5

explicitly update notifies _after_ queuing the GroupCommitApply

61613ae

use a shareable timestamp oracle when available, make sure we don't e…

af81802

…rror after creating a connection

fix issue with creating connections

fa01c77

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from 79845a0 to fa01c77 Compare December 4, 2023 15:17

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch 2 times, most recently from e308e8b to 6dff1ed Compare December 4, 2023 20:27

remove unused metric label, update comment, fix client responder issue

7bf7a40

ParkMyCar force-pushed the coord/ddl-parallelize-a-few-things branch from 6dff1ed to 7bf7a40 Compare December 4, 2023 21:01

ParkMyCar merged commit 06d8f66 into MaterializeInc:main Dec 5, 2023

ParkMyCar deleted the coord/ddl-parallelize-a-few-things branch June 17, 2024 20:36

def- mentioned this pull request Apr 13, 2026

adapter: Clean up secrets instead of leaking them on error #36033

Open

		// Run our side effects concurrently with the table updates.
		let ((), ()) = futures::future::join(side_effects_fut, table_updates).await;

Conversation

ParkMyCar commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open Telemetry Trace

Motivation

Tips for reviewer

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

def- commented Nov 30, 2023

Uh oh!

jkosh44 commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkosh44 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParkMyCar Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shepherdlybot bot commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mitigations

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ParkMyCar commented Nov 29, 2023 •

edited

Loading

jkosh44 commented Nov 30, 2023 •

edited

Loading

ParkMyCar Dec 4, 2023 •

edited

Loading

shepherdlybot bot commented Dec 4, 2023 •

edited

Loading