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/pgwire: use OIDs when encoding some datum types #34818

Merged
merged 1 commit into from
Feb 12, 2019
Merged

sql/pgwire: use OIDs when encoding some datum types #34818

merged 1 commit into from
Feb 12, 2019

Conversation

maddyblue
Copy link
Contributor

Teach pgwire how to encode int and float with the various widths. Do
this by saving the oids during SetColumns.

Teach cmd/generate-binary to also record oids and use those in tests.

Fix varbits to expect the same OID as postgres produces.

Sadly, our int2 and int4 types don't yet propagate all the way down
and so we still encode them as an int8. This commit is a precursor to
supporting that.

Release note: None

@maddyblue maddyblue requested review from knz, bobvawter and a team February 12, 2019 07:26
@cockroach-teamcity
Copy link
Member

This change is 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: thanks!

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bobvawter)

Copy link
Contributor

@bobvawter bobvawter left a comment

Choose a reason for hiding this comment

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

:lgtm: w/ nits.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mjibson)


pkg/sql/pgwire/command_result.go, line 81 at r1 (raw file):

	formatCodes []pgwirebase.FormatCode

	oids []oid.Oid

Document this. I'm assuming it's a map of column index to Oid like formatCodes?


pkg/sql/pgwire/types.go, line 248 at r1 (raw file):

			b.putInt32(8)
			b.putInt64(int64(*v))
		default:

Do you need to detect the Oid == 0 case in order for the doc comment to be correct?

Teach pgwire how to encode int and float with the various widths. Do
this by saving the oids during SetColumns.

Teach cmd/generate-binary to also record oids and use those in tests.

Fix varbits to expect the same OID as postgres produces.

Sadly, our int2 and int4 types don't yet propagate all the way down
and so we still encode them as an int8. This commit is a precursor to
supporting that.

Release note: None
Copy link
Contributor Author

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @bobvawter and @knz)


pkg/sql/pgwire/command_result.go, line 81 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Document this. I'm assuming it's a map of column index to Oid like formatCodes?

Done.


pkg/sql/pgwire/types.go, line 248 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Do you need to detect the Oid == 0 case in order for the doc comment to be correct?

No, the 0 case is for types that have a 1:1 mapping. The oid must be specified otherwise. I've updated the comment to reflect this.

@maddyblue
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Feb 12, 2019
34818: sql/pgwire: use OIDs when encoding some datum types r=mjibson a=mjibson

Teach pgwire how to encode int and float with the various widths. Do
this by saving the oids during SetColumns.

Teach cmd/generate-binary to also record oids and use those in tests.

Fix varbits to expect the same OID as postgres produces.

Sadly, our int2 and int4 types don't yet propagate all the way down
and so we still encode them as an int8. This commit is a precursor to
supporting that.

Release note: None

34830: c-deps: bump RocksDB to pick up ingest-external-file deadlock r=tbg a=petermattis

Fixes #34212

Release note (bug fix): Fix a deadlock that could occur during IMPORT
and RESTORE, causing all writes on a node to be stopped.

Co-authored-by: Matt Jibson <[email protected]>
Co-authored-by: Peter Mattis <[email protected]>
@craig
Copy link
Contributor

craig bot commented Feb 12, 2019

Build succeeded

@craig craig bot merged commit ea1c4fc into cockroachdb:master Feb 12, 2019
@maddyblue maddyblue deleted the pgwire-oid branch February 12, 2019 19:21
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