On 19.08.2020 22:20, Pavel Stehule wrote:


st 19. 8. 2020 v 20:59 odesílatel Konstantin Knizhnik <k.knizh...@postgrespro.ru <mailto:k.knizh...@postgrespro.ru>> napsal:



    On 19.08.2020 21:50, Pavel Stehule wrote:
    Hi

    st 19. 8. 2020 v 19:22 odesílatel Konstantin Knizhnik
    <k.knizh...@postgrespro.ru <mailto:k.knizh...@postgrespro.ru>>
    napsal:

        Hi hackers,

        More than month ago I have sent bug report to pgsql-bugs:

        
https://www.postgresql.org/message-id/flat/5d335911-fb25-60cd-4aa7-a5bd0954aea0%40postgrespro.ru

        with the proposed patch but have not received any response.

        I wonder if there is some other way to fix this issue and
        does somebody
        working on it.
        While the added check itself is trivial (just one line) the
        total patch
        is not so small because I have added walker for
        plpgsql statements tree. It is not strictly needed in this
        case (it is
        possible to find some other way to determine that stored
        procedure
        contains transaction control statements), but I hope such
        walker may be
        useful in other cases.

        In any case, I will be glad to receive any response,
        because this problem was reported by one of our customers and
        we need to
        provide some fix.
        It is better to include it in vanilla, rather than in our
        pgpro-ee fork.

        If it is desirable, I can add this patch to commitfest.



    I don't like this design. It is not effective to repeat the
    walker for every execution. Introducing a walker just for this
    case looks like overengineering.
    Personally I am not sure if a walker for plpgsql is a good idea
    (I thought about it more times, when I wrote plpgsql_check). But
    anyway - there should be good reason for introducing the walker
    and clean use case.

    If you want to introduce stmt walker, then it should be a
    separate patch with some benefit on plpgsql environment length.

    If you think that plpgsql statement walker is not needed, then I
    do not insist.
    Are you going to commit your version of the patch?


I am afraid so it needs significantly much more work :(. My version is correct just for the case that you describe, but it doesn't fix the possibility of the end of the transaction inside the nested CALL.

Some like

DO $$ DECLARE v_r record; BEGIN FOR v_r in SELECT data FROM toasted LOOP INSERT INTO toasted(data) VALUES(v_r.data) CALL check_and_commit();END LOOP;END;$$;

Probably my patch (or your patch) will fix on 99%, but still there will be a possibility of this issue. It is very similar to fixing releasing expr plans inside CALL statements. Current design of CALL statement is ugly workaround - it is slow, and there is brutal memory leak. Fixing memory leak is not hard. Fixing every time replaning (and sometimes useless) needs depper fix. Please check patch https://www.postgresql.org/message-id/attachment/112489/plpgsql-stmt_call-fix-2.patch Maybe this mechanism can be used for a clean fix of the problem mentioned in this thread.

Sorry for delay with answer.
Today we have received another bug report from the client.
And now as you warned, there was no direct call of COMMIT/ROLLBACK statements in stored procedures, but instead of it it is calling some other pprocedures
which I suspect contains some transaction control statements.

I looked at the plpgsql-stmt_call-fix-2.patch <https://www.postgresql.org/message-id/attachment/112489/plpgsql-stmt_call-fix-2.patch>
It invalidates prepared plan in case of nested procedure call.
But here invalidation approach will not work. We have already prefetched rows and to access them we need snapshot.
We can not restore the same snapshot after CALL - it will be not correct.
In case of ATX (autonomous transactions supported by PgPro) we really  save/restore context after ATX. But transaction control semantic in procedures is different:
we commit current transaction and start new one.

So I didn't find better solution than just slightly extend you patch and consider any procedures containing CALLs as potentially performing transaction control.
I updated version of your patch.
What do you think about it?

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b4c70aa..3c074cd 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5827,6 +5827,12 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
 	PinPortal(portal);
 
 	/*
+	 * Disable prefetch if procedure contains COMMIT, ROLLBACK or CALL statements
+	 */
+	if (estate->func->fn_xactctrl)
+		prefetch_ok = false;
+
+	/*
 	 * Fetch the initial tuple(s).  If prefetching is allowed then we grab a
 	 * few more rows to avoid multiple trips through executor startup
 	 * overhead.
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 34e0520..ae57475 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -935,6 +935,8 @@ stmt_perform	: K_PERFORM
 						check_sql_expr(new->expr->query, new->expr->parseMode,
 									   startloc + 1);
 
+						plpgsql_curr_compile->fn_xactctrl = true;
+
 						$$ = (PLpgSQL_stmt *)new;
 					}
 				;
@@ -2249,6 +2251,8 @@ stmt_commit		: K_COMMIT opt_transaction_chain ';'
 						new->stmtid = ++plpgsql_curr_compile->nstatements;
 						new->chain = $2;
 
+						plpgsql_curr_compile->fn_xactctrl = true;
+
 						$$ = (PLpgSQL_stmt *)new;
 					}
 				;
@@ -2263,6 +2267,8 @@ stmt_rollback	: K_ROLLBACK opt_transaction_chain ';'
 						new->stmtid = ++plpgsql_curr_compile->nstatements;
 						new->chain = $2;
 
+						plpgsql_curr_compile->fn_xactctrl = true;
+
 						$$ = (PLpgSQL_stmt *)new;
 					}
 				;
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index d501086..3210116 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -992,6 +992,7 @@ typedef struct PLpgSQL_function
 	bool		fn_retisdomain;
 	bool		fn_retset;
 	bool		fn_readonly;
+	bool		fn_xactctrl;
 	char		fn_prokind;
 
 	int			fn_nargs;

Reply via email to