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

SET results_buffer_size doesn't work as a session variable #124360

Open
dikshant opened this issue May 17, 2024 · 7 comments
Open

SET results_buffer_size doesn't work as a session variable #124360

dikshant opened this issue May 17, 2024 · 7 comments
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Foundations

Comments

@dikshant
Copy link

dikshant commented May 17, 2024 •
edited by cockroach-jira-scripts

SET results_buffer_size='2MiB'; doesn't seem to work. We likely didn't implement it based on this TODO:

cockroach/pkg/sql/vars.go

Line 1177 in 8fb653b

// TODO(dan): This should also work with SET.

Jira issue: CRDB-38849
@dikshant dikshant added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels May 17, 2024
@blathers-crl blathers-crl bot added this to Triage in SQL Foundations May 17, 2024
@dikshant
Copy link
Author

A customer indicated this would be good to have in a backport to 23.2.7 since they are testing read committed and this helps minimize transaction retry errors.

@rafiss
Copy link
Collaborator

rafiss commented May 17, 2024

The setting should be configured in one of two ways:

  • use the cluster setting sql.defaults.results_buffer.size.
  • specify results_buffer_size in the connection string.

Changing the buffer size in a running session is not supported, and would be a somewhat high effort to implement, since it requires changing the size of the buffer while it's being used in different goroutines.

cockroach/pkg/sql/pgwire/conn.go

Lines 1259 to 1263 in bf28084

// Note that ConnResultsBufferSize cannot be changed during a session, so it
// is safe to use the value stored on sessionArgs.
if !bufferingDisabled && int64(c.writerState.buf.Len()) <= c.sessionArgs.ConnResultsBufferSize {
return nil
}

@rafiss
Copy link
Collaborator

rafiss commented May 17, 2024

Related issue: #124049

@dikshant
Copy link
Author

Thanks! Someday we may wanna do it if restarting the session becomes unacceptable. But for now we should probably remove it from this doc page, I'll make a separate issue for that:
https://www.cockroachlabs.com/docs/v23.2/set-vars#supported-variables

@jameslupolthrd
Copy link

jameslupolthrd commented May 20, 2024 •
edited

Specifying results_buffer_size in the connection string does not seem to work:

~ % cockroach sql --insecure --url 'postgresql://root@localhost:26257/foo?options=-c%20results_buffer_size=4mib' ERROR: options: parameter "results_buffer_size" cannot be changed SQLSTATE: 55P02 Failed running "sql"

@rafiss
Copy link
Collaborator

rafiss commented May 20, 2024

Ah I think there's a bit of a rough edge. It doesn't work in the options parameter, but it does work as a top-level query parameter.

❯ ./cockroach sql --insecure --url 'postgresql://root@localhost:26257/defaultdb?results_buffer_size=4mib'
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
# Server version: CockroachDB CCL v24.2.0-alpha.00000000-dev (darwin arm64, built , go1.22.3 X:nocoverageredesign) (same version as client)
# Cluster ID: 4f3c4c15-b9a5-4bb9-a744-b7c39228c19d
#
# Enter \? for a brief introduction.
#
root@localhost:26257/defaultdb> show results_buffer_size;
  results_buffer_size
-----------------------
  4194304
(1 row)

Let me try to get the bug with options fixed; ideally that should work as well.

@jameslupolthrd
Copy link

Thanks. Some (maybe all?) JDBC libraries don't seem to honor results_buffer_size as a top-level connection setting, so being able to set this as a session variable on connect with options would be helpful.

@rafiss rafiss mentioned this issue May 20, 2024
Merged
craig bot pushed a commit that referenced this issue May 28, 2024
@rafiss
Merge #124437
4c1e969 
124437: pgwire: support results_buffer_size in connection string options r=fqazi a=rafiss

informs #124360
Release note (bug fix): The results_buffer_size session variable previously could not be configured by using the "options" query parameter in the connection string; it could only be configured as a top-level query parameter. Now, it can be configured in either part of the connection string. (This variable still cannot be changed with the SET command after the session begins.)

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
This was referenced May 28, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Foundations
  
Triage
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rafiss @dikshant @jameslupolthrd