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.

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

sql: add a sql.defaults.conn_results_buffer_size cluster setting #32366

Merged
merged 2 commits into from Nov 26, 2018

Conversation

Projects
None yet
4 participants
@andreimatei
Copy link
Member

andreimatei commented Nov 14, 2018

... controlling the buffering of results on (all) future connections.
This has come up with users enough to warrant it: some would rather
buffer more to get more auto-retries.
Controlling this for each connection individually is not done.

Release note: None

@andreimatei andreimatei requested review from cockroachdb/core-prs as code owners Nov 14, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 14, 2018

This change is Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented Nov 14, 2018

cc @danhhz

@andreimatei andreimatei force-pushed the andreimatei:conn-buff branch 3 times, most recently from a9dddce to b2a3ee2 Nov 15, 2018

@danhhz

This comment has been minimized.

Copy link
Contributor

danhhz commented Nov 15, 2018

changefeedccl stuff lgtm. I'll defer to @knz for the rest

@knz
Copy link
Member

knz left a comment

LGTM modulo improvement of the setting name + description, see my comment below.

However I am not sure a cluster setting is the best way to go.
If it was just me I would have went the route of a client status parameter (a new case in parseOptions(), pgwire/server.go), so that the buffer can be set to a different value per session.

Reviewed 11 of 11 files at r1, 5 of 5 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/pgwire/conn.go, line 107 at r1 (raw file):

// corresponding session variable.
var connResultsBufferSize = settings.RegisterByteSizeSetting(
	"sql.defaults.conn_results_buffer_size",

There is a tech note / RFC / guideline somewhere that should have informed you that we don't shorten words in setting names. So the setting name should be

sql.defaults.connection_results_buffer.size

(I think that this is even too verbose -- we have just one results buffer so it could be sql.defaults.results_buffer.size


pkg/sql/pgwire/conn.go, line 108 at r1 (raw file):

var connResultsBufferSize = settings.RegisterByteSizeSetting(
	"sql.defaults.conn_results_buffer_size",
	"the size of each connection's results buffer. Within a query (or a batch "+
  1. no "the" at the beginning

  2. I think the text is too long -- you could summarize to:

    "size of the buffer that accumulates query results before they are sent to the client. Note that auto-retries by the server only happen on buffered results, so reducing this size exposes clients to more manual retries."

  3. don't use \n (we don't use newlines in setting descriptions)

  4. if you keep your text use "the server" instead of "CRDB"


pkg/sql/pgwire/conn.go, line 109 at r1 (raw file):

	"sql.defaults.conn_results_buffer_size",
	"the size of each connection's results buffer. Within a query (or a batch "+
		"of queries), CRDB buffers results until the buffer overflows, at which point "+

... until the buffer is full, at which point ...


pkg/sql/pgwire/conn.go, line 112 at r1 (raw file):

		"the results are delivered to the client. Once any results are delivered, "+
		"CRDB can no longer perform automatic retries for the transactions in "+
		"question. On the other hand, increasing the buffer size can increase the "+

... increasing the buffer size increases the delay until the client receives the first result row.


pkg/sql/pgwire/conn.go, line 115 at r1 (raw file):

		"latency the client perceives until the first result row for a query is "+
		"delivered.\n"+
		"Updating the setting only affects future connections.\n"+

... only affects new connections.

@andreimatei andreimatei force-pushed the andreimatei:conn-buff branch from b2a3ee2 to 00a227e Nov 19, 2018

@andreimatei
Copy link
Member

andreimatei left a comment

I agree that having control over buffering per-session would be nice, but I'll let someone else do it. Even when we do, a configurable default still makes sense (which is why I put the "defaults" in the setting name. In fact, ideally the control would be per-txn, but how to do that is less clear.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/pgwire/conn.go, line 107 at r1 (raw file):

Previously, knz (kena) wrote…

There is a tech note / RFC / guideline somewhere that should have informed you that we don't shorten words in setting names. So the setting name should be

sql.defaults.connection_results_buffer.size

(I think that this is even too verbose -- we have just one results buffer so it could be sql.defaults.results_buffer.size

sql.defaults.results_buffer.size it is


pkg/sql/pgwire/conn.go, line 108 at r1 (raw file):

Previously, knz (kena) wrote…
  1. no "the" at the beginning

  2. I think the text is too long -- you could summarize to:

    "size of the buffer that accumulates query results before they are sent to the client. Note that auto-retries by the server only happen on buffered results, so reducing this size exposes clients to more manual retries."

  3. don't use \n (we don't use newlines in setting descriptions)

  4. if you keep your text use "the server" instead of "CRDB"

rephrased.


pkg/sql/pgwire/conn.go, line 109 at r1 (raw file):

Previously, knz (kena) wrote…

... until the buffer is full, at which point ...

Done.


pkg/sql/pgwire/conn.go, line 112 at r1 (raw file):

Previously, knz (kena) wrote…

... increasing the buffer size increases the delay until the client receives the first result row.

Done.


pkg/sql/pgwire/conn.go, line 115 at r1 (raw file):

Previously, knz (kena) wrote…

... only affects new connections.

Done.

@knz
Copy link
Member

knz left a comment

Reviewed 2 of 7 files at r3, 5 of 5 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/pgwire/server.go, line 54 at r4 (raw file):

var connResultsBufferSize = settings.RegisterByteSizeSetting(
	"sql.defaults.conn_results_buffer_size",
	"the size of each connection's results buffer. Within a query (or a batch "+

you forgot to commit the changes to this file.

andreimatei added some commits Nov 14, 2018

sql: add a sql.defaults.conn_results_buffer_size cluster setting
... controlling the buffering of results on (all) future connections.
This has come up with users enough to warrant it: some would rather
buffer more to get more auto-retries.
Controlling this for each connection individually is not done.

Release note (general change): A new cluster setting
(sql.defaults.conn_results_buffer_size) can be used to control
server-side buffering of results.
pgwire: get rid of conn.execCfg
The reference from a connection to an ExecutorConfig is no longer
needed.

Release note: None

@andreimatei andreimatei force-pushed the andreimatei:conn-buff branch from 00a227e to f25ac68 Nov 21, 2018

@andreimatei
Copy link
Member

andreimatei left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/pgwire/server.go, line 54 at r4 (raw file):

Previously, knz (kena) wrote…

you forgot to commit the changes to this file.

done. damn it, I thought I paid attention to this. thanks.

@knz

knz approved these changes Nov 26, 2018

Copy link
Member

knz left a comment

:lgtm:

Reviewed 6 of 6 files at r5, 5 of 5 files at r6.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@andreimatei
Copy link
Member

andreimatei left a comment

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

craig bot pushed a commit that referenced this pull request Nov 26, 2018

Merge #32366
32366: sql: add a sql.defaults.conn_results_buffer_size cluster setting r=andreimatei a=andreimatei

... controlling the buffering of results on (all) future connections.
This has come up with users enough to warrant it: some would rather
buffer more to get more auto-retries.
Controlling this for each connection individually is not done.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 26, 2018

Build succeeded

@craig craig bot merged commit f25ac68 into cockroachdb:master Nov 26, 2018

2 of 3 checks passed

GitHub CI (Cockroach) TeamCity build failed
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@andreimatei andreimatei deleted the andreimatei:conn-buff branch Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment