Edits for Postgres compatibility #2887

lnhsingh · 2018-04-09T18:57:23Z

Closes #2256, #2175.

Closes #2175.

cockroach-teamcity · 2018-04-09T18:57:29Z

This change is

cockroach-teamcity · 2018-04-09T18:58:54Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/6e39ea044d1d0ed5fb1fe5cafcb6db5a82eb562d/

cockroach-teamcity · 2018-04-10T20:01:00Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/6d909d09eb744a2d68209aba92da6b1b356b9612/

cockroach-teamcity · 2018-04-10T20:02:58Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/c619dcf1eb0dfc0a7c5c860c787af03d7fa3a8b0/

knz

Thanks Lauren for tackling this difficult and nuanced change (there's more to it than appears at first).

I am sorry to dump so much nuance and technicalities on you, but I believe this page is a bit a sensitive topic and we should spend more time on technical accuracy. I hope my comments below help.

knz · 2018-04-10T21:21:43Z

v2.0/set-transaction.md

@@ -22,8 +22,8 @@ No [privileges](privileges.html) are required to set the transaction isolation l

 | Parameter | Description |
 |-----------|-------------|
-| `ISOLATION LEVEL` | By default, transactions in CockroachDB implement the strongest ANSI isolation level: `SERIALIZABLE`. At this isolation level, transactions will never result in anomalies. The `SNAPSHOT` isolation level is still supported as well for backwards compatibility, but you should avoid using it. It provides little benefit in terms of performance and can result in inconsistent state under certain complex workloads. For more information, see [Transactions: Isolation Levels](transactions.html#isolation-levels).<br/><br/>**Default**: `SERIALIZABLE` |
-| `PRIORITY` | If you don't want the transaction to run with `NORMAL` priority, you can set it to `LOW` or `HIGH`.<br/><br/>Transactions with higher priority are less likely to need to be retried.<br/><br/>For more information, see [Transactions: Priorities](transactions.html#transaction-priorities).<br/><br/>**Default**: `NORMAL` |
+| `ISOLATION LEVEL` | By default, transactions in CockroachDB implement the strongest ANSI isolation level: `SERIALIZABLE`. At this isolation level, transactions will never result in anomalies. The `SNAPSHOT` isolation level is still supported as well for backwards compatibility, but you should avoid using it. It provides little benefit in terms of performance and can result in inconsistent state under certain complex workloads. For more information, see [Transactions: Isolation Levels](transactions.html#isolation-levels).<br><br><span class="version-tag">New in v2.0:</span> Alias: `transaction_isolation`<br><br>**Default**: `SERIALIZABLE` |


Here and below:

 New in v2.0: The current isolation level is also exposed as the [session variable](show-vars.html) `transaction_isolation`.

(it's not an alias.)

Also btw this is now new in v2.0.

knz · 2018-04-10T21:24:20Z

v2.0/set-vars.md

@@ -40,8 +40,12 @@ The variable name is case insensitive. The value can be a list of one or more it
 | `default_transaction_isolation` | The default transaction isolation level for the current session. See [Transaction parameters](transactions.html#transaction-parameters) and [`SET TRANSACTION`](set-transaction.html) for more details. | Settings in connection string, or "`SERIALIZABLE`" if not specified  | Yes |
 | `sql_safe_updates` | If `true`, disallow potentially unsafe SQL statements, including `DELETE` without a `WHERE` clause, `UPDATE` without a `WHERE` clause, and `ALTER TABLE ... DROP COLUMN`. See [Allow Potentially Unsafe SQL Statements](use-the-built-in-sql-client.html#allow-potentially-unsafe-sql-statements) for more details. | `true` for interactive sessions from the [built-in SQL client](use-the-built-in-sql-client.html) unless `--safe-updates=false` is specified,<br>`false` for sessions from other clients | Yes |
 | `search_path` | <span class="version-tag">Changed in v2.0:</span> A list of schemas that will be searched to resolve unqualified table or function names. For more details, see [Name Resolution](sql-name-resolution.html). | "`{public}`" | Yes |
-| `time zone` | The default time zone for the current session.<br><br>This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`.</br><br>See [`SET TIME ZONE`](#set-time-zone) for more details. | `UTC` | Yes |
+| `server_version_num` | <span class="version-tag">New in v2.0:</span> The version of PostgreSQL that CockroachDB emulates. | Version-dependent | Yes |
+| `time zone` | The default time zone for the current session.<br><br>This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`.</br><br>See [`SET TIME ZONE`](#set-time-zone) for more details. <br><br><span class="version-tag">New in v2.0:</span> Alias: `"timezone"` | `UTC` | Yes |


The variable has been renamed.

This entire line must be replaced with:

| `timezone` | The default time zone for the current session. This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`. See [Setting the Time Zone](#set-time-zone) for more details. Changed in v2.0: This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `UTC` | Yes |

knz · 2018-04-10T21:26:40Z

v2.0/set-vars.md

 | `tracing` | The trace recording state.<br><br>See [`SET TRACING`](#set-tracing) for more details. | `off` | Yes |
+| `transaction isolation level` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">New in v2.0:</span> Alias: `transaction_isolation` | `SERIALIZABLE` | Yes |


The variables have been renamed.

These three lines must be entirely replaced by:

| `transaction_isolation` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. Changed in v2.0: This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes | | `transaction_priority` | The priority of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. Changed in v2.0: This session variable was called `transaction priority` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NORMAL` | Yes | | `transaction_status` | The state of the current transaction. See [Transactions](transactions.html) for more details. Changed in v2.0: This session variable was called `transaction status` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NoTxn` | Yes |

knz · 2018-04-10T21:27:05Z

v2.0/set-vars.md

@@ -159,7 +163,7 @@ You can control your client's default time zone for the current session with <co

 {{site.data.alerts.callout_info}}With setting <code>SET TIME ZONE</code>, CockroachDB uses UTC as the default time zone.{{site.data.alerts.end}}

-`SET TIME ZONE` uses a special syntax form used to configure the `"time zone"` session parameter because `SET` cannot assign to parameter names containing spaces.
+`SET TIME ZONE` uses a special syntax form used to configure the `"timezone"` session parameter because `SET` cannot assign to parameter names containing spaces.


This entire line must be deleted as it is no longer accurate.

knz · 2018-04-10T21:28:08Z

v2.0/show-vars.md

 | `session_user` | The user connected for the current session. | User in connection string | No |
 | `sql_safe_updates` | If `false`, potentially unsafe SQL statements are allowed, including `DROP` of a non-empty database and all dependent objects, `DELETE` without a `WHERE` clause, `UPDATE` without a `WHERE` clause, and `ALTER TABLE .. DROP COLUMN`. See [Allow Potentially Unsafe SQL Statements](use-the-built-in-sql-client.html#allow-potentially-unsafe-sql-statements) for more details. | `true` for interactive sessions from the [built-in SQL client](use-the-built-in-sql-client.html),<br>`false` for sessions from other clients | Yes |
-| `time zone` | The default time zone for the current session   | `UTC` | Yes |
+| `time zone` | The default time zone for the current session. <br><br><span class="version-tag">New in v2.0:</span> Alias: `"timezone"`  | `UTC` | Yes |


Replace:

| `timezone` | The default time zone for the current session. Changed in v2.0: This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `UTC` | Yes |

knz · 2018-04-10T21:29:07Z

v2.0/show-vars.md

-| `transaction isolation level` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. | `SERIALIZABLE` | Yes |
-| `transaction priority` | The priority of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. | `NORMAL` | Yes |
-| `transaction status` | The state of the current transaction. See [Transactions](transactions.html) for more details. | `NoTxn` | No |
+| `transaction isolation level` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">New in v2.0:</span> Alias: `transaction_isolation` | `SERIALIZABLE` | Yes |


Take the 3 lines over from my comment on set-vars.md, adjusting the last column as needed.

knz · 2018-04-10T21:29:31Z

v2.0/string.md

@@ -16,6 +16,7 @@ In CockroachDB, the following are aliases for `STRING`:

 - `CHARACTER`
 - `CHAR`
+- `"char"` 


this line is not accurate and should be removed. Just curious: why did you add it?

lnhsingh · 2018-04-11T15:54:55Z

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

v2.0/set-transaction.md, line 25 at r2 (raw file):

Previously, knz (kena) wrote…

Also btw this is now new in v2.0.

Done.

v2.0/set-vars.md, line 44 at r2 (raw file):

Previously, knz (kena) wrote…

The variable has been renamed.

This entire line must be replaced with:

| `timezone` | The default time zone for the current session.<br><br>This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`.</br><br>See [Setting the Time Zone](#set-time-zone) for more details. <br><br><span class="version-tag">Changed in v2.0:</span> This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `UTC` | Yes |

Done.

v2.0/set-vars.md, line 46 at r2 (raw file):

Previously, knz (kena) wrote…

The variables have been renamed.

These three lines must be entirely replaced by:

| `transaction_isolation` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes |
| `transaction_priority` | The priority of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction priority` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NORMAL` | Yes |
| `transaction_status` | The state of the current transaction. See [Transactions](transactions.html) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction status` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NoTxn` | Yes |

Done.

v2.0/set-vars.md, line 158 at r2 (raw file):

~~~

## `SET TIME ZONE`

@knz - so you can still SET TIME ZONE? Or SET timezone. Both are valid?

v2.0/set-vars.md, line 166 at r2 (raw file):

Previously, knz (kena) wrote…

This entire line must be deleted as it is no longer accurate.

Done.

v2.0/show-vars.md, line 46 at r2 (raw file):

Previously, knz (kena) wrote…

Replace:

| `timezone` | The default time zone for the current session. <br><br><span class="version-tag">Changed in v2.0:</span> This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `UTC` | Yes |

Done.

v2.0/show-vars.md, line 48 at r2 (raw file):

Previously, knz (kena) wrote…

Take the 3 lines over from my comment on set-vars.md, adjusting the last column as needed.

Done.

v2.0/string.md, line 19 at r2 (raw file):

Previously, knz (kena) wrote…

this line is not accurate and should be removed. Just curious: why did you add it?

Removed. Edited per #2175: "The type name "char" (with quotes) is recognized as an alias for CHAR. Need to update https://www.cockroachlabs.com/docs/dev/string.html#aliases."

Does this need to be added somewhere else instead?

Comments from Reviewable

cockroach-teamcity · 2018-04-11T15:56:55Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/e2e77f2bca82761e40c7512ea362c73125f98116/

knz · 2018-04-11T16:25:56Z

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

v2.0/set-vars.md, line 158 at r2 (raw file):

Previously, lhirata wrote…

@knz - so you can still SET TIME ZONE? Or SET timezone. Both are valid?

Yes. That's what the table "Special syntax cases" above explains.

v2.0/set-vars.md, line 61 at r3 (raw file):

| `SET NAMES ...` | `SET client_encoding = ...` | This is provided for compatibility with PostgreSQL clients.
| `SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL ...` | `SET default_transaction_isolation = ...` | This is provided for compatibility with standard SQL.
| `SET TIME ZONE ...` | `SET "timezone" = ...` | This is provided for compatibility with PostgreSQL clients.

You can change SET "timezone" = to SET timezone =

v2.0/string.md, line 19 at r2 (raw file):

Previously, lhirata wrote…

Removed. Edited per #2175: "The type name "char" (with quotes) is recognized as an alias for CHAR. Need to update https://www.cockroachlabs.com/docs/dev/string.html#aliases."

Does this need to be added somewhere else instead?

Thanks for bringing my attention to this. The code has a bug. I filed cockroachdb/cockroach#24686 to track this. Let's not document this until the code becomes correct.

Comments from Reviewable

lnhsingh · 2018-04-11T16:50:22Z

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

v2.0/set-vars.md, line 61 at r3 (raw file):

Previously, knz (kena) wrote…

You can change SET "timezone" = to SET timezone =

Done.

Comments from Reviewable

cockroach-teamcity · 2018-04-11T16:53:02Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/441e4c7e92399dfba61fdcc519febec76efa9156/

rmloveland

Couple of small phrasing suggestions.

rmloveland · 2018-04-12T16:18:55Z

v2.0/set-vars.md

 | `tracing` | The trace recording state.<br><br>See [`SET TRACING`](#set-tracing) for more details. | `off` | Yes |
+| `transaction_isolation` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes |
+| `transaction_priority` | The priority of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction priority` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NORMAL` | Yes |
+| `transaction_status` | The state of the current transaction. See [Transactions](transactions.html) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction status` (with a space) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `NoTxn` | Yes |


I would change all uses of "It was renamed ..." to "It has been renamed ..."

Reasoning: The sentence just before says "This ... variable was called X", which denotes a previous state, so IMO saying "It was" again recalls that previous state, whereas saying "it has been" makes clear that the variable name moved to a new state.

rmloveland · 2018-04-12T16:21:06Z

v2.0/show-vars.md

-| `transaction isolation level` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. | `SERIALIZABLE` | Yes |
-| `transaction priority` | The priority of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details. | `NORMAL` | Yes |
-| `transaction status` | The state of the current transaction. See [Transactions](transactions.html) for more details. | `NoTxn` | No |
+| `transaction_isolation` | The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">Changed in v2.0:</span> This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It was renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes |


Re: timezone and transaction_*, same comment/suggestion as above re: using "has been renamed for compatibility" in the closing sentence.

lnhsingh · 2018-04-18T16:43:08Z

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

v2.0/set-vars.md, line 48 at r4 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I would change all uses of "It was renamed ..." to "It has been renamed ..."

Reasoning: The sentence just before says "This ... variable was called X", which denotes a previous state, so IMO saying "It was" again recalls that previous state, whereas saying "it has been" makes clear that the variable name moved to a new state.

Done.

v2.0/show-vars.md, line 48 at r4 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Re: timezone and transaction_*, same comment/suggestion as above re: using "has been renamed for compatibility" in the closing sentence.

Done.

Comments from Reviewable

cockroach-teamcity · 2018-04-18T16:46:03Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/b5cde5dfcb8ecadc16bff1fee73cd8b113fd0474/

cockroach-teamcity · 2018-04-18T16:55:41Z

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/fd8b8ba96011363b0e7d3083dedb80638142ab74/

Lauren added 2 commits April 9, 2018 10:43

Various PostgreSQL compat updates

8858a95

Closes #2175.

Closes #2256.

6e39ea0

lnhsingh added the in progress label Apr 9, 2018

Lauren added 2 commits April 10, 2018 15:57

update timezone session variable

6d909d0

edit timezone alias

c619dcf

lnhsingh requested a review from knz April 10, 2018 20:01

knz reviewed Apr 10, 2018

View reviewed changes

Edits based on Raphael's feedback

e2e77f2

knz mentioned this pull request Apr 11, 2018

sql: incomplete desugaring of type keywords into type names cockroachdb/cockroach#24686

Closed

minor edit

441e4c7

lnhsingh requested a review from rmloveland April 11, 2018 16:52

rmloveland reviewed Apr 12, 2018

View reviewed changes

minor phasing edits

b5cde5d

Merge branch 'master' into postgres-compat

fd8b8ba

lnhsingh changed the title ~~Edits for Postgres compatability~~ Edits for Postgres compatibility Apr 18, 2018

lnhsingh merged commit eb3a061 into master Apr 18, 2018

lnhsingh deleted the postgres-compat branch April 18, 2018 16:56

lnhsingh removed the in progress label Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edits for Postgres compatibility #2887

Edits for Postgres compatibility #2887

lnhsingh commented Apr 9, 2018

cockroach-teamcity commented Apr 9, 2018

cockroach-teamcity commented Apr 9, 2018

cockroach-teamcity commented Apr 10, 2018

cockroach-teamcity commented Apr 10, 2018

knz left a comment

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

knz Apr 10, 2018

lnhsingh commented Apr 11, 2018

cockroach-teamcity commented Apr 11, 2018

knz commented Apr 11, 2018

lnhsingh commented Apr 11, 2018

cockroach-teamcity commented Apr 11, 2018

rmloveland left a comment

rmloveland Apr 12, 2018

rmloveland Apr 12, 2018

lnhsingh commented Apr 18, 2018

cockroach-teamcity commented Apr 18, 2018

cockroach-teamcity commented Apr 18, 2018

		\| `tracing` \| The trace recording state.<br><br>See [`SET TRACING`](#set-tracing) for more details. \| `off` \| Yes \|
		\| `transaction isolation level` \| The isolation level of the current transaction. See [Transaction parameters](transactions.html#transaction-parameters) for more details.<br><br><span class="version-tag">New in v2.0:</span> Alias: `transaction_isolation` \| `SERIALIZABLE` \| Yes \|

Edits for Postgres compatibility #2887

Edits for Postgres compatibility #2887

Conversation

lnhsingh commented Apr 9, 2018

cockroach-teamcity commented Apr 9, 2018

cockroach-teamcity commented Apr 9, 2018

cockroach-teamcity commented Apr 10, 2018

cockroach-teamcity commented Apr 10, 2018

knz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnhsingh commented Apr 11, 2018

cockroach-teamcity commented Apr 11, 2018

knz commented Apr 11, 2018

lnhsingh commented Apr 11, 2018

cockroach-teamcity commented Apr 11, 2018

rmloveland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnhsingh commented Apr 18, 2018

cockroach-teamcity commented Apr 18, 2018

cockroach-teamcity commented Apr 18, 2018