Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support CrateDB #320

Closed
felixge opened this issue Sep 9, 2017 · 9 comments
Closed

Support CrateDB #320

felixge opened this issue Sep 9, 2017 · 9 comments

Comments

@felixge
Copy link
Contributor

felixge commented Sep 9, 2017

Hi,

thanks for this great library :).

I'm currently working on implementing CrateDB support for influxdata/telegraf and was asked by the maintainers if I could use your library instead of lib/pq.

Unfortunately it doesn't work because pgx queries the postgres catalog when creating a new connection, and CrateDB doesn't implement the catalog tables.

Please let me know how you feel about supporting CrateDB (or other databases pretending to be postgres on the protocol level). If you're interested, I might be able to contribute a patch. If not, feel free to simply close this issue.

Cheers
Felix

PS: I'm a huge fan of your writing. Your post Materialized View Strategies Using PostgreSQL gave me the inspiration to migrate a large project (> 1 billion rows) from NoSQL to PostgreSQL. The migration was a huge success, and the analytics queries are blazing fast thanks to to some eager materialized views :).

@jackc
Copy link
Owner

jackc commented Sep 9, 2017

I'm in favor of supporting PostgreSQL-like servers provided the changes required can be cleanly implemented. This has recently come up with RedShift SSL issues as well. In both these cases, it appears that the changes required would be localized to the connection code so it seems promising.

PS: I'm a huge fan of your writing. Your post Materialized View Strategies Using PostgreSQL gave me the inspiration to migrate a large project (> 1 billion rows) from NoSQL to PostgreSQL. The migration was a huge success, and the analytics queries are blazing fast thanks to to some eager materialized views :).

Thanks! So glad to hear it was helpful. I might have to write an update after PG10 is released. Transition table triggers have the potential to be much more efficient for certain operations.

@jackc
Copy link
Owner

jackc commented Sep 29, 2017

Resolved by #336.

@jackc jackc closed this as completed Sep 29, 2017
felixge added a commit to felixge/telegraf that referenced this issue Oct 16, 2017
@chetananand
Copy link

I tried the version of pgx with this patch - I still run into a similar error on https://github.com/jackc/pgx/blob/master/conn.go#L430 from initConnInfoEnumArray(). That query also is using the pg_type table without the schema pg_catalog and therefore initConnInfo() will still fail.

@felixge
Copy link
Contributor Author

felixge commented Oct 22, 2017

@chetananand it seems like this is a regression introduced in ab9a1af . The problem with my patch is that it's not included in the automatic test suite, which only runs against postgres, not CrateDB. You might want to consider using f71bf5d for now.

@jackc would you like me to submit another patch to fix the regression?

@jackc
Copy link
Owner

jackc commented Nov 4, 2017

@jackc would you like me to submit another patch to fix the regression?

Sure that'd be great.

The problem with my patch is that it's not included in the automatic test suite, which only runs against postgres, not CrateDB.

I don't know how difficult this would be to add, but some optional tests that run against CrateDB would help those of us who don't use CrateDB from breaking it in the future.

@felixge
Copy link
Contributor Author

felixge commented Nov 9, 2017

@jackc I've fixed the regression in #352 . I've also started to look into adding some cratedb testing to the .travis.yml, but I'll need some more time to get it working. But hopefully you can merge the fix before that?

Question: Would you be okay if I move some of the bash commands from the YAML file into a dedicated bash script? This might simplify things a bit.

@chetananand
Copy link

Sorry for the late response @felixge - Moving to f71bf5d helped to get past the initial issue. Because of other unrelated limitations with CrateDB itself, I moved onto pure PostgreSQL itself. Thanks for your help.

@jackc
Copy link
Owner

jackc commented Nov 11, 2017

Question: Would you be okay if I move some of the bash commands from the YAML file into a dedicated bash script? This might simplify things a bit.

Sure. I don't have any strong opinions on that.

@felixge
Copy link
Contributor Author

felixge commented Nov 12, 2017

Closing this again as it's fixed as of 4878d92 .

PR #354 adds test coverage for CrateDB.

@felixge felixge closed this as completed Nov 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants