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.

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

[wip] sql: pgwire support for limited result sets #33174

Closed
wants to merge 1 commit into from

Conversation

jordanlewis
Copy link
Member

This commit adds support for portal execution with limits, which is the
part of the pgwire protocol that JDBC uses if you use its native
pagination with setFetchSize.

Closes #4035.

Release note (sql change): add support for row limits in prepared
portals at the pgwire layer, enabling support for client-side pagination
with things like JDBC's setFetchSize.

Co-authored-by: Jordan Lewis jordanthelewis@gmail.com
Co-authored-by: Andrei Matei andrei@cockroachlabs.com

@jordanlewis jordanlewis requested review from maddyblue and a team December 14, 2018 23:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

stmtRes := ex.clientComm.CreateStatementResult(
tcmd.Stmt, NeedRowDesc, pos, nil, /* formatCodes */
ex.sessionData.DataConversion)
stmtRes := ex.clientComm.CreateStatementResult(tcmd.Stmt, NeedRowDesc, pos, nil, ex.sessionData.DataConversion, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nil and 0 should be documented inline.

r.seenTuples = 0

return r.moreResultsNeeded()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newline

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/conn_executor_exec.go, line 934 at r1 (raw file):

			}
			ex.sessionTracing.TraceExecRowsResult(consumeCtx, values)
			quitEarly, commErr = res.AddRow(consumeCtx, values)

AddRow returns what seems to be the inverse of "quit early".

This commit adds support for portal execution with limits, which is the
part of the pgwire protocol that JDBC uses if you use its native
pagination with `setFetchSize`.

Release note (sql change): add support for row limits in prepared
portals at the pgwire layer, enabling support for client-side pagination
with things like JDBC's `setFetchSize`.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@andreimatei
Copy link
Contributor

Jordan can you give me write permissions on your repo? I can't push my changes.
Pushed here in the meantime: https://github.com/andreimatei/cockroach/pull/new/portal-limit

We still need to figure out the testing story.
Saving our pgx play here: https://gist.github.com/andreimatei/5d0b7fcd47336937d0aa25bbd14b0747

@knz
Copy link
Contributor

knz commented Mar 16, 2019

@jordanlewis @andreimatei could you brush up this PR to have a first MVP in 19.1 before the branch is cut? We can fix up with backports as needed afterwards.

@andreimatei
Copy link
Contributor

Well this was not the right way to go, in my opinion - the "command result" taking over the stmtBuf. I've got better stuff in https://github.com/andreimatei/cockroach/commits/distsqlrun.async-exec - that's the portal stuff that I've demoed. I've been slowly continuing work on it, most recently two fridays ago. And I plan on continuing to do so. But also I'd love for someone to take over that one. In any case, for it the branch cut has sailed away.

For this one in particular, my heart is definitely not in it.

@jordanlewis jordanlewis added the X-noremind Bots won't notify about PRs with X-noremind label May 22, 2019
@jordanlewis jordanlewis deleted the portal-limit branch August 19, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/pgwire: missing support for row count limits in pgwire
6 participants