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

Commit2e51781

Browse files
committed
Fix SPI's handling of errors during transaction commit.
SPI_commit previously left it up to the caller to recover from any erroroccurring during commit. Since that's complicated and requires use oflow-level xact.c facilities, it's not too surprising that no caller gotit right. Let's move the responsibility for cleanup into spi.c. Doingthat requires redefining SPI_commit as starting a new transaction, sothat it becomes equivalent to SPI_commit_and_chain except that you getdefault transaction characteristics instead of preserving the priortransaction's characteristics. We can make this pretty transparentAPI-wise by redefining SPI_start_transaction() as a no-op. Callersthat expect to do something in between might be surprised, butavailable evidence is that no callers do so.Having made that API redefinition, we can fix this mess by havingSPI_commit[_and_chain] trap errors and start a new, clean transactionbefore re-throwing the error. Likewise for SPI_rollback[_and_chain].Some cleanup is also needed in AtEOXact_SPI, which was nowhere nearsmart enough to deal with SPI contexts nested inside a committingcontext.While plperl and pltcl need no changes beyond removing their now-uselessSPI_start_transaction() calls, plpython needs some more work because ithadn't gotten the memo about catching commit/rollback errors in thefirst place. Such an error resulted in longjmp'ing out of the Pythoninterpreter, which leaks Python stack entries at present and is reportedto crash Python 3.11 altogether. Add the missing logic to catch sucherrors and convert them into Python exceptions.We are probably going to have to back-patch this once Python 3.11 ships,but it's a sufficiently basic change that I'm a bit nervous about doingso immediately. Let's let it bake awhile in HEAD first.Peter Eisentraut and Tom LaneDiscussion:https://postgr.es/m/3375ffd8-d71c-2565-e348-a597d6e739e3@enterprisedb.comDiscussion:https://postgr.es/m/17416-ed8fe5d7213d6c25@postgresql.org
1 parentb15f254 commit2e51781

File tree

17 files changed

+535
-142
lines changed

17 files changed

+535
-142
lines changed

‎doc/src/sgml/spi.sgml

Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,9 @@ int SPI_connect_ext(int <parameter>options</parameter>)
9999
<listitem>
100100
<para>
101101
Sets the SPI connection to be <firstterm>nonatomic</firstterm>, which
102-
means that transaction control calls <function>SPI_commit</function>,
103-
<function>SPI_rollback</function>, and
104-
<function>SPI_start_transaction</function> are allowed. Otherwise,
105-
calling these functions will result in an immediate error.
102+
means that transaction control calls (<function>SPI_commit</function>,
103+
<function>SPI_rollback</function>) are allowed. Otherwise,
104+
calling those functions will result in an immediate error.
106105
</para>
107106
</listitem>
108107
</varlistentry>
@@ -5040,15 +5039,17 @@ void SPI_commit_and_chain(void)
50405039
<para>
50415040
<function>SPI_commit</function> commits the current transaction. It is
50425041
approximately equivalent to running the SQL
5043-
command <command>COMMIT</command>. After a transaction is committed, a new
5044-
transaction has to be started
5045-
using <function>SPI_start_transaction</function> before further database
5046-
actions can be executed.
5042+
command <command>COMMIT</command>. After the transaction is committed, a
5043+
new transaction is automatically started using default transaction
5044+
characteristics, so that the caller can continue using SPI facilities.
5045+
If there is a failure during commit, the current transaction is instead
5046+
rolled back and a new transaction is started, after which the error is
5047+
thrown in the usual way.
50475048
</para>
50485049

