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

sql: report precise oid type names to pg_type #13355

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Feb 1, 2017

pg_catalog.pg_type now reports the postgres-supplied name for oids that
correspond to types such as int8, int4, and int2, despite the fact that
we don't support those names syntactically and represent them all as the
same type internally.

This enables support for various ORMs, such as ActiveRecord, that might
rely on the reported typname for each of the rows in pg_type.

cc @nvanbenschoten

Resolves #12145.
Resolves #12530.
Updates #13045.


This change is Reviewable

@jordanlewis jordanlewis requested a review from knz February 1, 2017 23:57
@cuongdo
Copy link
Contributor

cuongdo commented Feb 2, 2017

Have you tested that this doesn't break Hibernate, GORM, or sqlalchemy? (Yeah, we need CI for all these ORMS...)


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nvanbenschoten
Copy link
Member

:lgtm:

@cuongdo I'm curious what you think the best solution is for making sure we don't break ORM support accidentally. Nightly tests? Adding them as a build check to Cockroach itself? Some combination of the two?


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


pkg/sql/pg_catalog.go, line 1462 at r1 (raw file):

}

// oidToName maps Postgres object IDs to type names for those OIDs that map to

This name is a little broad for what the map is actually doing, which you say is mapping OIDs to types that have more than one associated OID. Perhaps aliasedOidToName.


Comments from Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but same question as Cuong

pg_catalog.pg_type now reports the postgres-supplied name for oids that
correspond to types such as int8, int4, and int2, despite the fact that
we don't support those names syntactically and represent them all as the
same type internally.

This enables support for various ORMs, such as ActiveRecord, that might
rely on the reported typname for each of the rows in pg_type.
@jordanlewis
Copy link
Member Author

TFTR; I confirmed that this doesn't break any of the other ORMs. (Ugh)


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/sql/pg_catalog.go, line 1462 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This name is a little broad for what the map is actually doing, which you say is mapping OIDs to types that have more than one associated OID. Perhaps aliasedOidToName.

Done.


Comments from Reviewable

@jordanlewis jordanlewis merged commit 1f825e6 into cockroachdb:master Feb 3, 2017
@jordanlewis jordanlewis deleted the precise-oid-types branch February 3, 2017 18:46
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.

4 participants