Skip to content

sql: fix username parsing for CURRENT_USER/SESSION_USER #70439

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:fix_username_parsing_09202021
Oct 12, 2021
Merged

sql: fix username parsing for CURRENT_USER/SESSION_USER #70439
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:fix_username_parsing_09202021

Conversation

@RichardJCai
Copy link
Copy Markdown
Contributor

@RichardJCai RichardJCai commented Sep 20, 2021 •
edited
Loading

Release note (sql change): Fix bug where previously CURRENT_USER and
SESSION_USER were parsed incorrectly.

Fixes #54696

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Sep 20, 2021

This change is Reviewable

@RichardJCai RichardJCai force-pushed the fix_username_parsing_09202021 branch 3 times, most recently from cbcc861 to 937f033 Compare September 21, 2021 18:23
@RichardJCai RichardJCai changed the title WIP: Refactor username parsing, support CURRENT_USER/SESSION_USER sql: fix username parsing for CURRENT_USER/SESSION_USER Sep 21, 2021
@RichardJCai RichardJCai requested review from a team, knz and rafiss September 21, 2021 18:24
@RichardJCai RichardJCai marked this pull request as ready for review September 21, 2021 18:24
@RichardJCai RichardJCai requested review from a team and dt and removed request for a team September 21, 2021 18:24
knz
knz reviewed Sep 21, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

I reviewed the tree and parser changes only. Generally OK, but I think there's some unnecessary complexity related to the formatting code.

Reviewed 23 of 38 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt, @rafiss, and @RichardJCai)

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

type AlterDatabaseOwner struct {
	Name Name
	// TODO(solon): Adjust this, see

Here and elsewhere, remove the TODOs that refer to this specific issue.

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

// FormatRoleSpec formats a role specification safely.
func (ctx *FmtCtx) FormatRoleSpec(spec RoleSpec) {

I see you also have a Format method on type RoleSpec
Then you don't need this one. Instead, do ctx.FormatNode(&n.RoleSpec) on the other nodes, which will also be easier to read and less error-prone.

@RichardJCai RichardJCai force-pushed the fix_username_parsing_09202021 branch 3 times, most recently from 4819ae0 to 2064b15 Compare September 22, 2021 16:57
@dt
Copy link
Copy Markdown
Contributor

dt commented Sep 22, 2021

how disruptive to your work would it be to wait on merging sweeping refactors like this, at least if they're also hitting bulk io's packages, until we have a 21.2 RC cut? some teams, mine included, are trying to minimize refactor churn until RC 1 is tagged, so that if we need to prepare an urgent fix for a release-blocking issue, we don't have as much risk of unclean backports delaying it further

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Sep 22, 2021

how disruptive to your work would it be to wait on merging sweeping refactors like this, at least if they're also hitting bulk io's packages, until we have a 21.2 RC cut? some teams, mine included, are trying to minimize refactor churn until RC 1 is tagged, so that if we need to prepare an urgent fix for a release-blocking issue, we don't have as much risk of unclean backports delaying it further

This can definitely wait, I'm actually just picking this up and other smaller issues because I'm waiting on the 21.2 RC cut to merge my other huge PR.

rafiss
rafiss approved these changes Sep 23, 2021
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.

this is great! just had a nit, then feel free to merge after 21.2 RC

after this are there any other usages of name_list or name for usernames in sql.y?

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

pkg/sql/sem/tree/create.go, line 1577 at r2 (raw file):

	// TODO(solon): Adjust this, see
	// https://github.com/cockroachdb/cockroach/issues/54696
	AuthRole RoleSpec

nit: remove the todo

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Oct 11, 2021

This should be ok to merge now right?

knz
knz reviewed Oct 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

This should be ok to merge now right?

I don't have further comments, but there's still one unaddressed comment by rafi.

Reviewed 1 of 38 files at r1, 8 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt, @knz, and @RichardJCai)

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Oct 11, 2021

This should be ok to merge now right?

I don't have further comments, but there's still one unaddressed comment by rafi.

Reviewed 1 of 38 files at r1, 8 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt, @knz, and @RichardJCai)

Ah yeah I haven't made those changes yet, more asking in reference to about waiting for 21.2 RC before merging this,

@dt
Copy link
Copy Markdown
Contributor

dt commented Oct 11, 2021 •
edited
Loading

asking in reference to about waiting for 21.2 RC before merging this

I no longer have any concern about refactor churn, at least w.r.t. bulk's packages, so no objection here.

@dt dt removed their request for review October 11, 2021 16:58
@RichardJCai RichardJCai force-pushed the fix_username_parsing_09202021 branch from 2064b15 to 5f83110 Compare October 11, 2021 17:44
@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Oct 11, 2021

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 11, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 12, 2021

Build failed:

@RichardJCai RichardJCai force-pushed the fix_username_parsing_09202021 branch 2 times, most recently from 60e09ec to 439d758 Compare October 12, 2021 16:02
@RichardJCai
sql: fix username parsing for CURRENT_USER/SESSION_USER
d13a9a3 
Release note (sql change): Fix bug where previously CURRENT_USER and
SESSION_USER were parsed incorrectly.
@RichardJCai RichardJCai force-pushed the fix_username_parsing_09202021 branch from 439d758 to d13a9a3 Compare October 12, 2021 18:01
@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Oct 12, 2021

Fixed merge skew
bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 12, 2021

Build succeeded:

@craig craig bot merged commit 9dc17a3 into cockroachdb:master Oct 12, 2021
@cockroach-teamcity cockroach-teamcity mentioned this pull request Oct 12, 2021
Closed
@rafiss rafiss mentioned this pull request Nov 4, 2021
Open
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

+1 more reviewer

@knz knz knz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sql: OWNED BY SESSION_USER/CURRENT_USER does not work

5 participants

@RichardJCai @cockroach-teamcity @dt @knz @rafiss