From c5b28184132d30ce7f77b24b0e7f302a8f37f407 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 8 Jun 2021 18:40:06 -0400 Subject: [PATCH] Force NO SCROLL for plpgsql's implicit cursors. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Further thought about bug #17050 suggests that it's a good idea to use CURSOR_OPT_NO_SCROLL for the implicit cursor opened by a plpgsql FOR-over-query loop. This ensures that, if somebody commits inside the loop, PersistHoldablePortal won't try to rewind and re-read the cursor. While we'd have selected NO_SCROLL anyway if FOR UPDATE/SHARE appears in the query, there are other hazards with volatile functions; and in any case, it's silly to expend effort storing rows that we know for certain won't be needed. (While here, improve the comment in exec_run_select, which was a bit confused about the rationale for when we can use parallel mode. Cursor operations aren't a hazard for nameless portals.) This wasn't an issue until v11, which introduced the possibility of persisting such cursors. Hence, back-patch to v11. Per bug #17050 from Алексей Булгаков. Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org --- src/pl/plpgsql/src/pl_exec.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index b5d20c09be..dd72317743 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -4558,7 +4558,7 @@ exec_stmt_dynfors(PLpgSQL_execstate *estate, PLpgSQL_stmt_dynfors *stmt) int rc; portal = exec_dynquery_with_params(estate, stmt->query, stmt->params, - NULL, 0); + NULL, CURSOR_OPT_NO_SCROLL); /* * Execute the loop @@ -5903,14 +5903,21 @@ exec_run_select(PLpgSQL_execstate *estate, * On the first call for this expression generate the plan. * * If we don't need to return a portal, then we're just going to execute - * the query once, which means it's OK to use a parallel plan, even if the - * number of rows being fetched is limited. If we do need to return a - * portal, the caller might do cursor operations, which parallel query - * can't support. + * the query immediately, which means it's OK to use a parallel plan, even + * if the number of rows being fetched is limited. If we do need to + * return a portal (i.e., this is for a FOR loop), the user's code might + * invoke additional operations inside the FOR loop, making parallel query + * unsafe. In any case, we don't expect any cursor operations to be done, + * so specify NO_SCROLL for efficiency and semantic safety. */ if (expr->plan == NULL) - exec_prepare_plan(estate, expr, - portalP == NULL ? CURSOR_OPT_PARALLEL_OK : 0, true); + { + int cursorOptions = CURSOR_OPT_NO_SCROLL; + + if (portalP == NULL) + cursorOptions |= CURSOR_OPT_PARALLEL_OK; + exec_prepare_plan(estate, expr, cursorOptions, true); + } /* * Set up ParamListInfo to pass to executor