50495050
<para>
5050-
<function>SPI_commit_and_chain</function> is the same, buta new
5051-
transaction isimmediatelystarted with the same transaction
5051+
<function>SPI_commit_and_chain</function> is the same, butthe new
5052+
transaction is started with the same transaction
50525053
characteristics as the just finished one, like with the SQL command
50535054
<command>COMMIT AND CHAIN</command>.
50545055
</para>
@@ -5093,14 +5094,13 @@ void SPI_rollback_and_chain(void)
50935094
<para>
50945095
<function>SPI_rollback</function> rolls back the current transaction. It
50955096
is approximately equivalent to running the SQL
5096-
command <command>ROLLBACK</command>. After a transaction is rolled back, a
5097-
new transaction has to be started
5098-
using <function>SPI_start_transaction</function> before further database
5099-
actions can be executed.
5097+
command <command>ROLLBACK</command>. After the transaction is rolled back,
5098+
a new transaction is automatically started using default transaction
5099+
characteristics, so that the caller can continue using SPI facilities.
51005100
</para>
51015101
<para>
5102-
<function>SPI_rollback_and_chain</function> is the same, buta new
5103-
transaction isimmediatelystarted with the same transaction
5102+
<function>SPI_rollback_and_chain</function> is the same, butthe new
5103+
transaction is started with the same transaction
51045104
characteristics as the just finished one, like with the SQL command
51055105
<command>ROLLBACK AND CHAIN</command>.
51065106
</para>
@@ -5124,7 +5124,7 @@ void SPI_rollback_and_chain(void)
51245124

51255125
<refnamediv>
51265126
<refname>SPI_start_transaction</refname>
5127-
<refpurpose>start a new transaction</refpurpose>
5127+
<refpurpose>obsolete function</refpurpose>
51285128
</refnamediv>
51295129

51305130
<refsynopsisdiv>
@@ -5137,17 +5137,12 @@ void SPI_start_transaction(void)
51375137
<title>Description</title>
51385138

51395139
<para>
5140-
<function>SPI_start_transaction</function> starts a new transaction. It
5141-
can only be called after <function>SPI_commit</function>
5142-
or <function>SPI_rollback</function>, as there is no transaction active at
5143-
that point. Normally, when an SPI-using procedure is called, there is already a
5144-
transaction active, so attempting to start another one before closing out
5145-
the current one will result in an error.
5146-
</para>
5147-
5148-
<para>
5149-
This function can only be executed if the SPI connection has been set as
5150-
nonatomic in the call to <function>SPI_connect_ext</function>.
5140+
<function>SPI_start_transaction</function> does nothing, and exists
5141+
only for code compatibility with
5142+
earlier <productname>PostgreSQL</productname> releases. It used to
5143+
be required after calling <function>SPI_commit</function>
5144+
or <function>SPI_rollback</function>, but now those functions start
5145+
a new transaction automatically.
51515146
</para>
51525147
</refsect1>
51535148
</refentry>

‎src/backend/executor/spi.c

Lines changed: 157 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,8 @@ SPI_connect_ext(int options)
156156
* XXX It could be better to use PortalContext as the parent context in
157157
* all cases, but we may not be inside a portal (consider deferred-trigger
158158
* execution). Perhaps CurTransactionContext could be an option? For now
159-
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
159+
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
160+
* but see also AtEOXact_SPI().
160161
*/
161162
_SPI_current->procCxt=AllocSetContextCreate(_SPI_current->atomic ?TopTransactionContext :PortalContext,
162163
"SPI Proc",
@@ -214,20 +215,26 @@ SPI_finish(void)
214215
returnSPI_OK_FINISH;
215216
}
216217

218+
/*
219+
* SPI_start_transaction is a no-op, kept for backwards compatibility.
220+
* SPI callers are *always* inside a transaction.
221+
*/
217222
void
218223
SPI_start_transaction(void)
219224
{
220-
MemoryContextoldcontext=CurrentMemoryContext;
221-
222-
StartTransactionCommand();
223-
MemoryContextSwitchTo(oldcontext);
224225
}
225226

