Re: Making plpython 2 and 3 coexist a bit better - Mailing list pgsql-hackers

I wrote:
> I think we might be able to improve this if we don't do the check in
> plpython's _PG_init(), but delay it until we have a need to call into
> libpython; which we could skip doing at _PG_init() time, at the cost
> of perhaps one more flag check per plpython function call to see if
> we've initialized libpython yet.

> The behavior would then be that if you load both language .so's
> into one session, neither one would be willing to do anything
> anymore --- not run a function, and not syntax-check one either,
> because any attempted call into either libpython might crash.
> But restoring a dump has no need to do either of those things;
> it just wants to be able to LOAD the .so.  Also, the error for
> not being able to do something wouldn't have to be FATAL.

Attached is a draft patch along this line.  It seems to work as intended.

I can think of at least a couple of ways in which this test might be
inadequate.  One is that a plpython2 function might call a plpython3
function or vice versa.  The inner plpython_call_handler would correctly
throw an error, but if the outer function trapped the error and continued
execution, it would be at risk.  Another, more hypothetical risk is that
once we've executed anything out of one libpython, it might have changed
process state (eg, installed hooks) in such a way that it can get control
back even without an explicit call to plpython.  In that case, a
subsequent load of another Python version would put things at risk for
such execution in the first libpython.

We could ameliorate the first of these cases by putting the can't-run-
with-two-pythons error back up to FATAL rather than ERROR, but I'm not
sure that that would be a net improvement --- FATAL errors aren't very
friendly.  In any case errors of the second type seem unpreventable
unless we stick with the immediate-FATAL-error approach.

Since plpython only comes in an untrusted (hence superuser-only) flavor,
holes like this wouldn't be security issues but only "well, you shouldn't
do that" problems.  I'm inclined to think it's a good tradeoff for being
able to dump/restore/upgrade mixed installations, which are surely
likely to get more popular.

Thoughts?

            regards, tom lane

diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c
index 3c2ebfa..059adfa 100644
*** a/src/pl/plpython/plpy_main.c
--- b/src/pl/plpython/plpy_main.c
*************** static void PLy_init_interp(void);
*** 63,69 ****
  static PLyExecutionContext *PLy_push_execution_context(void);
  static void PLy_pop_execution_context(void);

! static const int plpython_python_version = PY_MAJOR_VERSION;

  /* initialize global variables */
  PyObject   *PLy_interp_globals = NULL;
--- 63,70 ----
  static PLyExecutionContext *PLy_push_execution_context(void);
  static void PLy_pop_execution_context(void);

! static int *plpython_version_bitmask_ptr = NULL;
! static int    plpython_version_bitmask = 0;

  /* initialize global variables */
  PyObject   *PLy_interp_globals = NULL;
