Re: protect dll lib initialisation against any exception, for 8.5 - Mailing list pgsql-hackers

2009/4/2 Tom Lane <tgl@sss.pgh.pa.us>:
> 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.
>

I see it. I thing , an safety of this exception should be solved only
by programmer. It's important to release all hooks, and then raise an
exception. It is in developer responsibility.

regards
Pavel Stehule

>                        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)
>
>