8000 Fix SPI's handling of errors during transaction commit. · postgres/postgres@2f6b8c2 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2f6b8c2

Browse files
tglsfdctglsfdc
committed
Fix SPI's handling of errors during transaction commit.
SPI_commit previously left it up to the caller to recover from any error occurring during commit. Since that's complicated and requires use of low-level xact.c facilities, it's not too surprising that no caller got it right. Let's move the responsibility for cleanup into spi.c. Doing that requires redefining SPI_commit as starting a new transaction, so that it becomes equivalent to SPI_commit_and_chain except that you get default transaction characteristics instead of preserving the prior transaction's characteristics. We can make this pretty transparent API-wise by redefining SPI_start_transaction() as a no-op. Callers that expect to do something in between might be surprised, but available evidence is that no callers do so. Having made that API redefinition, we can fix this mess by having SPI_commit[_and_chain] trap errors and start a new, clean transaction before re-throwing the error. Likewise for SPI_rollback[_and_chain]. Some cleanup is also needed in AtEOXact_SPI, which was nowhere near smart enough to deal with SPI contexts nested inside a committing context. While plperl and pltcl need no changes beyond removing their now-useless SPI_start_transaction() calls, plpython needs some more work because it hadn't gotten the memo about catching commit/rollback errors in the first place. Such an error resulted in longjmp'ing out of the Python interpreter, which leaks Python stack entries at present and is reported to crash Python 3.11 altogether. Add the missing logic to catch such errors and convert them into Python exceptions. This is a back-patch of commit 2e51781. That's now aged long enough to reduce the concerns about whether it will break something, and we do need to ensure that supported branches will work with Python 3.11. Peter Eisentraut and Tom Lane Discussion: https://postgr.es/m/3375ffd8-d71c-2565-e348-a597d6e739e3@enterprisedb.com Discussion: https://postgr.es/m/17416-ed8fe5d7213d6c25@postgresql.org
1 parent f779774 commit 2f6b8c2

File tree

17 files changed

+524
-124
lines changed

17 files changed

+524
-124
lines changed

‎doc/src/sgml/spi.sgml

Lines changed: 19 additions & 24 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>
@@ -4399,10 +4398,12 @@ void SPI_commit(void)
43994398
<para>
44004399
<function>SPI_commit</function> commits the current transaction. It is
44014400
approximately equivalent to running the SQL
4402-
command <command>COMMIT</command>. After a transaction is committed, a new
4403-
transaction has to be started
4404-
using <function>SPI_start_transaction</function> before further database
4405-
actions can be executed.
4401+
command <command>COMMIT</command>. After the transaction is committed, a
4402+
new transaction is automatically started using default transaction
4403+
characteristics, so that the caller can continue using SPI facilities.
4404+
If there is a failure during commit, the current transaction is instead
4405+
rolled back and a new transaction is started, after which the error is
4406+
thrown in the usual way.
44064407
</para>
44074408

44084409
<para>
@@ -4439,10 +4440,9 @@ void SPI_rollback(void)
44394440
<para>
44404441
<function>SPI_rollback</function> rolls back the current transaction. It
44414442
is approximately equivalent to running the SQL
4442-
command <command>ROLLBACK</command>. After a transaction is rolled back, a
4443-
new transaction has to be started
4444-
using <function>SPI_start_transaction</function> before further database
4445-
actions can be executed.
4443+
command <command>ROLLBACK</command>. After the transaction is rolled back,
4444+
a new transaction is automatically started using default transaction
4445+
characteristics, so that the caller can continue using SPI facilities.
44464446
</para>
44474447

44484448
<para>
@@ -4464,7 +4464,7 @@ void SPI_rollback(void)
44644464

44654465
<refnamediv>
44664466
<refname>SPI_start_transaction</refname>
4467-
<refpurpose>start a new transaction</refpurpose>
4467+
<refpurpose>obsolete function</refpurpose>
44684468
</refnamediv>
44694469

44704470
<refsynopsisdiv>
@@ -4477,17 +4477,12 @@ void SPI_start_transaction(void)
44774477
<title>Description</title>
44784478

44794479
<para>
4480-
<function>SPI_start_transaction</function> starts a new transaction. It
4481-
can only be called after <function>SPI_commit</function>
4482-
or <function>SPI_rollback</function>, as there is no transaction active at
4483-
that point. Normally, when an SPI-using procedure is called, there is already a
4484-
transaction active, so attempting to start another one before closing out
4485-
the current one will result in an error.
4486-
</para>
4487-
4488-
<para>
4489-
This function can only be executed if the SPI connection has been set as
4490-
nonatomic in the call to <function>SPI_connect_ext</function>.
4480+
<function>SPI_start_transaction</function> does nothing, and exists
4481+
only for code compatibility with
4482+
earlier <productname>PostgreSQL</productname> releases. It used to
4483+
be required after calling <function>SPI_commit</function>
4484+
or <function>SPI_rollback</function>, but now those functions start
4485+
a new transaction automatically.
44914486
</para>
44924487
</refsect1>
44934488
</refentry>