226227
staticvoid
227228
_SPI_commit(boolchain)
228229
{
229230
MemoryContextoldcontext=CurrentMemoryContext;
230231

232+
/*
233+
* Complain if we are in a context that doesn't permit transaction
234+
* termination. (Note: here and _SPI_rollback should be the only places
235+
* that throw ERRCODE_INVALID_TRANSACTION_TERMINATION, so that callers can
236+
* test for that with security that they know what happened.)
237+
*/
231238
if (_SPI_current->atomic)
232239
ereport(ERROR,
233240
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -240,40 +247,74 @@ _SPI_commit(bool chain)
240247
* top-level transaction in such a block violates that idea. A future PL
241248
* implementation might have different ideas about this, in which case
242249
* this restriction would have to be refined or the check possibly be
243-
* moved out of SPI into the PLs.
250+
* moved out of SPI into the PLs. Note however that the code below relies
251+
* on not being within a subtransaction.
244252
*/
245253
if (IsSubTransaction())
246254
ereport(ERROR,
247255
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
248256
errmsg("cannot commit while a subtransaction is active")));
249257

250-
/*
251-
* Hold any pinned portals that any PLs might be using. We have to do
252-
* this before changing transaction state, since this will run
253-
* user-defined code that might throw an error.
254-
*/
255-
HoldPinnedPortals();
258+
/* XXX this ain't re-entrant enough for my taste */
259+
if (chain)
260+
SaveTransactionCharacteristics();
256261

257-
/* Start the actual commit */
258-
_SPI_current->internal_xact= true;
262+
/* Catch any error occurring during the COMMIT */
263+
PG_TRY();
264+
{
265+
/* Protect current SPI stack entry against deletion */
266+
_SPI_current->internal_xact= true;
259267

260-
/* Release snapshots associated with portals */
261-
ForgetPortalSnapshots();
268+
/*
269+
* Hold any pinned portals that any PLs might be using. We have to do
270+
* this before changing transaction state, since this will run
271+
* user-defined code that might throw an error.
272+
*/
273+
HoldPinnedPortals();
262274

263-
if (chain)
264-
SaveTransactionCharacteristics();
275+
/* Release snapshots associated with portals */
276+
ForgetPortalSnapshots();
265277

266-
CommitTransactionCommand();
278+
/* Do the deed */
279+
CommitTransactionCommand();
267280

268-
if (chain)
269-
{
281+
/* Immediately start a new transaction */
270282
StartTransactionCommand();
271-
RestoreTransactionCharacteristics();
283+
if (chain)
284+
RestoreTransactionCharacteristics();
285+
286+
MemoryContextSwitchTo(oldcontext);
287+
288+
_SPI_current->internal_xact= false;
272289
}
290+
PG_CATCH();
291+
{
292+
ErrorData*edata;
273293

274-
MemoryContextSwitchTo(oldcontext);
294+
/* Save error info in caller's context */
295+
MemoryContextSwitchTo(oldcontext);
296+
edata=CopyErrorData();
297+
FlushErrorState();
275298

276-
_SPI_current->internal_xact= false;
299+
/*
300+
* Abort the failed transaction. If this fails too, we'll just
301+
* propagate the error out ... there's not that much we can do.
302+
*/
303+
AbortCurrentTransaction();
304+
305+
/* ... and start a new one */
306+
StartTransactionCommand();
307+
if (chain)
308+
RestoreTransactionCharacteristics();
309+
310+
MemoryContextSwitchTo(oldcontext);
311+
312+
_SPI_current->internal_xact= false;
313+
314+
/* Now that we've cleaned up the transaction, re-throw the error */
315+
ReThrowError(edata);
316+
}
317+
PG_END_TRY();
277318
}
278319

279320
void
@@ -293,6 +334,7 @@ _SPI_rollback(bool chain)
293334
{
294335
MemoryContextoldcontext=CurrentMemoryContext;
295336

337+
/* see under SPI_commit() */
296338
if (_SPI_current->atomic)
297339
ereport(ERROR,
298340
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -304,34 +346,68 @@ _SPI_rollback(bool chain)
304346
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
305347
errmsg("cannot roll back while a subtransaction is active")));
306348

307-
/*
308-
* Hold any pinned portals that any PLs might be using. We have to do
309-
* this before changing transaction state, since this will run
310-
* user-defined code that might throw an error, and in any case couldn't
311-
* be run in an already-aborted transaction.
312-
*/
313-
HoldPinnedPortals();
349+
/* XXX this ain't re-entrant enough for my taste */
350+
if (chain)
351+
SaveTransactionCharacteristics();
314352

315-
/* Start the actual rollback */
316-
_SPI_current->internal_xact= true;
353+
/* Catch any error occurring during the ROLLBACK */
354+
PG_TRY();
355+
{
356+
/* Protect current SPI stack entry against deletion */
357+
_SPI_current->internal_xact= true;
317358

318-
/* Release snapshots associated with portals */
319-
ForgetPortalSnapshots();
359+
/*
360+
* Hold any pinned portals that any PLs might be using. We have to do
361+
* this before changing transaction state, since this will run
362+
* user-defined code that might throw an error, and in any case
363+
* couldn't be run in an already-aborted transaction.
364+
*/
365+
HoldPinnedPortals();
320366

321-
if (chain)
322-
SaveTransactionCharacteristics();
367+
/* Release snapshots associated with portals */
368+
ForgetPortalSnapshots();
323369

324-
AbortCurrentTransaction();
370+
/* Do the deed */
371+
AbortCurrentTransaction();
325372

326-
if (chain)
327-
{
373+
/* Immediately start a new transaction */
328374
StartTransactionCommand();
329-
RestoreTransactionCharacteristics();
375+
if (chain)
376+
RestoreTransactionCharacteristics();
377+
378+
MemoryContextSwitchTo(oldcontext);
379+
380+
_SPI_current->internal_xact= false;
330381
}
382+
PG_CATCH();
383+
{
384+
ErrorData*edata;
331385

332-
MemoryContextSwitchTo(oldcontext);
386+
/* Save error info in caller's context */
387+
MemoryContextSwitchTo(oldcontext);
388+
edata=CopyErrorData();
389+
FlushErrorState();
333390

334-
_SPI_current->internal_xact= false;
391+
/*
392+
* Try again to abort the failed transaction. If this fails too,
393+
* we'll just propagate the error out ... there's not that much we can
394+
* do.
395+
*/
396+
AbortCurrentTransaction();
397+
398+
/* ... and start a new one */
399+
StartTransactionCommand();
400+
if (chain)
401+
RestoreTransactionCharacteristics();
402+
403+
MemoryContextSwitchTo(oldcontext);
404+
405+
_SPI_current->internal_xact= false;
406+
407+
/* Now that we've cleaned up the transaction, re-throw the error */
408+
ReThrowError(edata);
409+
}
410+
PG_END_TRY();
335411
}
336412

337413
void
@@ -346,38 +422,55 @@ SPI_rollback_and_chain(void)
346422
_SPI_rollback(true);
347423
}
348424

