sql: SHOW QUERIES lazily interpolates placeholders #86968

jordanlewis · 2022-08-26T22:09:45Z

SHOW QUERIES (and crdb_internal.node_queries, cluster_queries)
interpolates placeholder values into the statement so that it is
possible to see the placeholder values of a prepared statement - but it
used to do this unconditionally during statement execution.

This is an expensive process that spends a lot of CPU for little reason,
since the interpolation was happening in the hot path of every query.

Now, we include the placeholder values as a separate array in the
internal representation of active queries, and interpolate the values
only when they're being pulled out to examine, to avoid the
unconditional runtime interpolation costs.

As a side effect of this change, the original comments in a query are
now included in SHOW QUERIES and the two active queries tables.

Release note (sql change): the query field in the
crdb_internal.node_queries, crdb_internal.cluster_queries, and SHOW
QUERIES commands now includes the original comments in the queries.

Informs CRDB-17299

cockroach-teamcity · 2022-08-26T22:09:57Z

This change is

rafiss

nice improvements! both changes need tests

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

pkg/sql/conn_executor.go line 3163 at r1 (raw file):

		placeholders := make([]string, nPlaceholders)
		for i := range placeholders {
			placeholders[i] = query.placeholders.Values[i].String()

nit: maybe use tree.AsStringWithFlags(..., tree.FmtPgwireText) here

pkg/sql/crdb_internal.go line 1894 at r1 (raw file):

				return err
			}
			arr := tree.NewDArray(types.String)

nit: name it placeholders instead of arr

pkg/sql/crdb_internal.go line 1897 at r1 (raw file):

			arr.Array = make(tree.Datums, len(query.Placeholders))
			for i, v := range query.Placeholders {
				arr.Array[i] = tree.NewDString(v)

does this need to use (*Darray).Append?

rafiss

the first commit needs new tests, and fixes to existing ones

jordanlewis · 2022-08-31T14:50:00Z

Yes, sorry, it's not ready for review yet.

jordanlewis · 2022-09-23T20:53:12Z

I reworked this to lazily interpolate the output of crdb_internal.cluster_statements etc as well as SHOW QUERIES, to avoid changing the API. If we want to change the API we can do it later. Now, we still gain the performance enhancements because we avoid having to unconditionally interpolate during the hot path - instead, we interpolate just when printing the active queries on the other end.

jordanlewis · 2022-10-18T03:05:24Z

PTAL @rafiss

rafiss

this lgtm! just had one optional comment about testing.

does the first commit mean that we will get comments in telemetry logging?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)

pkg/sql/show_test.go line 735 at r11 (raw file):

		},
		{
			"SELECT /* test */ 'hi'::string",

what if there are two comments?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)

-- commits line 28 at r11:
this release note is old

This commit adds a new array to the return value from the parser, which contains the comments that were in the parsed SQL statement. Release note: None

SHOW QUERIES (and crdb_internal.node_queries, cluster_queries) interpolates placeholder values into the statement so that it is possible to see the placeholder values of a prepared statement - but it used to do this unconditionally during statement execution. This is an expensive process that spends a lot of CPU for little reason, since the interpolation was happening in the hot path of every query. Now, we include the placeholder values as a separate array in the internal representation of active queries, and interpolate the values only when they're being pulled out to examine, to avoid the unconditional runtime interpolation costs. As a side effect of this change, the original comments in a query are now included in SHOW QUERIES and the two active queries tables. Release note (sql change): the query field in the crdb_internal.node_queries, crdb_internal.cluster_queries, and SHOW QUERIES commands now includes the original comments in the queries.

jordanlewis · 2022-10-25T02:34:37Z

does the first commit mean that we will get comments in telemetry logging?

Yes, I believe so.

Previously, the parser (actually the scanner) removed comments that came at the end of a statement from the raw sql that was returned along with the parsed AST. This behavior is now removed. Release note: None

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

-- commits line 28 at r11:

Previously, rafiss (Rafi Shamim) wrote…

this release note is old

Done.

pkg/sql/crdb_internal.go line 1897 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

does this need to use (*Darray).Append?

Done.

pkg/sql/show_test.go line 735 at r11 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

what if there are two comments?

Done.

jordanlewis · 2022-10-26T21:20:46Z

@rafiss PTAL

rafiss

lgtm! rebase to fix the TestComposeGSSPython failure

