sql: RETAIN HISTORY for materialized views #23788

maddyblue · 2023-12-09T21:53:50Z

Add RETAIN HISTORY to materialized views.

See https://github.com/MaterializeInc/database-issues/issues/4840

Motivation

This PR adds a known-desirable feature.

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:
- Add COMPACTION WINDOW to MATERIALIZED VIEWS.

frankmcsherry · 2023-12-09T21:59:25Z

Is this a good place to bikeshed the name? I think users may not understand "compaction" but might understand e.g. "history" or "historical" windows instead?

jkosh44

Code LGTM, ~~I don't really have a strong opinion on the naming.~~

EDIT: I forgot I'm on the SQL council this month. I will form an opinion next week.

jkosh44 · 2023-12-10T14:57:13Z

src/environmentd/tests/sql.rs

+            let source = ts.sources.into_element();
+            let upper = source.write_frontier.into_element();
+            let since = source.read_frontier.into_element();
+            if upper.saturating_sub(since) > Timestamp::from(2000u64) {


Should this be >=?

I don't know! During testing the diff was usually just a bit above 2000. I'm not sure what the guarantees are for read and write frontiers in the storage controller though. I changed it?

benesch

Requesting changes since there's a pending discussion on the naming of the option in Slack.

maddyblue · 2023-12-11T23:36:48Z

Changed to RETAIN HISTORY = FOR <interval>.

benesch

Syntax looks good modulo the point about the equals sign being optional.

Just to confirm: it's not possible with this change to alter the compaction window for a materialized view, yeah? Seems fine, since this is plenty to unblock testing with rETL tools.

src/sql-parser/src/parser.rs

benesch · 2023-12-12T04:25:16Z

src/sql/src/plan.rs

+    /// Disable logical compaction entirely.
+    Disabled,


Is this meant to be constructible at the moment? It doesn't seem to be! Is the idea that we'll introduce RETAIN HISTORY = FOREVER in the future?

It is written this way to have compatibility with index options which allow for this:

materialize/src/sql/src/plan.rs

Line 1548 in a75510b

/// Configures the logical compaction window for an index. `None` disables

In a later PR I would like to refactor the index planning and controller read policy code to use this new enum instead of passing around Option<milliseconds or Timestamp or Duration>.

The principal motivation for this struct is to describe exactly what a None Option means. There are some places it means "I want NO user compaction performed" and others it means "no one specified anything, use the default". With the current scheme we'd have some Option<Option<Duration>> in a few places. I thought this would improve that greatly, although the refactor is getting annoying.

That plan sounds good to me! I just couldn't tell in the current code where the Disabled variant was getting used.

maddyblue · 2023-12-12T05:48:24Z

it's not possible with this change to alter the compaction window for a materialized view, yeah?

Correct. ALTER is a bit harder because it has to muck with the SQL. I'd like to ship retain history for CREATE tables, sources and mvs. Then do ALTER afterward.

maddyblue · 2023-12-12T06:00:55Z

Hold off on any more reviews, this doesn't work on restart.

maddyblue · 2023-12-13T07:35:25Z

RFAL. First commit is #23854. This now works on restart and has a test for that.

benesch

Syntax LGTM. Not going to have time to look through the rest.

ggevay · 2023-12-13T11:19:54Z

Oh, great! I'm happy about this, because I'm planning to also increase the compaction window of REFRESH EVERY materialized views to the refresh interval, and I guess that should be easy after this PR.

I have a question: What happens with indexes on a materialized view that has a custom compaction window? Does the compaction window of the index inherit the MV's window? If not, then this might be surprising to users: they might expect that creating an index can only speed up querying from the MV but never alter the semantics.

jkosh44 · 2023-12-13T14:40:39Z

src/catalog/src/memory/objects.rs

@@ -448,7 +446,7 @@ pub struct Source {
    pub desc: RelationDesc,
    pub timeline: Timeline,
    pub resolved_ids: ResolvedIds,
-    pub custom_logical_compaction_window: Option<Duration>,
+    pub custom_logical_compaction_window: Option<CustomCompactionWindow>,


For these objects, what's the difference between None and Some(CustomCompactionWindow::Default)?

none means the user did not specify a custom window in the syntax. maybe customcompactionwindow::default should not exist then since it's implied by none!

maddyblue · 2023-12-14T05:09:35Z

test/restart/mzcompose.py

@@ -29,6 +32,44 @@
 ]


+def workflow_retain_history(c: Composition) -> None:


Putting this test in restart doesn't feel quite right. Is there a better place to put it?

restart seems right to me, actually! In the future #team-testing will get you a very fast answer to these sorts of questions. They'd probably recommend adding a platform check for this, since platform checks get interspersed with all sorts of disruptions, like restarts. Can do that as a followup though. cc @MaterializeInc/testing for thoughts.

maddyblue · 2023-12-14T05:10:47Z

The restart test is failing in another test in the file because

$ docker volume rm --force restart_mzdata
--
  | Error response from daemon: remove restart_mzdata: volume is in use - [76a26dde16d3d6561f4829e78674f07de904cbc6a5c330e8cbb2c643b076f28d]

Retrying didn't fix it and I'm unable to reproduce locally. Any ideas?

nrainer-materialize · 2023-12-14T08:57:04Z

The restart test is failing in another test in the file because
$ docker volume rm --force restart_mzdata
--
  | Error response from daemon: remove restart_mzdata: volume is in use - [76a26dde16d3d6561f4829e78674f07de904cbc6a5c330e8cbb2c643b076f28d]
Retrying didn't fix it and I'm unable to reproduce locally. Any ideas?

The full stack trace is:

Error response from daemon: remove restart_mzdata: volume is in use - [76a26dde16d3d6561f4829e78674f07de904cbc6a5c330e8cbb2c643b076f28d]
--- mzcompose: test case stash failed: <class 'subprocess.CalledProcessError'>: Command '['docker', 'volume', 'rm', '--force', 'restart_mzdata']' returned non-zero exit status 1.
Traceback (most recent call last):
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/misc/python/materialize/mzcompose/composition.py", line 475, in test_case
    yield
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/test/restart/mzcompose.py", line 433, in workflow_default
    c.workflow(name)
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/misc/python/materialize/mzcompose/composition.py", line 371, in workflow
    func(self)
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/test/restart/mzcompose.py", line 185, in workflow_stash
    c.rm_volumes("mzdata", force=True)
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/misc/python/materialize/mzcompose/composition.py", line 939, in rm_volumes
    spawn.runv(
  File "/var/lib/buildkite-agent/builds/buildkite-15f2293-i-01becf557c126887b-1/materialize/tests/misc/python/materialize/spawn.py", line 72, in runv
    return subprocess.run(

test/restart/mzcompose.py:433 just triggers the workflow.
test/restart/mzcompose.py:185 is c.rm_volumes("mzdata", force=True) in workflow_stash.

@philip-stoev, have you seen this before? Any ideas?

maddyblue · 2023-12-14T16:52:40Z

What happens with indexes on a materialized view that has a custom compaction window? Does the compaction window of the index inherit the MV's window?

Good point! This is a thing we decided on in the read holds design

materialize/doc/developer/design/20231102_read_holds.md

Line 39 in 633db6e

    
           An `AS OF` query will restrict the indexes or object it considers to those that are valid for the requested time.

And it needs to be tested and probably implemented here.

maddyblue · 2023-12-14T17:41:34Z

The docker ps -a log shows restart-testdrive_no_reset-1 is still running. Could that be related?

maddyblue · 2023-12-14T22:04:46Z

What happens with indexes on a materialized view that has a custom compaction window?

Did some testing and adding an index seems to disable the compaction. Working on a fix.

maddyblue · 2023-12-16T05:51:53Z

We decided that indexes breaking this is in spec for now. Tests were added to assert that behavior. I'm going to merge this once I figure out the test failure.

Adding indexes currently breaks this, but we've decided that's in spec for now.

maddyblue · 2023-12-17T01:56:00Z

@benesch Merging is blocked until your change request is resolved.

benesch · 2023-12-17T02:12:13Z

FYI you can dismiss stale reviews here!

SQL council has approved syntax

maddyblue · 2023-12-17T03:54:09Z

TIL! Thanks.

maddyblue requested a review from a team as a code owner December 9, 2023 21:53

maddyblue requested review from a team and jkosh44 December 9, 2023 21:53

jkosh44 approved these changes Dec 10, 2023

View reviewed changes

benesch previously requested changes Dec 10, 2023

View reviewed changes

dehume mentioned this pull request Dec 11, 2023

Add COMPACTION WINDOW Materialized Views MaterializeInc/terraform-provider-materialize#404

Open

maddyblue force-pushed the mv-lcw branch from 31ccc6a to b59f6c7 Compare December 11, 2023 23:34

maddyblue changed the title ~~sql: COMPACTION WINDOW for materialized views~~ sql: RETAIN HISTORY for materialized views Dec 11, 2023

maddyblue force-pushed the mv-lcw branch from b59f6c7 to 15e6312 Compare December 12, 2023 02:02

benesch reviewed Dec 12, 2023

View reviewed changes

dehume mentioned this pull request Dec 12, 2023

Add RETAIN HISTORY Materialized Views MaterializeInc/terraform-provider-materialize#409

Open

maddyblue force-pushed the mv-lcw branch from 15e6312 to 8d02f66 Compare December 13, 2023 07:34

maddyblue requested a review from a team December 13, 2023 07:34

benesch reviewed Dec 13, 2023

View reviewed changes

jkosh44 approved these changes Dec 13, 2023

View reviewed changes

maddyblue force-pushed the mv-lcw branch from 8d02f66 to 15321ff Compare December 14, 2023 02:15

maddyblue requested a review from a team December 14, 2023 02:15

maddyblue requested a review from a team as a code owner December 14, 2023 02:15

maddyblue force-pushed the mv-lcw branch 4 times, most recently from d14d5bf to 86ee2ca Compare December 14, 2023 04:45

maddyblue commented Dec 14, 2023

View reviewed changes

maddyblue force-pushed the mv-lcw branch from 86ee2ca to e6f8ee0 Compare December 14, 2023 16:52

maddyblue force-pushed the mv-lcw branch from e6f8ee0 to 2c2dbfb Compare December 16, 2023 05:50

sql: RETAIN HISTORY in CREATE MATERIALIZED VIEW

fae8081

Adding indexes currently breaks this, but we've decided that's in spec for now.

maddyblue force-pushed the mv-lcw branch from 2c2dbfb to fae8081 Compare December 17, 2023 00:47

maddyblue enabled auto-merge December 17, 2023 01:55

maddyblue merged commit fb26da5 into MaterializeInc:main Dec 17, 2023
63 checks passed

maddyblue deleted the mv-lcw branch December 17, 2023 02:14

nrainer-materialize mentioned this pull request Jan 16, 2024

tests: retain history feature #24344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: RETAIN HISTORY for materialized views #23788

sql: RETAIN HISTORY for materialized views #23788

maddyblue commented Dec 9, 2023 •

edited

Loading

frankmcsherry commented Dec 9, 2023

jkosh44 left a comment •

edited

Loading

jkosh44 Dec 10, 2023

maddyblue Dec 11, 2023

benesch left a comment

maddyblue commented Dec 11, 2023

benesch left a comment

benesch Dec 12, 2023

maddyblue Dec 12, 2023

maddyblue Dec 12, 2023

benesch Dec 12, 2023

maddyblue commented Dec 12, 2023

maddyblue commented Dec 12, 2023

maddyblue commented Dec 13, 2023

benesch left a comment

ggevay commented Dec 13, 2023

jkosh44 Dec 13, 2023

maddyblue Dec 13, 2023

maddyblue Dec 14, 2023

benesch Dec 17, 2023

maddyblue commented Dec 14, 2023

nrainer-materialize commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 16, 2023 •

edited

Loading

maddyblue commented Dec 17, 2023

benesch commented Dec 17, 2023

maddyblue commented Dec 17, 2023

		@@ -29,6 +32,44 @@
		]


		def workflow_retain_history(c: Composition) -> None:

sql: RETAIN HISTORY for materialized views #23788

sql: RETAIN HISTORY for materialized views #23788

Conversation

maddyblue commented Dec 9, 2023 • edited Loading

Motivation

Checklist

frankmcsherry commented Dec 9, 2023

jkosh44 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

maddyblue commented Dec 11, 2023

benesch 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

maddyblue commented Dec 12, 2023

maddyblue commented Dec 12, 2023

maddyblue commented Dec 13, 2023

benesch left a comment

Choose a reason for hiding this comment

ggevay commented Dec 13, 2023

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

maddyblue commented Dec 14, 2023

nrainer-materialize commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 14, 2023

maddyblue commented Dec 16, 2023 • edited Loading

maddyblue commented Dec 17, 2023

benesch commented Dec 17, 2023

maddyblue commented Dec 17, 2023

maddyblue commented Dec 9, 2023 •

edited

Loading

jkosh44 left a comment •

edited

Loading

maddyblue commented Dec 16, 2023 •

edited

Loading