sql: add syntax for experimental domiciling #68068
Conversation
|
This change is |
pawalt
force-pushed
the
domiciling_syntax
branch
from
81d4abf to
eb3f985
Compare
There was a problem hiding this comment.
Reviewed 3 of 7 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @pawalt)
pkg/sql/parser/sql.y, line 8398 at r1 (raw file):
| /* EMPTY */ { $$.val = tree.DataPlacementUnspecified
Could we just make this tree.DataPlacementDefault? Having a separate Unspecified type doesn't add much, does it?
pkg/sql/parser/testdata/create_database, line 350 at r1 (raw file):
CREATE DATABASE IF NOT EXISTS _ PRIMARY REGION _ -- identifiers removed error
Let's add some parser tests here too?
Also, in the alter_database file as well.
pkg/sql/sem/tree/data_placement.go, line 27 at r1 (raw file):
case DataPlacementDefault: ctx.WriteString("PLACEMENT DEFAULT") default:
You're missing a case for DtaPlacementUnspecified, but I think we should just get rid of it altogether.
pawalt
force-pushed
the
domiciling_syntax
branch
from
eb3f985 to
af6652c
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/parser/sql.y, line 8398 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Could we just make this
tree.DataPlacementDefault? Having a separateUnspecifiedtype doesn't add much, does it?
I did this the same way survival goals are implemented; having an unspecified type lets us know if the user tried to use PLACEMENT on a database that isn't multi-region. The naming is a bit weird because survival goals can use SurvivalGoalDefault but here DataPlacementDefault would be ambiguous, but it's the same effect.
pkg/sql/parser/testdata/create_database, line 350 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Let's add some parser tests here too?
Also, in the
alter_databasefile as well.
Added the create tests. I can't add the alter tests yet as it throws unimplemented errors. I'll add alter tests in the PR that stitches the syntax together with the logic.
pkg/sql/sem/tree/data_placement.go, line 27 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
You're missing a case for
DtaPlacementUnspecified, but I think we should just get rid of it altogether.
Added a section for unspecifed pending talk about using it.
There was a problem hiding this comment.
I'd encourage you to add support for parsing for ALTER DATABASE in this PR as well, but if you'd rather do it in a followup that's fine too. Looks like a few tests need a re-write, but other than that this PR looks good to me.
Reviewable status:
pkg/sql/parser/sql.y, line 8398 at r1 (raw file):
Previously, pawalt (Peyton Walters) wrote…
I did this the same way survival goals are implemented; having an unspecified type lets us know if the user tried to use
PLACEMENTon a database that isn't multi-region. The naming is a bit weird because survival goals can useSurvivalGoalDefaultbut hereDataPlacementDefaultwould be ambiguous, but it's the same effect.
That makes sense, didn't realize we actually needed it late.
pkg/sql/parser/testdata/create_database, line 350 at r1 (raw file):
Previously, pawalt (Peyton Walters) wrote…
Added the create tests. I can't add the alter tests yet as it throws unimplemented errors. I'll add alter tests in the PR that stitches the syntax together with the logic.
Is it worth tackling both the parsing of ALTER DATABASE and CREATE DATABASE in this PR? Or would you rather do the ALTER in a separate one?
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1, 2 of 2 files at r2.
Reviewable status:
pkg/sql/parser/sql.y, line 1657 at r2 (raw file):
} alter_database_placement_stmt:
add /* SKIP DOC */ here to hide it.
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
// Only want to print if the user has turned on restricted so that // SHOW CREATE DATABASE doesn't expose PLACEMENT. if node.Placement == DataPlacementRestricted {
should this be != DataPlacementDefault
pkg/sql/sem/tree/data_placement.go, line 27 at r1 (raw file):
Previously, pawalt (Peyton Walters) wrote…
Added a section for unspecifed pending talk about using it.
unspecified should just return early and not print imo.
|
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file): Previously, otan (Oliver Tan) wrote…
oops, |
pawalt
force-pushed
the
domiciling_syntax
branch
from
af6652c to
3d037c8
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/parser/sql.y, line 8398 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
That makes sense, didn't realize we actually needed it late.
Done.
pkg/sql/parser/sql.y, line 1657 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
add
/* SKIP DOC */here to hide it.
Done.
pkg/sql/parser/testdata/create_database, line 350 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Is it worth tackling both the parsing of
ALTER DATABASEandCREATE DATABASEin this PR? Or would you rather do theALTERin a separate one?
Makes sense to do both here. I had it in my head that if I created a tree.AlterDatabasePlacement, I'd have to fill in the alterDatabasePlacementNode and implementation, but realize that I can just parse now.
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
oops,
!= DataPlacementUnspecified
I don't think so because if we want to use the formatting from tree to generate SHOW CREATE DATABASE, we don't know if the placement policy we're reading off the table descriptor is "manually-input default" or "unspecified default". Since we don't want to expose PLACEMENT unless we're sure the user knows about it, we only want to write out PLACEMENT if we're in restricted mode.
pkg/sql/sem/tree/data_placement.go, line 27 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
unspecified should just return early and not print imo.
Yeah just made another case for it.
pawalt
force-pushed
the
domiciling_syntax
branch
from
3d037c8 to
5ecf79f
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r3, 1 of 1 files at r4.
Reviewable status:
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
Previously, pawalt (Peyton Walters) wrote…
I don't think so because if we want to use the formatting from
treeto generateSHOW CREATE DATABASE, we don't know if the placement policy we're reading off the table descriptor is "manually-input default" or "unspecified default". Since we don't want to expose PLACEMENT unless we're sure the user knows about it, we only want to write out PLACEMENT if we're in restricted mode.
I think in general we expand out things that would "have the same effect". For example, even if the Survival Goal wasn't explicitly set to "Zone", we'd expand it out to say so.
That being said, if we want to hide this feature as much as possible from our users, I think the conditional as is is fine.
pawalt
force-pushed
the
domiciling_syntax
branch
3 times, most recently
from
a4971bf to
c5c6bed
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/parser/sql.y, line 1668 at r5 (raw file):
/* SKIP DOC */ alter_database_placement_stmt:
it's still showing up in the documentation. i think it has to be after the line.
pkg/sql/parser/sql.y, line 8431 at r5 (raw file):
} opt_placement_clause:
we should place a SKIP DOC here too to hide it from appearing in documentation
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I think in general we expand out things that would "have the same effect". For example, even if the Survival Goal wasn't explicitly set to "Zone", we'd expand it out to say so.
That being said, if we want to hide this feature as much as possible from our users, I think the conditional as is is fine.
i'd rather hide it if the user did not specify it on the AST. This is what we did for survive (which is why we have unspecified in the first place).
|
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file): Previously, otan (Oliver Tan) wrote…
If we're not showing PLACEMENT DEFAULT if the user specifies it (which I am not advocating for), we should get rid of DataPlacementUnspecified. |
There was a problem hiding this comment.
Reviewable status:
docs/generated/sql/bnf/stmt_block.bnf, line 2287 at r5 (raw file):
placement_clause ::= 'PLACEMENT' 'RESTRICTED' | 'PLACEMENT' 'DEFAULT'
See my comment on the other PR. I'm wondering if we should move away from using DEFAULT here.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
If we're not showing PLACEMENT DEFAULT if the user specifies it (which I am not advocating for), we should get rid of DataPlacementUnspecified.
I thought we needed unspecified for survive because we need the distinction between unspecified and default in CREATE DATABASE here:
cockroach/pkg/sql/create_database.go
Line 89 in 0388efe
PLACEMENT syntax, right?
Regardless, I don't have strong opinions here. If we care about SHOW CREATE DATABASE not expanding placement in the default case we can do it in other ways as well.
This commit adds the PLACEMENT [RESTRICTED | DEFAULT] flag to CREATE DATABASE and adds the ALTER DATABASE PLACEMENT statement. These will be used to support cockroachdb#59756. Release note: None
pawalt
force-pushed
the
domiciling_syntax
branch
from
c5c6bed to
84e4d41
Compare
There was a problem hiding this comment.
Reviewable status:
docs/generated/sql/bnf/stmt_block.bnf, line 2287 at r5 (raw file):
Previously, ajstorm (Adam Storm) wrote…
See my comment on the other PR. I'm wondering if we should move away from using DEFAULT here.
I don't have strong opinions on it, but I don't think DEFAULT that confusing, especially if PLACEMENT is a feature hidden behind a cluster setting. EVERYWHERE/ALL_REGIONS seems ambiguous (everywhere in db or everywhere in cluster?).
This is very subjective, though, so if people feel strongly about this and we can find syntax that means "every database region", I'm happy to change.
pkg/sql/parser/sql.y, line 1668 at r5 (raw file):
Previously, otan (Oliver Tan) wrote…
it's still showing up in the documentation. i think it has to be after the line.
I think it worked this time.
pkg/sql/parser/sql.y, line 8431 at r5 (raw file):
Previously, otan (Oliver Tan) wrote…
we should place a SKIP DOC here too to hide it from appearing in documentation
Done.
pkg/sql/sem/tree/create.go, line 117 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I thought we needed unspecified for survive because we need the distinction between unspecified and default in CREATE DATABASE here:
. We'll want something similar for thiscockroach/pkg/sql/create_database.go
Line 89 in 0388efe
PLACEMENTsyntax, right?Regardless, I don't have strong opinions here. If we care about
SHOW CREATE DATABASEnot expanding placement in the default case we can do it in other ways as well.
Yep we need unspecified for checking "was PLACEMENT used with no primary region" and "was PLACEMENT used with the cluster setting not set".
I've made the modification to print on either DEFAULT or RESRICTED, and I've got another path I can go down to make sure we don't expose PLACEMENT on SHOW CREATE DATABASE.
There was a problem hiding this comment.
Reviewable status:
docs/generated/sql/bnf/stmt_block.bnf, line 2287 at r5 (raw file):
Previously, pawalt (Peyton Walters) wrote…
I don't have strong opinions on it, but I don't think DEFAULT that confusing, especially if PLACEMENT is a feature hidden behind a cluster setting. EVERYWHERE/ALL_REGIONS seems ambiguous (everywhere in db or everywhere in cluster?).
This is very subjective, though, so if people feel strongly about this and we can find syntax that means "every database region", I'm happy to change.
I agree with you that the hiddenness helps with the risk. My main concern is that if we want to change the "default" behaviour at some point, DEFAULT becomes a bit problematic.
I do agree with you on the ambiguity of the options I suggested. What about something super descriptive like REPLICAS_IN_ALL_REGIONS? It does leave RESTRICTED as feeling a bit underdescribed though. TBH, I don't have a good solution here, but DEFAULT leaves me a bit uneasy.
There was a problem hiding this comment.
Talked offline, the use of "DEFAULT" in this context is probably not right but we can (and will) come back to change the exact syntax later. For now, to unblock the other PRs on top of it, let's merge this in as is.
Reviewable status:
|
bors r=otan,arulajmani |
|
Build failed (retrying...): |
|
Build succeeded: |
The parser was updated in cockroachdb#68068 to support a new syntax for altering database placement: ALTER DATABASE d SET PLACEMENT DEFAULT ALTER DATABASE d SET PLACEMENT RESTRICTED Running these statements causes internal errors because there is no execution support for them yet. This commit prevents an internal error when they are executed, and returns a user-friendly error instead. Informs cockroachdb#65475 Release note: None
67547: auth: set up periodic deletion of old sessions in the `web_sessions` system table r=knz,mberhault a=cameronnunez Fixes [#51169](#51169). Expired sessions are not cleaned up in the web_sessions system table quickly enough. The table should be kept from growing indefinitely in the long run. This patch sets up periodic deletion of these expired sessions. Release note (security update): Old authentication web session rows in the system.web_sessions table no longer accumulate indefinitely in the long run. These rows are periodically deleted. Refer to the reference docs for details about the new cluster settings for system.web_sessions. 68310: util/cgroups: method to read file-backed memory on inactive LRU list r=abarganier a=abarganier util/cgroups: method to read file-backed memory on inactive LRU list Currently we have methods that allows us to read the cgroup memory limit, as well as the current memory usage, for processes running in unix containers. However, to more accurately determine the current memory usage in the eyes of the container provider, we must subtract the "cache usage" from the total memory usage, which is represented by the inactive file-backed memory stat. From the Docker documentation: "On Linux, the Docker CLI reports memory usage by subtracting cache usage from the total memory usage. The API does not perform such a calculation but rather provides the total memory usage and the amount from the cache so that clients can use the data as needed. The cache usage is defined as the value of total_inactive_file field in the memory.stat file on cgroup v1 hosts...On cgroup v2 hosts, the cache usage is defined as the value of inactive_file field." https://docs.docker.com/engine/reference/commandline/stats/#extended-description In an effort to gain better observability into current memory usage in the eyes of the container provider for purposes of identifying whether or not a CRDB node is on its way to OOM, we add the ability to read these values from the memory subsystem. This heuristic can then be used in debug tools such as periodic query dumps and eventually, more generalized crash dump platforms. Informs #66901 68337: importccl: only initialize progress fields on first import attempt r=pbardea a=pbardea Fixes #68247. Release note (bug fix): Fix a bug where IMPORT would incorrectly reset its progress upon resumption. 68394: storage: elide nonexistent files from registry r=jbowens a=jbowens When the encryption-at-rest registry is loaded, elide any file entries corresponding to files that do not exist on the filesystem. These entries may exist because files were manually deleted by an operator, an operation to update the registry failed or because the files were deleted through a codepath that failed to update the registry. Release note (bug fix): Fixes a bug where encryption-at-rest registry would accumulate nonexistent file entries forever, contributing to filesystem operations' latency on the store. ---- I'd like to follow this up with a change that implements `RemoveAll` on `encryptedFS`. It's not as straightforward of a change. This patch at least ensures process restarts clear all accumulated garbage. 68697: opt: add format=hide-hist option r=mgartner a=mgartner The `format=hide-hist` option for optimizer tests has been added. It allows stats to be shown in optimizer test output without histograms. This is useful during debugging when you want to view stats like row count, but do not want the clutter of histograms. Additionally, using `format=show-stats` with `optstepsweb` creates base-64 encoded URLs longer than the maximum URL length supported by browsers (typically ~2000 characters). You can now use `optstepsweb format=(show-stats,hide-hist)` to view high-level statistics in `optstepsweb`. Release note: None 68743: sql: prevent internal error when altering database placement r=mgartner a=mgartner The parser was updated in #68068 to support a new syntax for altering database placement: ALTER DATABASE d SET PLACEMENT DEFAULT ALTER DATABASE d SET PLACEMENT RESTRICTED Running these statements causes internal errors because there is no execution support for them yet. This commit prevents an internal error when they are executed, and returns a user-friendly error instead. Informs #65475 Release note: None 68796: sql: fix panic using Reset() when using sqlstats iterator r=knz,maryliag a=Azhng Previously, when if SQLStats.Reset() is called while iterating through an iterator, this will cause panics since the iterator will then return a nil pointer. This commit changed the iterator to gracefully handle this situation and prevents panic. Resolves #68785 Release note: None Co-authored-by: Cameron Nunez <[email protected]> Co-authored-by: Alex Barganier <[email protected]> Co-authored-by: Paul Bardea <[email protected]> Co-authored-by: Jackson Owens <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Azhng <[email protected]>
This commit adds the PLACEMENT [RESTRICTED | DEFAULT] flag to CREATE
DATABASE and adds the ALTER DATABASE PLACEMENT statement. These will
be used to support #59756.
Release note: None
This PR was split out from #67946.