coord: Apply schema migrations to builtin tables and views #11741

jkosh44 · 2022-04-12T14:25:10Z

This commit adds the functionality to be able to migrate the schema of
builtin objects. It accomplishes this by dropping all objects that have
changed schema, along with all objects that depend on the original
object. Then it recreates all dropped objects with new GlobalIds.

Previously in the single binary world, schema migrations were
accomplished by restarting the whole system. In the platform world,
each component restarts independently which is why that approach no
longer worked.

Works towards resolving MaterializeInc/database-issues#3332

Motivation

This PR adds a known-desirable feature.

Tips for reviewer

The content of Catalog::transact for Action::DropItem was moved entirely to CatalogState::drop_item. The one change is that if !id.is_system() was added to

            if !id.is_system() {
                assert!(
                    self.compute_instances_by_id
                        .get_mut(&compute_instance)
                        .unwrap()
                        .exports
                        .remove(&id),
                    "catalog out of sync"
                );
            }

because introspection source indexes are not included in exports.

Testing

This PR has adequate test coverage / QA involvement has been duly considered.
- Manual testing is not possible until command reconciliation is complete. I also don't think we have the testing infrastructure in place to do automated testing for this feature.

Release notes

This PR includes the following user-facing behavior changes:

There are no user-facing behavior changes.

Dependencies

https://github.com/MaterializeInc/database-issues/issues/3515

jkosh44 · 2022-04-15T16:48:24Z

@mjibson This PR isn't exactly ready yet, but the changes to storage.rs are done in case you wanted to look.

jkosh44 · 2022-07-26T18:39:51Z

Idea for a test: mzcompose.py workflow that starts up some hard coded mz version that is known to need some migrations. The test asserts the ids of a couple of things, shuts it down, spins up a newer mz (this part seems brittle because recreated ids might change over time), then asserts that the ids of those same things have changed. To make this less brittle, it could assert that the recreated ids are just greater than their previous ids, but not care about what the specific ids are.

Maybe it should also include having a user created view and materiaized view over some system objects that will be migrated.

This is a good idea, but I think that since we started working on platform we haven't increased the version number. We also won't be able to upgrade from a pre-platform version to the current version. So I don't think that there exists a valid version to upgrade from to test this.

EDIT: Maybe we can teach the upgrade tests to use a specific commit hash instead of a version? 594230e added a column to mz_cluster_replicas_status and would be a good candidate.

maddyblue · 2022-07-26T18:54:25Z

Yeah, you can specify a specific docker tag with a git commit.

benesch · 2022-07-26T23:42:22Z

We're also (finally) increasing the version numbers again: v0.27.0-alpha.1, v0.27.0-alpha.2, etc.

philip-stoev

I wrote a test using the "old Git revision" approach but unfortunately it will not work out -- the revision is apparently so old that the catalog is not preserved when attempting to upgrade from it ...

I will have to defer adding a test to CI until a future specific migration happens between two tagged and supported releases. At that time I will be able to implement the "id-of-the-object-has-increased" idea.

I have also added a checkbox to the Epic around adding a test that generally exercises all builtin database objects by creating a materialized view on top of them and making sure the view is readable post-upgrade. Thanks a lot for this idea too.

jkosh44 · 2022-07-27T13:37:05Z

Thanks for the help @philip-stoev! I had an idea this morning. I can open a PR that adds one of the pg_catalog tables but omit a column. Then follow it up with a PR that adds in the missing column. We could then use those two commits for testing purposes.

jkosh44 · 2022-07-27T15:49:57Z

@philip-stoev Once these two PRs pass CI, I think if we merge them back to back it's OK that we don't have a proper migration. Then we could use those for your tests. Do you think that would work?

philip-stoev · 2022-07-27T15:58:08Z

@jkosh44 do not merge them just yet -- once their containers arrive in docker hub I will be able to give it a try without merging them to main. Please give me until tomorrow.

jkosh44 · 2022-07-27T16:07:44Z

@jkosh44 do not merge them just yet -- once their containers arrive in docker hub I will be able to give it a try without merging them to main. Please give me until tomorrow.

Awesome! No rush, I'll hold off on merging until you let me know. Also if you send me the branch, I can try experimenting.

philip-stoev · 2022-07-28T11:53:18Z

I was able to construct a test that seems to do the job. The materialized view created on top of the table that is being migrated survives the migration. Please feel free to push it if you think it is good enough:

