New connector migration logic to support SQL moving to extension #1132

jgpruitt · 2022-02-09T19:46:29Z

Description

Make the extension required and delegate migrations to the extension

This PR makes the promscale extension required, while the timescaledb extension remains optional.
The minimum timescaledb version supported is now 2.6.0

The extension now handles applying database migrations. The connector's migration logic was updated accordingly.

If this is a fresh install, the connector will

install the extension at the latest version

If promscale is installed at a database version prior to 0.5.0, the connector will

apply any pending migrations from the old way of doing migrations
drop the old version of the extension if it exists
install the extension at 0.0.0 which will do some "takeover" logic
update the extension to 0.5.0 (or whatever the latest version of the extension is)

If promscale is installed at database version 0.5.0 or greater, the connector will

update the extension to 0.5.0 (or whatever the latest version of the extension is)

Merge requirements

Please take into account the following non-code changes that you may need to make with your PR:

CHANGELOG entry for user-facing changes
Updated the relevant documentation

cevian

Good progress. Some comments.

The error need more context because Golang is stupid in that it doesn't output stack traces with errors. I put in some examples, but there are more places in the code to fix

pkg/pgmodel/common/extension/extension.go

pkg/pgmodel/new_migrate.go

pkg/runner/client.go

pkg/tests/end_to_end_tests/main_test.go

cevian · 2022-02-10T21:13:04Z

pkg/tests/end_to_end_tests/migrate_test.go