‎src/backend/executor/spi.c

Lines changed: 150 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ SPI_connect_ext(int options)
153153
* XXX It could be better to use PortalContext as the parent context in
154154
* all cases, but we may not be inside a portal (consider deferred-trigger
155155
* execution). Perhaps CurTransactionContext could be an option? For now
156-
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
156+
* it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
157+
* but see also AtEOXact_SPI().
157158
*/
158159
_SPI_current->procCxt = AllocSetContextCreate(_SPI_current->atomic ? TopTransactionContext : PortalContext,
159160
"SPI Proc",
@@ -213,20 +214,26 @@ SPI_finish(void)
213214
return SPI_OK_FINISH;
214215
}
215216

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

225226
void
226227
SPI_commit(void)
227228
{
228229
MemoryContext oldcontext = CurrentMemoryContext;
229230

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

249-
/*
250-
* Hold any pinned portals that any PLs might be using. We have to do
251-
* this before changing transaction state, since this will run
252-
* user-defined code that might throw an error.
253-
*/
254-
HoldPinnedPortals();
257+
/* Catch any error occurring during the COMMIT */
258+
PG_TRY();
259+
{
260+
/* Protect current SPI stack entry against deletion */
261+
_SPI_current->internal_xact = true;
255262

256-
/* Start the actual commit */
257-
_SPI_current->internal_xact = true;
263+
/*
264+
* Hold any pinned portals that any PLs might be using. We have to do
265+
* this before changing transaction state, since this will run
266+
* user-defined code that might throw an error.
267+
*/
268+
HoldPinnedPortals();
258269

259-
/* Release snapshots associated with portals */
260-
ForgetPortalSnapshots();
270+
/* Release snapshots associated with portals */
271+
ForgetPortalSnapshots();
261272

262-
CommitTransactionCommand();
263-
MemoryContextSwitchTo(oldcontext);
273+
/* Do the deed */
274+
CommitTransactionCommand();
264275

265-
_SPI_current->internal_xact = false;
276+
/* Immediately start a new transaction */
277+
StartTransactionCommand();
278+
279+
MemoryContextSwitchTo(oldcontext);
280+
281+
_SPI_current->internal_xact = false;
282+
}
283+
PG_CATCH();
284+
{
285+
ErrorData *edata;
286+
287+
/* Save error info in caller's context */
288+
MemoryContextSwitchTo(oldcontext);
289+
edata = CopyErrorData();
290+
FlushErrorState();
291+
292+
/*
293+
* Abort the failed transaction. If this fails too, we'll just
294+
* propagate the error out ... there's not that much we can do.
295+
*/
296+
AbortCurrentTransaction();
297+
298+
/* ... and start a new one */
299+
StartTransactionCommand();
300+
301+
MemoryContextSwitchTo(oldcontext);
302+
303+
_SPI_current->internal_xact = false;
304+
305+
/* Now that we've cleaned up the transaction, re-throw the error */
306+
ReThrowError(edata);
307+
}
308+
PG_END_TRY();
266309
}
267310

268311
void
269312
SPI_rollback(void)
270313
{
271314
MemoryContext oldcontext = CurrentMemoryContext;
272315

316+
/* see under SPI_commit() */
273317
if (_SPI_current->atomic)
274318
ereport(ERROR,
275319
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -281,40 +325,60 @@ SPI_rollback(void)
281325
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
282326
errmsg("cannot roll back while a subtransaction is active")));
283327

284-
/*
285-
* Hold any pinned portals that any PLs might be using. We have to do
286-
* this before changing transaction state, since this will run
287-
* user-defined code that might throw an error, and in any case couldn't
288-
* be run in an already-aborted transaction.
289-
*/
290-
HoldPinnedPortals();
328+
/* Catch any error occurring during the ROLLBACK */
329+
PG_TRY();
330+
{
331+
/* Protect current SPI stack entry against deletion */
332+
_SPI_current->internal_xact = true;
291333

292-
/* Start the actual rollback */
293-
_SPI_current->internal_xact = true;
334+
/*
335+
* Hold any pinned portals that any PLs might be using. We have to do
336+
* this before changing transaction state, since this will run
337+
* user-defined code that might throw an error, and in any case
338+
* couldn't be run in an already-aborted transaction.
339+
*/
340+
HoldPinnedPortals();
294341

295-
/* Release snapshots associated with portals */
296-
ForgetPortalSnapshots();
342+
/* Release snapshots associated with portals */
343+
ForgetPortalSnapshots();
297344

298-
AbortCurrentTransaction();
299-
MemoryContextSwitchTo(oldcontext);
345+
/* Do the deed */
346+
AbortCurrentTransaction();
300347

301-
_SPI_current->internal_xact = false;
302-
}
348+
/* Immediately start a new transaction */
349+
StartTransactionCommand();
303350