Reviewable status: complete! 0 of 0 LGTMs obtained

jordanlewis · 2022-10-28T02:15:26Z

TFTR! I will just bors it I think that should get past the flake right?

bors r+

craig · 2022-10-28T04:21:13Z

Build failed:

Bazel Essential CI (Cockroach)

jordanlewis · 2022-10-28T15:23:54Z

Unrelated flake

bors r+

craig · 2022-10-28T16:45:49Z

Build succeeded:

Bazel Essential CI (Cockroach)

In cockroachdb#86968 the scanner gained the ability to retain comments in scanned SQL strings, and this was an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table, see `sql.formatActiveQuery`. Now, comments are only retained when the parser is used from this function, reducing allocations for all other cases. Fixes cockroachdb#127713 Release note: None

128032: Reapply "bootstrap: create an explicit zoneconfig for timeseries data" r=rafiss a=rafiss This partially reverts commit f9d47ce. This time, rather than copying over the full zone config for the default range to the timeseries range, we only copy over gc.ttlseconds. This means that other attributes, most notably the replication factor, will be inherited from the default range. This will make it more clear that the timeseries zone config can be changed independently from all the other zone configs. fixes #123762 Release note (ops change): New clusters that are initialized for the first time will now have a zone config defined for the `timeseries` range. This zone config only specifies the gc.ttlseconds, so all the other attributes are inherited from the zone config of the `default` range, as they were before. Clusters that are upgraded to v24.3 from a previous version will also have this zone configuration applied to the timeseries range, as long as that range does not already have a zone config. 128987: sql/sem/tree: remove duplicate `tree.NewDOidWithName` function r=mgartner a=mgartner The `tree.NewDOidWithName` function has been removed. All previous callers now use `tree.NewDOidWithTypeAndName` which is exactly the same. Epic: None Release note: None 129053: sql/parser: only retain scanned SQL comments when necessary r=mgartner a=mgartner In #86968 the scanner gained the ability to retain comments in scanned SQL strings, and this was an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table, see `sql.formatActiveQuery`. Now, comments are only retained when the parser is used from this function, reducing allocations for all other cases. Fixes #127713 Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

128032: Reapply "bootstrap: create an explicit zoneconfig for timeseries data" r=rafiss a=rafiss This partially reverts commit f9d47ce. This time, rather than copying over the full zone config for the default range to the timeseries range, we only copy over gc.ttlseconds. This means that other attributes, most notably the replication factor, will be inherited from the default range. This will make it more clear that the timeseries zone config can be changed independently from all the other zone configs. fixes #123762 Release note (ops change): New clusters that are initialized for the first time will now have a zone config defined for the `timeseries` range. This zone config only specifies the gc.ttlseconds, so all the other attributes are inherited from the zone config of the `default` range, as they were before. Clusters that are upgraded to v24.3 from a previous version will also have this zone configuration applied to the timeseries range, as long as that range does not already have a zone config. 129053: sql/parser: only retain scanned SQL comments when necessary r=mgartner a=mgartner In #86968 the scanner gained the ability to retain comments in scanned SQL strings, and this was an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table, see `sql.formatActiveQuery`. Now, comments are only retained when the parser is used from this function, reducing allocations for all other cases. Fixes #127713 Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

In cockroachdb#86968 the scanner gained the ability to retain comments in scanned SQL strings, and this was an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table, see `sql.formatActiveQuery`. Now, comments are only retained when the parser is used from this function, reducing allocations for all other cases. Fixes cockroachdb#127713 Release note: None

jordanlewis requested a review from a team August 26, 2022 22:09

jordanlewis requested a review from a team as a code owner August 26, 2022 22:09

jordanlewis requested a review from a team August 26, 2022 22:09

jordanlewis requested a review from a team as a code owner August 26, 2022 22:09

rafiss requested changes Aug 26, 2022

View reviewed changes

jordanlewis force-pushed the no-interpolate branch 2 times, most recently from af99535 to ce18c4b Compare August 29, 2022 03:10

rafiss self-requested a review August 29, 2022 14:52

dhartunian removed the request for review from a team August 31, 2022 14:30

rafiss requested changes Aug 31, 2022

View reviewed changes

jordanlewis force-pushed the no-interpolate branch from ce18c4b to 7e2a309 Compare September 23, 2022 20:49

