sql: add builtin to request statement bundles #79693

Azhng · 2022-04-08T20:28:26Z

Makes #79547 less painful.

Previously, there was no way to request statement bundle builtins for
internal statements.
This commit introduces crdb_internal.request_statement_bundle builtin
which allows statement bundle being requested from SQL CLI. The new
builtin takes three parameters:

statement fingerprint text
minimum execution latency for the statement
length of duration the statement bundle request will stay valid for
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Release note (sql change): new crdb_internal.request_statement_bundle builtin
allows statement bundle being requested from SQL CLI. The new builtin takes
three parameters:

statement fingerprint text
minimum execution latency for the statement
length of duration the statement bundle request will stay valid for.
VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has
VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

cockroach-teamcity · 2022-04-08T20:28:34Z

This change is

matthewtodd

Reviewable status: complete! 1 of 0 LGTMs obtained

Azhng · 2022-04-13T18:33:50Z

pkg/sql/sem/builtins/builtins.go

+			ReturnType: tree.FixedReturnType(types.Bool),
+			Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				hasViewActivityRedacted, err := evalCtx.SessionAccessor.HasRoleOption(
+					evalCtx.Ctx(), roleoption.VIEWACTIVITYREDACTED)


Note to self: this is problematic, view activity redacted should not be able to request statement bundles

maryliag

Reviewed 4 of 6 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

-- commits, line 23 at r1:
VIEWACTIVITYREDACTED should not have access to statement bundle, only VIEWACTIVITY

pkg/sql/sem/builtins/builtins.go, line 6894 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Note to self: this is problematic, view activity redacted should not be able to request statement bundles

from your comment, this should not be here, VIEWACTIVITYREDACTED should not have access to statement bundle, so check for admin and VIEWACTIVITY
we do have function about require VIEWACTIVITY and NOT VIEWACTIVITYREDACTED

pkg/server/status_test.go, line 2753 at r1 (raw file):

	})

	t.Run("builtin", func(t *testing.T) {

after you make the changes to allow only admin and VIEWACTIVITY, create a test that checks those have access but that it will fail for VIEWACTIVITYREDACTED

otan · 2022-04-25T23:48:32Z

pkg/sql/sem/builtins/builtins.go

+
+				return tree.DBoolTrue, nil
+			},
+			Volatility: tree.VolatilityVolatile,


we should probably add an info field

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @otan)

-- commits line 23 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

VIEWACTIVITYREDACTED should not have access to statement bundle, only VIEWACTIVITY

Done.

pkg/sql/sem/builtins/builtins.go line 6894 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

from your comment, this should not be here, VIEWACTIVITYREDACTED should not have access to statement bundle, so check for admin and VIEWACTIVITY
we do have function about require VIEWACTIVITY and NOT VIEWACTIVITYREDACTED

Done.

pkg/sql/sem/builtins/builtins.go line 6925 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

we should probably add an info field

Done.

pkg/server/status_test.go line 2753 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

after you make the changes to allow only admin and VIEWACTIVITY, create a test that checks those have access but that it will fail for VIEWACTIVITYREDACTED

Added the check in the TestCreateStatementDiagnosticsReportWithViewActivityOptions below

maryliag

small nits from me

Reviewed 2 of 8 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, and @otan)

pkg/sql/sem/builtins/builtins.go line 6894 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Done.

you can move this part to just below the if !hasViewActivity { check, so you don't need to make this check if the user is not admin or doesn't have view activity role

pkg/sql/sem/builtins/builtins.go line 7004 at r2 (raw file):

				if !hasViewActivity {
					return nil, errors.New("requesting statement bundle requires " +
						"VIEWACTIVITY option")

nit: VIEWACTIVITY or ADMIN

pkg/sql/sem/builtins/builtins.go line 7006 at r2 (raw file):

						"VIEWACTIVITY option")
				}

move to here

