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

Rewind tuple store to fix scrollable with hold cursor fetches #7014

Merged
merged 1 commit into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/backend/distributed/commands/utility_hook.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ multi_ProcessUtility(PlannedStmt *pstmt,
IsA(parsetree, ExecuteStmt) ||
IsA(parsetree, PrepareStmt) ||
IsA(parsetree, DiscardStmt) ||
IsA(parsetree, DeallocateStmt))
IsA(parsetree, DeallocateStmt) ||
IsA(parsetree, DeclareCursorStmt) ||
IsA(parsetree, FetchStmt))
{
/*
* Skip additional checks for common commands that do not have any
Expand Down
14 changes: 13 additions & 1 deletion src/backend/distributed/executor/citus_custom_scan.c
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,19 @@ CitusEndScan(CustomScanState *node)
*/
static void
CitusReScan(CustomScanState *node)
{ }
{
if (node->ss.ps.ps_ResultTupleSlot)
{
ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);
}
ExecScanReScan(&node->ss);

CitusScanState *scanState = (CitusScanState *) node;
if (scanState->tuplestorestate)
Copy link
Member

Choose a reason for hiding this comment

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

other, similar scan nodes in PG also have

    if (node->ss.ps.ps_ResultTupleSlot)
        ExecClearTuple(node->ss.ps.ps_ResultTupleSlot);

    ExecScanReScan(&node->ss);

do we need that there too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. We need to free slot for the heap tuple. And ExecScanReScan must be called according to the comments on the function. (refer function's comments)

{
tuplestore_rescan(scanState->tuplestorestate);
}
}


/*
Expand Down
70 changes: 70 additions & 0 deletions src/test/regress/expected/multi_utility_statements.out
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,76 @@ FETCH FORWARD 3 FROM holdCursor;
1 | 19
(3 rows)

CLOSE holdCursor;
-- Test DECLARE CURSOR .. WITH HOLD inside transaction block
BEGIN;
DECLARE holdCursor CURSOR WITH HOLD FOR
SELECT * FROM cursor_me WHERE x = 1 ORDER BY y;
FETCH 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 10
1 | 11
1 | 12
(3 rows)

FETCH BACKWARD 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 11
1 | 10
(2 rows)

FETCH FORWARD 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 10
1 | 11
1 | 12
(3 rows)

COMMIT;
FETCH 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 13
1 | 14
1 | 15
(3 rows)

CLOSE holdCursor;
-- Test DECLARE NO SCROLL CURSOR .. WITH HOLD inside transaction block
BEGIN;
DECLARE holdCursor NO SCROLL CURSOR WITH HOLD FOR
SELECT * FROM cursor_me WHERE x = 1 ORDER BY y;
FETCH 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 10
1 | 11
1 | 12
(3 rows)

FETCH FORWARD 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 13
1 | 14
1 | 15
(3 rows)

COMMIT;
FETCH 3 FROM holdCursor;
x | y
---------------------------------------------------------------------
1 | 16
1 | 17
1 | 18
(3 rows)

FETCH BACKWARD 3 FROM holdCursor;
ERROR: cursor can only scan forward
HINT: Declare it with SCROLL option to enable backward scan.
CLOSE holdCursor;
-- Test DECLARE CURSOR .. WITH HOLD with parameter
CREATE OR REPLACE FUNCTION declares_cursor(p int)
Expand Down
24 changes: 24 additions & 0 deletions src/test/regress/sql/multi_utility_statements.sql
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,30 @@ FETCH FORWARD 3 FROM holdCursor;

CLOSE holdCursor;

-- Test DECLARE CURSOR .. WITH HOLD inside transaction block
BEGIN;
DECLARE holdCursor CURSOR WITH HOLD FOR
SELECT * FROM cursor_me WHERE x = 1 ORDER BY y;
FETCH 3 FROM holdCursor;
FETCH BACKWARD 3 FROM holdCursor;
FETCH FORWARD 3 FROM holdCursor;
COMMIT;

FETCH 3 FROM holdCursor;
CLOSE holdCursor;

-- Test DECLARE NO SCROLL CURSOR .. WITH HOLD inside transaction block
BEGIN;
DECLARE holdCursor NO SCROLL CURSOR WITH HOLD FOR
SELECT * FROM cursor_me WHERE x = 1 ORDER BY y;
FETCH 3 FROM holdCursor;
FETCH FORWARD 3 FROM holdCursor;
COMMIT;

FETCH 3 FROM holdCursor;
FETCH BACKWARD 3 FROM holdCursor;
CLOSE holdCursor;

-- Test DECLARE CURSOR .. WITH HOLD with parameter
CREATE OR REPLACE FUNCTION declares_cursor(p int)
RETURNS void AS $$
Expand Down