Re: PL/TCL Patch to prevent postgres from becoming multithreaded

This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Marshall, Steve wrote:
> There is a problem in PL/TCL that can cause the postgres backend to
> become multithreaded. Postgres is not designed to be multithreaded, so
> this causes downstream errors in signal handling. We have seen this
> cause a number of "unexpected state" errors associated with notification
> handling; however, unpredictable signal handling would be likely to
> cause other errors as well.
>
> Some sample scripts are attached which will reproduce this problem when
> running against a multithreaded version of TCL, but will work without
> error with single-threaded TCL library. The scripts are a combination
> of Unix shell, perl DBI, and SQL commands. The postgres process can be
> seen to have multiple threads using the Linux command "ps -Lwfu
> postgres". In this command the NLWP columns will be 2 for multithreaded
> backend processes. The threaded/non-threaded state of the TCL library
> can be ascertained on Linux using ldd to determine if libpthread.so is
> linked to the TCL library (e.g. "ldd /usr/lib/libtcl8.4.so").
>
> The multithreaded behavior occurs the first time PL/TCL is used in a
> postgres backend, but only when postgres is linked against a
> multithread-enabled version of libtcl. Thus, this problem can be
> side-stepped by linking against the proper TCL library. However
> multithreaded TCL libraries are becoming the norm in Linux distributions
> and seems ubiquitous in the Windows world. Therefore a fix to the
> PL/TCL code is warrented.
>
> We determined that postgres became multithreaded during the creation of
> the TCL interpreter in a function called tcl_InitNotifier. This
> function is part of TCL's Notifier subsystem, which is used to monitor
> for events asynchronously from the TCL event loop. Although initialized
> when an interpreter is created, the Notifier subsystem is not used until
> a process enters the TCL event loop. This never happens within a
> postgres process, because postgres implements its own event loop.
> Therefore the initialization of the Notifier subsystem is not necessary
> within the context of PL/TCL.
>
> Our solution was to disable the Notifier subsystem by overriding the
> functions associated with it using the Tcl_SetNotifier function. This
> allows 8 functions related to the Notifier to overriden. Even though we
> found only two of the functions were ever called within postgres, we
> overrode 8 functions with no-op versions, just for completeness. A
> patch file containing the changes to pltcl.c from its 8.2.4 version is
> also attached.
>
> We tested this patch with PostgreSQL 8.2.4 on both RedHat Enterprise 4.0
> usingTCL 8.4 (single threaded) and RHE 5.0 using TCL 8.4.13
> (multithreaded). We expect this solution to work with Windows as well,
> although we have not tested it. There may be some problems using this
> solution with old versions of TCL that pre-date the Tcl_SetNotifier
> function. However this function has been around for quite a while; it
> was added in in the TCL 8.2 release, circa 2000.
>
> We hope this patch will be considered for a future PostgreSQL release.
>
> Steve Marshall
> Paul Bayer
> Doug Knight
> WSI Corporation
>
>

[ application/x-gzip is not supported, skipping... ]

