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

sql: support DECLARE cursor WITH HOLD #77101

Open
rafiss opened this issue Feb 27, 2022 · 9 comments
Open

sql: support DECLARE cursor WITH HOLD #77101

rafiss opened this issue Feb 27, 2022 · 9 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-aws-dms Blocking support for AWS Database Migration Service C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-anchored-telemetry The issue number is anchored by telemetry references.

Comments

@rafiss
Copy link
Collaborator

rafiss commented Feb 27, 2022

Split off from #41412

WITH HOLD support allows keeping a cursor open for longer than a transaction by writing its results into a buffer.

postgres docs on declare.

Unless WITH HOLD is specified, the cursor created by this command can only be used within the current transaction. Thus, DECLARE without WITH HOLD is useless outside a transaction block: the cursor would survive only to the completion of the statement. Therefore PostgreSQL reports an error if such a command is used outside a transaction block. Use BEGIN and COMMIT (or ROLLBACK) to define a transaction block.

If WITH HOLD is specified and the transaction that created the cursor successfully commits, the cursor can continue to be accessed by subsequent transactions in the same session. (But if the creating transaction is aborted, the cursor is removed.) A cursor created with WITH HOLD is closed when an explicit CLOSE command is issued on it, or the session ends. In the current implementation, the rows represented by a held cursor are copied into a temporary file or memory area so that they remain available for subsequent transactions.

WITH HOLD may not be specified when the query includes FOR UPDATE or FOR SHARE.

Epic CRDB-11397

Jira issue: CRDB-13412

@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgcompat Semantic compatibility with PostgreSQL X-anchored-telemetry The issue number is anchored by telemetry references. labels Feb 27, 2022
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Feb 27, 2022
@otan otan added the A-tools-aws-dms Blocking support for AWS Database Migration Service label May 17, 2022
@vy-ton
Copy link
Contributor

vy-ton commented Jun 15, 2022

User reported running into this unsupported feature with Tibco Spotfire. Looks like it also blocks CRDB as AWS DMS source thread

@rafiss Whats the level of effort here since I see it was separated from original DECLARE issue?

@rafiss
Copy link
Collaborator Author

rafiss commented Jun 22, 2022

@jordanlewis has the best understanding at the moment. But my impression is that this shouldn't be too much extra work. Unlike Postgres, which requires an extra buffer (as mentioned in the issue description), I think this will be easier in CRDB since we implemented this using internal executors.

@rafiss
Copy link
Collaborator Author

rafiss commented Jun 22, 2022

I just checked with Jordan, who pointed out that it's actually a more significant effort.

Implementation notes: The challenge is that the internal executor is still bound to the transaction. So we can't just close the transaction and keep using the internal executor. We actually would need to create an extra buffer that lives outside the transaction, and also rework the execution logic for fetching from the cursor.

I bet if we read through Postgres source code, we could get some good ideas.

@otan
Copy link
Contributor

otan commented Dec 11, 2022

I wonder if we can no-op if we detect this outside a transaction, or error only if a WITH HOLD cursor is held outside the transaction? edit: nvm, doesn't help me

DMS seems to declare WITH HOLD all the time, but always use their cursor within the transaction. i wonder if we could have a session var that allows WITH HOLD inside the transaction and on commit/rollback error if we detect any WITH HOLD cursors.

@jordanlewis
Copy link
Member

WITH HOLD can be ignored outside of transactions, yes.

That session variable is an interesting idea, though it might not always work well - it's common to leave a cursor not completely closed. Do you know if DMS always completely consumes and closes their cursors?

@otan
Copy link
Contributor

otan commented Dec 12, 2022

Do you know if DMS always completely consumes and closes their cursors?

it appears to

@rafiss
Copy link
Collaborator Author

rafiss commented Dec 21, 2022

i wonder if we could have a session var that allows WITH HOLD inside the transaction and on commit/rollback error if we detect any WITH HOLD cursors.

do we actually need a session variable for this? what about just supporting WITH HOLD, and always returning an error if we detect any WITH HOLD cursors when the txn is closed?

@jordanlewis
Copy link
Member

That works for me, Rafi.

craig bot pushed a commit that referenced this issue Dec 27, 2022
94127: sql: allow cursor WITH HOLD inside of txn as long as it gets closed r=otan a=rafiss

informs #77101

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

94244: sql: add comment for TableIsHydrated and ColumnIsHydrated r=chengxiong-ruan a=chengxiong-ruan

Epic: None
Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>
@DrewKimball
Copy link
Collaborator

It might be nice to also introduce a session setting to control the default behavior of cursor declarations, since oracle has the opposite default from postgres: https://docs.oracle.com/javadb/10.8.3.0/devguide/rdevconceptsholdablecursors.html

DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Jan 18, 2024
This patch introduces a new setting, `close_cursors_at_commit`, which causes
cursors opened using a PL/pgSQL OPEN statement to remain open once the calling
transaction commits. This is similar to oracle behavior, and will be useful
for enabling migrations.