diff --git a/test/cluster/mzcompose.py b/test/cluster/mzcompose.py
index 35d66a951..b1126b21d 100644
--- a/test/cluster/mzcompose.py
+++ b/test/cluster/mzcompose.py
@@ -8,6 +8,7 @@
 # by the Apache License, Version 2.0.
 
 import time
+from textwrap import dedent
 
 from pg8000.dbapi import ProgrammingError
 
@@ -71,6 +72,7 @@ def workflow_default(c: Composition, parser: WorkflowArgumentParser) -> None:
         "test-github-12251",
         "test-remote-storaged",
         "test-drop-default-cluster",
+        "test-schema-migration",
     ]:
         with c.test_case(name):
             c.workflow(name)
@@ -210,3 +212,54 @@ def workflow_test_drop_default_cluster(c: Composition) -> None:
 
     c.sql("DROP CLUSTER default CASCADE")
     c.sql("CREATE CLUSTER default REPLICAS (default (SIZE '1'))")
+
+
+def workflow_test_builtin_migration(c: Composition) -> None:
+    """Exercise the builtin object migration code by upgrading between two versions
+    that will have a migration triggered between them. Create a materialized view
+    over the affected builtin object to confirm that the migration was successful
+    """
+
+    c.down(destroy_volumes=True)
+    with c.override(
+        Testdrive(default_timeout="15s", no_reset=True, consistent_seed=True),
+    ):
+        c.up("testdrive", persistent=True)
+
+        with c.override(
+            Materialized(
+                image="materialize/materialized:devel-ce016efcfd04931e95a2ce6fd90431f68c84804d"
+            )
+        ):
+            c.up("materialized")
+
+            c.testdrive(
+                input=dedent(
+                    """
+            > CREATE MATERIALIZED VIEW v1 AS SELECT COUNT(*) > 0 FROM pg_authid;
+            > CREATE DEFAULT INDEX ON v1;
+        """
+                )
+            )
+
+            c.kill("materialized")
+
+        with c.override(
+            Materialized(
+                image="materialize/materialized:devel-d5328ec05cd391e6b8f16c2e4724f91a15eaa095"
+            )
+        ):
+            c.up("materialized")
+
+            c.testdrive(
+                input=dedent(
+                    """
+       > SELECT * FROM v1;
+       true
+
+       # This column is new after the migration
+       > SELECT DISTINCT rolconnlimit FROM pg_authid;
+       -1
+    """
+                )
+            )

philip-stoev · 2022-07-28T11:55:35Z

The test as pasted upgrades to materialize/materialized:devel-d5328ec05cd391e6b8f16c2e4724f91a15eaa095 but I guess it should upgrade to just Materialized() if you are going to add it to the CI. This way the test will work until the "from" image, ce016efcfd04931e95a2ce6fd90431f68c84804d becomes too old to upgrade to the latest "main" (which may be a couple of weeks from now, or sooner).

…ltin-schema � Conflicts: � src/adapter/src/coord.rs

…ltin-schema

jkosh44 · 2022-07-29T13:23:07Z

@mjibson This has a test now which is passing, if you want to re-review whenever you have time.

benesch · 2022-07-29T14:47:34Z

test/cluster/mzcompose.py

+        # "test-cluster",
+        # "test-github-12251",
+        # "test-remote-storaged",
+        # "test-drop-default-cluster",


Don't forget to comment these back in!

Oh wow, good catch.

…ltin-schema

benesch · 2022-10-11T08:44:48Z

I put this in my queue! Probably won't get to this until this weekend though, sorry Joe!

…

On Wed, Jul 20, 2022 at 1:20 PM Joseph Koshakow ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/adapter/src/coord.rs <#11741 (comment)> : > + // Migrate builtin objects + for (compute_id, sink_ids) in builtin_migration_metadata.previous_sink_ids { + self.controller + .compute_mut(compute_id) + .unwrap() + .drop_sinks_unvalidated(sink_ids) + .await?; + } + for (compute_id, index_ids) in builtin_migration_metadata.previous_index_ids { + self.controller + .compute_mut(compute_id) + .unwrap() + .drop_indexes_unvalidated(index_ids) + .await?; + } + for (compute_id, recorded_view_ids) in builtin_migration_metadata.previous_recorded_view_ids + { + self.controller + .compute_mut(compute_id) + .unwrap() + .drop_sinks_unvalidated(recorded_view_ids.clone()) + .await?; + self.controller + .storage_mut() + .drop_sources_unvalidated(recorded_view_ids) + .await?; + } + self.controller + .storage_mut() + .drop_sources_unvalidated(builtin_migration_metadata.previous_source_ids) + .await?; + I'm not really a big fan of all these "unvalidated" methods, but I'm not sure what else to do. — Reply to this email directly, view it on GitHub <#11741 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGXSIF4PAAZMCW7WUJ5KWDVVAYM3ANCNFSM5THQAXXA> . You are receiving this because you commented.Message ID: ***@***.***>

