Add SQL docs for survival, region, table locality #9936
Conversation
|
This change is |
|
Hi Arul, Oliver, and Adam: I wanted to get your feedback on these docs as the SQL Triumvirate. They are very basic for now, but I think they have the essentials. Please let me know if anything essential is missing, or if something is incorrect. There are a few pages in this PR, so I am happy if y'all want to split them up, or everyone looking at everything also works. Your call. Thanks! |
There was a problem hiding this comment.
cc @ajstorm in case you missed it
Reviewed 9 of 10 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @rmloveland)
v21.1/add-region.md, line 32 at r1 (raw file):
## Examples ### Set the primary region
does this need a higher level callout than "Examples", i.e. a PRIMARY REGION must be set first?
v21.1/add-region.md, line 78 at r1 (raw file):
database | region | primary | zones -----------+----------+---------+------------------------------------------------ {db} | us-east1 | true | {us-east1-b,us-east1-b,us-east1-b,us-east1-b}
is {db} a template? if so, does the SQL output misalign?
v21.1/alter-table.md, line 35 at r1 (raw file):
[`UNSPLIT AT`](unsplit-at.html) | Remove a range split enforcement at a specified row in the table. | No [`VALIDATE CONSTRAINT`](validate-constraint.html) | Check whether values in a column match a [constraint](constraints.html) on the column. | Yes [`SET LOCALITY {REGIONAL, GLOBAL}`](set-locality.html) | <span class="version-tag">New in v21.1:</span> Set the table locality for a table in a [multi-region database](multiregion-overview.html). | No
does REGIONAL need to be expanded?
v21.1/multiregion-overview.md, line 60 at r1 (raw file):
_Database regions_ are a high-level abstraction for a geographic region. Each region is broken into multiple zones. These terms are meant to correspond directly to the region and zone terminology used by cloud providers. The regions added during node startup become _Database Regions_ when they are added to a database. To add the first region, use the [`ALTER DATABASE .. PRIMARY REGION` statement](add-region.html#set-the-primary-region).
nit: nix extra space before "To add the first region", wondering if .. should be ... because i like 3's. same with below, but if it's consistent with docs than no biggie.
v21.1/multiregion-overview.md, line 153 at r1 (raw file):
Use regional by row tables when your application requires low-latency reads and writes at a row level where individual rows are primarily accessed from a single region. For example, a users table in a global application may need to keep some users' data in specific regions due to regulations (such as GDPR), for better performance, or both. For an example of a table that can benefit from the _regional by row_ setting in a multi-region deployment, see the `users` table from the [MovR application](movr.html).
hmm we definitely haven't set this up yet. do we still need an example?
v21.1/set-locality.md, line 27 at r1 (raw file):
s:··
what are those ·· characters o_o
v21.1/set-locality.md, line 70 at r1 (raw file):
~~~ Every row in a regional by row table has a hidden `crdb_region` column that represents the row's home region. To see a row's region, issue a statement like the following:
not if you use REGIONAL BY ROW AS col_name, in which case it would be col_name
v21.1/set-locality.md, line 86 at r1 (raw file):
To add a new row to a regional by row table, you must choose one of the following options. - Let CockroachDB set the row's home region automatically. It will use the region of the gateway node from which the row is inserted.
is gateway node a well known term here (oops, i almost said WKT)
v21.1/set-locality.md, line 114 at r1 (raw file):
~~~ For more information about how this table locality works, see [Global tables](multiregion-overview.html#global-tables).
should the tradeoff for this be elaborated here or is it already in the hyperlink?
(same with RBR and RBT)
rmloveland
force-pushed
the
20210226-sql-add-region-set-locality-etc
branch
from
53411ee
to
1745d73
Compare
There was a problem hiding this comment.
thanks Oliver! Updated stuff. let me know what you think!
(PS sorry for the delay, got sidetracked this week!)
Reviewable status:
v21.1/add-region.md, line 32 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
does this need a higher level callout than "Examples", i.e. a PRIMARY REGION must be set first?
I added a callout to the top of the page as well that links down to this example. Do you think that does the job?
v21.1/add-region.md, line 78 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
is {db} a template? if so, does the SQL output misalign?
Yes, but I think using it is more confusing than it needs to be. Changed to use the venerable foo, which displays more normally in the examples
v21.1/alter-table.md, line 35 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
does REGIONAL need to be expanded?
Yes I think so. Expanded to SET LOCALITY {REGIONAL BY TABLE, REGIONAL BY ROW, GLOBAL}
v21.1/multiregion-overview.md, line 60 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
nit: nix extra space before "To add the first region", wondering if
..should be...because i like 3's. same with below, but if it's consistent with docs than no biggie.
I also like 3 dots better, updated that. And I removed the extra space!
v21.1/multiregion-overview.md, line 153 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
hmm we definitely haven't set this up yet. do we still need an example?
in a way it's nicer that it's not built in IMO. that way we can do it in a tutorial and show how it helps. Trying to work on something in that direction in this wiki page which I think I have already spammed you with elsewhere: https://cockroachlabs.atlassian.net/wiki/x/BQDpXg
v21.1/set-locality.md, line 27 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
s:··what are those ·· characters o_o
hahaha yeah - markdown needs that to put a newline there, otherwise it gets confused when switching back and forth between md and html syntax as in this section. sorry if your editor shows bright red or something :-}
v21.1/set-locality.md, line 70 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
not if you use
REGIONAL BY ROW AS col_name, in which case it would becol_name
Ah so you can do ALTER TABLE foo SET LOCALITY REGIONAL BY ROW AS bar
and then do
SELECT bar, id FROM foo ?
So the idea is that you as the user just want something other than crdb_region?
If I understood this correctly, I added a note to this effect at the bottom of the section. PTAL.
v21.1/set-locality.md, line 86 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
is gateway node a well known term here (oops, i almost said WKT)
Perhaps! but I added a link to a definition elsewhere in our docs just in case
v21.1/set-locality.md, line 114 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
should the tradeoff for this be elaborated here or is it already in the hyperlink?
(same with RBR and RBT)
I think the linked docs already describe how these work and the tradeoffs and stuff
|
Hey friends, I'm going to pass this along for docs team review. If there is something egregiously wrong here (which I don't think there is based on Oliver's review), this is good enough to merge for now IMO - we can update again as needed as release gets closer. Think it will provide value to get this up on the site sooner than later. |
|
I agree. Sorry I wasn't able to get to it in time for pushing. Still on
my todo list.
…On Tue, Mar 23, 2021 at 2:17 PM Rich Loveland ***@***.***> wrote:
Hey friends, I'm going to pass this along for docs team review. If there
is something egregiously wrong here (which I don't think there is based on
Oliver's review), this is good enough to merge for now IMO - we can update
again as needed as release gets closer. Think it will provide value to get
this up on the site sooner than later.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9936 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEMXVOTE26EQXLS2A6Z3OI3TFDLJ7ANCNFSM4YVVA5TQ>
.
|
|
No need to apologize Adam! Appreciate your many careful docs reviews, you can't catch em all. Being busy with other things is ok |
There was a problem hiding this comment.
Took a look and things look great overall! Mostly nits below.
Reviewed 1 of 10 files at r1, 3 of 4 files at r2.
Reviewable status:
v21.1/add-region.md, line 27 at r2 (raw file):
| Parameter | Description | |-----------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `database_name` | The database you are adding a [region](multiregion-overview.html#database-regions) to. |
Nit: "The database to which you are adding the region"?
v21.1/add-region.md, line 28 at r2 (raw file):
|-----------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `database_name` | The database you are adding a [region](multiregion-overview.html#database-regions) to. | | `region_name` | The [region](multiregion-overview.html#database-regions) being added to this database. Allowed values include any region added at [node startup](cockroach-start.html#locality). |
Do we want to say "Allowed values include any region present in SHOW REGIONS FROM CLUSTER"? Might be more actionable than the way it's written right now.
v21.1/add-region.md, line 32 at r2 (raw file):
## Required privileges The user must be a member of [the `admin` role](authorization.html) or must have the [`CREATEDB`](create-role.html#create-a-role-that-can-create-and-rename-databases) parameter set.
This changed recently to be Admin, Owner, or CREATE. @arulajmani FYI.
v21.1/add-region.md, line 38 at r2 (raw file):
### Set the primary region To add the first region, or to set an already-added region as the primary region, use the following statement:
Would this benefit from a bit more detail as to how adding and setting differ? The differences are pretty dramatic.
v21.1/add-region.md, line 67 at r2 (raw file):
{{site.data.alerts.callout_info}} You can only add regions to a multi-region database that were defined at node startup time. For more information, see [Cluster regions](multiregion-overview.html#cluster-regions).
Nit: The splitting of "add regions" and "that were defined at node startup time" by "to a multi-region database" is throwing me off a bit. What do you think about rewriting this to "Only regions that are defined at node startup time can be added to a multi-region database".
v21.1/add-region.md, line 101 at r2 (raw file):
~~~ You can not drop the primary region from a multi-region database. If you want to drop the current primary region, you must designate a new primary region first.
I don't think this is entirely accurate. You can drop a primary region, so long as it's the last region defined on the database.
v21.1/add-region.md, line 104 at r2 (raw file):
{{site.data.alerts.callout_danger}} A multi-region database must have at least one region. If you try to drop the only remaining database region, an error will be signalled.
See above. There won't be an error so long as you drop the primary region last.
v21.1/multiregion-overview.md, line 64 at r2 (raw file):
While the database has only one region assigned to it, it is considered a "multi-region database." This means that all data in that database is stored within its assigned regions, and CockroachDB optimizes access to the database's data from the primary region. To add another database region, use the [`ALTER DATABASE .. ADD REGION` statement](add-region.html#add-a-region-to-a-database).
Nit: One more instance of .. to ...
v21.1/multiregion-overview.md, line 97 at r2 (raw file):
With the zone level survival goal, the database will remain fully available for reads and writes, even if a zone goes down. However, the database may not remain fully available if multiple zones fail in the same region. You can configure a database to survive zone failures using the [`ALTER DATABASE .. SURVIVE ZONE FAILURE` statement](survive-failure.html).
Nit: more ..
v21.1/multiregion-overview.md, line 109 at r2 (raw file):
The region level survival goal has the property that the database will remain fully available for reads and writes, even if an entire region goes down. This added survival comes at a cost: write latency will be increased by at least as much as the round-trip time to the nearest region. Read performance will be unaffected. In other words, you are adding network hops and making writes slower in exchange for robustness. You can configure a database to survive region failures using the [`ALTER DATABASE .. SURVIVE REGION FAILURE` statement](survive-failure.html).
Nit: more ..
v21.1/multiregion-overview.md, line 141 at r2 (raw file):
For _regional_ tables, access to the table will be fast in the table's "home region" and slower in other regions. In other words, CockroachDB optimizes access to data in regional tables from a single region. By default, a regional table's home region is the [database's primary region](#database-regions), but that can be changed to use any region added to the database. For instructions showing how to set a table's locality to `REGIONAL BY TABLE`, see [`ALTER TABLE .. SET LOCALITY`](set-locality.html#regional-by-table)
Nit: more ..
v21.1/set-locality.md, line 7 at r2 (raw file):
--- <span class="version-tag">New in v21.1:</span> The `ALTER TABLE .. SET LOCALITY` [statement](sql-statements.html) changes the [table locality](multiregion-overview.html#table-locality) of a [table](create-table.html) in a [multi-region database](multiregion-overview.html).
Nit: ..
v21.1/set-locality.md, line 35 at r2 (raw file):
## Required privileges The user must have the `DROP` [privilege](authorization.html#assign-privileges) on the table.
This has also changed recently. It should be either admin, owner of the table, or create on the table. @arulajmani
v21.1/set-locality.md, line 47 at r2 (raw file):
{% include copy-clipboard.html %} ~~~ sql ALTER TABLE {table} SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
Do we want to mention somewhere that REGIONAL BY TABLE defaults to PRIMARY REGION?
v21.1/set-locality.md, line 57 at r2 (raw file):
~~~ For more information about how this table locality works, see [Regional tables](multiregion-overview.html#regional-tables).
Nit: "For more information about how table localities work"?
v21.1/set-locality.md, line 100 at r2 (raw file):
{{site.data.alerts.callout_success}} You can also use a name other than `crdb_region` for the hidden column by using the following syntax:
It's more than just a different name. You can specify any column definition for the REGIONAL BY ROW AS column, so long as the column is of type crdb_internal_region. This allows customers to have columns generated in any way they'd like. For example, we could modify movr to have a region column generated as:
ALTER TABLE rides ADD COLUMN region crdb_internal_region AS (
CASE WHEN city = 'amsterdam' THEN 'europe-west1'
WHEN city = 'paris' THEN 'europe-west1'
WHEN city = 'rome' THEN 'europe-west1'
WHEN city = 'new york' THEN 'us-east1'
WHEN city = 'boston' THEN 'us-east1'
WHEN city = 'washington dc' THEN 'us-east1'
WHEN city = 'san francisco' THEN 'us-west1'
WHEN city = 'seattle' THEN 'us-east1'
WHEN city = 'los angeles' THEN 'us-east1'
END
) STORED
v21.1/survive-failure.md, line 7 at r2 (raw file):
--- <span class="version-tag">New in v21.1:</span> The `ALTER DATABASE .. SURVIVE {ZONE,REGION} FAILURE` [statement](sql-statements.html) sets the [survival goal](multiregion-overview.html#survival-goals) for a [multi-region database](multiregion-overview.html).
Nit: ..
v21.1/survive-failure.md, line 27 at r2 (raw file):
## Required privileges The user must be a member of [the `admin` role](authorization.html) or must have the [`CREATEDB`](create-role.html#create-a-role-that-can-create-and-rename-databases) parameter set.
I think this is Admin, database owner, or CREATE. @arulajmani
There was a problem hiding this comment.
Sorry for taking so long to look at this, but while I have you here, I have one more privilege related change that might need capturing cockroachdb/cockroach#62250. I'll add a release note when I fix it, but just wanted to put it on your radar as well!
Reviewable status:
v21.1/add-region.md, line 32 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
This changed recently to be Admin, Owner, or CREATE. @arulajmani FYI.
Yup, this just went in cockroachdb/cockroach#62453 yesterday. Sorry about the late change 😅
v21.1/set-locality.md, line 35 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
This has also changed recently. It should be either admin, owner of the table, or create on the table. @arulajmani
FWIW this didn't change recently, I just moved some code around. It was always admin, owner of the table, or CREATE privilege.
v21.1/survive-failure.md, line 27 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
I think this is Admin, database owner, or CREATE. @arulajmani
Yup, this changed recently.
rmloveland
force-pushed
the
20210226-sql-add-region-set-locality-etc
branch
from
1745d73
to
7896a93
Compare
There was a problem hiding this comment.
TFTR Adam and Arul!
Arul, re cockroachdb/cockroach#62250 it will def get to me via the release notes process as you alluded to - I'll make sure that info goes into the SET PRIMARY REGION docs (yet to be written) - thanks for the pointer! I also added a note to #9299 for myself to make absolutely sure it's captured
Reviewable status:
v21.1/add-region.md, line 27 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: "The database to which you are adding the region"?
Done!
v21.1/add-region.md, line 28 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Do we want to say "Allowed values include any region present in
SHOW REGIONS FROM CLUSTER"? Might be more actionable than the way it's written right now.
Sounds good! Updated. Not a link yet but will become a link once those docs are written
v21.1/add-region.md, line 32 at r2 (raw file):
his changed recently to be Admin, Owner, or CREATE
OK! thanks for the heads up Adam!
Sorry about the late change
no problem, Arul! thanks for the PR link, reading the tests was helpful
Updated to say the following - let me know if it's slightly off somehow with the ands and ors:
"The user must be a member of the admin or owner roles, or have the CREATE privilege on the database."
v21.1/add-region.md, line 38 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Would this benefit from a bit more detail as to how adding and setting differ? The differences are pretty dramatic.
I would like to add that but in a later PR just for the sake of getting this one in - added a note to myself on to make absolutely sure this gets updated tho
Do you mind adding a comment to #9299 laying out the differences? if the answer is "re-read the relevant MR functional spec" that is also OK :-)
v21.1/add-region.md, line 67 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: The splitting of "add regions" and "that were defined at node startup time" by "to a multi-region database" is throwing me off a bit. What do you think about rewriting this to "Only regions that are defined at node startup time can be added to a multi-region database".
Sounds good to me - updated to use that phrasing
v21.1/add-region.md, line 101 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
I don't think this is entirely accurate. You can drop a primary region, so long as it's the last region defined on the database.
Ah, thanks! Removed that part, consolidating all into the note text discussed below
v21.1/add-region.md, line 104 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
See above. There won't be an error so long as you drop the primary region last.
Edited to say
"You can only drop the primary region from a multi-region database if it's the last remaining region. After that, the database will no longer be a multi-region database"
v21.1/multiregion-overview.md, line 64 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: One more instance of
..to...
Fixed
v21.1/multiregion-overview.md, line 97 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: more
..
Fixed this and all the others on this page - thanks!
v21.1/multiregion-overview.md, line 109 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: more
..
Done
v21.1/multiregion-overview.md, line 141 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: more
..
✔️
v21.1/set-locality.md, line 7 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit:
..
✔️
v21.1/set-locality.md, line 35 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
FWIW this didn't change recently, I just moved some code around. It was always admin, owner of the table, or CREATE privilege.
Thanks - updated to use the same description as from ADD REGION (modulo s/database/table)
v21.1/set-locality.md, line 47 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Do we want to mention somewhere that
REGIONAL BY TABLEdefaults toPRIMARY REGION?
Great, added a note to this section saying that
v21.1/set-locality.md, line 57 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: "For more information about how table localities work"?
Fixed!
v21.1/set-locality.md, line 100 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
It's more than just a different name. You can specify any column definition for the REGIONAL BY ROW AS column, so long as the column is of type
crdb_internal_region. This allows customers to have columns generated in any way they'd like. For example, we could modify movr to have a region column generated as:ALTER TABLE rides ADD COLUMN region crdb_internal_region AS ( CASE WHEN city = 'amsterdam' THEN 'europe-west1' WHEN city = 'paris' THEN 'europe-west1' WHEN city = 'rome' THEN 'europe-west1' WHEN city = 'new york' THEN 'us-east1' WHEN city = 'boston' THEN 'us-east1' WHEN city = 'washington dc' THEN 'us-east1' WHEN city = 'san francisco' THEN 'us-west1' WHEN city = 'seattle' THEN 'us-east1' WHEN city = 'los angeles' THEN 'us-east1' END ) STORED
Sweet! Very nice. Added basically all of this to the doc after the "you can use another name" bit - moved everything out of a note so it's not so visually noisy. Figure the people who just want another name can use just that, and the folks who want something more sophisticated can also have the info you provided. Thank you.
v21.1/survive-failure.md, line 7 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit:
..
Fixed
v21.1/survive-failure.md, line 27 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Yup, this changed recently.
Updated as in the other cases!
There was a problem hiding this comment.
Just a few nits, but
Reviewable status:
v21.1/add-region.md, line 28 at r2 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
Sounds good! Updated. Not a link yet but will become a link once those docs are written
If you want to link to something before the other docs are written, you could link to #view-a-databases-regions
v21.1/add-region.md, line 10 at r3 (raw file):
{{site.data.alerts.callout_info}} `ADD REGION` is a subcommand of [`ALTER DATABASE`](alter-database.html)
nit: missing period
v21.1/set-locality.md, line 10 at r3 (raw file):
{{site.data.alerts.callout_info}} `SET LOCALITY` is a subcommand of [`ALTER TABLE`](alter-table.html)
nit: missing period
v21.1/survive-failure.md, line 10 at r3 (raw file):
{{site.data.alerts.callout_info}} `SURVIVE {ZONE,REGION} FAILURE` is a subcommand of [`ALTER DATABASE`](alter-database.html)
nit: missing period
Summary of changes:
- Add docs for ALTER TABLE .. SET LOCALITY {REGIONAL,GLOBAL}
- Add docs for ALTER DATABASE .. ADD REGION (also PRIMARY REGION)
- Add docs for ALTER DATABASE .. SURVIVE {REGION,ZONE} FAILURE
- Edit the 'Multi-region Overview' page to point users to the SQL
reference pages above for syntax examples
Fixes #9096
Fixes #9115
Fixes #9298
Fixes #9301
Fixes #9598
Fixes #9610
Fixes #9617
Addresses #9341
Addresses #9708
Addresses #9709
Addresses #9884
Addresses #9886
rmloveland
force-pushed
the
20210226-sql-add-region-set-locality-etc
branch
from
7896a93
to
eb8c541
Compare
There was a problem hiding this comment.
TFTR Laurennnnnnn!!!!
Reviewable status:
v21.1/add-region.md, line 28 at r2 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
If you want to link to something before the other docs are written, you could link to
#view-a-databases-regions
Thanks Lauren, I think I'll punt for now until those are written!
v21.1/add-region.md, line 10 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
nit: missing period
Fixed!
v21.1/set-locality.md, line 10 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
nit: missing period
Fixed!
v21.1/survive-failure.md, line 10 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
nit: missing period
Fixed!
Summary of changes:
Add docs for ALTER TABLE .. SET LOCALITY {REGIONAL,GLOBAL}
Add docs for ALTER DATABASE .. ADD REGION (also PRIMARY REGION)
Add docs for ALTER DATABASE .. SURVIVE {REGION,ZONE} FAILURE
Edit the 'Multi-region Overview' page to point users to the SQL
reference pages above for syntax examples
Fixes #9096
Fixes #9115
Fixes #9298
Fixes #9301
Fixes #9598
Fixes #9610
Fixes #9617
Addresses #9341
Addresses #9708
Addresses #9709
Addresses #9884
Addresses #9886