349-
/*
350-
* Clean up SPI state. Called on transaction end (of non-SPI-internal
351-
* transactions) and when returning to the main loop on error.
352-
*/
353-
void
354-
SPICleanup(void)
355-
{
356-
_SPI_current=NULL;
357-
_SPI_connected=-1;
358-
/* Reset API global variables, too */
359-
SPI_processed=0;
360-
SPI_tuptable=NULL;
361-
SPI_result=0;
362-
}
363-
364425
/*
365426
* Clean up SPI state at transaction commit or abort.
366427
*/
367428
void
368429
AtEOXact_SPI(boolisCommit)
369430
{
370-
/* Do nothing if the transaction end was initiated by SPI. */
371-
if (_SPI_current&&_SPI_current->internal_xact)
372-
return;
431+
boolfound= false;
373432

374-
if (isCommit&&_SPI_connected!=-1)
433+
/*
434+
* Pop stack entries, stopping if we find one marked internal_xact (that
435+
* one belongs to the caller of SPI_commit or SPI_abort).
436+
*/
437+
while (_SPI_connected >=0)
438+
{
439+
_SPI_connection*connection=&(_SPI_stack[_SPI_connected]);
440+
441+
if (connection->internal_xact)
442+
break;
443+
444+
found= true;
445+
446+
/*
447+
* We need not release the procedure's memory contexts explicitly, as
448+
* they'll go away automatically when their parent context does; see
449+
* notes in SPI_connect_ext.
450+
*/
451+
452+
/*
453+
* Restore outer global variables and pop the stack entry. Unlike
454+
* SPI_finish(), we don't risk switching to memory contexts that might
455+
* be already gone.
456+
*/
457+
SPI_processed=connection->outer_processed;
458+
SPI_tuptable=connection->outer_tuptable;
459+
SPI_result=connection->outer_result;
460+
461+
_SPI_connected--;
462+
if (_SPI_connected<0)
463+
_SPI_current=NULL;
464+
else
465+
_SPI_current=&(_SPI_stack[_SPI_connected]);
466+
}
467+
468+
/* We should only find entries to pop during an ABORT. */
469+
if (found&&isCommit)
375470
ereport(WARNING,
376471
(errcode(ERRCODE_WARNING),
377472
errmsg("transaction left non-empty SPI stack"),
378473
errhint("Check for missing \"SPI_finish\" calls.")));
379-
380-
SPICleanup();
381474
}
382475

383476
/*

0 commit comments

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