-		if err == nil {
-			t.Errorf("Expected error in CheckDependencies")
-		}
+		/*


hmm this seems like a key part of the test

Ping on this

pkg/pgmodel/new_migrate.go

JamesGuthrie · 2022-02-14T15:26:37Z

Also, can you rebase against current master, or merge master into extension-refactor, so that we can get this change: #1139

jgpruitt · 2022-02-14T20:54:26Z

Also, can you rebase against current master, or merge master into extension-refactor, so that we can get this change: #1139

Done. Rebased extension-refactor on master and then this branch on it.

JamesGuthrie · 2022-02-15T10:43:55Z

There's a minor change that we need to make with how Promscale handles schema generation. Here's the patch that I've been using to test it. It also adds some logging around the install/upgrade process which I found helpful to understand what's going on.

diff --git a/pkg/pgmodel/common/extension/extension.go b/pkg/pgmodel/common/extension/extension.go
index 3c2beec..0be24cf 100644
--- a/pkg/pgmodel/common/extension/extension.go
+++ b/pkg/pgmodel/common/extension/extension.go
@@ -202,9 +202,18 @@ func MigrateExtension(conn *pgx.Conn, extName string, extSchemaName string, vali
 	}
 
 	if !isInstalled {
-		_, extErr := conn.Exec(context.Background(),
-			fmt.Sprintf("CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s VERSION '%s'",
-				extName, extSchemaName, getSqlVersion(*newVersion, extName)))
+		var query = ""
+		switch extName {
+		case "timescaledb":
+			query = fmt.Sprintf("CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s VERSION '%s'",
+				extName, extSchemaName, getSqlVersion(*newVersion, extName))
+		case "promscale":
+			query = fmt.Sprintf("CREATE EXTENSION IF NOT EXISTS %s VERSION '%s'",
+				extName, getSqlVersion(*newVersion, extName))
+		default:
+			return fmt.Errorf("unknown extension: %s", extName)
+		}
+		_, extErr := conn.Exec(context.Background(), query)
 		if extErr != nil {
 			return extErr
 		}
diff --git a/pkg/pgmodel/new_migrate.go b/pkg/pgmodel/new_migrate.go
index ec3eaa4..af945f5 100644
--- a/pkg/pgmodel/new_migrate.go
+++ b/pkg/pgmodel/new_migrate.go
@@ -46,6 +46,7 @@ func removeOldExtensionIfExists(db *pgx.Conn) error {
 	}
 
 	if installed && installedVersion.LT(semver.MustParse(transition)) {
+		log.Info("msg", "Dropping extension at version '"+installedVersion.String()+"'")
 		_, err := db.Exec(
 			context.Background(),
 			stmt,
@@ -59,6 +60,7 @@ func removeOldExtensionIfExists(db *pgx.Conn) error {
 }
 
 func installExtensionAllBalls(db *pgx.Conn) error {
+	log.Info("msg", "Installing extension at version '0.0.0'")
 	const stmt = "CREATE EXTENSION promscale SCHEMA _prom_ext VERSION '0.0.0'"
 	_, err := db.Exec(
 		context.Background(),

cevian

Mostly looks good, but there are a few issues.

cevian · 2022-03-07T22:15:11Z

pkg/internal/testhelpers/postgres_container.go

-		image = "timescaledev/timescaledb:nightly-" + PGTag
-	}
-	return image, nil
+	return "timescaledev/promscale-extension:0.5.0-ts2-pg14", nil


Note this will have to wait for my changes in PR #1199

pkg/pgmodel/new_migrate.go

cevian · 2022-03-07T22:28:27Z

pkg/tests/end_to_end_tests/functions_test.go

-		if searchPath != expected {
-			t.Errorf("incorrect search path\nexpected\n\t%s\nfound\n\t%s", expected, searchPath)
-		}
+		// TODO (james): I'm not sure that we should be removing the search path modification, I've started a discussion.


what I think: I don't think we should be removing search path modifications but they should also probably NOT be done in the extension. Rather they should still be done by the connector.

Not sure about this.

Although our tests should mostly test without search_path modifications

ok to leave for now. Tracked here: https://github.com/timescale/promscale/issues/1256

cevian · 2022-03-07T22:29:11Z

pkg/tests/end_to_end_tests/migrate_test.go

-		if err == nil {
-			t.Errorf("Expected error in CheckDependencies")
-		}
+		/*


Ping on this

cevian · 2022-03-07T22:36:21Z

pkg/tests/end_to_end_tests/create_test.go

-		if _, err := db.Exec(context.Background(), "SELECT prom_api.register_metric_view('prom_data', 'metric_view_in_data_schema')"); err == nil {
-			t.Fatal("Should not be able to register a metric view in data schema")
-		}
+		// TODO (james): It doesn't seem as though what this test is testing is still valid.


why is this not a valid test? Even a superuser shouldn't be able to do this...

pkg/pgmodel/common/extension/extension.go

cevian · 2022-03-07T22:50:35Z

.github/workflows/go.yml

-        - {name: "W/O Promscale Extension",  shortname: "wo-prom-ext",  ext: false, tsdb: true,  tsdb2: true,  tsdboss: false, multi: false, pg: 14}
-        - {name: "Plain Postgres",           shortname: "plain-pg",     ext: false, tsdb: false, tsdb2: false, tsdboss: false, multi: false, pg: 14}
-        - {name: "TimescaleDB-OSS",          shortname: "tsdb-oss",     ext: false, tsdb: true,  tsdb2: true,  tsdboss: true,  multi: false, pg: 14}
+        - {name: "Plain Postgres (12)",      shortname: "plain-pg-12",  ext: true, tsdb: false, tsdb2: false, tsdboss: false, multi: false, pg: 12}


wait pending #1199

cevian · 2022-03-07T22:59:31Z

pkg/runner/client.go

-		return err
-	}
-	return pgmodel.CheckSchemaVersion(ctx, conn, appVersion, false)
+	return util.GetSharedLease(ctx, conn, schema.LockID)


shouldn't we call extension.CheckVersions here?

I was thinking it wasn't necessary since we check on startup and then hold a schema lease lock, but I guess that might not be foolproof.

addressed in 089d60e

cevian

Looks much better only a few comments

pkg/pgmodel/new_migrate.go

cevian · 2022-03-11T21:30:25Z

pkg/runner/client.go

-		return err
-	}
-	return pgmodel.CheckSchemaVersion(ctx, conn, appVersion, false)
+	return util.GetSharedLease(ctx, conn, schema.LockID)


cevian · 2022-03-11T21:30:57Z

pkg/tests/end_to_end_tests/create_test.go

-		if _, err := db.Exec(context.Background(), "SELECT prom_api.register_metric_view('prom_data', 'metric_view_in_data_schema')"); err == nil {
-			t.Fatal("Should not be able to register a metric view in data schema")
-		}
+		// TODO (james): It doesn't seem as though what this test is testing is still valid.


cevian · 2022-03-11T21:31:34Z

pkg/tests/end_to_end_tests/functions_test.go

-		if searchPath != expected {
-			t.Errorf("incorrect search path\nexpected\n\t%s\nfound\n\t%s", expected, searchPath)
-		}
+		// TODO (james): I'm not sure that we should be removing the search path modification, I've started a discussion.


pkg/tests/end_to_end_tests/migrate_test.go

pkg/tests/upgrade_tests/shapshot.go

cevian · 2022-03-11T21:45:28Z

pkg/tests/upgrade_tests/upgrade_test.go

@@ -75,13 +75,40 @@ func getDBImages(extensionState testhelpers.ExtensionState) (prev string, clean
 		//but migration code that works in an older PG version should generally work in a newer one.
 		panic("Only use pg12 for upgrade tests")
 	}
-	return "timescaledev/promscale-extension:0.1.1-ts2-pg12", testhelpers.LatestDBWithPromscaleImageBase + ":latest-ts2-pg12"
+	return "timescaledev/promscale-extension:0.1.2-ts2-pg13", testhelpers.LatestDBWithPromscaleImageBase + ":latest-ts2-pg13"
+	// using pg13 until we deal with the lack of "trusted" support in pg12


just a note to block merge on this.

Tracked in #1257

pkg/tests/upgrade_tests/upgrade_test.go

cevian · 2022-03-11T21:48:51Z

pkg/tests/upgrade_tests/upgrade_test.go

@@ -425,7 +464,7 @@ func withNewDBAtCurrentVersion(t testing.TB, DBName string, extensionState testh
 		addNode2(t, *testDatabase)
 	}
 	defer func() { _ = closer.Close() }()
-	connectURL := testhelpers.PgConnectURL(*testDatabase, testhelpers.NoSuperuser)
+	connectURL := testhelpers.PgConnectURL(*testDatabase, testhelpers.Superuser)


why do we need this change?

addressed in 5ce1a3c

This is back to Superuser now?

pkg/tests/end_to_end_tests/main_test.go

pkg/pgmodel/common/extension/extension.go

pkg/version/version.go

pkg/internal/testhelpers/postgres_container.go

pkg/pgmodel/common/extension/extension.go

pkg/tests/end_to_end_tests/migrate_test.go

JamesGuthrie · 2022-03-17T16:08:24Z

Can we also squash the commits down?

pkg/pgmodel/new_migrate.go

cevian · 2022-03-18T16:03:04Z

pkg/tests/end_to_end_tests/functions_test.go

-		if searchPath != expected {
-			t.Errorf("incorrect search path\nexpected\n\t%s\nfound\n\t%s", expected, searchPath)
-		}
+		// TODO (james): I'm not sure that we should be removing the search path modification, I've started a discussion.


ok to leave for now. Tracked here: https://github.com/timescale/promscale/issues/1256

cevian · 2022-03-18T16:43:06Z

pkg/tests/upgrade_tests/upgrade_test.go

@@ -75,13 +75,40 @@ func getDBImages(extensionState testhelpers.ExtensionState) (prev string, clean
 		//but migration code that works in an older PG version should generally work in a newer one.
 		panic("Only use pg12 for upgrade tests")
 	}
-	return "timescaledev/promscale-extension:0.1.1-ts2-pg12", testhelpers.LatestDBWithPromscaleImageBase + ":latest-ts2-pg12"
+	return "timescaledev/promscale-extension:0.1.2-ts2-pg13", testhelpers.LatestDBWithPromscaleImageBase + ":latest-ts2-pg13"
+	// using pg13 until we deal with the lack of "trusted" support in pg12


Tracked in #1257

cevian · 2022-03-18T16:48:53Z

pkg/tests/upgrade_tests/upgrade_test.go

@@ -425,7 +464,7 @@ func withNewDBAtCurrentVersion(t testing.TB, DBName string, extensionState testh
 		addNode2(t, *testDatabase)
 	}
 	defer func() { _ = closer.Close() }()
-	connectURL := testhelpers.PgConnectURL(*testDatabase, testhelpers.NoSuperuser)
+	connectURL := testhelpers.PgConnectURL(*testDatabase, testhelpers.Superuser)


This is back to Superuser now?

This PR makes the promscale extension required, while the timescaledb extension remains optional. The minimum timescaledb version supported is now 2.6.0 The extension now handles applying database migrations. The connector's migration logic was updated accordingly. If this is a fresh install, the connector will 1. install the extension at the latest version If promscale is installed at a database version prior to 0.5.0, the connector will 1. apply any pending migrations from the old way of doing migrations 2. drop the old version of the extension if it exists 3. install the extension at 0.0.0 which will do some "takeover" logic 4. update the extension to 0.5.0 (or whatever the latest version of the extension is) If promscale is installed at database version 0.5.0 or greater, the connector will 1. update the extension to 0.5.0 (or whatever the latest version of the extension is) Co-authored-by: Matvey Arye <mat@timescale.com> Co-authored-by: James Guthrie <jguthrie@timescale.com>

The main goal of this change (which is probably too large) is to ensure that we can build the promscale connector against a version of the promscale extension which contains all of the necessary bits. This change is motivated by the fact that there is now a hard dependency on the extension, and we need to be able to test in-progress changes across repositories. The basic mechanism is that for each open PR in the promscale_extension repo, there will be a docker image built and pushed to the registry at: `ghcr.io/timescale/dev_promscale_extension:<special-tag-here>`. `<special-tag-here>` is constructed from the branch name from which the PR originated. When a PR is opened in the promscale repository, we look to see if there is already a docker image corresponding to the branch name. If not, we fall back to the most recent docker image which was built on the `develop` branch in the extension repository. `<special-tag-here>` has the basic form of: `<extension-version>-<timescale-major>-<pg-version>` Some examples are: - `jg-ha-dockerfile-ts2-pg14` - `develop-ts2-pg14` - `0.5.0-ts2-pg14` Local development is supported through two mechanisms: 1. When running e.g. `make docker-image-14` in the promscale_extension repo, the docker image is tagged as: `local/dev_promscale_extension` with the `<extension-version>` component of the tag set to `head`. 2. When running `make test` in the promscale repo, it first looks to see if a local image is available matching the `local/` prefix, then checks for a docker image derived from the current branch name, and then for built from the `develop` branch. A number of things have changed tangentially as well: - end_to_end tests now take the docker image which they should run against as an argument instead of using some magic to derive it - `make test` was split into two components: `make unit` and `make e2e` - multinode tests are being skipped because we broke them in the develop branch of the promscale extension

Addressed requests

jgpruitt self-assigned this Feb 9, 2022

jgpruitt added the Feature New features label Feb 9, 2022

jgpruitt requested review from cevian and removed request for cevian February 9, 2022 19:46

jgpruitt changed the title ~~reorganizing a bit to prep for refactor~~ New migration logic Feb 9, 2022

jgpruitt mentioned this pull request Feb 9, 2022

New migration logic to use extension #1110

Closed

2 tasks

jgpruitt requested review from cevian, JamesGuthrie and bitwalker February 10, 2022 17:04

cevian suggested changes Feb 10, 2022

View reviewed changes

JamesGuthrie suggested changes Feb 14, 2022

View reviewed changes

pkg/pgmodel/new_migrate.go Outdated Show resolved Hide resolved

JamesGuthrie removed the request for review from bitwalker February 14, 2022 15:26

jgpruitt force-pushed the jp/migration-refactor3 branch from 49d90e1 to 7ef3aab Compare February 14, 2022 20:53

JamesGuthrie force-pushed the jp/migration-refactor3 branch from 623a8d3 to 2ebf73c Compare February 25, 2022 16:45

cevian suggested changes Mar 7, 2022

View reviewed changes

JamesGuthrie force-pushed the jp/migration-refactor3 branch from 0e4dd38 to b302598 Compare March 8, 2022 17:10

jgpruitt force-pushed the jp/migration-refactor3 branch from 3be649a to de85699 Compare March 9, 2022 19:59

cevian added this to the 0.11.0 milestone Mar 11, 2022

jgpruitt force-pushed the jp/migration-refactor3 branch from fc101c2 to 3081dcd Compare March 11, 2022 16:57

cevian changed the title ~~New migration logic~~ New connector migration logic to support SQL moving to extension Mar 11, 2022

jgpruitt force-pushed the jp/migration-refactor3 branch from 3081dcd to 3e83144 Compare March 11, 2022 17:31

jgpruitt changed the base branch from extension-refactor to master March 11, 2022 17:32

jgpruitt force-pushed the jp/migration-refactor3 branch 3 times, most recently from aed2de5 to 5a87893 Compare March 11, 2022 18:43

cevian suggested changes Mar 11, 2022

View reviewed changes

jgpruitt mentioned this pull request Mar 11, 2022

Add unit test(s) for extension.CheckVersions #1236

Closed

jgpruitt mentioned this pull request Mar 11, 2022

Handle _ps_catalog.migration better in Upgrade tests #1237

Closed

jgpruitt requested review from cevian and JamesGuthrie March 15, 2022 13:42

JamesGuthrie previously requested changes Mar 17, 2022

View reviewed changes

jgpruitt force-pushed the jp/migration-refactor3 branch 2 times, most recently from 09ffc49 to 923826d Compare March 17, 2022 19:46

jgpruitt requested a review from JamesGuthrie March 17, 2022 19:46

jgpruitt force-pushed the jp/migration-refactor3 branch from d3aa16d to a3ba1f9 Compare March 17, 2022 21:58

jgpruitt marked this pull request as ready for review March 18, 2022 13:48

jgpruitt requested review from antekresic and niksajakovljevic as code owners March 18, 2022 13:48

cevian approved these changes Mar 18, 2022

View reviewed changes

jgpruitt requested a review from a team as a code owner March 18, 2022 18:44

jgpruitt and others added 2 commits March 18, 2022 13:52

jgpruitt force-pushed the jp/migration-refactor3 branch from d7c76e8 to 66096c3 Compare March 18, 2022 18:52

sumerman approved these changes Mar 18, 2022

View reviewed changes

jgpruitt enabled auto-merge (rebase) March 18, 2022 20:05

jgpruitt merged commit df8041a into master Mar 18, 2022

jgpruitt deleted the jp/migration-refactor3 branch March 18, 2022 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New connector migration logic to support SQL moving to extension #1132

New connector migration logic to support SQL moving to extension #1132

jgpruitt commented Feb 9, 2022 •

edited

cevian left a comment

cevian Feb 10, 2022

cevian Mar 7, 2022

JamesGuthrie commented Feb 14, 2022

jgpruitt commented Feb 14, 2022

JamesGuthrie commented Feb 15, 2022

cevian left a comment

cevian Mar 7, 2022

cevian Mar 7, 2022

cevian Mar 7, 2022

cevian Mar 11, 2022

cevian Mar 18, 2022

cevian Mar 7, 2022

cevian Mar 7, 2022

cevian Mar 11, 2022

cevian Mar 7, 2022

cevian Mar 7, 2022

cevian Mar 11, 2022

jgpruitt Mar 14, 2022

jgpruitt Mar 14, 2022

cevian left a comment

cevian Mar 11, 2022

cevian Mar 11, 2022

cevian Mar 11, 2022

cevian Mar 11, 2022

cevian Mar 18, 2022

cevian Mar 11, 2022

jgpruitt Mar 11, 2022

cevian Mar 18, 2022

JamesGuthrie commented Mar 17, 2022

cevian Mar 18, 2022

cevian Mar 18, 2022

cevian Mar 18, 2022

New connector migration logic to support SQL moving to extension #1132

New connector migration logic to support SQL moving to extension #1132

Conversation

jgpruitt commented Feb 9, 2022 • edited

Description

Merge requirements

cevian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGuthrie commented Feb 14, 2022

jgpruitt commented Feb 14, 2022

JamesGuthrie commented Feb 15, 2022

cevian 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

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

cevian 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesGuthrie commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgpruitt commented Feb 9, 2022 •

edited