sql: allow cursor WITH HOLD inside of txn as long as it gets closed#94127
sql: allow cursor WITH HOLD inside of txn as long as it gets closed#94127craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
jordanlewis
left a comment
There was a problem hiding this comment.
Looks good, one code organization comment to please take a look at.
pkg/sql/conn_executor_exec.go
Outdated
| ctx, sp := tracing.EnsureChildSpan(ctx, ex.server.cfg.AmbientCtx.Tracer, "commit sql txn") | ||
| defer sp.Finish() | ||
|
|
||
| for n, c := range ex.extraTxnState.sqlCursors.list() { |
There was a problem hiding this comment.
I think it would be better to put this logic nearby where we close all of the cursors, perhaps even inside of closeAll. You might need to pass a boolean to closeAll because we also use it on server shutdown, but I feel this would prevent future refactorings from "forgetting" to do this check.
There was a problem hiding this comment.
also need one for rollback too right?
There was a problem hiding this comment.
yeah, we'll need to make sure txnRollback and txnRestart transitions are handled too, but that seems fine
|
was gonna do this today, but thank you :') |
| name: s.String(), | ||
| constructor: func(ctx context.Context, p *planner) (_ planNode, _ error) { | ||
| if p.extendedEvalCtx.TxnImplicit { | ||
| if s.Hold { |
There was a problem hiding this comment.
can this no-op? or do we have an internal limitation that cursors require txns?
otan=# declare a cursor with hold for select 1;
DECLARE CURSOR
also wonder if we should have separate issues numbers for this and the closeAll scenario below for telemetry purposes
There was a problem hiding this comment.
by default, cursors require transactions (in PG too). with hold is special because it does not require a transaction, but that is the thing we don't support. no-oping would be confusing IMO, since the user may think that it was created successfully.
There was a problem hiding this comment.
i'm fine with one issue personally, because the underlying limitation for both is "cannot use a WITH HOLD cursor outside of an explicit transaction. both places where we return the unimplemented error will be fixed at the same time.
pkg/sql/conn_executor.go
Outdated
| ) | ||
| ex.extraTxnState.prepStmtsNamespaceMemAcc.Close(ctx) | ||
| ex.extraTxnState.sqlCursors.closeAll() | ||
| _ = ex.extraTxnState.sqlCursors.closeAll(false /* errorOnWithHold */) |
There was a problem hiding this comment.
should we log any errors here just in case? same with below.
The `DECLARE foo CURSOR WITH HOLD ...` syntax is now supported in a limited fashion. Unlike PG's `WITH HOLD` this version can only be executed in a transaction block still. If there are any open cursors that have `WITH HOLD` open at the time a transaction is commited, the COMMIT fails. No release note, since there's not actually any new functionality. Release note: None
|
tftr! bors r=otan |
|
Build succeeded: |
informs #77101
The
DECLARE foo CURSOR WITH HOLD ...syntax is now supported in a limited fashion. Unlike PG'sWITH HOLD, this version can only be executed in a transaction block still. If there are any open cursors that haveWITH HOLDopen at the time a transaction is commited, the COMMIT fails.No release note, since there's not actually any new functionality.
Release note: None