Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit4d5840c

Browse files
committed
Fix problems with auto-held portals.
HoldPinnedPortals() did things in the wrong order: it must not marka portal autoHeld until it's been successfully held. Otherwise,a failure while persisting the portal results in a server crashbecause we think the portal is in a good state when it's not.Also add a check that portal->status is READY before attempting tohold a pinned portal. We have such a check before the only otheruse of HoldPortal(), so it seems unwise not to check it here.Lastly, rethink the responsibility for where to call HoldPinnedPortals.The comment for it imagined that it was optional for any individual PLto call it or not, but that cannot be the case: if some outer level ofprocedure has a pinned portal, failing to persist it when an innerprocedure commits is going to be trouble. Let's have SPI do it insteadof the individual PLs. That's not a complete solution, since in theorya PL might not be using SPI to perform commit/rollback, but such a PLis going to have to be aware of lots of related requirements anyway.(This change doesn't cause an API break for any external PLs that mightbe calling HoldPinnedPortals per the old regime, because calling ittwice during a commit or rollback sequence won't hurt.)Per bug #15703 from Julian Schauder. Back-patch to v11 where this codecame in.Discussion:https://postgr.es/m/15703-c12c5bc0ea34ba26@postgresql.org
1 parent148266f commit4d5840c

File tree

7 files changed

+117
-20
lines changed

7 files changed

+117
-20
lines changed

‎src/backend/executor/spi.c

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,14 @@ _SPI_commit(bool chain)
241241
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
242242
errmsg("cannot commit while a subtransaction is active")));
243243

244+
/*
245+
* Hold any pinned portals that any PLs might be using. We have to do
246+
* this before changing transaction state, since this will run
247+
* user-defined code that might throw an error.
248+
*/
249+
HoldPinnedPortals();
250+
251+
/* Start the actual commit */
244252
_SPI_current->internal_xact= true;
245253

246254
/*
@@ -294,6 +302,15 @@ _SPI_rollback(bool chain)
294302
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
295303
errmsg("cannot roll back while a subtransaction is active")));
296304

305+
/*
306+
* Hold any pinned portals that any PLs might be using. We have to do
307+
* this before changing transaction state, since this will run
308+
* user-defined code that might throw an error, and in any case couldn't
309+
* be run in an already-aborted transaction.
310+
*/
311+
HoldPinnedPortals();
312+
313+
/* Start the actual rollback */
297314
_SPI_current->internal_xact= true;
298315

299316
if (chain)

‎src/backend/utils/mmgr/portalmem.c

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1226,13 +1226,19 @@ ThereAreNoReadyPortals(void)
12261226
/*
12271227
* Hold all pinned portals.
12281228
*
1229-
* A procedural language implementation that uses pinned portals for its
1230-
* internally generated cursors can call this in its COMMIT command to convert
1231-
* those cursors to held cursors, so that they survive the transaction end.
1232-
* We mark those portals as "auto-held" so that exception exit knows to clean
1233-
* them up. (In normal, non-exception code paths, the PL needs to clean those
1234-
* portals itself, since transaction end won't do it anymore, but that should
1235-
* be normal practice anyway.)
1229+
* When initiating a COMMIT or ROLLBACK inside a procedure, this must be
1230+
* called to protect internally-generated cursors from being dropped during
1231+
* the transaction shutdown. Currently, SPI calls this automatically; PLs
1232+
* that initiate COMMIT or ROLLBACK some other way are on the hook to do it
1233+
* themselves. (Note that we couldn't do this in, say, AtAbort_Portals
1234+
* because we need to run user-defined code while persisting a portal.
1235+
* It's too late to do that once transaction abort has started.)
1236+
*
1237+
* We protect such portals by converting them to held cursors. We mark them
1238+
* as "auto-held" so that exception exit knows to clean them up. (In normal,
1239+
* non-exception code paths, the PL needs to clean such portals itself, since
1240+
* transaction end won't do it anymore; but that should be normal practice
1241+
* anyway.)
12361242
*/
12371243
void
12381244
HoldPinnedPortals(void)
@@ -1262,8 +1268,12 @@ HoldPinnedPortals(void)
12621268
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
12631269
errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));
12641270

1265-
portal->autoHeld= true;
1271+
/* Verify it's in a suitable state to be held */
1272+
if (portal->status!=PORTAL_READY)
1273+
elog(ERROR,"pinned portal is not ready to be auto-held");
1274+
12661275
HoldPortal(portal);
1276+
portal->autoHeld= true;
12671277
}
12681278
}
12691279
}

‎src/pl/plperl/plperl.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3988,8 +3988,6 @@ plperl_spi_commit(void)
39883988

39893989
PG_TRY();
39903990
{
3991-
HoldPinnedPortals();
3992-
39933991
SPI_commit();
39943992
SPI_start_transaction();
39953993
}
@@ -4015,8 +4013,6 @@ plperl_spi_rollback(void)
40154013