As part of this change, the `sqlCursors.closeAll` method has been expanded to
take in the reason for closing as a parameter. It now uses the following rules:
* If the reason for closing is txn commit, non-HOLD cursors are closed.
* If the reason for closing is txn rollback, all cursors created by the current
  transaction are closed.
* If the reason for closing is an explicit CLOSE ALL or the session closing,
  all cursors are closed unconditionally.

Note that the behavior has not changed for SQL cursors - if a SQL cursor is
declared using `WITH HOLD` and is open at txn commit, an error will result.

Informs cockroachdb#77101

Release note (sql change): Introduced a new setting, `close_cursors_at_commit`,
which causes a cursor to remain open even after its calling transaction
commits. Note that transaction rollback still closes any cursor created in
that transaction.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Jan 18, 2024
This patch introduces a new setting, `close_cursors_at_commit`, which causes
cursors opened using a PL/pgSQL OPEN statement to remain open once the calling
transaction commits. This is similar to oracle behavior, and will be useful
for enabling migrations.

As part of this change, the `sqlCursors.closeAll` method has been expanded to
take in the reason for closing as a parameter. It now uses the following rules:
* If the reason for closing is txn commit, non-HOLD cursors are closed.
* If the reason for closing is txn rollback, all cursors created by the current
  transaction are closed.
* If the reason for closing is an explicit CLOSE ALL or the session closing,
  all cursors are closed unconditionally.

Note that the behavior has not changed for SQL cursors - if a SQL cursor is
declared using `WITH HOLD` and is open at txn commit, an error will result.

Informs cockroachdb#77101

Release note (sql change): Introduced a new setting, `close_cursors_at_commit`,
which causes a cursor to remain open even after its calling transaction
commits. Note that transaction rollback still closes any cursor created in
that transaction.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 6, 2024
This patch introduces a new setting, `close_cursors_at_commit`, which causes
cursors opened using a PL/pgSQL OPEN statement to remain open once the calling
transaction commits. This is similar to oracle behavior, and will be useful
for enabling migrations.

As part of this change, the `sqlCursors.closeAll` method has been expanded to
take in the reason for closing as a parameter. It now uses the following rules:
* If the reason for closing is txn commit, non-HOLD cursors are closed.
* If the reason for closing is txn rollback, all cursors created by the current
  transaction are closed.
* If the reason for closing is an explicit CLOSE ALL or the session closing,
  all cursors are closed unconditionally.

Note that the behavior has not changed for SQL cursors - if a SQL cursor is
declared using `WITH HOLD` and is open at txn commit, an error will result.

Informs cockroachdb#77101

Release note (sql change): Introduced a new setting, `close_cursors_at_commit`,
which causes a cursor to remain open even after its calling transaction
commits. Note that transaction rollback still closes any cursor created in
that transaction.
craig bot pushed a commit that referenced this issue Feb 7, 2024
117910: sql,plpgsql: add setting to give cursors default WITH HOLD behavior r=DrewKimball a=DrewKimball

This patch introduces a new setting, `close_cursors_at_commit`, which causes cursors opened using a PL/pgSQL OPEN statement to remain open once the calling transaction commits. This is similar to oracle behavior, and will be useful for enabling migrations.

As part of this change, the `sqlCursors.closeAll` method has been expanded to take in the reason for closing as a parameter. It now uses the following rules:
* If the reason for closing is txn commit, non-HOLD cursors are closed.
* If the reason for closing is txn rollback, all cursors created by the current transaction are closed.
* If the reason for closing is an explicit CLOSE ALL or the session closing, all cursors are closed unconditionally.

Note that the behavior has not changed for SQL cursors - if a SQL cursor is declared using `WITH HOLD` and is open at txn commit, an error will result.

Informs #77101

Release note (sql change): Introduced a new setting, `close_cursors_at_commit`, which causes a cursor to remain open even after its calling transaction commits. Note that transaction rollback still closes any cursor created in that transaction.

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
wenyihu6 pushed a commit to wenyihu6/cockroach that referenced this issue Feb 21, 2024
This patch introduces a new setting, `close_cursors_at_commit`, which causes
cursors opened using a PL/pgSQL OPEN statement to remain open once the calling
transaction commits. This is similar to oracle behavior, and will be useful
for enabling migrations.

As part of this change, the `sqlCursors.closeAll` method has been expanded to
take in the reason for closing as a parameter. It now uses the following rules:
* If the reason for closing is txn commit, non-HOLD cursors are closed.
* If the reason for closing is txn rollback, all cursors created by the current
  transaction are closed.
* If the reason for closing is an explicit CLOSE ALL or the session closing,
  all cursors are closed unconditionally.

Note that the behavior has not changed for SQL cursors - if a SQL cursor is
declared using `WITH HOLD` and is open at txn commit, an error will result.

Informs cockroachdb#77101

Release note (sql change): Introduced a new setting, `close_cursors_at_commit`,
which causes a cursor to remain open even after its calling transaction
commits. Note that transaction rollback still closes any cursor created in
that transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-aws-dms Blocking support for AWS Database Migration Service C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
None yet
Development

No branches or pull requests

5 participants