Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
mgartner opened this issue Jul 25, 2024 · 0 comments · Fixed by #129053
Closed

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

mgartner opened this issue Jul 25, 2024 · 0 comments · Fixed by #129053
Assignees
@mgartner
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@mgartner
Copy link
Collaborator

mgartner commented Jul 25, 2024 •
edited by cockroach-jira-scripts
Loading

In #86968 the scanner gained the ability to retain comments in scanned SQL strings, and this is an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table here:

cockroach/pkg/sql/crdb_internal.go

Lines 2852 to 2855 in 7974916

for i := range parsed.Comments {
sb.WriteString(" ")
sb.WriteString(parsed.Comments[i])
}

We can reduce allocations by only retaining scanned comments for this specific use case of the scanner.

Jira issue: CRDB-40564
@mgartner mgartner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jul 25, 2024
@mgartner mgartner self-assigned this Jul 25, 2024
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Jul 25, 2024
@mgartner mgartner mentioned this issue Jul 25, 2024
Closed
@mgartner mgartner moved this from Triage to Active in SQL Queries Jul 25, 2024
@mgartner mgartner mentioned this issue Aug 15, 2024
Merged
mgartner added a commit to mgartner/cockroach that referenced this issue Aug 15, 2024
@mgartner
sql/parser: only retain scanned SQL comments when necessary
c4b9525 
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
craig bot pushed a commit that referenced this issue Aug 15, 2024
@rafiss @mgartner
Merge #128032 #129053
ec66ae7 
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 <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in f9e0f16 Aug 15, 2024
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mgartner mgartner

Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql/parser: only retain scanned SQL comments when necessary mgartner/cockroach
1 participant
@mgartner