jordanlewis changed the title ~~sql: SHOW QUERIES doesn't interpolate placeholders~~ sql: SHOW QUERIES lazily interpolates placeholders Sep 23, 2022

jordanlewis force-pushed the no-interpolate branch from 7e2a309 to 477d515 Compare September 23, 2022 21:13

jordanlewis requested a review from a team as a code owner September 23, 2022 21:13

jordanlewis mentioned this pull request Sep 23, 2022

sql: data race in schema changer / plan visitor #88628

Closed

jordanlewis force-pushed the no-interpolate branch from 477d515 to b769e27 Compare September 23, 2022 23:28

ajwerner mentioned this pull request Sep 27, 2022

sql,*: tracking issue for sql overheads #88777

Open

jordanlewis force-pushed the no-interpolate branch 2 times, most recently from 1bded82 to ea5ac32 Compare September 30, 2022 19:11

jordanlewis force-pushed the no-interpolate branch 2 times, most recently from 092c3af to 87cad44 Compare October 18, 2022 03:05

rafiss approved these changes Oct 21, 2022

View reviewed changes

rafiss requested changes Oct 21, 2022

View reviewed changes

jordanlewis added 2 commits October 24, 2022 22:15

parser: retain comments

750f64b

This commit adds a new array to the return value from the parser, which contains the comments that were in the parsed SQL statement. Release note: None

jordanlewis force-pushed the no-interpolate branch from 87cad44 to f23aba7 Compare October 25, 2022 02:34

parser: don't cut off end-of-line comments

1de5495

Previously, the parser (actually the scanner) removed comments that came at the end of a statement from the raw sql that was returned along with the parsed AST. This behavior is now removed. Release note: None

jordanlewis force-pushed the no-interpolate branch from f23aba7 to 1de5495 Compare October 26, 2022 16:34

jordanlewis requested a review from rafiss October 26, 2022 17:56

jordanlewis commented Oct 26, 2022

View reviewed changes

rafiss approved these changes Oct 27, 2022

View reviewed changes

craig bot merged commit 3064717 into cockroachdb:master Oct 28, 2022

cockroach-teamcity mentioned this pull request Oct 28, 2022

PR #86968 - sql: SHOW QUERIES lazily interpolate placeholders cockroachdb/docs#15499

Closed

jordanlewis deleted the no-interpolate branch October 28, 2022 17:27

ajwerner mentioned this pull request Oct 31, 2022

sql: race condition writing to ResolvableFunctionReference.FunctionReference #90965

Closed

rafiss mentioned this pull request Dec 1, 2022

release-22.2: parser: retain comments #92844

Closed

mgartner mentioned this pull request Jul 25, 2024

sql/parser: only retain scanned comments when necessary #127713

Closed

mgartner mentioned this pull request Aug 15, 2024

sql/parser: only retain scanned SQL comments when necessary #129053

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: SHOW QUERIES lazily interpolates placeholders #86968

sql: SHOW QUERIES lazily interpolates placeholders #86968

jordanlewis commented Aug 26, 2022 •

edited

Loading

cockroach-teamcity commented Aug 26, 2022

rafiss left a comment

rafiss left a comment

jordanlewis commented Aug 31, 2022

jordanlewis commented Sep 23, 2022

jordanlewis commented Oct 18, 2022

rafiss left a comment

rafiss left a comment

jordanlewis commented Oct 25, 2022

jordanlewis left a comment

jordanlewis commented Oct 26, 2022

rafiss left a comment

jordanlewis commented Oct 28, 2022

craig bot commented Oct 28, 2022

jordanlewis commented Oct 28, 2022

craig bot commented Oct 28, 2022

sql: SHOW QUERIES lazily interpolates placeholders #86968

sql: SHOW QUERIES lazily interpolates placeholders #86968

Conversation

jordanlewis commented Aug 26, 2022 • edited Loading

cockroach-teamcity commented Aug 26, 2022

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jordanlewis commented Aug 31, 2022

jordanlewis commented Sep 23, 2022

jordanlewis commented Oct 18, 2022

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jordanlewis commented Oct 25, 2022

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis commented Oct 26, 2022

rafiss left a comment

Choose a reason for hiding this comment

jordanlewis commented Oct 28, 2022

craig bot commented Oct 28, 2022

jordanlewis commented Oct 28, 2022

craig bot commented Oct 28, 2022

jordanlewis commented Aug 26, 2022 •

edited

Loading