40164014
PG_TRY();
40174015
{
4018-
HoldPinnedPortals();
4019-
40204016
SPI_rollback();
40214017
SPI_start_transaction();
40224018
}

‎src/pl/plpgsql/src/expected/plpgsql_transaction.out

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,50 @@ SELECT * FROM test3;
401401
1
402402
(1 row)
403403

404+
-- failure while trying to persist a cursor across a transaction (bug #15703)
405+
CREATE PROCEDURE cursor_fail_during_commit()
406+
LANGUAGE plpgsql
407+
AS $$
408+
DECLARE id int;
409+
BEGIN
410+
FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
411+
INSERT INTO test1 VALUES(id);
412+
COMMIT;
413+
END LOOP;
414+
END;
415+
$$;
416+
TRUNCATE test1;
417+
CALL cursor_fail_during_commit();
418+
ERROR: division by zero
419+
CONTEXT: PL/pgSQL function cursor_fail_during_commit() line 6 at COMMIT
420+
-- note that error occurs during first COMMIT, hence nothing is in test1
421+
SELECT count(*) FROM test1;
422+
count
423+
-------
424+
0
425+
(1 row)
426+
427+
CREATE PROCEDURE cursor_fail_during_rollback()
428+
LANGUAGE plpgsql
429+
AS $$
430+
DECLARE id int;
431+
BEGIN
432+
FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
433+
INSERT INTO test1 VALUES(id);
434+
ROLLBACK;
435+
END LOOP;
436+
END;
437+
$$;
438+
TRUNCATE test1;
439+
CALL cursor_fail_during_rollback();
440+
ERROR: division by zero
441+
CONTEXT: PL/pgSQL function cursor_fail_during_rollback() line 6 at ROLLBACK
442+
SELECT count(*) FROM test1;
443+
count
444+
-------
445+
0
446+
(1 row)
447+
404448
-- SET TRANSACTION
405449
DO LANGUAGE plpgsql $$
406450
BEGIN

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4791,8 +4791,6 @@ exec_stmt_close(PLpgSQL_execstate *estate, PLpgSQL_stmt_close *stmt)
47914791
staticint
47924792
exec_stmt_commit(PLpgSQL_execstate*estate,PLpgSQL_stmt_commit*stmt)
47934793
{
4794-
HoldPinnedPortals();
4795-
47964794
if (stmt->chain)
47974795
SPI_commit_and_chain();
47984796
else
@@ -4815,8 +4813,6 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
48154813
staticint
48164814
exec_stmt_rollback(PLpgSQL_execstate*estate,PLpgSQL_stmt_rollback*stmt)
48174815
{
4818-
HoldPinnedPortals();
4819-
48204816
if (stmt->chain)
48214817
SPI_rollback_and_chain();
48224818
else

‎src/pl/plpgsql/src/sql/plpgsql_transaction.sql

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,44 @@ $$;
329329

330330
SELECT*FROM test3;
331331

332+
-- failure while trying to persist a cursor across a transaction (bug #15703)
333+
CREATE PROCEDURE cursor_fail_during_commit()
334+
LANGUAGE plpgsql
335+
AS $$
336+
DECLARE idint;
337+
BEGIN
338+
FOR idINSELECT1/(x-1000)FROM generate_series(1,1000) x LOOP
339+
INSERT INTO test1VALUES(id);
340+
COMMIT;
341+
END LOOP;
342+
END;
343+
$$;
344+
345+
TRUNCATE test1;
346+
347+
CALL cursor_fail_during_commit();
348+
349+
-- note that error occurs during first COMMIT, hence nothing is in test1
350+
SELECTcount(*)FROM test1;
351+
352+
CREATE PROCEDURE cursor_fail_during_rollback()
353+
LANGUAGE plpgsql
354+
AS $$
355+
DECLARE idint;
356+
BEGIN
357+
FOR idINSELECT1/(x-1000)FROM generate_series(1,1000) x LOOP
358+
INSERT INTO test1VALUES(id);
359+
ROLLBACK;
360+
END LOOP;
361+
END;
362+
$$;
363+
364+
TRUNCATE test1;
365+
366+
CALL cursor_fail_during_rollback();
367+
368+
SELECTcount(*)FROM test1;
369+
332370

333371
-- SET TRANSACTION
334372
DO LANGUAGE plpgsql $$

‎src/pl/plpython/plpy_plpymodule.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -588,8 +588,6 @@ PLy_commit(PyObject *self, PyObject *args)
588588
{
589589
PLyExecutionContext*exec_ctx=PLy_current_execution_context();
590590

591-
HoldPinnedPortals();
592-
593591
SPI_commit();
594592
SPI_start_transaction();
595593

@@ -604,8 +602,6 @@ PLy_rollback(PyObject *self, PyObject *args)
604602
{
605603
PLyExecutionContext*exec_ctx=PLy_current_execution_context();
606604

607-
HoldPinnedPortals();
608-
609605
SPI_rollback();
610606
SPI_start_transaction();
611607

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp