feat(pg): Support CockroachDB #2531

yevgenypats · 2022-10-08T21:47:33Z

Summary

CockroachDB is PostgreSQL compliant but there always some differences so this PR address those and adds it to the testing workflow.

Closes #1710

bbernays · 2022-10-08T22:34:34Z

plugins/destination/postgresql/client/types.go

+	case pgTypeCockroachDB:
+		return c.SchemaTypeToCockroach(t)
+	default:
+		return c.SchemaTypeToPg10(t)


I thought our minimum supported pg version was 11

I tested it locally, we should change our Postgresql In CI to 10. there is no feature in 11 right now that we use that is not available in 10. https://www.postgresql.org/about/featurematrix/

bbernays · 2022-10-08T22:38:46Z

Overall this looks good. My question is why add support for Cockroach DB to the PG plugin destination? Why not make a standalone plugin for CockroachDB? That way we can add any special optimizations to each plugin without worrying about breaking the other

yevgenypats · 2022-10-09T07:39:37Z

Overall this looks good. My question is why add support for Cockroach DB to the PG plugin destination? Why not make a standalone plugin for CockroachDB? That way we can add any special optimizations to each plugin without worrying about breaking the other

Our pg plugin should work with cockroachdb out of the box. The whole point of CockroachDB is that is is PG compliant. Right now we have 95% of the same code so unless the plugin will turn into 50% code for Cockroach and 50% for DB only then we should look into splitting this. It's even the same driver ('pgx').

A good rule of thumb should be driver/library per plugin .

Also this way it provides good user experience so users can use the same plugin for both and less work for us to maintain duplicate documentation and releases.

erezrokah

The code looks good, but a few comments:

~~[non-blocking] Shouldn't this be a separate destination plugin? I get that the logic is mostly the same, but maybe we can re-use the code via a shared module, etc?~~ Saw that you commented to @bbernays but I'd rather duplicate the code first and then pull the duplication into a shared library. That way we won't need to worry about breaking CockroachDB when we change Postgres and vice versa. Also we could de-couple the tests.
[non-blocking] How would the configuration for CockroachDB look like? Looks like the same and we auto detect the DB type. Maybe better to be explicit and set the DB type in the spec?
[blocking] I'd keep the minimal requirement as Postgres 11, at least until we verify all of our policies queries work on 10 (or modify them to do so). Is 10 a requirement for this PR?
[blocking] Looks like there a few differences for CockroachDB:

It can have only 1 primary key. And dropping a key needs to be via a single transaction. Do we need to handle it in

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

Line 121 in 283fca9

c.logger.Info().Str("table", table.Name).Msg("Recreating primary keys")
Maybe there are some other changes too but I'll need to test a bit more

yevgenypats · 2022-10-09T09:06:31Z

The code looks good, but a few comments:

~~[non-blocking] Shouldn't this be a separate destination plugin? I get that the logic is mostly the same, but maybe we can re-use the code via a shared module, etc?~~ Saw that you commented to @bbernays but I'd rather duplicate the code first and then pull the duplication into a shared library. That way we won't need to worry about breaking CockroachDB when we change Postgres and vice versa. Also we could de-couple the tests.

[non-blocking] How would the configuration for CockroachDB look like? Looks like the same and we auto detect the DB type. Maybe better to be explicit and set the DB type in the spec?

[blocking] I'd keep the minimal requirement as Postgres 11, at least until we verify all of our policies queries work on 10 (or modify them to do so). Is 10 a requirement for this PR?

[blocking] Looks like there a few differences for CockroachDB:

It can have only 1 primary key. And dropping a key needs to be via a single transaction. Do we need to handle it in

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

Line 121 in 283fca9

c.logger.Info().Str("table", table.Name).Msg("Recreating primary keys")

Maybe there are some other changes too but I'll need to test a bit more

1 primary key

I don't think we should separate it, pgx has the save driver and so should we. If we are breaking something in pg it means we are doing something wrong as for an insertion plugin it really should work for both.
Configuration is the same for cockroachDB as this is basically the idea behind Cockroach
I don't think we should keep minimal support at 11 because it is just artificial version. our policies are not related to data types supported at insertion time so for policies we should mention that is is minimum 11 but not for a pg destination when it is clearly working for 10. If we will see in the code that we are starting to put a lot of effort to support 10 then sure we can drop it but at the moment this works out-of-the-box
This is a PostgreSQL limitation as well it can have only one primary key (it can be composite - and the same in cockroach). The only different that you rightfully mentioned is that cockroach must have a pk and this is already solved as we have _cq_id for the exact reason when we don't have other pks. The other thing we might want to do is to alter the pk in transaction and/or use alter statement - but this is a best practices actually also for PostgreSQL so I'll push this in the PR.

yevgenypats · 2022-10-09T10:00:38Z

In anycase even if we will split in the future due to unforseen requirements/constraints I don't think we should do it now given how easy it is to support both with current constraints/requirements. So basically let's go with the easy solution first and then see if any requirements will come up that will require splitting it and much more work.

erezrokah

Assuming this doesn't break Postgres I think we can give it a go, though I would maybe keep it as a hidden feature until we can do more testing on it

erezrokah · 2022-10-09T11:47:33Z

plugins/destination/postgresql/client/migrate.go