304-
/*
305-
* Clean up SPI state. Called on transaction end (of non-SPI-internal
306-
* transactions) and when returning to the main loop on error.
307-
*/
308-
void
309-
SPICleanup(void)
310-
{
311-
_SPI_current = NULL;
312-
_SPI_connected = -1;
313-
/* Reset API global variables, too */
314-
SPI_processed = 0;
315-
SPI_lastoid = InvalidOid;
316-
SPI_tuptable = NULL;
317-
SPI_result = 0;
351+
MemoryContextSwitchTo(oldcontext);
352+
353+
_SPI_current->internal_xact = false;
354+
}
355+
PG_CATCH();
356+
{
357+
ErrorData *edata;
358+
359+
/* Save error info in caller's context */
360+
MemoryContextSwitchTo(oldcontext);
361+
edata = CopyErrorData();
362+
FlushErrorState();
363+
364+
/*
365+
* Try again to abort the failed transaction. If this fails too,
366+
* we'll just propagate the error out ... there's not that much we can
367+
* do.
368+
*/
369+
AbortCurrentTransaction();
370+
371+
/* ... and start a new one */
372+
StartTransactionCommand();
373+
374+
MemoryContextSwitchTo(oldcontext);
375+
376+
_SPI_current->internal_xact = false;
377+
378+
/* Now that we've cleaned up the transaction, re-throw the error */
379+
ReThrowError(edata);
380+
}
381+
PG_END_TRY();
318382
}
319383

320384
/*
@@ -323,17 +387,49 @@ SPICleanup(void)
323387
void
324388
AtEOXact_SPI(bool isCommit)
325389
{
326-
/* Do nothing if the transaction end was initiated by SPI. */
327-
if (_SPI_current && _SPI_current->internal_xact)
328-
return;
390+
bool found = false;
391+
392+
/*
393+
* Pop stack entries, stopping if we find one marked internal_xact (that
394+
* one belongs to the caller of SPI_commit or SPI_abort).
395+
*/
396+
while (_SPI_connected >= 0)
397+
{
398+
_SPI_connection *connection = &(_SPI_stack[_SPI_connected]);
399+
400+
if (connection->internal_xact)
401+
break;
329402

330-
if (isCommit && _SPI_connected != -1)
403+
found = true;
404+
405+
/*
406+
* We need not release the procedure's memory contexts explicitly, as
407+
* they'll go away automatically when their parent context does; see
408+
* notes in SPI_connect_ext.
409+
*/
410+
411+
/*
412+
* Restore outer global variables and pop the stack entry. Unlike
413+
* SPI_finish(), we don't risk switching to memory contexts that might
414+
* be already gone.
415+
*/
416+
SPI_processed = connection->outer_processed;
417+
SPI_tuptable = connection->outer_tuptable;
418+
SPI_result = connection->outer_result;
419+
420+
_SPI_connected--;
421+
if (_SPI_connected < 0)
422+
_SPI_current = NULL;
423+
else
424+
_SPI_current = &(_SPI_stack[_SPI_connected]);
425+
}
426+
427+
/* We should only find entries to pop during an ABORT. */
428+
if (found && isCommit)
331429
ereport(WARNING,
332430
(errcode(ERRCODE_WARNING),
333431
errmsg("transaction left non-empty SPI stack"),
334432
errhint("Check for missing \"SPI_finish\" calls.")));
335-
336-
SPICleanup();
337433
}
338434

339435
/*

‎src/backend/tcop/postgres.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
#include "catalog/pg_type.h"
4343
#include "commands/async.h"
4444
#include "commands/prepare.h"
45-
#include "executor/spi.h"
4645
#include "jit/jit.h"
4746
#include "libpq/libpq.h"
4847
#include "libpq/pqformat.h"
@@ -3993,7 +3992,6 @@ PostgresMain(int argc, char *argv[],
39933992
WalSndErrorCleanup();
39943993

39953994
PortalErrorCleanup();
3996-
SPICleanup();
39973995

39983996
/*
39993997
* We can't release replication slots inside AbortTransaction() as we

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1274,7 +1274,7 @@ HoldPinnedPortals(void)
12741274
*/
12751275
if (portal->strategy != PORTAL_ONE_SELECT)
12761276
ereport(ERROR,
1277-
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
1277+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
12781278
errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));
12791279

12801280
/* Verify it's in a suitable state to be held */

‎src/include/executor/spi.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,6 @@ extern void SPI_start_transaction(void);
163163
extern void SPI_commit(void);
164164
extern void SPI_rollback(void);
165165

166-
extern void SPICleanup(void);
167166
extern void AtEOXact_SPI(bool isCommit);
168167
extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid);
169168
extern bool SPI_inside_nonatomic_context(void);

0 commit comments

Comments
 (0)
0