Re: protect dll lib initialisation against any exception, for 8.5

Pavel Stehule <pavel.stehule@gmail.com> writes:
> attached patch allows raising exception from _PG_init function as was
> discussed before.

I fooled around with this and came up with the attached improved
version, which allows reporting the full error status.  However,
after thinking some more I feel that this is probably a cure worse
than the disease.  If we simply leave the code as it stands, an
elog(ERROR) in an init function doesn't corrupt dfmgr.c's internal list,
which is what I had been fearing when I complained about the issue.
The worst that happens is that we leave the library loaded and leak
a little bit of memory.  Unloading the library, as the patch does,
could easily make things worse not better.  Consider the not-unlikely
case that the library installs itself in a few callback hooks and
then fails.  If we unload the library, those hooks represent
*guaranteed* core dumps on next use.  If we don't unload, the hook
functions might or might not work too well --- presumably not everything
they need has been initialized --- but it's hard to imagine an outcome
that's worse than a guaranteed core dump.

So I'm thinking this is really unnecessary and we should leave well
enough alone.

            regards, tom lane

Index: dfmgr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/fmgr/dfmgr.c,v
retrieving revision 1.98
diff -c -r1.98 dfmgr.c
*** dfmgr.c    1 Jan 2009 17:23:51 -0000    1.98
--- dfmgr.c    1 Apr 2009 23:41:37 -0000
***************
*** 178,184 ****
  static void *
  internal_load_library(const char *libname)
  {
!     DynamicFileList *file_scanner;
      PGModuleMagicFunction magic_func;
      char       *load_error;
      struct stat stat_buf;
--- 178,184 ----
  static void *
  internal_load_library(const char *libname)
  {
!     DynamicFileList * volatile file_scanner;
      PGModuleMagicFunction magic_func;
      char       *load_error;
      struct stat stat_buf;
***************
*** 277,287 ****
          }

          /*
!          * If the library has a _PG_init() function, call it.
           */
          PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
          if (PG_init)
!             (*PG_init) ();

          /* OK to link it into list */
          if (file_list == NULL)
--- 277,329 ----
          }

          /*
!          * If the library has a _PG_init() function, call it.  Guard against
!          * the function possibly throwing elog(ERROR).
           */
          PG_init = (PG_init_t) pg_dlsym(file_scanner->handle, "_PG_init");
          if (PG_init)
!         {
!             MemoryContext    oldcontext = CurrentMemoryContext;
!
!             PG_TRY();
!             {
!                 (*PG_init) ();
!             }
!             PG_CATCH();
!             {
!                 ErrorData  *edata;
!
!                 /* fetch the error status so we can change it */
!                 MemoryContextSwitchTo(oldcontext);
!                 edata = CopyErrorData();
!                 FlushErrorState();
!
!                 /*
!                  * The const pointers in the error status very likely point
!                  * at constant strings in the library, which we are about to
!                  * unload.  Copy them so we don't dump core while reporting
!                  * the error.  This might leak a bit of memory but it
!                  * beats the alternatives.
!                  */
!                 if (edata->filename)
!                     edata->filename = pstrdup(edata->filename);
!                 if (edata->funcname)
!                     edata->funcname = pstrdup(edata->funcname);
!                 if (edata->domain)
!                     edata->domain = pstrdup(edata->domain);
!
!                 /* library might have already called some of its functions */
!                 clear_external_function_hash(file_scanner->handle);
!
!                 /* try to unlink library */
!                 pg_dlclose(file_scanner->handle);
!                 free((char *) file_scanner);
!
!                 /* complain */
!                 ReThrowError(edata);
!             }
!             PG_END_TRY();
!         }

          /* OK to link it into list */
          if (file_list == NULL)