+	return nil
+}
+
+func (c *Client) setNullOnPks(ctx context.Context, table *schema.Table) error {


Should this be called setNotNullOnPks?

Also let's say someone changes an existing column to be a primary key and it has a null value. What will happen? Will this fail and if so, how do we handle this failure?

Yes this will fail. we don't have (and will never have) a way to deal with that. The user will have to either drop the table or migrate columns to contain non-null.

auto-migrations can fail in data integrations platform so it's best effort and it is acceptable/industry standard as maintaining migrations for all databases*all_apis is not really feasible.

yevgenypats · 2022-10-09T12:00:51Z

Assuming this doesn't break Postgres I think we can give it a go, though I would maybe keep it as a hidden feature until we can do more testing on it

Thanks! I don't think this will break Postgres and I wouldn't keep it under feature flag as otherwise no one will be able to help us test it (I think just one or two users asked for it - so now we wait for people to try out so we can fix the bugs)

erezrokah · 2022-10-09T12:03:43Z

Thanks! I don't think this will break Postgres and I wouldn't keep it under feature flag as otherwise no one will be able to help us test it (I think just one or two users asked for it - so now we wait for people to try out so we can fix the bugs)

Yeah, no need for a flag, "hidden" means no documentation for it :)

🤖 I have created a release *beep* *boop* --- ## [1.1.0](plugins-destination-postgresql-v1.0.1...plugins-destination-postgresql-v1.1.0) (2022-10-09) ### Features * **pg:** Support CockroachDB ([#2531](#2531)) ([0b4c2c1](0b4c2c1)) ### Bug Fixes * **deps:** Update plugin-sdk for postgresql to v0.12.10 ([#2556](#2556)) ([571346e](571346e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

feat(pg): Support CockroachDB

e8a7fa8

yevgenypats requested review from a team, erezrokah and hermanschaaf as code owners October 8, 2022 21:47

cq-bot added the postgresql label Oct 8, 2022

yevgenypats added 2 commits October 9, 2022 00:55

try pass args to cockroach docker

fe980a9

move cockroach github action to standard docker

30af7c9

yevgenypats removed the request for review from a team October 8, 2022 22:03

yevgenypats added enhancement labels Oct 8, 2022

yevgenypats added 2 commits October 9, 2022 01:04

Add go clean -testcache before rerun

56e1a9f

update readme

56aa4d7

bbernays reviewed Oct 8, 2022

View reviewed changes

update pg to minimum 10

5199582

yevgenypats requested a review from bbernays October 9, 2022 07:52

erezrokah requested changes Oct 9, 2022

View reviewed changes

make pks change in transaction

50ac8e9

yevgenypats requested a review from erezrokah October 9, 2022 09:48

Merge branch 'main' into feat/pg/cockroachdb

8f7841a

vercel bot deployed to Preview – cloudquery-web October 9, 2022 09:50 View deployment

vercel bot deployed to Preview – cloudquery-versions October 9, 2022 09:50 View deployment

gofmt

944c590

Merge branch 'main' into feat/pg/cockroachdb

61be4b4

vercel bot deployed to Preview – cloudquery-versions October 9, 2022 10:28 View deployment

vercel bot deployed to Preview – cloudquery-web October 9, 2022 10:29 View deployment

yevgenypats added the cockroachdb label Oct 9, 2022

erezrokah approved these changes Oct 9, 2022

View reviewed changes

yevgenypats added the automerge Automatically merge once required checks pass label Oct 9, 2022

Merge branch 'main' into feat/pg/cockroachdb

7cd7b0b

vercel bot deployed to Preview – cloudquery-versions October 9, 2022 12:23 View deployment

vercel bot deployed to Preview – cloudquery-web October 9, 2022 12:24 View deployment

kodiakhq bot merged commit 0b4c2c1 into main Oct 9, 2022

kodiakhq bot deleted the feat/pg/cockroachdb branch October 9, 2022 12:25

cq-bot mentioned this pull request Oct 9, 2022

chore(main): Release plugins-destination-postgresql v1.1.0 #2566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pg): Support CockroachDB #2531

feat(pg): Support CockroachDB #2531

yevgenypats commented Oct 8, 2022

bbernays Oct 8, 2022

yevgenypats Oct 9, 2022 •

edited

bbernays commented Oct 8, 2022

yevgenypats commented Oct 9, 2022 •

edited

erezrokah left a comment •

edited

yevgenypats commented Oct 9, 2022

yevgenypats commented Oct 9, 2022 •

edited

erezrokah left a comment

erezrokah Oct 9, 2022

yevgenypats Oct 9, 2022

yevgenypats commented Oct 9, 2022

erezrokah commented Oct 9, 2022

feat(pg): Support CockroachDB #2531

feat(pg): Support CockroachDB #2531

Conversation

yevgenypats commented Oct 8, 2022

Summary

bbernays Oct 8, 2022

Choose a reason for hiding this comment

yevgenypats Oct 9, 2022 • edited

Choose a reason for hiding this comment

bbernays commented Oct 8, 2022

yevgenypats commented Oct 9, 2022 • edited

erezrokah left a comment • edited

Choose a reason for hiding this comment

yevgenypats commented Oct 9, 2022

yevgenypats commented Oct 9, 2022 • edited

erezrokah left a comment

Choose a reason for hiding this comment

erezrokah Oct 9, 2022

Choose a reason for hiding this comment

yevgenypats Oct 9, 2022

Choose a reason for hiding this comment

yevgenypats commented Oct 9, 2022

erezrokah commented Oct 9, 2022

yevgenypats Oct 9, 2022 •

edited

yevgenypats commented Oct 9, 2022 •

edited

erezrokah left a comment •

edited

yevgenypats commented Oct 9, 2022 •

edited