pkg/sql/sem/builtins/builtins.go line 7029 at r2 (raw file):

			Volatility: volatility.Volatile,
			Info: `Used to request statement bundle for a given statement fingerprint
that has execution latency than the 'minExecutionLatency'. If the 'expiresAfter'

nit: ...latency greater than the...

pkg/sql/sem/eval/context.go line 208 at r2 (raw file):

	// StmtDiagnosticsRequestInserter is used by the
	// crdb_internal.request_statement_bundle builtin to insert statement bundle

nit: insert a statement

pkg/sql/sem/eval/deps.go line 494 at r2 (raw file):

// StmtDiagnosticsRequestInsertFunc is an interface embedded in EvalCtx that can
// be used by the builtins to insert statement diagnostics request. This

nit: insert a statement

pkg/server/status_test.go line 2810 at r2 (raw file):

		"SELECT crdb_internal.request_statement_bundle('SELECT _', 0::INTERVAL, 0::INTERVAL)",
	)
	require.Contains(t, err.Error(), "requesting statement bundle requires VIEWACTIVITY option")

nit: or ADMIN

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @otan)

pkg/sql/sem/builtins/builtins.go line 7004 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: VIEWACTIVITY or ADMIN

Done.

pkg/sql/sem/builtins/builtins.go line 7006 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

move to here

Done.

pkg/sql/sem/builtins/builtins.go line 7029 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: ...latency greater than the...

Done.

pkg/sql/sem/eval/context.go line 208 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: insert a statement

Done.

pkg/sql/sem/eval/deps.go line 494 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: insert a statement

Done.

pkg/server/status_test.go line 2810 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: or ADMIN

Done.

maryliag

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @otan)

Makes cockroachdb#79547 less painful. Previously, there was no way to request statement bundle builtins for internal statements. This commit introduces crdb_internal.request_statement_bundle builtin which allows statement bundle being requested from SQL CLI. The new builtin takes three parameters: * statement fingerprint text * minimum execution latency for the statement * length of duration the statement bundle request will stay valid for VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin. Release note (sql change): new crdb_internal.request_statement_bundle builtin allows statement bundle being requested from SQL CLI. The new builtin takes three parameters: * statement fingerprint text * minimum execution latency for the statement * length of duration the statement bundle request will stay valid for. VIEWACTIVITY or ADMIN role option is required to use this builtin. If user has VIEWACTIVITYREDACTED role option, user is not allowed to use this builtin.

Azhng · 2022-05-06T22:23:42Z

TFTR!

bors r+

craig · 2022-05-07T00:28:13Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl · 2022-05-07T00:28:23Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from ce606f5 to blathers/backport-release-22.1-79693: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

Azhng requested a review from a team April 8, 2022 20:28

Azhng force-pushed the builtin-diag branch from 5113301 to 01b4168 Compare April 11, 2022 15:41

Azhng marked this pull request as ready for review April 11, 2022 17:32

Azhng requested a review from a team as a code owner April 11, 2022 17:32

Azhng requested a review from a team April 11, 2022 17:32

Azhng requested a review from a team as a code owner April 11, 2022 17:32

matthewtodd approved these changes Apr 12, 2022

View reviewed changes

Azhng commented Apr 13, 2022

View reviewed changes

maryliag suggested changes Apr 20, 2022

View reviewed changes

otan approved these changes Apr 25, 2022

View reviewed changes

otan requested review from otan and removed request for otan April 25, 2022 23:49

Azhng force-pushed the builtin-diag branch 4 times, most recently from abf6a5d to d5c5152 Compare May 6, 2022 18:08

Azhng commented May 6, 2022

View reviewed changes

Azhng added the backport-22.1.x label May 6, 2022

maryliag reviewed May 6, 2022

View reviewed changes

Azhng force-pushed the builtin-diag branch 2 times, most recently from cc41448 to f248c50 Compare May 6, 2022 19:58

Azhng commented May 6, 2022

View reviewed changes

maryliag approved these changes May 6, 2022

View reviewed changes

Azhng force-pushed the builtin-diag branch from f248c50 to ce606f5 Compare May 6, 2022 20:25

craig bot merged commit e04373b into cockroachdb:master May 7, 2022

cockroach-teamcity mentioned this pull request May 7, 2022

sql: add builtin to request statement bundles cockroachdb/docs#13828

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add builtin to request statement bundles #79693

sql: add builtin to request statement bundles #79693

Azhng commented Apr 8, 2022 •

edited

Loading

cockroach-teamcity commented Apr 8, 2022

matthewtodd left a comment

Azhng Apr 13, 2022

maryliag left a comment

otan Apr 25, 2022

Azhng left a comment

maryliag left a comment

Azhng left a comment

maryliag left a comment

Azhng commented May 6, 2022

craig bot commented May 7, 2022

blathers-crl bot commented May 7, 2022

sql: add builtin to request statement bundles #79693

sql: add builtin to request statement bundles #79693

Conversation

Azhng commented Apr 8, 2022 • edited Loading

cockroach-teamcity commented Apr 8, 2022

matthewtodd left a comment

Choose a reason for hiding this comment

Azhng Apr 13, 2022

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

otan Apr 25, 2022

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

Azhng commented May 6, 2022

craig bot commented May 7, 2022

blathers-crl bot commented May 7, 2022

Azhng commented Apr 8, 2022 •

edited

Loading