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 #336

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Support CrateDB #336

merged 1 commit into from
Sep 29, 2017

Conversation

felixge
Copy link
Contributor

@felixge felixge commented Sep 27, 2017

See #320

@jackc I spend a decent amount of time trying to come up with a minimally invasive patch and added some detailed in-line comments explaining my approach. Let me know what you think.

@jackc jackc merged commit f71bf5d into jackc:master Sep 29, 2017
@jackc
Copy link
Owner

jackc commented Sep 29, 2017

Thanks! Looks like as clean and narrow a change as we could hope for.

@jackc jackc mentioned this pull request Sep 29, 2017
@felixge
Copy link
Contributor Author

felixge commented Sep 29, 2017

Thanks a lot! I'll be around to help if anybody runs into any unexpected issues with this.

felixge added a commit to felixge/telegraf that referenced this pull request Oct 16, 2017
@mfussenegger
Copy link

Nice to see CrateDB support in pgx.

One question to the pg_type query: Would it be possible to change this in
pgx to query explicitly on pg_catalog.pg_type?
I'm not sure if this could break anything with older postgres versions. Afaik
9+ should be fine.

It would be fairly simple for us to add the typtype column, but extending the
schema lookup logic is a bit more work.

The reason I'm asking is that this way we could add typtype and then no
longer have to worry about breaking anything here if we change something with
the error codes / error messages.

@felixge
Copy link
Contributor Author

felixge commented Nov 14, 2017

@mfussenegger not sure about @jackc, but from my POV changing the query to explicitly target the pg_catalog schema should be a reasonable change. As far as I can tell the pg_catalog schema has been around and can be explicitly specified in queries since at least Postgres 7.3.

That being said, I think the workaround code should probably still need to stay in place to support older CrateDB versions for a while.

@jackc
Copy link
Owner

jackc commented Nov 19, 2017

No objections from me for explicitly qualifying the pg_catalog schema.

@felixge
Copy link
Contributor Author

felixge commented Nov 20, 2017

@mfussenegger let me know when typtype gets implemented in CrateDB (is there a GH issue to subscribe to?). Then I'll do the PR and testing for pgx.

mfussenegger added a commit to crate/crate that referenced this pull request Nov 26, 2017
See https://www.postgresql.org/docs/10/static/catalog-pg-type.html

For now all types default to `b` (base type).

`pgx` is one client that uses this column. See
jackc/pgx#336
@mfussenegger
Copy link

@felixge We've added the column (see crate/crate#6582) - will be part of 2.3 (or in the nightly builds starting tomorrow)

@felixge
Copy link
Contributor Author

felixge commented Nov 27, 2017

@mfussenegger cool. I'm traveling for the next three weeks, but I'll try to find some time to make a PR for this ASAP.

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

Successfully merging this pull request may close these issues.

3 participants