feat(pg): Faster migrations #7819

yevgenypats · 2023-02-08T13:54:22Z

This streamlines migrations, introduce force option and in general should speed up migrations substantially as we only do now one query instead of query per table.

Blocked by another take in SDK - cloudquery/plugin-sdk#688

plugins/destination/postgresql/client/client.go

This adds diff methods for `Table`. This will help simplify migrations so we can transform the destination/database into `Table` struct and then understand what was changed/added in the same way across all destination plugins. PostgreSQL PR example - cloudquery/cloudquery#7819

candiduslynx

I sense that there are issues with the append mode now (_cq_id was used as PK there).

plugins/destination/postgresql/client/write.go

plugins/destination/postgresql/client/types.go

plugins/destination/postgresql/client/migrate.go

hermanschaaf · 2023-02-09T09:34:44Z

plugins/destination/postgresql/client/migrate.go

-		sql := "alter table " + pgx.Identifier{table.Name}.Sanitize() + " alter column " + pgx.Identifier{col}.Sanitize() + " set not null"
-		if _, err := c.conn.Exec(ctx, sql); err != nil {
-			return fmt.Errorf("failed to set not null on column %s on table %s: %w", col, table.Name, err)
+	if c.enabledPks() && !table.IsPrimaryKeyEqual(pgTable) {


I think we may want to move this check to be above the GetAddedColumns check, so that no changes are made to the table if migrate-forced is false and a forced migration is necessary. Otherwise the table can be left in a half-migrated state. (Either that or move the added columns first, so that the user only needs to manually do the forced operations.)

why it's half baked? but I can't move it up because potentially the primary keys are on the columns that were just added.

ok yeah you are right. I now moved that check up to fail early.

hermanschaaf · 2023-02-09T09:44:41Z

plugins/destination/postgresql/client/types.go

+	switch t {
+	case "boolean":
+		return schema.TypeBool
+	case "bigint", "integer":


Interesting, I guess there is some information lost in the conversion here because our types don't map 1:1 to postgres 🤔 So if you have a source table and write it back to postgres, the schemas won't match exactly (integers will become bigints). In this case it's fine, but hopefully we don't run into irreconcilable cases in other databases...

Good catch. I guess we need to transform the incoming tables to pgtables so we can compare them?

Yeah I think in this case it's fine until we add BigInt type to our type system but if the information loss is the otherway around then we will need to transform first the Table -> DbTable and only then compare what we got from database table.

plugins/destination/postgresql/client/migrate.go

hermanschaaf

It seems like the query to select all tables is not accounting for tables with composite PKs in the way we expect

For each test in the destination testing suite we use unique table now. This should solve the bug I've hit this PR - cloudquery/cloudquery#7819

Co-authored-by: Erez Rokah <[email protected]>

Migration detection APIs take 2. - This creates **one** API that returns all the changes instead of multiple growing APIs for different set of changes. - Improves Migration tests for different use-cases This will come up also with a Postgres PR and documentation PR: Basically each destination plugin would have the following table: Migrations Logic | Change | Behaviour | |----------|----------------------------------| | Add Column | Auto Migrate/Drop Table | | Add Column with Not Null | Auto Migrate/Drop Table | | Remove Column | Auto Migrate/Drop Table | | Remove Column with Not Null | Auto Migrate/Drop Table | | Change Column | Auto Migrate/Drop Table | and we can easily expand it in the code, tests and documentations. Example of usage in cloudquery/cloudquery#7819

plugins/destination/postgresql/client/migrate.go

Co-authored-by: candiduslynx <[email protected]>

erezrokah

I don't see anything blocking 🚀 I'm still testing, but if you'd like to merge I think that's fine and we can follow up with fixes if we have them

plugins/destination/postgresql/client/migrate.go

erezrokah

Sorry, found a bug

plugins/destination/postgresql/client/migrate.go

Co-authored-by: Erez Rokah <[email protected]>

hermanschaaf · 2023-02-16T14:48:29Z

plugins/destination/postgresql/client/types.go

+		return schema.TypeByteArray
+	case "text[]":
+		return schema.TypeStringArray
+	case "timestamp without time zone":


This will only become a problem with CDC, but at some point we'll also need to support (or at least handle without panicking?) all the other postgres types, like timestamptz.

🤖 I have created a release *beep* *boop* --- ## [2.1.0](plugins-destination-postgresql-v2.0.10...plugins-destination-postgresql-v2.1.0) (2023-02-16) ### Features * **pg:** Faster migrations ([#7819](#7819)) ([8f51733](8f51733)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-sdk to v1.36.0 ([#7809](#7809)) ([c85a9cb](c85a9cb)) * Postgresql timestamps ([#7840](#7840)) ([e2c8b61](e2c8b61)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

#### Summary Same as #7819 for SQLite <!--

#### Summary Same as #7819 for MSSQL. This is a breaking change due to the type change from `datetimeoffset` to `datetime2` as we don't want to save time zone offset, see https://github.com/cloudquery/cloudquery/blob/ee4b2c01867b2140537ee4656d943d2e0cf2898b/plugins/destination/postgresql/client/types.go#L74 I suggest following the code in an IDE as it's hard to understand the changes from the diff. BEGIN_COMMIT_OVERRIDE feat(deps): Update MSSQL plugin-sdk to v1.38.2 BREAKING-CHANGE: Update MSSQL plugin-sdk to v1.38.2. You'll need to drop the database and start fresh due to a change in the schema for all timestamp columns from `datetimeoffset` to `datetime2`. END_COMMIT_OVERRIDE <!--

cq-bot added the postgresql label Feb 8, 2023

yevgenypats mentioned this pull request Feb 8, 2023

feat: Add table diff methods cloudquery/plugin-sdk#668

Merged

yevgenypats marked this pull request as ready for review February 8, 2023 13:54

disq reviewed Feb 8, 2023

View reviewed changes

plugins/destination/postgresql/client/client.go Outdated Show resolved Hide resolved

vercel bot deployed to Preview February 8, 2023 17:49 View deployment

candiduslynx mentioned this pull request Feb 9, 2023

feat(clickhouse): ClickHouse destination #6982

Merged

candiduslynx suggested changes Feb 9, 2023

View reviewed changes

hermanschaaf reviewed Feb 9, 2023

View reviewed changes

plugins/destination/postgresql/client/migrate.go Outdated Show resolved Hide resolved

hermanschaaf requested changes Feb 9, 2023

View reviewed changes

yevgenypats added 9 commits February 11, 2023 11:28

feat(pg): Faster migrations

00258df

update to released sdk

ab8b8ec

more fixes

8af27e8

fix lint

8018fcc

why

58c5e54

fix tests

b91e2d0

fmt

659178c

more fixes

5568818

more fixes

722d997

yevgenypats force-pushed the feat/pg_better_migrations branch from d769e8a to 722d997 Compare February 11, 2023 09:29

update go mod

86025f5

yevgenypats requested review from candiduslynx and hermanschaaf February 12, 2023 12:13

fmt

9c533fd

yevgenypats mentioned this pull request Feb 12, 2023

fix: Potential database lock/race in destination testing cloudquery/plugin-sdk#678

Merged

yevgenypats and others added 2 commits February 12, 2023 16:32

update sdk

ed0402f

Merge branch 'main' into feat/pg_better_migrations

f9c27a6

yevgenypats and others added 4 commits February 14, 2023 15:02

Update plugins/destination/postgresql/client/migrate.go

e78cbc7

Co-authored-by: Erez Rokah <[email protected]>

fix review

19f07b1

add early return

b16511f

update to new sdk

0e7bf79

yevgenypats mentioned this pull request Feb 15, 2023

feat: Improve migration detection APIs cloudquery/plugin-sdk#688

Merged

yevgenypats requested a review from erezrokah February 15, 2023 20:47

yevgenypats added 2 commits February 16, 2023 14:01

fix tests

d65c57f

fix cockroach

416cd26

candiduslynx reviewed Feb 16, 2023

View reviewed changes

plugins/destination/postgresql/client/migrate.go Outdated Show resolved Hide resolved

Update plugins/destination/postgresql/client/migrate.go

172ece0

Co-authored-by: candiduslynx <[email protected]>

erezrokah approved these changes Feb 16, 2023

View reviewed changes

erezrokah requested changes Feb 16, 2023

View reviewed changes

plugins/destination/postgresql/client/migrate.go Show resolved Hide resolved

yevgenypats and others added 2 commits February 16, 2023 16:23

upgrade sdk ot v1.38.0

92e624f

Update plugins/destination/postgresql/client/migrate.go

f5eae88

Co-authored-by: Erez Rokah <[email protected]>

yevgenypats requested a review from erezrokah February 16, 2023 14:28

erezrokah approved these changes Feb 16, 2023

View reviewed changes

yevgenypats added the automerge Automatically merge once required checks pass label Feb 16, 2023

hermanschaaf approved these changes Feb 16, 2023

View reviewed changes

Merge branch 'main' into feat/pg_better_migrations

8d141ff

vercel bot deployed to Preview February 16, 2023 14:46 View deployment

kodiakhq bot merged commit 8f51733 into main Feb 16, 2023

kodiakhq bot deleted the feat/pg_better_migrations branch February 16, 2023 14:47

hermanschaaf reviewed Feb 16, 2023

View reviewed changes

cq-bot mentioned this pull request Feb 16, 2023

chore(main): Release plugins-destination-postgresql v2.1.0 #7842

Merged

erezrokah mentioned this pull request Feb 16, 2023

feat(sqlite-migration): Switch to SDK methods for migrating data #8166

Merged

kodiakhq bot pushed a commit that referenced this pull request Feb 19, 2023

feat(sqlite-migration): Switch to SDK methods for migrating data (#8166)

8716f79

#### Summary Same as #7819 for SQLite <!--

erezrokah mentioned this pull request Feb 21, 2023

feat(deps)!: Update MSSQL plugin-sdk to v1.39.1 #8318

Merged

feat(pg): Faster migrations #7819

feat(pg): Faster migrations #7819

Uh oh!

Conversation

yevgenypats commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

candiduslynx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hermanschaaf Feb 9, 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!

yevgenypats Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

yevgenypats Feb 11, 2023

Choose a reason for hiding this comment

Uh oh!

hermanschaaf Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

yevgenypats Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

yevgenypats Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hermanschaaf Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yevgenypats commented Feb 8, 2023 •

edited

Loading

hermanschaaf Feb 9, 2023 •

edited

Loading