Skip to content

sql,ccl: improve the implementation of the TENANT clauses in sql syntax#77218

Merged
craig[bot] merged 4 commits intocockroachdb:masterfrom
knz:20220301-tenant-id
Mar 6, 2022
Merged

sql,ccl: improve the implementation of the TENANT clauses in sql syntax#77218
craig[bot] merged 4 commits intocockroachdb:masterfrom
knz:20220301-tenant-id

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Mar 1, 2022 •
edited
Loading

This ensures that the tenant ID is subject to formatting flags.

Release justification: bug fix

Release note: None

@knz knz requested review from a team, ajstorm and rafiss March 1, 2022 14:35
@knz knz requested a review from a team as a code owner March 1, 2022 14:35
@knz knz requested review from shermanCRL and removed request for a team March 1, 2022 14:35
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Mar 1, 2022

This change is Reviewable

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 1, 2022

I'm not super well-versed in the backup code, but it looks to me that making this support more complex expressions is actually not too hard. Might be more elegant!

@shermanCRL shermanCRL requested review from a team and stevendanna and removed request for shermanCRL March 1, 2022 16:01
rafiss
rafiss approved these changes Mar 2, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this change is still an improvement, so i vote to merge this

does it need to change to account for the cluster setting syntax that just merged?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @knz, and @stevendanna)

pkg/sql/sem/tree/tenant.go, line 28 at r1 (raw file):

	// the case when the value was anonymized. In other places, the
	// value used for anonymized integer literals is 0, but we can't use
	// 0 for TenantID as this is refused during parsing, nor can we use

do we have a good reason for rejecting 0 at the parsing step rather than rejecting it during semantic analysis?

knz
knz commented Mar 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @rafiss, and @stevendanna)

pkg/sql/sem/tree/tenant.go, line 28 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

do we have a good reason for rejecting 0 at the parsing step rather than rejecting it during semantic analysis?

IMHO, no, but as long as the tree object embeds a roachpb.TenantID, it's going to be checked in roachpb.MakeTenantID.

My preference would be for this to use an Expr, and have the translation Expr -> TenantID done at planning time (with expr eval).

@knz knz force-pushed the 20220301-tenant-id branch from a390526 to 77b4ca3 Compare March 5, 2022 11:30
@knz
sql,ccl: make backup target TenantID a tree.NodeFormatter
77b4ca3 
This ensures that the tenant ID is subject to formatting flags.

This commit is sadly much larger than it deserves to be, due to a
unique combination of factors:

- the TENANT clause is one of the extremely few other clauses
  that specify anonymizable integer literals, and do not (currently)
  support arbitrary expressions.
  (The other clauses are the SEQUENCE caching options.)

  Due to this special case, we cannot rely on the existing machinery
  in `fmtHideConstants()` and need to handle the anonymization in the
  TenantID's Format method directly.

- the TENANT clause is the only clause where the default value
  is not parseable: the value 0 causes a syntax error at parse-time,
  not at plan-time like other clauses that accept numeric arguments.

  This makes it impossible to use the value 0 as anonymized value.

  Due to this special case, we need to add a new parsing rule
  for the anonymized value of the TENANT clause alongside the main
  rule.
  This complexity would not be needed if the value 0 could be
  parsed and the validation delayed to plan-time.
  We could also remove it if the TENANT clause properly accepted
  expressions.

Release justification: bug fix

Release note: None
@knz knz requested a review from a team March 5, 2022 12:07
knz
knz commented Mar 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need to change to account for the cluster setting syntax that just merged?

Oh good catch. That's a different situation though. This can be made to support expressions right away. Actually, the way this syntax was implemented... 🤦‍♂️ Good time to clean it up. Done.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @rafiss, and @stevendanna)

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

bors r=rafiss

@knz knz changed the title sql,ccl: make backup target TenantID a tree.NodeFormatter sql,ccl: improve the implementation of the TENANT clauses in sql syntax Mar 5, 2022
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 5, 2022

Build failed:

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

Just noticed SHOW CLUSTER SETTING FOR TENANT was affected too. Fixing that as well.

@knz knz force-pushed the 20220301-tenant-id branch 2 times, most recently from e703cd6 to 478021e Compare March 5, 2022 14:27
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 5, 2022

Build failed:

@knz knz force-pushed the 20220301-tenant-id branch 2 times, most recently from 4489163 to 9888484 Compare March 5, 2022 17:44
@knz knz requested a review from a team as a code owner March 5, 2022 17:44
knz added 2 commits March 5, 2022 18:44
@knz
sql: fix the syntax for the tenant-specific setting statements
cbbc843 
Release justification: low risk, high benefit changes to existing functionality

Release note: None
@knz
sql: fix the syntax for the tenant-specific show cluster settings stmt
9888484 
Release justification:  low risk, high benefit changes to existing functionality

Release note: None
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 5, 2022

Build failed:

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

This is failing in CI in the backupccl package, seemingly for unrelated reasons? Unsure. Will need to look next week.

@knz
backupccl: skip long test under race
2ec4b38 
Release justification:  non-production code changes

Release note: None
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 5, 2022

added a skip

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 6, 2022

Build succeeded:

@craig craig bot merged commit 608369b into cockroachdb:master Mar 6, 2022
@knz knz deleted the 20220301-tenant-id branch March 6, 2022 08:43
@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Mar 7, 2022

I noticed this changed the syntax to ALTER ALL TENANTS instead of ALTER TENANT ALL. Is that required?

I guess it's not a big deal, but ALTER ALL TENANTS is misaligned from other syntax we've implemented, like ALTER ROLE ALL ... SET .... (That syntax was borrowed from Postgres.)

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 7, 2022

yes I changed this to avoid a syntax ambiguity (all is a valid identifier)

I guess it's not a big deal, but ALTER ALL TENANTS is misaligned from other syntax we've implemented, like ALTER ROLE ALL

AFAIK ROLE does not accept an expression in pg's dialect? And if we wanted it to, it'd probably be painful?

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 7, 2022

The alternative was TENANT * since * is an expression. But I thought this would feel a little too "magic".

@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Mar 7, 2022

Can't we use a lookahead, like we do for RESET ALL and ROLE ALL?

cockroach/pkg/sql/parser/lexer.go

Lines 123 to 131 in fc7b49f

case RESET:
switch nextID {
case ALL:
lval.id = RESET_ALL
}
case ROLE:
switch nextID {
case ALL:
lval.id = ROLE_ALL

I bring this up since the syntax was discussed at length in the RFC discussion, so I'd prefer to have this match where the consensus landed.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 7, 2022

I was not aware of this lookahead technique (or, I had forgotten it)
In any case, I felt confident merging this because this feature is not restricted by backward-compatibility. If you think that's important, feel free to propose the LA solution.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 7, 2022

(regardless of the RFC, SQL is meant to be "readable english", and I find "TENANT ALL" less grammatical and less readable than "ALL TENANTS")

@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Mar 7, 2022

Sure, I don't mind making that PR. Just wanted to understand the context before I did.

@knz knz mentioned this pull request Mar 11, 2022
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rafiss rafiss rafiss approved these changes

@ajstorm ajstorm Awaiting requested review from ajstorm

@stevendanna stevendanna Awaiting requested review from stevendanna

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@knz @cockroach-teamcity @rafiss