sql,streamingest: introduce ALTER TENANT ... PAUSE/RESUME REPLICATION#92363
sql,streamingest: introduce ALTER TENANT ... PAUSE/RESUME REPLICATION#92363craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
45f51a6 to
20dca78
Compare
pkg/sql/parser/sql.y
Outdated
| // ALTER TENANT '<tenant_name>' PAUSE REPLICATION | ||
| // ALTER TENANT '<tenant_name>' RESUME REPLICATION | ||
| alter_tenant_replication_stmt: | ||
| ALTER TENANT sconst_or_placeholder PAUSE REPLICATION |
There was a problem hiding this comment.
@knz / @dt or anyone else who better understands the rules we have to accept an expression, is sconst_or_placeholder the correct rule to accept a tenant name. Currently, CREATE TENANT accepts a name but we want to change that as well so that we can support placeholders, Additionally, we want to reduce the possibility of us misquoting these names when storing them in the system table (an error I already made in another PR and has been fixed since).
lidorcarmel
left a comment
There was a problem hiding this comment.
looks good, a couple of small questions.
| {`ALTER TENANT ALL RESET ??`, `ALTER TENANT CLUSTER SETTING`}, | ||
|
|
||
| {`ALTER TENANT 'foo' RESUME REPLICATION ??`, `ALTER TENANT REPLICATION`}, | ||
| {`ALTER TENANT 'foo' PAUSE REPLICATION ??`, `ALTER TENANT REPLICATION`}, |
There was a problem hiding this comment.
{
ALTER TENANT ALL ??,ALTER TENANT CLUSTER SETTING},
{ALTER TENANT ALL SET ??,ALTER TENANT CLUSTER SETTING},
{ALTER TENANT ALL RESET ??,ALTER TENANT CLUSTER SETTING},
{ALTER TENANT 'foo' RESUME REPLICATION ??,ALTER TENANT REPLICATION},
{ALTER TENANT 'foo' PAUSE REPLICATION ??,ALTER TENANT REPLICATION},
shouldn't the keys on the right be ALTER TENANT?
There was a problem hiding this comment.
I believe the way to read this test is the more tokens you type on the left the more specific help text you get, atleast thats my understanding 😅 . So an ALTER TENANT ALL ?? is currently only supported for altering a tenant cluster setting and so that's the help text we see.
| alter_ddl_stmt // help texts in sub-rule | ||
| | alter_role_stmt // EXTEND WITH HELP: ALTER ROLE | ||
| | alter_tenant_csetting_stmt // EXTEND WITH HELP: ALTER TENANT | ||
| | alter_tenant_stmt /* SKIP DOC */ |
There was a problem hiding this comment.
why SKIP? I thought we decided to keep all of these? here and below.
There was a problem hiding this comment.
I thought we were starting with SKIP DOC but don't skip the help text?
There was a problem hiding this comment.
I think we don't skip for create/drop/show tenant now..
There was a problem hiding this comment.
Yup i have this PR I need to cleanup - #91995, let me go do that.
stevendanna
left a comment
There was a problem hiding this comment.
Thanks for getting this going. Looks good overall, but just a few nits.
6f520da to
8357cfc
Compare
|
friendly ping @stevendanna / @lidorcarmel |
|
The CI race failure is interestingggg, some sort of data race. Looking into it. |
|
stress race on TFTR! bors r=lidorcarmel |
|
👎 Rejected by code reviews |
|
bors r=lidorcarmel |
|
👎 Rejected by code reviews |
|
boo, maybe I need @stevendanna's stamp |
This change introduces: `ALTER TENANT <name> RESUME REPLICATION` `ALTER TENANT <name> PAUSE REPLICATION` These statements can be used to pause and resume the replication job if the tenant is being replicated into. Fixes: cockroachdb#91246 Release note: None
8357cfc to
a90b0f7
Compare
|
bors r=lidorcarmel |
|
Build succeeded: |
This change introduces:
ALTER TENANT <name> RESUME REPLICATIONALTER TENANT <name> PAUSE REPLICATIONThese statements can be used to pause and resume the replication job if the tenant is being replicated into.
Fixes: #91246
Release note: None