> *** pltcl.c.orig 2007-09-10 12:58:34.000000000 -0400
> --- pltcl.c 2007-09-11 11:37:33.363222114 -0400
> ***************
> *** 163,168 ****
> --- 163,258 ----
> static void pltcl_build_tuple_argument(HeapTuple tuple, TupleDesc tupdesc,
> Tcl_DString *retval);
>
> + /**********************************************************************
> + * Declarations for functions overriden using Tcl_SetNotifier.
> + **********************************************************************/
> + static int fakeThreadKey; /* To give valid address for ClientData */
> +
> + static ClientData
> + pltcl_InitNotifier _ANSI_ARGS_((void));
> +
> + static void
> + pltcl_FinalizeNotifier _ANSI_ARGS_((ClientData clientData));
> +
> + static void
> + pltcl_SetTimer _ANSI_ARGS_((Tcl_Time *timePtr));
> +
> + static void
> + pltcl_AlertNotifier _ANSI_ARGS_((ClientData clientData));
> +
> + static void
> + pltcl_CreateFileHandler _ANSI_ARGS_((int fd, int mask, Tcl_FileProc *proc, ClientData clientData));
> +
> + static void
> + pltcl_DeleteFileHandler _ANSI_ARGS_((int fd));
> +
> + static void
> + pltcl_ServiceModeHook _ANSI_ARGS_((int mode));
> +
> + static int
> + pltcl_WaitForEvent _ANSI_ARGS_((Tcl_Time *timePtr));
> +
> + /**********************************************************************
> + * Definitions for functions overriden using Tcl_SetNotifier.
> + * These implementations effectively disable the TCL Notifier subsystem.
> + * This is okay because we never enter the TCL event loop from postgres,
> + * so the notifier capabilities are initialized, but never used.
> + *
> + * NOTE: Only InitNotifier and DeleteFileHandler ever seem to get called
> + * by postgres, but we implement all the functions for completeness.
> + **********************************************************************/
> +
> + ClientData
> + pltcl_InitNotifier()
> + {
> + return (ClientData) &(fakeThreadKey);
> + }
> +
> + void
> + pltcl_FinalizeNotifier(clientData)
> + ClientData clientData; /* Not used. */
> + {
> + }
> +
> + void
> + pltcl_SetTimer(timePtr)
> + Tcl_Time *timePtr;
> + {
> + }
> +
> + void
> + pltcl_AlertNotifier(clientData)
> + ClientData clientData;
> + {
> + }
> +
> + void
> + pltcl_CreateFileHandler(fd, mask, proc, clientData)
> + int fd;
> + int mask;
> + Tcl_FileProc *proc;
> + ClientData clientData;
> + {
> + }
> +
> + void
> + pltcl_DeleteFileHandler(fd)
> + int fd;
> + {
> + }
> +
> + void
> + pltcl_ServiceModeHook(mode)
> + int mode;
> + {
> + }
> +
> + int
> + pltcl_WaitForEvent(timePtr)
> + Tcl_Time *timePtr; /* Maximum block time, or NULL. */
> + {
> + return 0;
> + }
>
> /*
> * This routine is a crock, and so is everyplace that calls it. The problem
> ***************
> *** 189,194 ****
> --- 279,287 ----
> void
> _PG_init(void)
> {
> + /* Notifier structure used to override functions in Notifier subsystem*/
> + Tcl_NotifierProcs notifier;
> +
> /* Be sure we do initialization only once (should be redundant now) */
> if (pltcl_pm_init_done)
> return;
> ***************
> *** 199,204 ****
> --- 292,316 ----
> #endif
>
> /************************************************************
> + * Override the functions in the Notifier subsystem.
> + *
> + * We do this to prevent the postgres backend from becoming
> + * multithreaded, which happens in the default version of
> + * Tcl_InitNotifier if the TCL library has been compiled with
> + * multithreading support (i.e. when TCL_THREADS is defined
> + * under Unix, and in all cases under Windows).
> + ************************************************************/
> + notifier.setTimerProc = pltcl_SetTimer;
> + notifier.waitForEventProc = pltcl_WaitForEvent;
> + notifier.createFileHandlerProc = pltcl_CreateFileHandler;
> + notifier.deleteFileHandlerProc = pltcl_DeleteFileHandler;
> + notifier.initNotifierProc = pltcl_InitNotifier;
> + notifier.finalizeNotifierProc = pltcl_FinalizeNotifier;
> + notifier.alertNotifierProc = pltcl_AlertNotifier;
> + notifier.serviceModeHookProc = pltcl_ServiceModeHook;
> + Tcl_SetNotifier(&notifier);
> +
> + /************************************************************
> * Create the dummy hold interpreter to prevent close of
> * stdout and stderr on DeleteInterp
> ************************************************************/

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +