sql: allow cursor WITH HOLD inside of txn as long as it gets closed #94127
Conversation
|
This change is |
pkg/sql/conn_executor_exec.go
Outdated
| @@ -988,6 +989,12 @@ func (ex *connExecutor) commitSQLTransactionInternal(ctx context.Context) error | |||
| 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 :') |
| @@ -45,6 +42,9 @@ func (p *planner) DeclareCursor(ctx context.Context, s *tree.DeclareCursor) (pla | |||
| 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
| @@ -1153,7 +1153,7 @@ func (ex *connExecutor) close(ctx context.Context, closeType closeType) { | |||
| ctx, &ex.extraTxnState.prepStmtsNamespaceMemAcc, | |||
| ) | |||
| 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