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

ysql: Import Avoid misbehavior when persisting a non-stable cursor. #10121

Closed
tedyu opened this issue Sep 27, 2021 · 0 comments
Closed

ysql: Import Avoid misbehavior when persisting a non-stable cursor. #10121

tedyu opened this issue Sep 27, 2021 · 0 comments
Assignees

Comments

@tedyu
Copy link
Contributor

tedyu commented Sep 27, 2021

Commit was 2757865fa7fb213eb83088927d5c8d5a9425e555

Commit message was

PersistHoldablePortal has long assumed that it should store the
entire output of the query-to-be-persisted, which requires rewinding
and re-reading the output. This is problematic if the query is not
stable: we might get different row contents, or even a different
number of rows, which'd confuse the cursor state mightily.

In the case where the cursor is NO SCROLL, this is very easy to
solve: just store the remaining query output, without any rewinding,
and tweak the portal's cursor state to match. Aside from removing
the semantic problem, this could be significantly more efficient
than storing the whole output.

If the cursor is scrollable, there's not much we can do, but it
was already the case that scrolling a volatile query's result was
pretty unsafe. We can just document more clearly that getting
correct results from that is not guaranteed.

There are already prohibitions in place on using SCROLL with
FOR UPDATE/SHARE, which is one way for a SELECT query to have
non-stable results. We could imagine prohibiting SCROLL when
the query contains volatile functions, but that would be
expensive to enforce. Moreover, it could break applications
that work just fine, if they have functions that are in fact
stable but the user neglected to mark them so. So settle for
documenting the hazard.

While this problem has existed in some guise for a long time,
it got a lot worse in v11, which introduced the possibility
of persisting plpgsql cursors (perhaps implicit ones) even
when they violate the rules for what can be marked WITH HOLD.
Hence, I've chosen to back-patch to v11 but not further.

Per bug #17050 from Алексей Булгаков.

Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org

@tedyu tedyu self-assigned this Sep 27, 2021
tedyu added a commit that referenced this issue Sep 29, 2021
…cursor.

Summary:
Commit was 2757865fa7fb213eb83088927d5c8d5a9425e555

Commit message was

    PersistHoldablePortal has long assumed that it should store the
    entire output of the query-to-be-persisted, which requires rewinding
    and re-reading the output.  This is problematic if the query is not
    stable: we might get different row contents, or even a different
    number of rows, which'd confuse the cursor state mightily.

    In the case where the cursor is NO SCROLL, this is very easy to
    solve: just store the remaining query output, without any rewinding,
    and tweak the portal's cursor state to match.  Aside from removing
    the semantic problem, this could be significantly more efficient
    than storing the whole output.

    If the cursor is scrollable, there's not much we can do, but it
    was already the case that scrolling a volatile query's result was
    pretty unsafe.  We can just document more clearly that getting
    correct results from that is not guaranteed.

    There are already prohibitions in place on using SCROLL with
    FOR UPDATE/SHARE, which is one way for a SELECT query to have
    non-stable results.  We could imagine prohibiting SCROLL when
    the query contains volatile functions, but that would be
    expensive to enforce.  Moreover, it could break applications
    that work just fine, if they have functions that are in fact
    stable but the user neglected to mark them so.  So settle for
    documenting the hazard.

    While this problem has existed in some guise for a long time,
    it got a lot worse in v11, which introduced the possibility
    of persisting plpgsql cursors (perhaps implicit ones) even
    when they violate the rules for what can be marked WITH HOLD.
    Hence, I've chosen to back-patch to v11 but not further.

    Per bug #17050 from Алексей Булгаков.

    Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org

Test Plan: Build Yugabyte DB and verify there is no test regression.

Reviewers: amartsinchyk, myang

Reviewed By: myang

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D13209
@tedyu tedyu closed this as completed Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant