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

Commitcfc86f9

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.This is a back-patch of commit2e51781. That's now aged long enoughto reduce the concerns about whether it will break something, and wedo need to ensure that supported branches will work with Python 3.11.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 parent419c727 commitcfc86f9

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>
@@ -4414,15 +4413,17 @@ void SPI_commit_and_chain(void)
44144413
<para>
44154414
<function>SPI_commit</function> commits the current transaction. It is
44164415
approximately equivalent to running the SQL
4417-
command <command>COMMIT</command>. After a transaction is committed, a new
4418-
transaction has to be started
4419-
using <function>SPI_start_transaction</function> before further database
4420-
actions can be executed.
4416+
command <command>COMMIT</command>. After the transaction is committed, a
4417+
new transaction is automatically started using default transaction
4418+
characteristics, so that the caller can continue using SPI facilities.
4419+
If there is a failure during commit, the current transaction is instead
4420+
rolled back and a new transaction is started, after which the error is
4421+
thrown in the usual way.
44214422
</para>
44224423

44234424
<para>
4424-
<function>SPI_commit_and_chain</function> is the same, buta new
4425-
transaction isimmediatelystarted with the same transaction
4425+
<function>SPI_commit_and_chain</function> is the same, butthe new
4426+
transaction is started with the same transaction
44264427
characteristics as the just finished one, like with the SQL command
44274428
<command>COMMIT AND CHAIN</command>.
44284429
</para>
@@ -4467,14 +4468,13 @@ void SPI_rollback_and_chain(void)
44674468
<para>
44684469
<function>SPI_rollback</function> rolls back the current transaction. It
44694470
is approximately equivalent to running the SQL
4470-
command <command>ROLLBACK</command>. After a transaction is rolled back, a
4471-
new transaction has to be started
4472-
using <function>SPI_start_transaction</function> before further database
4473-
actions can be executed.
4471+
command <command>ROLLBACK</command>. After the transaction is rolled back,
4472+
a new transaction is automatically started using default transaction
4473+
characteristics, so that the caller can continue using SPI facilities.
44744474
</para>
44754475
<para>
4476-
<function>SPI_rollback_and_chain</function> is the same, buta new
4477-
transaction isimmediatelystarted with the same transaction
4476+
<function>SPI_rollback_and_chain</function> is the same, butthe new
4477+
transaction is started with the same transaction
44784478
characteristics as the just finished one, like with the SQL command
44794479
<command>ROLLBACK AND CHAIN</command>.
44804480
</para>
@@ -4498,7 +4498,7 @@ void SPI_rollback_and_chain(void)
44984498

44994499
<refnamediv>
45004500
<refname>SPI_start_transaction</refname>
4501-
<refpurpose>start a new transaction</refpurpose>
4501+
<refpurpose>obsolete function</refpurpose>
45024502
</refnamediv>
45034503

45044504
<refsynopsisdiv>
@@ -4511,17 +4511,12 @@ void SPI_start_transaction(void)
45114511
<title>Description</title>
45124512

45134513
<para>
4514-
<function>SPI_start_transaction</function> starts a new transaction. It
4515-
can only be called after <function>SPI_commit</function>
4516-
or <function>SPI_rollback</function>, as there is no transaction active at
4517-
that point. Normally, when an SPI-using procedure is called, there is already a
4518-
transaction active, so attempting to start another one before closing out
4519-
the current one will result in an error.
4520-
</para>
4521-
4522-
<para>
4523-
This function can only be executed if the SPI connection has been set as
4524-
nonatomic in the call to <function>SPI_connect_ext</function>.
4514+
<function>SPI_start_transaction</function> does nothing, and exists
4515+
only for code compatibility with
4516+
earlier <productname>PostgreSQL</productname> releases. It used to
4517+
be required after calling <function>SPI_commit</function>
4518+
or <function>SPI_rollback</function>, but now those functions start
4519+
a new transaction automatically.
45254520
</para>
45264521
</refsect1>
45274522
</refentry>

‎src/backend/executor/spi.c

Lines changed: 157 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,8 @@ SPI_connect_ext(int options)
150150
* XXX It could be better to use PortalContext as the parent context in
151151
* all cases, but we may not be inside a portal (consider deferred-trigger
152152
* execution). Perhaps CurTransactionContext could be an option? For now
153-
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
153+
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
154+
* but see also AtEOXact_SPI().
154155
*/
155156
_SPI_current->procCxt=AllocSetContextCreate(_SPI_current->atomic ?TopTransactionContext :PortalContext,
156157
"SPI Proc",
@@ -208,20 +209,26 @@ SPI_finish(void)
208209
returnSPI_OK_FINISH;
209210
}
210211

212+
/*
213+
* SPI_start_transaction is a no-op, kept for backwards compatibility.
214+
* SPI callers are *always* inside a transaction.
215+
*/
211216
void
212217
SPI_start_transaction(void)
213218
{
214-
MemoryContextoldcontext=CurrentMemoryContext;
215-
216-
StartTransactionCommand();
217-
MemoryContextSwitchTo(oldcontext);
218219
}
219220

220221
staticvoid
221222
_SPI_commit(boolchain)
222223
{
223224
MemoryContextoldcontext=CurrentMemoryContext;
224225

226+
/*
227+
* Complain if we are in a context that doesn't permit transaction
228+
* termination. (Note: here and _SPI_rollback should be the only places
229+
* that throw ERRCODE_INVALID_TRANSACTION_TERMINATION, so that callers can
230+
* test for that with security that they know what happened.)
231+
*/
225232
if (_SPI_current->atomic)
226233
ereport(ERROR,
227234
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -234,40 +241,74 @@ _SPI_commit(bool chain)
234241
* top-level transaction in such a block violates that idea. A future PL
235242
* implementation might have different ideas about this, in which case
236243
* this restriction would have to be refined or the check possibly be
237-
* moved out of SPI into the PLs.
244+
* moved out of SPI into the PLs. Note however that the code below relies
245+
* on not being within a subtransaction.
238246
*/
239247
if (IsSubTransaction())
240248
ereport(ERROR,
241249
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
242250
errmsg("cannot commit while a subtransaction is active")));
243251

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();
252+
/* XXX this ain't re-entrant enough for my taste */
253+
if (chain)
254+
SaveTransactionCharacteristics();
250255

251-
/* Start the actual commit */
252-
_SPI_current->internal_xact= true;
256+
/* Catch any error occurring during the COMMIT */
257+
PG_TRY();
258+
{
259+
/* Protect current SPI stack entry against deletion */
260+
_SPI_current->internal_xact= true;
253261

254-
/* Release snapshots associated with portals */
255-
ForgetPortalSnapshots();
262+
/*
263+
* Hold any pinned portals that any PLs might be using. We have to do
264+
* this before changing transaction state, since this will run
265+
* user-defined code that might throw an error.
266+
*/
267+
HoldPinnedPortals();
256268

257-
if (chain)
258-
SaveTransactionCharacteristics();
269+
/* Release snapshots associated with portals */
270+
ForgetPortalSnapshots();
259271

260-
CommitTransactionCommand();
272+
/* Do the deed */
273+
CommitTransactionCommand();
261274

262-
if (chain)
263-
{
275+
/* Immediately start a new transaction */
264276
StartTransactionCommand();
265-
RestoreTransactionCharacteristics();
277+
if (chain)
278+
RestoreTransactionCharacteristics();
279+
280+
MemoryContextSwitchTo(oldcontext);
281+
282+
_SPI_current->internal_xact= false;
266283
}
284+
PG_CATCH();
285+
{
286+
ErrorData*edata;
267287

268-
MemoryContextSwitchTo(oldcontext);
288+
/* Save error info in caller's context */
289+
MemoryContextSwitchTo(oldcontext);
290+
edata=CopyErrorData();
291+
FlushErrorState();
269292

270-
_SPI_current->internal_xact= false;
293+
/*
294+
* Abort the failed transaction. If this fails too, we'll just
295+
* propagate the error out ... there's not that much we can do.
296+
*/
297+
AbortCurrentTransaction();
298+
299+
/* ... and start a new one */
300+
StartTransactionCommand();
301+
if (chain)
302+
RestoreTransactionCharacteristics();
303+
304+
MemoryContextSwitchTo(oldcontext);
305+
306+
_SPI_current->internal_xact= false;
307+
308+
/* Now that we've cleaned up the transaction, re-throw the error */
309+
ReThrowError(edata);
310+
}
311+
PG_END_TRY();
271312
}
272313

273314
void
@@ -287,6 +328,7 @@ _SPI_rollback(bool chain)
287328
{
288329
MemoryContextoldcontext=CurrentMemoryContext;
289330

331+
/* see under SPI_commit() */
290332
if (_SPI_current->atomic)
291333
ereport(ERROR,
292334
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -298,34 +340,68 @@ _SPI_rollback(bool chain)
298340
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
299341
errmsg("cannot roll back while a subtransaction is active")));
300342

301-
/*
302-
* Hold any pinned portals that any PLs might be using. We have to do
303-
* this before changing transaction state, since this will run
304-
* user-defined code that might throw an error, and in any case couldn't
305-
* be run in an already-aborted transaction.
306-
*/
307-
HoldPinnedPortals();
343+
/* XXX this ain't re-entrant enough for my taste */
344+
if (chain)
345+
SaveTransactionCharacteristics();
308346

309-
/* Start the actual rollback */
310-
_SPI_current->internal_xact= true;
347+
/* Catch any error occurring during the ROLLBACK */
348+
PG_TRY();
349+
{
350+
/* Protect current SPI stack entry against deletion */
351+
_SPI_current->internal_xact= true;
311352

312-
/* Release snapshots associated with portals */
313-
ForgetPortalSnapshots();
353+
/*
354+
* Hold any pinned portals that any PLs might be using. We have to do
355+
* this before changing transaction state, since this will run
356+
* user-defined code that might throw an error, and in any case
357+
* couldn't be run in an already-aborted transaction.
358+
*/
359+
HoldPinnedPortals();
314360

315-
if (chain)
316-
SaveTransactionCharacteristics();
361+
/* Release snapshots associated with portals */
362+
ForgetPortalSnapshots();
317363

318-
AbortCurrentTransaction();
364+
/* Do the deed */
365+
AbortCurrentTransaction();
319366

320-
if (chain)
321-
{
367+
/* Immediately start a new transaction */
322368
StartTransactionCommand();
323-
RestoreTransactionCharacteristics();
369+
if (chain)
370+
RestoreTransactionCharacteristics();
371+
372+
MemoryContextSwitchTo(oldcontext);
373+
374+
_SPI_current->internal_xact= false;
324375
}
376+
PG_CATCH();
377+
{
378+
ErrorData*edata;
325379

326-
MemoryContextSwitchTo(oldcontext);
380+
/* Save error info in caller's context */
381+
MemoryContextSwitchTo(oldcontext);
382+
edata=CopyErrorData();
383+
FlushErrorState();
327384

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

331407
void
@@ -340,38 +416,55 @@ SPI_rollback_and_chain(void)
340416
_SPI_rollback(true);
341417
}
342418

343-
/*
344-
* Clean up SPI state. Called on transaction end (of non-SPI-internal
345-
* transactions) and when returning to the main loop on error.
346-
*/
347-
void
348-
SPICleanup(void)
349-
{
350-
_SPI_current=NULL;
351-
_SPI_connected=-1;
352-
/* Reset API global variables, too */
353-
SPI_processed=0;
354-
SPI_tuptable=NULL;
355-
SPI_result=0;
356-
}
357-
358419
/*
359420
* Clean up SPI state at transaction commit or abort.
360421
*/
361422
void
362423
AtEOXact_SPI(boolisCommit)
363424
{
364-
/* Do nothing if the transaction end was initiated by SPI. */
365-
if (_SPI_current&&_SPI_current->internal_xact)
366-
return;
425+
boolfound= false;
367426

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

377470
/*

0 commit comments

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