*************** static PLyExecutionContext *PLy_executio
*** 75,102 ****
  void
  _PG_init(void)
  {
!     /* Be sure we do initialization only once (should be redundant now) */
!     static bool inited = false;
      const int **version_ptr;

!     if (inited)
!         return;

!     /* Be sure we don't run Python 2 and 3 in the same session (might crash) */
      version_ptr = (const int **) find_rendezvous_variable("plpython_python_version");
!     if (!(*version_ptr))
!         *version_ptr = &plpython_python_version;
!     else
!     {
!         if (**version_ptr != plpython_python_version)
!             ereport(FATAL,
!                     (errmsg("Python major version mismatch in session"),
!                      errdetail("This session has previously used Python major version %d, and it is now attempting to
usePython major version %d.", 
!                                **version_ptr, plpython_python_version),
!                      errhint("Start a new session to use a different Python major version.")));
!     }

!     pg_bindtextdomain(TEXTDOMAIN);

  #if PY_MAJOR_VERSION >= 3
      PyImport_AppendInittab("plpy", PyInit_plpy);
--- 76,144 ----
  void
  _PG_init(void)
  {
!     int          **bitmask_ptr;
      const int **version_ptr;

!     /*
!      * Set up a shared bitmask variable telling which Python version(s) are
!      * loaded into this process's address space.  If there's more than one, we
!      * cannot call into libpython for fear of causing crashes.  But postpone
!      * the actual failure for later, so that operations like pg_restore can
!      * load more than one plpython library so long as they don't try to do
!      * anything much with the language.
!      */
!     bitmask_ptr = (int **) find_rendezvous_variable("plpython_version_bitmask");
!     if (!(*bitmask_ptr))        /* am I the first? */
!         *bitmask_ptr = &plpython_version_bitmask;
!     /* Retain pointer to the agreed-on shared variable ... */
!     plpython_version_bitmask_ptr = *bitmask_ptr;
!     /* ... and announce my presence */
!     *plpython_version_bitmask_ptr |= (1 << PY_MAJOR_VERSION);

!     /*
!      * This should be safe even in the presence of conflicting plpythons, and
!      * it's necessary to do it here for the next error to be localized.
!      */
!     pg_bindtextdomain(TEXTDOMAIN);
!
!     /*
!      * We used to have a scheme whereby PL/Python would fail immediately if
!      * loaded into a session in which a conflicting libpython is already
!      * present.  We don't like to do that anymore, but it seems possible that
!      * a plpython library adhering to the old convention is present in the
!      * session, in which case we have to fail.
!      */
      version_ptr = (const int **) find_rendezvous_variable("plpython_python_version");
!     if ((*version_ptr) != NULL)
!         ereport(FATAL,
!                 (errmsg("Python major version mismatch in session"),
!                  errdetail("This session has previously used Python major version %d, and it is now attempting to use
Pythonmajor version %d.", 
!                            **version_ptr, PY_MAJOR_VERSION),
!                  errhint("Start a new session to use a different Python major version.")));
! }

! /*
!  * Perform one-time setup of PL/Python, after checking for a conflict
!  * with other versions of Python.
!  */
! static void
! PLy_initialize(void)
! {
!     static bool inited = false;
!
!     /*
!      * Check for multiple Python libraries before actively doing anything with
!      * libpython.  This must be repeated on each entry to PL/Python, in case a
!      * conflicting library got loaded since we last looked.
!      */
!     if (*plpython_version_bitmask_ptr != (1 << PY_MAJOR_VERSION))
!         ereport(ERROR,
!                 (errmsg("multiple Python libraries are present in session"),
!                  errdetail("Only one Python major version can be used in one session.")));
!
!     /* The rest should only be done once per session */
!     if (inited)
!         return;

  #if PY_MAJOR_VERSION >= 3
      PyImport_AppendInittab("plpy", PyInit_plpy);
*************** _PG_init(void)
*** 120,126 ****
  }

  /*
!  * This should only be called once from _PG_init. Initialize the Python
   * interpreter and global data.
   */
  static void
--- 162,168 ----
  }

  /*
!  * This should be called only once, from PLy_initialize. Initialize the Python
   * interpreter and global data.
   */
  static void
*************** plpython_validator(PG_FUNCTION_ARGS)
*** 155,163 ****
          PG_RETURN_VOID();

      if (!check_function_bodies)
-     {
          PG_RETURN_VOID();
!     }

      /* Get the new function's pg_proc entry */
      tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcoid));
--- 197,206 ----
          PG_RETURN_VOID();

      if (!check_function_bodies)
          PG_RETURN_VOID();
!
!     /* Do this only after making sure we need to do something */
!     PLy_initialize();

      /* Get the new function's pg_proc entry */
      tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcoid));
*************** plpython_call_handler(PG_FUNCTION_ARGS)
*** 191,196 ****
--- 234,241 ----
      PLyExecutionContext *exec_ctx;
      ErrorContextCallback plerrcontext;

+     PLy_initialize();
+
      /* Note: SPI_finish() happens in plpy_exec.c, which is dubious design */
      if (SPI_connect() != SPI_OK_CONNECT)
          elog(ERROR, "SPI_connect failed");
*************** plpython_inline_handler(PG_FUNCTION_ARGS
*** 266,271 ****
--- 311,318 ----
      PLyExecutionContext *exec_ctx;
      ErrorContextCallback plerrcontext;

+     PLy_initialize();
+
      /* Note: SPI_finish() happens in plpy_exec.c, which is dubious design */
      if (SPI_connect() != SPI_OK_CONNECT)
          elog(ERROR, "SPI_connect failed");