Rewind tuple store to fix scrollable with hold cursor fetches #7014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aykut-bozkurt
force-pushed
the
scrollable-cursor-with-hold-fix
branch
from
7b15608
to
c89a559
Compare
Codecov Report
@@ Coverage Diff @@
## main #7014 +/- ##
==========================================
- Coverage 93.33% 93.10% -0.24%
==========================================
Files 273 273
Lines 58469 58474 +5
==========================================
- Hits 54572 54441 -131
- Misses 3897 4033 +136 |
aykut-bozkurt
force-pushed
the
scrollable-cursor-with-hold-fix
branch
from
c89a559
to
5f60985
Compare
marcocitus
reviewed
Jun 19, 2023
| { } | ||
| { | ||
| CitusScanState *scanState = (CitusScanState *) node; | ||
| if (scanState->tuplestorestate) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
aykut-bozkurt
force-pushed
the
scrollable-cursor-with-hold-fix
branch
from
5f60985
to
9b0f451
Compare
marcocitus
approved these changes
Jun 19, 2023
aykut-bozkurt
force-pushed
the
scrollable-cursor-with-hold-fix
branch
from
9b0f451
to
62fcde8
Compare
aykut-bozkurt
added a commit
that referenced
this pull request
Jun 21, 2023
We need to rewind the tuplestorestate's tuple index to get correct
results on fetching scrollable with hold cursors.
`PersistHoldablePortal` is responsible for persisting out
tuplestorestate inside a with hold cursor before commiting a
transaction.
It rewinds the cursor like below (`ExecutorRewindcalls` calls `rescan`):
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
ExecutorRewind(queryDesc);
}
```
At the end, it adjusts tuple index for holdStore in the portal properly.
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
if (!tuplestore_skiptuples(portal->holdStore,
portal->portalPos,
true))
elog(ERROR, "unexpected end of tuple stream");
}
```
DESCRIPTION: Fixes incorrect results on fetching scrollable with hold
cursors.
Fixes #7010
(cherry picked from commit f667f14)
aykut-bozkurt
added a commit
that referenced
this pull request
Jun 21, 2023
We need to rewind the tuplestorestate's tuple index to get correct
results on fetching scrollable with hold cursors.
`PersistHoldablePortal` is responsible for persisting out
tuplestorestate inside a with hold cursor before commiting a
transaction.
It rewinds the cursor like below (`ExecutorRewindcalls` calls `rescan`):
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
ExecutorRewind(queryDesc);
}
```
At the end, it adjusts tuple index for holdStore in the portal properly.
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
if (!tuplestore_skiptuples(portal->holdStore,
portal->portalPos,
true))
elog(ERROR, "unexpected end of tuple stream");
}
```
DESCRIPTION: Fixes incorrect results on fetching scrollable with hold
cursors.
Fixes #7010
(cherry picked from commit f667f14)
aykut-bozkurt
added a commit
that referenced
this pull request
Jun 21, 2023
We need to rewind the tuplestorestate's tuple index to get correct
results on fetching scrollable with hold cursors.
`PersistHoldablePortal` is responsible for persisting out
tuplestorestate inside a with hold cursor before commiting a
transaction.
It rewinds the cursor like below (`ExecutorRewindcalls` calls `rescan`):
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
ExecutorRewind(queryDesc);
}
```
At the end, it adjusts tuple index for holdStore in the portal properly.
```c
if (portal->cursorOptions & CURSOR_OPT_SCROLL)
{
if (!tuplestore_skiptuples(portal->holdStore,
portal->portalPos,
true))
elog(ERROR, "unexpected end of tuple stream");
}
```
DESCRIPTION: Fixes incorrect results on fetching scrollable with hold
cursors.
Fixes #7010
(cherry picked from commit f667f14)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We need to rewind the tuplestorestate's tuple index to get correct results on fetching scrollable with hold cursors.
PersistHoldablePortalis responsible for persisting out tuplestorestate inside a scrollable with hold cursor before commiting a transaction.It rewinds the scroll like below (
ExecutorRewindcallscallsrescan):At the end, it adjusts tuple index for holdStore in the portal properly.
DESCRIPTION: Fixes incorrect results on fetching scrollable with hold cursors.
Fixes #7010