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 some missing postgres vars for ORM compatibility #12149

Merged
merged 2 commits into from
Dec 8, 2016

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Dec 7, 2016

This PR adds no-op support for setting the following variables, to improve ORM compatability:

  • SEARCH_PATH
  • CLIENT_ENCODING
  • STANDARD_CONFORMING_STRINGS
  • CLIENT_MIN_MESSAGES

It also adds support forSHOW SERVER_VERSION, always returning the 9.5.0 postgres version that we return over pgwire.

Resolves #12137
Resolves #12134
Resolves #12113
Resolves #12109
Resolves #12112


This change is Reviewable

@jordanlewis jordanlewis added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Dec 7, 2016
@cuongdo
Copy link
Contributor

cuongdo commented Dec 8, 2016

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


pkg/sql/set.go, line 126 at r1 (raw file):

	// See https://www.postgresql.org/docs/9.6/static/runtime-config-logging.html#GUC-APPLICATION-NAME
	case `CLIENT_ENCODING`:
	// See https://www.postgresql.org/docs/9.6/static/multibyte.html

should we return an error if anything but UTF-8 is specified?


pkg/sql/set.go, line 134 at r1 (raw file):

	// See https://www.postgresql.org/docs/9.6/static/runtime-config-client.html
	case `STANDARD_CONFORMING_STRINGS`:
	// If true, escape backslash literals in strings. We do this by default.

should we return an error or log a warning if an attempt is made to set to disable this? what happens if a client sets this to off and assumes that we honor that?


pkg/sql/set.go, line 135 at r1 (raw file):

	case `STANDARD_CONFORMING_STRINGS`:
	// If true, escape backslash literals in strings. We do this by default.
	// See https://www.postgresql.org/docs/9.6/static/runtime-config-client.html

This is the right page:
https://www.postgresql.org/docs/9.1/static/runtime-config-compatible.html#GUC-STANDARD-CONFORMING-STRINGS


Comments from Reviewable

This commit adds no-op support for the following variables, to improve
ORM compatability:

- `SEARCH_PATH`
- `CLIENT_ENCODING`
- `STANDARD_CONFORMING_STRINGS`
- `CLIENT_MIN_MESSAGES`
@jordanlewis
Copy link
Member Author

TFTR!


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


pkg/sql/set.go, line 126 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

should we return an error if anything but UTF-8 is specified?

I suppose that would be nice of us, yes. Done.


pkg/sql/set.go, line 134 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

should we return an error or log a warning if an attempt is made to set to disable this? what happens if a client sets this to off and assumes that we honor that?

Done.


pkg/sql/set.go, line 135 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

This is the right page:
https://www.postgresql.org/docs/9.1/static/runtime-config-compatible.html#GUC-STANDARD-CONFORMING-STRINGS

Done.


Comments from Reviewable

@cuongdo
Copy link
Contributor

cuongdo commented Dec 8, 2016

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.


pkg/sql/set.go, line 126 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I suppose that would be nice of us, yes. Done.

Can you also add a test for the error case?


pkg/sql/set.go, line 134 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Done.

Could you add a test for the error case?


pkg/sql/set.go, line 151 at r3 (raw file):

		switch parser.Name(s).Normalize() {
		case parser.ReNormalizeName("off"):
			return nil, fmt.Errorf("STANDARD_CONFORMING_STRINGS=off not supported")

nit: errors.New instead of fmt.Errorf?


Comments from Reviewable

@jordanlewis
Copy link
Member Author

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.


pkg/sql/set.go, line 126 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

Can you also add a test for the error case?

Done.


pkg/sql/set.go, line 134 at r1 (raw file):

Previously, cuongdo (Cuong Do) wrote…

Could you add a test for the error case?

Done.


pkg/sql/set.go, line 151 at r3 (raw file):

Previously, cuongdo (Cuong Do) wrote…

nit: errors.New instead of fmt.Errorf?

Simplified this case.


Comments from Reviewable

@cuongdo
Copy link
Contributor

cuongdo commented Dec 8, 2016

:lgtm:


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


Comments from Reviewable

@jordanlewis jordanlewis merged commit 2b74120 into cockroachdb:master Dec 8, 2016
@jordanlewis jordanlewis deleted the add-pg-vars branch December 8, 2016 18:21
@jseldess
Copy link
Contributor

jseldess commented Dec 12, 2016

In the future, when we have a docs page collecting session variables, these might be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL docs-todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants