sql: automatically disable result buffering for sinkless changefeeds #35529
Conversation
|
This change is |
There was a problem hiding this comment.
nit on the commit message: i wouldn't mention bulk i/o. this is only turned on, and only will be turned on, for sinkless/pgwire/non-enterprise changefeeds
you should probably also include a release note
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @danhhz, and @knz)
pkg/sql/conn_io.go, line 710 at r1 (raw file):
RowsAffected() int // DisableBuffering can be called during execution to cause the next
can you define "next results" more precisely in this comment please?
pkg/sql/plan.go, line 513 at r1 (raw file):
} else if fn != nil { if avoidStreaming { p.curPlan.avoidBuffering = true
should be avoidBuffering, right? avoidStreaming sounds like it has the opposite semantics
pkg/sql/planhook.go, line 39 at r1 (raw file):
type planHookFn func( context.Context, tree.Statement, PlanHookState, ) (fn PlanHookRowFn, header sqlbase.ResultColumns, subplans []planNode, avoidStreaming bool, err error)
ditto
There was a problem hiding this comment.
this means we should revert my results_buffer_size pgwire parameter PR before 19.1 gets cut, right?
I don't see this to be true. IMHO this parameter is useful for testing/troubleshooting.
nit on the commit message: i wouldn't mention bulk i/o. this is only turned on, and only will be turned on, for sinkless/pgwire/non-enterprise changefeeds
Done.
you should probably also include a release note
Yes, agreed if only to say to people that results_buffer_size is not needed any more.
Reviewable status:
pkg/sql/conn_io.go, line 710 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
can you define "next results" more precisely in this comment please?
Done.
pkg/sql/plan.go, line 513 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
should be avoidBuffering, right? avoidStreaming sounds like it has the opposite semantics
Done.
pkg/sql/planhook.go, line 39 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
ditto
Done.
Prior to this patch, it was necesasry to set `results_buffer_size` to 1 for the entire SQL session in order to see changefeed results immediately. This patch removes this requirement: now using a sinkless changefeed (including in a subquery) will disable buffering for the current statement automatically. Other statements are unaffected. Release note (sql change): CockroachDB will now start emitting changefeed events immediately for sinkless changefeeds (an experimental feature). The connection parameter `results_buffer_size` is not needed any more for this purpose.
knz
changed the title
|
bors r+ |
Build failed |
|
failed on #35551 . bors r+ |
35529: sql: automatically disable result buffering for sinkless changefeeds r=knz a=knz Discussed on #34423. This is limited in scope, but it will do in 19.1 where the feature is experimental anyway. In 19.2, bulk i/o will come into the CBO, and we can build a more robust solution there. Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
Discussed on #34423.
This is limited in scope, but it will do in 19.1 where the feature is experimental anyway. In 19.2, bulk i/o will come into the CBO, and we can build a more robust solution there.