[draft] adapter: distributed timestamp oracle backed by "postgres" #21671

aljoscha · 2023-09-08T13:14:25Z

Motivation

A part of MaterializeInc/database-issues#6635 , but so far a draft for shopping the thing around.

The commits tell the story of the evolution, with intermediary steps, like the consensus-backed timestamp oracle that can be removed again. Also, there's more steps we could do like now share the final version of the postgres oracle using an Arc<dyn TimestampOracle>, meaning we wouldn't have to do the shallow clones anymore.

Tips for reviewer

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:

danhhz

left a few superficial comments from a very high-level scan, don't bother doing anything in response to them, i'm mostly flushing them out so github doesn't eat them. i'll do a deeper pass later this week

one high-level thing i noticed is that this makes quite a few non-async fns into async fns. from what I understand, it's possible that those were intentionally non-async so it's easier to reason about what's happening in the coord loop. we should check with folks on that

danhhz · 2023-09-11T15:07:46Z

src/adapter/src/coord/timestamp_oracle.rs

+/// reported completed write timestamps, and strictly less than all subsequently
+/// emitted write timestamps.
+#[async_trait]
+pub trait TimestampOracle<T> {


can this whole module be its own crate? if so, i'd lean toward do it

danhhz · 2023-09-11T15:14:10Z

src/adapter/tests/timestamp_selection.rs

@@ -261,6 +264,8 @@ fn parse_query_when(s: &str) -> QueryWhen {
 /// Transaction isolation can also be set. The `determine` directive runs determine_timestamp and
 /// returns the chosen timestamp. Append `full` as an argument to it to see the entire
 /// TimestampDetermination.
+// allow `futures::block_on` for testing.


this turns out to be a pretty big footgun in practice, even for tests. highly recommend figuring out a way to use tokio's block_on

danhhz · 2023-09-11T15:17:18Z

src/adapter/src/coord/timestamp_oracle.rs

+
+use crate::coord::timeline::WriteTimestamp;
+
+pub mod consensus;


are we planning on merging all three impls or just the final one?

danhhz · 2023-09-11T15:17:41Z

src/adapter/src/coord/timestamp_oracle.rs

+use crate::coord::timeline::WriteTimestamp;
+
+pub mod consensus;
+pub mod durable;


maybe a better name for this than "durable" is "catalog"? they're all durable, no?

danhhz · 2023-09-11T15:35:25Z

src/adapter/src/coord/message_handler.rs

    }

-    pub fn schedule_storage_usage_collection(&self) {
+    pub async fn schedule_storage_usage_collection(&self) {


might be nice to keep this non-async so it's obvious that it doesn't do anything expensive. which I think could be as easy as passing in the ts as an arg?

philip-stoev · 2023-09-18T11:08:01Z

src/adapter/src/coord/timestamp_oracle/postgres.rs

+            UPDATE timestamp_oracle SET write_ts = GREATEST(write_ts, $2), read_ts = GREATEST(read_ts, $2)
+                WHERE timeline = $1;
+        "#;
+        let client = self.get_connection().await.expect("todo");


This is being seen in Sentry - https://materializeinc.sentry.io/issues/4478589600/?project=6780145&query=is%3Aunresolved&referrer=issue-stream&statsPeriod=7d&stream_index=10

philip-stoev · 2023-09-18T11:09:16Z

@aljoscha this looks like something that could use some more chaos testing and such. Please ping the QA team and/or add me as a reviewer once the time arrives . Thank you!

jkosh44

This looks good to me, I didn't have many useful comments.

jkosh44 · 2023-09-18T15:35:09Z

src/adapter/src/coord/timestamp_oracle.rs

+    /// Mark a write at `write_ts` completed.
+    ///
+    /// All subsequent values of `self.read_ts()` will be greater or equal to
+    /// `write_ts`.


Maybe you meant lower_bound or you meant to change the name of the parameter?

jkosh44 · 2023-09-18T15:45:43Z

src/adapter/src/coord/timestamp_oracle/consensus.rs

+            let new_read_ts = if write_ts > self.read_ts {
+                write_ts.clone()
+            } else {
+                self.read_ts.clone()


Can't you just return here since the invariant

All subsequent values of self.read_ts() will be greater or equal to write_ts.

is true?

jkosh44 · 2023-09-18T15:53:47Z

src/adapter/src/coord/timestamp_oracle/durable.rs

+//! A timestamp oracle that relies on the [`Catalog`] for persistence/durability
+//! and reserves ranges of timestamps.


Am I missing something or is TimestampOracle never implemented in this file?

jkosh44 · 2023-09-18T15:57:30Z

src/adapter/src/coord/timestamp_oracle/durable.rs

+// TODO(aljoscha): These internal details of the oracle are leaking through to
+// multiple places in the coordinator.


I'm somewhat on the fence about who's responsibility this should be. TIMESTAMP_PERSIST_INTERVAL doesn't make a lot of sense anymore since we don't reserve ranges of timestamps and TIMESTAMP_INTERVAL_UPPER_BOUND only really makes sense with the EpochMillis timeline. For other timelines, do we actually care about the timestamp oracle jumping ahead by a large amount? monotonic_now especially makes some assumptions that now() is hooked up to some wall clock that is advancing at a rate of 1 per millisecond.

Really, a CRDB-backed oracle, of course...

…estampOracle

Note that the consensus oracle is implemented in a non-ideal way and could be further simplified if all operations where turned into purely SQL statements against CRDB. Then we wouldn't need the cached read/write ts anymore.

…timestamp We don't need to go through "linearize reads" anymore because the timestamp oracle by itself now linearizes peeks. We do still need this stage for peeks that are scheduled in the future (with respect to the current oracle-aware read timestamp).

…ites When there are already pending writes, a group commit has already been triggered so we can sneak into that one and don't have to trigger our own. Any superfluous group commits that get triggered will find an empty list of pending writes but still perform costly timestamp oracle operations and will forward all table uppers, which are costly persist operations. This becomes noticeable when timestamp operations, such as peek_write_ts and write_ts are more expensive, and before this "optimization" we were doing a copious amount of those, even with nothing to write.

At least we comment out the assert ... 😅

We can do that after a previous change for batching calls to the timestamp oracle, which factored calls to the oracle out of determine_timestamp_for, which was why we had to make it async in the first place.

philip-stoev · 2023-10-31T10:46:49Z

@aljoscha please re-request a review from the QA team once the merge conflicts are resolved. I would like to run another Nightly at the very least.

aljoscha · 2023-11-23T14:28:00Z

#22262 has been merged

aljoscha requested review from a team as code owners September 8, 2023 13:14

aljoscha requested a review from jkosh44 September 8, 2023 13:14

aljoscha marked this pull request as draft September 8, 2023 13:14

danhhz reviewed Sep 11, 2023

View reviewed changes

philip-stoev reviewed Sep 18, 2023

View reviewed changes

jkosh44 reviewed Sep 18, 2023

View reviewed changes

aljoscha force-pushed the adapter-distributed-ts-oracle branch from fc897f2 to 3245e31 Compare September 21, 2023 14:44

aljoscha force-pushed the adapter-distributed-ts-oracle branch from 3245e31 to 7c019ae Compare October 2, 2023 09:34

This was referenced Oct 2, 2023

adapter: make all DurableTimestampOracle methods async #22091

Merged

adapter: move timestamp oracle impl to it's own module, put behind trait object #22112

Merged

aljoscha added 17 commits October 5, 2023 15:10

adapter: add Postgres-backed TimestampOracle

77ecc26

Really, a CRDB-backed oracle, of course...

adapter: remove defensive checks that we don't need for consensus Tim…

18dbaf2

…estampOracle

adapter: make TimestampOracle shareable/shallow-clone-able

c2687c8

Note that the consensus oracle is implemented in a non-ideal way and could be further simplified if all operations where turned into purely SQL statements against CRDB. Then we wouldn't need the cached read/write ts anymore.

wip: rename PeekStageTimestamp to PeekStageReadHolds

4a8c54e

adapter/wip: batch calls to linearized timestamp oracle

210d77c

adapter: thread tracing Span through GroupCommitApply

42c6e8a

adapter: more debug logging around group_commit_ininiate/apply

186cb28

wip: comment out code that tries and remove timestamp state from Stash

2b77839

At least we comment out the assert ... 😅

adapter: remove async from TimestampProvider::determine_timestamp_*

b2685a9

We can do that after a previous change for batching calls to the timestamp oracle, which factored calls to the oracle out of determine_timestamp_for, which was why we had to make it async in the first place.

adapter: instrument submit_write

c1c4ad7

wip: log number of writes that are part of a group commit

d5a01da

adapter: de-prioritize group commit

d0a3158

adapter: apply_write off main loop

6e41bdd

wip: add retry module for timestamp oracles

13d723f

wip: system var for ts oracle

47e9868

aljoscha force-pushed the adapter-distributed-ts-oracle branch from 7c019ae to 47e9868 Compare October 5, 2023 13:13

aljoscha closed this Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[draft] adapter: distributed timestamp oracle backed by "postgres" #21671

[draft] adapter: distributed timestamp oracle backed by "postgres" #21671

aljoscha commented Sep 8, 2023 •

edited

Loading

danhhz left a comment

danhhz Sep 11, 2023

danhhz Sep 11, 2023

danhhz Sep 11, 2023

danhhz Sep 11, 2023

danhhz Sep 11, 2023

philip-stoev Sep 18, 2023

philip-stoev commented Sep 18, 2023

jkosh44 left a comment

jkosh44 Sep 18, 2023

jkosh44 Sep 18, 2023

jkosh44 Sep 18, 2023

jkosh44 Sep 18, 2023

philip-stoev commented Oct 31, 2023

aljoscha commented Nov 23, 2023


		use crate::coord::timeline::WriteTimestamp;

		pub mod consensus;

		//! A timestamp oracle that relies on the [`Catalog`] for persistence/durability
		//! and reserves ranges of timestamps.

		// TODO(aljoscha): These internal details of the oracle are leaking through to
		// multiple places in the coordinator.

[draft] adapter: distributed timestamp oracle backed by "postgres" #21671

[draft] adapter: distributed timestamp oracle backed by "postgres" #21671

Conversation

aljoscha commented Sep 8, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

danhhz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-stoev commented Sep 18, 2023

jkosh44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-stoev commented Oct 31, 2023

aljoscha commented Nov 23, 2023

aljoscha commented Sep 8, 2023 •

edited

Loading