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: add and link Postgres type i/o builtins #14529

Merged
merged 2 commits into from
Apr 1, 2017

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Mar 31, 2017

In Postgres, every type has 4 i/o builtins that correspond to it. For int8, for instance, there's int8in, int8out, int8send, and int8recv. We implement these as no-op builtins for ORM compatibility.

Additionally, we link these builtins to their corresponding fields in pg_type for all types.

Also, fix the name in pg_type for our Tuple type, which is called record in Postgres.

Fixes #12526.


This change is Reviewable

@jordanlewis jordanlewis force-pushed the pg-type-send-recv branch 3 times, most recently from c408897 to 7be5e84 Compare April 1, 2017 04:47
@knz
Copy link
Contributor

knz commented Apr 1, 2017

Woo, so many thanks for tackling this.
Two nits remaining though.


Reviewed 2 of 5 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/parser/pg_builtins.go, line 36 at r2 (raw file):

// programmatically determine whether or not this underscore is present, hence
// the existence of this map.
var typeBuiltinsHaveUnderscore = map[oid.Oid]bool{

map[oid.Oid]struct{}{ ... }


pkg/sql/parser/pg_builtins.go, line 91 at r2 (raw file):

	return map[string][]Builtin{
		builtinPrefix + "send": {
			Builtin{

I'd make a function returning a Builtin that takes just the desired return type and argument type as arguments, to construct the four things.


Comments from Reviewable

@jordanlewis
Copy link
Member Author

TFTR!


Review status: 1 of 5 files reviewed at latest revision, 2 unresolved discussions.


pkg/sql/parser/pg_builtins.go, line 36 at r2 (raw file):

Previously, knz (kena) wrote…

map[oid.Oid]struct{}{ ... }

Done.


pkg/sql/parser/pg_builtins.go, line 91 at r2 (raw file):

Previously, knz (kena) wrote…

I'd make a function returning a Builtin that takes just the desired return type and argument type as arguments, to construct the four things.

good call. done


Comments from Reviewable

In Postgres, every type has 4 i/o builtins that correspond to it. For
int8, for instances, there's int8in, int8out, int8send, and int8recv. We
implement these as no-op builtins for ORM compatibility.

Additionally, we link these builtins to their corresponding fields in
pg_type for all types.
Change the entry in pg_type to say record, not tuple.
@knz
Copy link
Contributor

knz commented Apr 1, 2017

:lgtm:


Reviewed 2 of 4 files at r3, 3 of 3 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@jordanlewis jordanlewis merged commit adc0f04 into cockroachdb:master Apr 1, 2017
@jordanlewis jordanlewis deleted the pg-type-send-recv branch April 1, 2017 23:58
@nvanbenschoten
Copy link
Member

Late to the game, but this is great! Really nice job.


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


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

				h.RegProc(builtinPrefix+"in"),   // typinput
				h.RegProc(builtinPrefix+"out"),  // typoutput
				h.RegProc(builtinPrefix+"recv"), // typrecv

nit: Why did this comment get changes? The column is typreceive.


Comments from Reviewable

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.

sql: list the conversion functions in pg_type
3 participants