jkosh44 · 2022-10-11T14:49:29Z

@benesch Did you mean to comment on this?

benesch · 2022-10-11T14:52:49Z

GitHub seems to have found a bunch of undelivered email comments overnight and posted them. So I did mean to comment that but ... like two months ago, lol.

jkosh44 marked this pull request as draft April 12, 2022 14:25

jkosh44 force-pushed the builtin-schema branch 27 times, most recently from 86ddf90 to dce467d Compare April 15, 2022 16:15

jkosh44 requested a review from maddyblue April 15, 2022 16:47

Fix typos

47b0100

philip-stoev approved these changes Jul 27, 2022

View reviewed changes

jkosh44 added 6 commits July 28, 2022 10:25

Add test

55369ee

Add test

32a9aa2

Merge branch 'main' of github.com:MaterializeInc/materialize into bui…

0540aa6

…ltin-schema � Conflicts: � src/adapter/src/coord.rs

Clean up test

58e4f1d

Fix lint

3d8c42b

Merge branch 'main' of github.com:MaterializeInc/materialize into bui…

4f2629c

…ltin-schema

jkosh44 requested a review from maddyblue July 29, 2022 13:23

benesch reviewed Jul 29, 2022

View reviewed changes

jkosh44 added 3 commits July 29, 2022 11:07

Clean up test

1709601

Clean up comments

bd77ba1

Merge branch 'main' of github.com:MaterializeInc/materialize into bui…

1bd9ebb

…ltin-schema

maddyblue approved these changes Jul 29, 2022

View reviewed changes

jkosh44 enabled auto-merge (squash) July 29, 2022 17:34

jkosh44 merged commit 9c79808 into MaterializeInc:main Jul 29, 2022

jkosh44 deleted the builtin-schema branch July 29, 2022 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coord: Apply schema migrations to builtin tables and views #11741

coord: Apply schema migrations to builtin tables and views #11741

jkosh44 commented Apr 12, 2022 •

edited

Loading

jkosh44 commented Apr 15, 2022

jkosh44 commented Jul 26, 2022 •

edited

Loading

maddyblue commented Jul 26, 2022

benesch commented Jul 26, 2022

philip-stoev left a comment

jkosh44 commented Jul 27, 2022

jkosh44 commented Jul 27, 2022

philip-stoev commented Jul 27, 2022

jkosh44 commented Jul 27, 2022

philip-stoev commented Jul 28, 2022

philip-stoev commented Jul 28, 2022

jkosh44 commented Jul 29, 2022

benesch Jul 29, 2022

jkosh44 Jul 29, 2022

jkosh44 Jul 29, 2022

benesch commented Oct 11, 2022 via email

jkosh44 commented Oct 11, 2022

benesch commented Oct 11, 2022

coord: Apply schema migrations to builtin tables and views #11741

coord: Apply schema migrations to builtin tables and views #11741

Conversation

jkosh44 commented Apr 12, 2022 • edited Loading

Motivation

Tips for reviewer

Testing

Release notes

Dependencies

jkosh44 commented Apr 15, 2022

jkosh44 commented Jul 26, 2022 • edited Loading

maddyblue commented Jul 26, 2022

benesch commented Jul 26, 2022

philip-stoev left a comment

Choose a reason for hiding this comment

jkosh44 commented Jul 27, 2022

jkosh44 commented Jul 27, 2022

philip-stoev commented Jul 27, 2022

jkosh44 commented Jul 27, 2022

philip-stoev commented Jul 28, 2022

philip-stoev commented Jul 28, 2022

jkosh44 commented Jul 29, 2022

benesch Jul 29, 2022

Choose a reason for hiding this comment

jkosh44 Jul 29, 2022

Choose a reason for hiding this comment

jkosh44 Jul 29, 2022

Choose a reason for hiding this comment

benesch commented Oct 11, 2022 via email

jkosh44 commented Oct 11, 2022

benesch commented Oct 11, 2022

jkosh44 commented Apr 12, 2022 •

edited

Loading

jkosh44 commented Jul 26, 2022 •

edited

Loading