public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: fix crash with Python 3.11
Date: Sun, 16 Jan 2022 17:53:44 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

Peter Eisentraut <[email protected]> writes:
> The way plpy.commit() and plpy.rollback() handle errors is not ideal. 
> They end up just longjmping back to the main loop, without telling the 
> Python interpreter about it.  This hasn't been a problem so far, 
> apparently, but with Python 3.11-to-be, this appears to cause corruption 
> in the state of the Python interpreter.  This is readily reproducible 
> and will cause crashes in the plpython_transaction test.
> The fix is that we need to catch the PostgreSQL error and turn it into a 
> Python exception, like we do for other places where plpy.* methods call 
> into PostgreSQL internals.

I agree that the existing code is broken and works only accidentally,
but I fear this patch does little to improve matters.  In particular,
it returns control to Python without having done anything to clean
up the now-invalid state of the transaction system.  (That is,
there's nothing analogous to PLy_spi_subtransaction_abort's
RollbackAndReleaseCurrentSubTransaction call in these new PG_CATCH
blocks).  The existing test cases apparently fail to trip over that
because Python just throws the exception again, but what if someone
writes a plpython function that catches the exception and then tries
to perform new SPI actions?

I think a possible fix is:

1. Before entering the PG_TRY block, check for active subtransaction(s)
and immediately throw a Python error if there is one.  (This corresponds
to the existing errors "cannot commit while a subtransaction is active"
and "cannot roll back while a subtransaction is active".  The point is
to reduce the number of system states we have to worry about below.)

2. In the PG_CATCH block, after collecting the error data do
	AbortOutOfAnyTransaction();
	StartTransactionCommand();
which gets us into a good state with no active subtransactions.

I'm not sure that those two are the best choices of xact.c
entry points, but there's precedent for that in autovacuum.c
among other places.  (autovacuum seems to think that blocking
interrupts is a good idea too.)

			regards, tom lane

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: fix crash with Python 3.11
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox