Thread: [HACKERS] I propose killing PL/Tcl's "modules" infrastructure
Over in https://www.postgresql.org/message-id/alpine.DEB.2.11.1702251701030.3920@Sandal.Woodpecker it's pointed out that pltcl_loadmod was never updated for the switch to standard_conforming_strings (and the patch proposed there doesn't begin to cover all the places that would need fixed for that). This means that the "modules" functionality is entirely broken in any installation that's got standard_conforming_strings turned on, which has been the default since 9.1 and was possible long before that. The fact that nobody has noticed seems to me to be clear proof that no one is using this feature in the field. Now, we could try to fix this bug, and add the regression test coverage that the code clearly lacks, and upgrade the documentation about it from its currently very sad state. But I think the right answer is just to remove the feature altogether. It's evidently not being used, and it's kind of insecure by design, and it would not be that hard for someone to provide equivalent functionality entirely in userland if they really wanted it. Comments? regards, tom lane
On 02/25/2017 01:14 PM, Tom Lane wrote: > Over in > https://www.postgresql.org/message-id/alpine.DEB.2.11.1702251701030.3920@Sandal.Woodpecker > it's pointed out that pltcl_loadmod was never updated for the switch > to standard_conforming_strings (and the patch proposed there doesn't > begin to cover all the places that would need fixed for that). > > This means that the "modules" functionality is entirely broken in any > installation that's got standard_conforming_strings turned on, which has > been the default since 9.1 and was possible long before that. > > The fact that nobody has noticed seems to me to be clear proof that no one > is using this feature in the field. > > Now, we could try to fix this bug, and add the regression test coverage > that the code clearly lacks, and upgrade the documentation about it from > its currently very sad state. But I think the right answer is just to > remove the feature altogether. It's evidently not being used, and it's > kind of insecure by design, and it would not be that hard for someone > to provide equivalent functionality entirely in userland if they really > wanted it. > > Comments? > > In PLv8 we added a parameter plv8.start_proc that names a parameterless function that's executed when plv8 is first called in each session. It can be used quite easily to implement something like a modules infrastructure - in fact I have used it to good effect for exactly that. Maybe something similar for pltcl would be a good thing. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 02/25/2017 10:14 AM, Tom Lane wrote: > Now, we could try to fix this bug, and add the regression test coverage > that the code clearly lacks, and upgrade the documentation about it from > its currently very sad state. But I think the right answer is just to > remove the feature altogether. It's evidently not being used, and it's > kind of insecure by design, and it would not be that hard for someone > to provide equivalent functionality entirely in userland if they really > wanted it. I don't see a reason to keep pl/tcl in core at all so ripping out the functionality seems the least disruptive and perhaps even a deprecation of the PL (at least from a core perspective) in v10. JD -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. Unless otherwise stated, opinions are my own.
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 02/25/2017 01:14 PM, Tom Lane wrote:
>> Now, we could try to fix this bug, and add the regression test coverage
>> that the code clearly lacks, and upgrade the documentation about it from
>> its currently very sad state. But I think the right answer is just to
>> remove the feature altogether. It's evidently not being used, and it's
>> kind of insecure by design, and it would not be that hard for someone
>> to provide equivalent functionality entirely in userland if they really
>> wanted it.
> In PLv8 we added a parameter plv8.start_proc that names a parameterless
> function that's executed when plv8 is first called in each session. It
> can be used quite easily to implement something like a modules
> infrastructure - in fact I have used it to good effect for exactly that.
> Maybe something similar for pltcl would be a good thing.
Yeah, the only part that's even a bit hard to replicate in userland is
initializing the autoloading mechanism in each session. It would be
cleaner to provide a feature similar to what you describe that could
be used for that purpose as well as others. However, where does the
"parameterless function" come from? Is it a regular PLv8 (or for this
purpose PL/Tcl) function expected to be present in pg_proc?
regards, tom lane
"Joshua D. Drake" <jd@commandprompt.com> writes:
> I don't see a reason to keep pl/tcl in core at all so ripping out the
> functionality seems the least disruptive and perhaps even a deprecation
> of the PL (at least from a core perspective) in v10.
I'm not in any hurry to remove or deprecate PL/Tcl as a whole. It's
gotten more love in the past year than it got in the previous ten,
so evidently there are still people out there who use it. But they
don't seem to be using this particular part of it.
regards, tom lane
On 02/25/2017 01:44 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 02/25/2017 01:14 PM, Tom Lane wrote:
>>> Now, we could try to fix this bug, and add the regression test coverage
>>> that the code clearly lacks, and upgrade the documentation about it from
>>> its currently very sad state. But I think the right answer is just to
>>> remove the feature altogether. It's evidently not being used, and it's
>>> kind of insecure by design, and it would not be that hard for someone
>>> to provide equivalent functionality entirely in userland if they really
>>> wanted it.
>> In PLv8 we added a parameter plv8.start_proc that names a parameterless
>> function that's executed when plv8 is first called in each session. It
>> can be used quite easily to implement something like a modules
>> infrastructure - in fact I have used it to good effect for exactly that.
>> Maybe something similar for pltcl would be a good thing.
> Yeah, the only part that's even a bit hard to replicate in userland is
> initializing the autoloading mechanism in each session. It would be
> cleaner to provide a feature similar to what you describe that could
> be used for that purpose as well as others. However, where does the
> "parameterless function" come from? Is it a regular PLv8 (or for this
> purpose PL/Tcl) function expected to be present in pg_proc?
>
>
Yes, it's a regular PLv8 function.Here's an example. It presupposes that
there is a table called plv8_modules (modname text, code text,
load_on_start boolean).
CREATE OR REPLACE FUNCTION public.plv8_startup() RETURNS void LANGUAGE plv8 AS $function$ if (typeof
plv8_loaded_modules== 'undefined') plv8_loaded_modules = {}; load_module = function(modname) { if
(plv8_loaded_modules[modname]) return; var rows = plv8.execute("SELECT code from plv8_modules " +
" where modname = $1", [modname]); for (var r = 0; r < rows.length; r++) {
varcode = rows[r].code; eval("(function() { " + code + "})")(); // plv8.elog(NOTICE,"loaded module
"+ modname); plv8_loaded_modules[modname] = 1; } }; reload_module = function(modname)
{ var rows = plv8.execute("SELECT code from plv8_modules " + " where modname =
$1",[modname]); for (var r = 0; r < rows.length; r++) { var code = rows[r].code;
eval("(function(){ " + code + "})")(); // plv8.elog(NOTICE,"loaded module " + modname);
plv8_loaded_modules[modname]= 1; }
};
var rows = plv8.execute("SELECT modname, code from plv8_modules where load_on_start"); for (var r = 0; r <
rows.length;r++) { var modname = rows[r].modname; if (plv8_loaded_modules[modname]) continue;
var code = rows[r].code; eval("(function() { " + code + "})")(); plv8_loaded_modules[modname] = 1;
// plv8.elog(NOTICE,"loaded module " + modname); }; $function$;
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 02/25/2017 01:44 PM, Tom Lane wrote:
>> Yeah, the only part that's even a bit hard to replicate in userland is
>> initializing the autoloading mechanism in each session. It would be
>> cleaner to provide a feature similar to what you describe that could
>> be used for that purpose as well as others. However, where does the
>> "parameterless function" come from? Is it a regular PLv8 (or for this
>> purpose PL/Tcl) function expected to be present in pg_proc?
> Yes, it's a regular PLv8 function.
OK ... how do you handle security considerations? Can the GUC be set
at any time/by anybody? What determines whether you have permissions
to call the particular function?
regards, tom lane
On 02/25/2017 02:21 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 02/25/2017 01:44 PM, Tom Lane wrote: >>> Yeah, the only part that's even a bit hard to replicate in userland is >>> initializing the autoloading mechanism in each session. It would be >>> cleaner to provide a feature similar to what you describe that could >>> be used for that purpose as well as others. However, where does the >>> "parameterless function" come from? Is it a regular PLv8 (or for this >>> purpose PL/Tcl) function expected to be present in pg_proc? >> Yes, it's a regular PLv8 function. > OK ... how do you handle security considerations? Can the GUC be set > at any time/by anybody? What determines whether you have permissions > to call the particular function? > > It can be set by anyone, IIRC. Maybe it should be SUSET only, I don't know. It's executed as the session owner. Execute permission on the function are determined the same way as for any function. It's an ordinary function call. The only difference is in how the call gets triggered. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I wrote:
> Now, we could try to fix this bug, and add the regression test coverage
> that the code clearly lacks, and upgrade the documentation about it from
> its currently very sad state. But I think the right answer is just to
> remove the feature altogether.
BTW, I tried to poke into what it would take to write some regression test
coverage, and immediately hit a show-stopper:
$ pltcl_loadmod --help
can't find package Pgtcl while executing
"package require Pgtcl" (file "/home/postgres/testversion/bin/pltcl_loadmod" line 10)
That is, these scripts depend on the old Tcl client library, which
we removed from our core distro in 2004 (cf commit 41fa9e9ba).
So we don't even have a way of creating self-contained tests for them.
At this point I think there's no question that src/pl/tcl/modules/
needs to go away. There might be some argument for retaining the
"autoload the unknown module" startup behavior in pltcl proper, but
I think that Andrew Dunstan's proposal of calling an initialization
function is a far cleaner solution.
regards, tom lane
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> [ we should borrow plv8's start_proc idea for pltcl ]
So after thinking about this for awhile, I propose the following
concrete spec for replacing pltcl's autoload-unknown behavior:
* Invent two GUCs, pltcl.start_proc and pltclu.start_proc, which default
to empty strings but can be set to the name (possibly schema-qualified)
of a parameterless function that must be of the corresponding language.
When so set, the specified function is called once just after creation of
any pltcl or pltclu interpreter.
* The function is called as the current SQL user, who must have
permissions to call it. (This decision is more or less forced by
the fact that pltcl interpreters are per-userid; we want whatever
initialization gets done to be done in the new interpreter, and it
would be very weird and probably a security hole if we weren't
running as the same SQL userid that owns the interpreter.)
* Pre-call error conditions (no such function, wrong language, or no
permissions) result in a WARNING but the original operation continues.
(Making these WARNING not ERROR is possibly debatable, but it looks
like that's what plv8 does.)
* If the function itself throws an error, we do a transaction abort,
but the Tcl interpreter remains in existence and is deemed usable
for later operations. So a failing start_proc can't lock you out
of pltcl operations altogether.
* I'm not terribly comfortable about what the permissions levels of the
GUCs ought to be. The call permissions check means that you can't use
either GUC to call a function you couldn't have called anyway. However
there's a separate risk of trojan-horse execution, analogous to what a
blackhat can get by controlling the search_path GUC setting used by a
SECURITY DEFINER function: the function might intend to invoke some pltcl
function, but you can get it to invoke some other pltcl function in
addition to that. I think this means we had better make pltclu.start_proc
be SUSET, but from a convenience standpoint it'd be nice if
pltcl.start_proc were just USERSET. An argument in favor of that is that
we don't restrict search_path which is just as dangerous; but on the other
hand, existing code should be expected to know that it needs to beware of
search_path, while it wouldn't know that start_proc needs to be locked
down. Maybe we'd better make them both SUSET.
Comments?
regards, tom lane
On Mon, Feb 27, 2017 at 1:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > * I'm not terribly comfortable about what the permissions levels of the > GUCs ought to be. The call permissions check means that you can't use > either GUC to call a function you couldn't have called anyway. However > there's a separate risk of trojan-horse execution, analogous to what a > blackhat can get by controlling the search_path GUC setting used by a > SECURITY DEFINER function: the function might intend to invoke some pltcl > function, but you can get it to invoke some other pltcl function in > addition to that. I think this means we had better make pltclu.start_proc > be SUSET, but from a convenience standpoint it'd be nice if > pltcl.start_proc were just USERSET. An argument in favor of that is that > we don't restrict search_path which is just as dangerous; but on the other > hand, existing code should be expected to know that it needs to beware of > search_path, while it wouldn't know that start_proc needs to be locked > down. Maybe we'd better make them both SUSET. Making them SUSET sounds like a usability fail to me. I'm not sure how bad the security risks of NOT making them SUSET are, but I think if we find that SUSET is required for safety then we've squeezed most of the value out of the feature. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 27, 2017 at 1:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I'm not terribly comfortable about what the permissions levels of the
>> GUCs ought to be. ... Maybe we'd better make them both SUSET.
> Making them SUSET sounds like a usability fail to me. I'm not sure
> how bad the security risks of NOT making them SUSET are, but I think
> if we find that SUSET is required for safety then we've squeezed most
> of the value out of the feature.
Well, the feature it's replacing (autoload an "unknown" module) had to be
squeezed down to being effectively superuser-only, so we're not really
losing anything compared to where we are now. And the more I think about
it, the less I think we can introduce a new security-critical GUC and just
leave it as USERSET.
regards, tom lane
On 02/26/2017 02:54 PM, Tom Lane wrote: > * I'm not terribly comfortable about what the permissions levels of the > GUCs ought to be. The call permissions check means that you can't use > either GUC to call a function you couldn't have called anyway. However > there's a separate risk of trojan-horse execution, analogous to what a > blackhat can get by controlling the search_path GUC setting used by a > SECURITY DEFINER function: the function might intend to invoke some pltcl > function, but you can get it to invoke some other pltcl function in > addition to that. I think this means we had better make pltclu.start_proc > be SUSET, but from a convenience standpoint it'd be nice if > pltcl.start_proc were just USERSET. An argument in favor of that is that > we don't restrict search_path which is just as dangerous; but on the other > hand, existing code should be expected to know that it needs to beware of > search_path, while it wouldn't know that start_proc needs to be locked > down. Maybe we'd better make them both SUSET. > plperl's on_plperl_init and on_plperlu_init settings are both SUSET. In practice with PLv8 this is usually set in the config file in my experience. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 02/26/2017 02:54 PM, Tom Lane wrote:
>> * I'm not terribly comfortable about what the permissions levels of the
>> GUCs ought to be.
> plperl's on_plperl_init and on_plperlu_init settings are both SUSET.
> In practice with PLv8 this is usually set in the config file in my
> experience.
Ah, I'd forgotten about that precedent. Being consistent with that seems
like a good thing --- and I agree with your point that this would likely
usually be set in postgresql.conf anyway, making the issue rather moot.
I noticed also that the precedent of plperl is that if the init code
fails, we give up on use of that interpreter, and try again to run
the init code if plperl is used again. This is different from what
I had in my draft spec but it probably is a better definition; without
it, people might find themselves running in Tcl interpreters that do
not behave as intended.
In sum, then, PFA a patch that adds these GUCs. They're still function
names but otherwise the details are closer to what plperl does.
regards, tom lane
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index 0a69380..ad216dd 100644
*** a/doc/src/sgml/pltcl.sgml
--- b/doc/src/sgml/pltcl.sgml
*************** if {[catch { spi_exec $sql_command }]} {
*** 902,907 ****
--- 902,981 ----
</para>
</sect1>
+ <sect1 id="pltcl-config">
+ <title>PL/Tcl Configuration</title>
+
+ <para>
+ This section lists configuration parameters that
+ affect <application>PL/Tcl</>.
+ </para>
+
+ <variablelist>
+
+ <varlistentry id="guc-pltcl-start-proc" xreflabel="pltcl.start_proc">
+ <term>
+ <varname>pltcl.start_proc</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>pltcl.start_proc</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter, if set to a nonempty string, specifies the name
+ (possibly schema-qualified) of a parameterless PL/Tcl function that
+ is to be executed whenever a new Tcl interpreter is created for
+ PL/Tcl. Such a function can perform per-session initialization, such
+ as loading additional Tcl code. A new Tcl interpreter is created
+ when a PL/Tcl function is first executed in a database session, or
+ when an additional interpreter has to be created because a PL/Tcl
+ function is called by a new SQL role.
+ </para>
+
+ <para>
+ The referenced function must be written in the <literal>pltcl</>
+ language, and must not be marked <literal>SECURITY DEFINER</>.
+ (These restrictions ensure that it runs in the interpreter it's
+ supposed to initialize.) The current user must have permission to
+ call it, too.
+ </para>
+
+ <para>
+ If the function fails with an error it will abort the function call
+ that caused the new interpreter to be created and propagate out to
+ the calling query, causing the current transaction or subtransaction
+ to be aborted. Any actions already done within Tcl won't be undone;
+ however, that interpreter won't be used again. If the language is
+ used again the initialization will be attempted again within a fresh
+ Tcl interpreter.
+ </para>
+
+ <para>
+ Only superusers can change this setting. Although this setting
+ can be changed within a session, such changes will not affect Tcl
+ interpreters that have already been created.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="guc-pltclu-start-proc" xreflabel="pltclu.start_proc">
+ <term>
+ <varname>pltclu.start_proc</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>pltclu.start_proc</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter is exactly like <varname>pltcl.start_proc</varname>,
+ except that it applies to PL/TclU. The referenced function must
+ be written in the <literal>pltclu</> language.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ </variablelist>
+ </sect1>
+
<sect1 id="pltcl-procnames">
<title>Tcl Procedure Names</title>
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 453e7ad..1096c4f 100644
*** a/src/pl/tcl/Makefile
--- b/src/pl/tcl/Makefile
*************** DATA = pltcl.control pltcl--1.0.sql pltc
*** 28,34 ****
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
! REGRESS = pltcl_setup pltcl_queries pltcl_unicode
# Tcl on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
--- 28,34 ----
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
! REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode
# Tcl on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
diff --git a/src/pl/tcl/expected/pltcl_start_proc.out b/src/pl/tcl/expected/pltcl_start_proc.out
index ...0b8afb7 .
*** a/src/pl/tcl/expected/pltcl_start_proc.out
--- b/src/pl/tcl/expected/pltcl_start_proc.out
***************
*** 0 ****
--- 1,28 ----
+ --
+ -- Test start_proc execution
+ --
+ SET pltcl.start_proc = 'no_such_function';
+ select tcl_int4add(1, 2);
+ ERROR: function no_such_function() does not exist
+ select tcl_int4add(1, 2);
+ ERROR: function no_such_function() does not exist
+ create function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl SECURITY DEFINER;
+ SET pltcl.start_proc = 'public.tcl_initialize';
+ select tcl_int4add(1, 2); -- fail
+ ERROR: function "public.tcl_initialize" must not be SECURITY DEFINER to use in pltcl.start_proc
+ create or replace function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl;
+ select tcl_int4add(1, 2);
+ NOTICE: in tcl_initialize
+ tcl_int4add
+ -------------
+ 3
+ (1 row)
+
+ select tcl_int4add(1, 2);
+ tcl_int4add
+ -------------
+ 3
+ (1 row)
+
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 11faa6d..a390b6e 100644
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
***************
*** 15,20 ****
--- 15,21 ----
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/objectaccess.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
#include "commands/event_trigger.h"
***************
*** 25,35 ****
--- 26,39 ----
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
+ #include "parser/parse_func.h"
#include "parser/parse_type.h"
+ #include "pgstat.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+ #include "utils/regproc.h"
#include "utils/rel.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
*************** typedef struct pltcl_call_state
*** 226,231 ****
--- 230,237 ----
/**********************************************************************
* Global data
**********************************************************************/
+ static char *pltcl_start_proc = NULL;
+ static char *pltclu_start_proc = NULL;
static bool pltcl_pm_init_done = false;
static Tcl_Interp *pltcl_hold_interp = NULL;
static HTAB *pltcl_interp_htab = NULL;
*************** static const TclExceptionNameMap excepti
*** 253,260 ****
**********************************************************************/
void _PG_init(void);
! static void pltcl_init_interp(pltcl_interp_desc *interp_desc, bool pltrusted);
! static pltcl_interp_desc *pltcl_fetch_interp(bool pltrusted);
static Datum pltcl_handler(PG_FUNCTION_ARGS, bool pltrusted);
--- 259,268 ----
**********************************************************************/
void _PG_init(void);
! static void pltcl_init_interp(pltcl_interp_desc *interp_desc,
! Oid prolang, bool pltrusted);
! static pltcl_interp_desc *pltcl_fetch_interp(Oid prolang, bool pltrusted);
! static void call_pltcl_start_proc(Oid prolang, bool pltrusted);
static Datum pltcl_handler(PG_FUNCTION_ARGS, bool pltrusted);
*************** _PG_init(void)
*** 441,446 ****
--- 449,472 ----
&hash_ctl,
HASH_ELEM | HASH_BLOBS);
+ /************************************************************
+ * Define PL/Tcl's custom GUCs
+ ************************************************************/
+ DefineCustomStringVariable("pltcl.start_proc",
+ gettext_noop("PL/Tcl function to call once when pltcl is first used."),
+ NULL,
+ &pltcl_start_proc,
+ NULL,
+ PGC_SUSET, 0,
+ NULL, NULL, NULL);
+ DefineCustomStringVariable("pltclu.start_proc",
+ gettext_noop("PL/TclU function to call once when pltclu is first used."),
+ NULL,
+ &pltclu_start_proc,
+ NULL,
+ PGC_SUSET, 0,
+ NULL, NULL, NULL);
+
pltcl_pm_init_done = true;
}
*************** _PG_init(void)
*** 448,454 ****
* pltcl_init_interp() - initialize a new Tcl interpreter
**********************************************************************/
static void
! pltcl_init_interp(pltcl_interp_desc *interp_desc, bool pltrusted)
{
Tcl_Interp *interp;
char interpname[32];
--- 474,480 ----
* pltcl_init_interp() - initialize a new Tcl interpreter
**********************************************************************/
static void
! pltcl_init_interp(pltcl_interp_desc *interp_desc, Oid prolang, bool pltrusted)
{
Tcl_Interp *interp;
char interpname[32];
*************** pltcl_init_interp(pltcl_interp_desc *int
*** 462,468 ****
if ((interp = Tcl_CreateSlave(pltcl_hold_interp, interpname,
pltrusted ? 1 : 0)) == NULL)
elog(ERROR, "could not create slave Tcl interpreter");
- interp_desc->interp = interp;
/************************************************************
* Initialize the query hash table associated with interpreter
--- 488,493 ----
*************** pltcl_init_interp(pltcl_interp_desc *int
*** 490,505 ****
pltcl_SPI_execute_plan, NULL, NULL);
Tcl_CreateObjCommand(interp, "spi_lastoid",
pltcl_SPI_lastoid, NULL, NULL);
}
/**********************************************************************
* pltcl_fetch_interp() - fetch the Tcl interpreter to use for a function
*
* This also takes care of any on-first-use initialization required.
- * Note: we assume caller has already connected to SPI.
**********************************************************************/
static pltcl_interp_desc *
! pltcl_fetch_interp(bool pltrusted)
{
Oid user_id;
pltcl_interp_desc *interp_desc;
--- 515,549 ----
pltcl_SPI_execute_plan, NULL, NULL);
Tcl_CreateObjCommand(interp, "spi_lastoid",
pltcl_SPI_lastoid, NULL, NULL);
+
+ /************************************************************
+ * Call the appropriate start_proc, if there is one.
+ *
+ * We must set interp_desc->interp before the call, else the start_proc
+ * won't find the interpreter it's supposed to use. But, if the
+ * start_proc fails, we want to abandon use of the interpreter.
+ ************************************************************/
+ PG_TRY();
+ {
+ interp_desc->interp = interp;
+ call_pltcl_start_proc(prolang, pltrusted);
+ }
+ PG_CATCH();
+ {
+ interp_desc->interp = NULL;
+ Tcl_DeleteInterp(interp);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
}
/**********************************************************************
* pltcl_fetch_interp() - fetch the Tcl interpreter to use for a function
*
* This also takes care of any on-first-use initialization required.
**********************************************************************/
static pltcl_interp_desc *
! pltcl_fetch_interp(Oid prolang, bool pltrusted)
{
Oid user_id;
pltcl_interp_desc *interp_desc;
*************** pltcl_fetch_interp(bool pltrusted)
*** 515,527 ****
HASH_ENTER,
&found);
if (!found)
! pltcl_init_interp(interp_desc, pltrusted);
return interp_desc;
}
/**********************************************************************
* pltcl_call_handler - This is the only visible function
* of the PL interpreter. The PostgreSQL
* function manager and trigger manager
--- 559,654 ----
HASH_ENTER,
&found);
if (!found)
! interp_desc->interp = NULL;
!
! /* If we haven't yet successfully made an interpreter, try to do that */
! if (!interp_desc->interp)
! pltcl_init_interp(interp_desc, prolang, pltrusted);
return interp_desc;
}
/**********************************************************************
+ * call_pltcl_start_proc() - Call user-defined initialization proc, if any
+ **********************************************************************/
+ static void
+ call_pltcl_start_proc(Oid prolang, bool pltrusted)
+ {
+ char *start_proc;
+ List *namelist;
+ Oid fargtypes[1]; /* dummy */
+ Oid procOid;
+ HeapTuple procTup;
+ Form_pg_proc procStruct;
+ AclResult aclresult;
+ FmgrInfo finfo;
+ FunctionCallInfoData fcinfo;
+ PgStat_FunctionCallUsage fcusage;
+
+ /* select appropriate GUC */
+ start_proc = pltrusted ? pltcl_start_proc : pltclu_start_proc;
+
+ /* Nothing to do if it's empty or unset */
+ if (start_proc == NULL || start_proc[0] == '\0')
+ return;
+
+ /* Parse possibly-qualified identifier and look up the function */
+ namelist = stringToQualifiedNameList(start_proc);
+ procOid = LookupFuncName(namelist, 0, fargtypes, false);
+
+ /* Current user must have permission to call function */
+ aclresult = pg_proc_aclcheck(procOid, GetUserId(), ACL_EXECUTE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_PROC, start_proc);
+
+ /* Get the function's pg_proc entry */
+ procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(procOid));
+ if (!HeapTupleIsValid(procTup))
+ elog(ERROR, "cache lookup failed for function %u", procOid);
+ procStruct = (Form_pg_proc) GETSTRUCT(procTup);
+
+ /* It must be same language as the function we're currently calling */
+ if (procStruct->prolang != prolang)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("function \"%s\" is in the wrong language to use in %s",
+ start_proc,
+ pltrusted ? "pltcl.start_proc" : "pltclu.start_proc")));
+
+ /*
+ * It must not be SECURITY DEFINER, either. This together with the
+ * language match check ensures that the function will execute in the same
+ * Tcl interpreter we just finished initializing.
+ */
+ if (procStruct->prosecdef)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("function \"%s\" must not be SECURITY DEFINER to use in %s",
+ start_proc,
+ pltrusted ? "pltcl.start_proc" : "pltclu.start_proc")));
+
+ /* A-OK */
+ ReleaseSysCache(procTup);
+
+ /*
+ * Call the function using the normal SQL function call mechanism. We
+ * could perhaps cheat and jump directly to pltcl_handler(), but it seems
+ * better to do it this way so that the call is exposed to, eg, call
+ * statistics collection.
+ */
+ InvokeFunctionExecuteHook(procOid);
+ fmgr_info(procOid, &finfo);
+ InitFunctionCallInfoData(fcinfo, &finfo,
+ 0,
+ InvalidOid, NULL, NULL);
+ pgstat_init_function_usage(&fcinfo, &fcusage);
+ (void) FunctionCallInvoke(&fcinfo);
+ pgstat_end_function_usage(&fcusage, true);
+ }
+
+
+ /**********************************************************************
* pltcl_call_handler - This is the only visible function
* of the PL interpreter. The PostgreSQL
* function manager and trigger manager
*************** compile_pltcl_function(Oid fn_oid, Oid t
*** 1319,1325 ****
/************************************************************
* Identify the interpreter to use for the function
************************************************************/
! prodesc->interp_desc = pltcl_fetch_interp(prodesc->lanpltrusted);
interp = prodesc->interp_desc->interp;
/************************************************************
--- 1446,1453 ----
/************************************************************
* Identify the interpreter to use for the function
************************************************************/
! prodesc->interp_desc = pltcl_fetch_interp(procStruct->prolang,
! prodesc->lanpltrusted);
interp = prodesc->interp_desc->interp;
/************************************************************
diff --git a/src/pl/tcl/sql/pltcl_start_proc.sql b/src/pl/tcl/sql/pltcl_start_proc.sql
index ...7a8e68e .
*** a/src/pl/tcl/sql/pltcl_start_proc.sql
--- b/src/pl/tcl/sql/pltcl_start_proc.sql
***************
*** 0 ****
--- 1,21 ----
+ --
+ -- Test start_proc execution
+ --
+
+ SET pltcl.start_proc = 'no_such_function';
+
+ select tcl_int4add(1, 2);
+ select tcl_int4add(1, 2);
+
+ create function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl SECURITY DEFINER;
+
+ SET pltcl.start_proc = 'public.tcl_initialize';
+
+ select tcl_int4add(1, 2); -- fail
+
+ create or replace function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl;
+
+ select tcl_int4add(1, 2);
+ select tcl_int4add(1, 2);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/27/17 2:42 PM, Tom Lane wrote: > + SET pltcl.start_proc = 'no_such_function'; > + select tcl_int4add(1, 2); > + ERROR: function no_such_function() does not exist Can the error message be more explicit somehow? Otherwise people will be quite confused as to where no_such_function() is coming from. <begin creature-feep> BTW, I'd think this functionality would be valuable for every PL. Maybe it's worth adding formal support for it to pg_language et all and leave it up to each language to decide whether it's supported or not? Multiple init functions might be useful too, similar to how we support multiple hook functions (though presumably a field of regproc[] is a better way to handle that...) I'm also wondering if there'd be value to supporting code that runs on each function invocation. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532)
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 2/27/17 2:42 PM, Tom Lane wrote:
>> + SET pltcl.start_proc = 'no_such_function';
>> + select tcl_int4add(1, 2);
>> + ERROR: function no_such_function() does not exist
> Can the error message be more explicit somehow? Otherwise people will be
> quite confused as to where no_such_function() is coming from.
After thinking about that for awhile, it seemed like the most useful thing
to do is to set up an errcontext callback that will be active throughout
execution of the start_proc. That will cover both setup failures like
the above, and errors occurring within the start_proc, which could be
equally confusing if you think they apply to the function you initially
tried to call. v2 patch attached that does it like that.
> <begin creature-feep>
> BTW, I'd think this functionality would be valuable for every PL.
Maybe for some. I see no value in putting anything about it in
pg_language though. I don't see that we could share any useful amount of
mechanism, and it won't necessarily look the same in every language ---
plperl for instance prefers code fragments over procedures.
In any case, I'm not going there in this patch.
regards, tom lane
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index 0a693803dd3432246ba5f9af6625b90eaa871ab1..ad216dd5b752160e47b9732ef603dbf3e0c56ebb 100644
*** a/doc/src/sgml/pltcl.sgml
--- b/doc/src/sgml/pltcl.sgml
*************** if {[catch { spi_exec $sql_command }]} {
*** 902,907 ****
--- 902,981 ----
</para>
</sect1>
+ <sect1 id="pltcl-config">
+ <title>PL/Tcl Configuration</title>
+
+ <para>
+ This section lists configuration parameters that
+ affect <application>PL/Tcl</>.
+ </para>
+
+ <variablelist>
+
+ <varlistentry id="guc-pltcl-start-proc" xreflabel="pltcl.start_proc">
+ <term>
+ <varname>pltcl.start_proc</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>pltcl.start_proc</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter, if set to a nonempty string, specifies the name
+ (possibly schema-qualified) of a parameterless PL/Tcl function that
+ is to be executed whenever a new Tcl interpreter is created for
+ PL/Tcl. Such a function can perform per-session initialization, such
+ as loading additional Tcl code. A new Tcl interpreter is created
+ when a PL/Tcl function is first executed in a database session, or
+ when an additional interpreter has to be created because a PL/Tcl
+ function is called by a new SQL role.
+ </para>
+
+ <para>
+ The referenced function must be written in the <literal>pltcl</>
+ language, and must not be marked <literal>SECURITY DEFINER</>.
+ (These restrictions ensure that it runs in the interpreter it's
+ supposed to initialize.) The current user must have permission to
+ call it, too.
+ </para>
+
+ <para>
+ If the function fails with an error it will abort the function call
+ that caused the new interpreter to be created and propagate out to
+ the calling query, causing the current transaction or subtransaction
+ to be aborted. Any actions already done within Tcl won't be undone;
+ however, that interpreter won't be used again. If the language is
+ used again the initialization will be attempted again within a fresh
+ Tcl interpreter.
+ </para>
+
+ <para>
+ Only superusers can change this setting. Although this setting
+ can be changed within a session, such changes will not affect Tcl
+ interpreters that have already been created.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="guc-pltclu-start-proc" xreflabel="pltclu.start_proc">
+ <term>
+ <varname>pltclu.start_proc</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>pltclu.start_proc</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter is exactly like <varname>pltcl.start_proc</varname>,
+ except that it applies to PL/TclU. The referenced function must
+ be written in the <literal>pltclu</> language.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ </variablelist>
+ </sect1>
+
<sect1 id="pltcl-procnames">
<title>Tcl Procedure Names</title>
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 453e7ad2ecb34ccb68e672c5c1637d332f05f1aa..1096c4faf04e3b29d00182414e20fd225ba64ffc 100644
*** a/src/pl/tcl/Makefile
--- b/src/pl/tcl/Makefile
*************** DATA = pltcl.control pltcl--1.0.sql pltc
*** 28,34 ****
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
! REGRESS = pltcl_setup pltcl_queries pltcl_unicode
# Tcl on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
--- 28,34 ----
pltclu.control pltclu--1.0.sql pltclu--unpackaged--1.0.sql
REGRESS_OPTS = --dbname=$(PL_TESTDB) --load-extension=pltcl
! REGRESS = pltcl_setup pltcl_queries pltcl_start_proc pltcl_unicode
# Tcl on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
diff --git a/src/pl/tcl/expected/pltcl_start_proc.out b/src/pl/tcl/expected/pltcl_start_proc.out
index ...9946cd965209ab6bbddb9544ec3db3229c79896d .
*** a/src/pl/tcl/expected/pltcl_start_proc.out
--- b/src/pl/tcl/expected/pltcl_start_proc.out
***************
*** 0 ****
--- 1,31 ----
+ --
+ -- Test start_proc execution
+ --
+ SET pltcl.start_proc = 'no_such_function';
+ select tcl_int4add(1, 2);
+ ERROR: function no_such_function() does not exist
+ CONTEXT: processing pltcl.start_proc parameter
+ select tcl_int4add(1, 2);
+ ERROR: function no_such_function() does not exist
+ CONTEXT: processing pltcl.start_proc parameter
+ create function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl SECURITY DEFINER;
+ SET pltcl.start_proc = 'public.tcl_initialize';
+ select tcl_int4add(1, 2); -- fail
+ ERROR: function "public.tcl_initialize" must not be SECURITY DEFINER
+ CONTEXT: processing pltcl.start_proc parameter
+ create or replace function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl;
+ select tcl_int4add(1, 2);
+ NOTICE: in tcl_initialize
+ tcl_int4add
+ -------------
+ 3
+ (1 row)
+
+ select tcl_int4add(1, 2);
+ tcl_int4add
+ -------------
+ 3
+ (1 row)
+
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 11faa6defe5a2e1f1c9ab558a3f584eeec1ac940..2cf7e6619b0c36ea22985ca45c04a26c09f80bcb 100644
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
***************
*** 15,20 ****
--- 15,21 ----
#include "access/htup_details.h"
#include "access/xact.h"
+ #include "catalog/objectaccess.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
#include "commands/event_trigger.h"
***************
*** 25,35 ****
--- 26,39 ----
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
+ #include "parser/parse_func.h"
#include "parser/parse_type.h"
+ #include "pgstat.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+ #include "utils/regproc.h"
#include "utils/rel.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
*************** typedef struct pltcl_call_state
*** 226,231 ****
--- 230,237 ----
/**********************************************************************
* Global data
**********************************************************************/
+ static char *pltcl_start_proc = NULL;
+ static char *pltclu_start_proc = NULL;
static bool pltcl_pm_init_done = false;
static Tcl_Interp *pltcl_hold_interp = NULL;
static HTAB *pltcl_interp_htab = NULL;
*************** static const TclExceptionNameMap excepti
*** 253,260 ****
**********************************************************************/
void _PG_init(void);
! static void pltcl_init_interp(pltcl_interp_desc *interp_desc, bool pltrusted);
! static pltcl_interp_desc *pltcl_fetch_interp(bool pltrusted);
static Datum pltcl_handler(PG_FUNCTION_ARGS, bool pltrusted);
--- 259,269 ----
**********************************************************************/
void _PG_init(void);
! static void pltcl_init_interp(pltcl_interp_desc *interp_desc,
! Oid prolang, bool pltrusted);
! static pltcl_interp_desc *pltcl_fetch_interp(Oid prolang, bool pltrusted);
! static void call_pltcl_start_proc(Oid prolang, bool pltrusted);
! static void start_proc_error_callback(void *arg);
static Datum pltcl_handler(PG_FUNCTION_ARGS, bool pltrusted);
*************** _PG_init(void)
*** 441,446 ****
--- 450,473 ----
&hash_ctl,
HASH_ELEM | HASH_BLOBS);
+ /************************************************************
+ * Define PL/Tcl's custom GUCs
+ ************************************************************/
+ DefineCustomStringVariable("pltcl.start_proc",
+ gettext_noop("PL/Tcl function to call once when pltcl is first used."),
+ NULL,
+ &pltcl_start_proc,
+ NULL,
+ PGC_SUSET, 0,
+ NULL, NULL, NULL);
+ DefineCustomStringVariable("pltclu.start_proc",
+ gettext_noop("PL/TclU function to call once when pltclu is first used."),
+ NULL,
+ &pltclu_start_proc,
+ NULL,
+ PGC_SUSET, 0,
+ NULL, NULL, NULL);
+
pltcl_pm_init_done = true;
}
*************** _PG_init(void)
*** 448,454 ****
* pltcl_init_interp() - initialize a new Tcl interpreter
**********************************************************************/
static void
! pltcl_init_interp(pltcl_interp_desc *interp_desc, bool pltrusted)
{
Tcl_Interp *interp;
char interpname[32];
--- 475,481 ----
* pltcl_init_interp() - initialize a new Tcl interpreter
**********************************************************************/
static void
! pltcl_init_interp(pltcl_interp_desc *interp_desc, Oid prolang, bool pltrusted)
{
Tcl_Interp *interp;
char interpname[32];
*************** pltcl_init_interp(pltcl_interp_desc *int
*** 462,468 ****
if ((interp = Tcl_CreateSlave(pltcl_hold_interp, interpname,
pltrusted ? 1 : 0)) == NULL)
elog(ERROR, "could not create slave Tcl interpreter");
- interp_desc->interp = interp;
/************************************************************
* Initialize the query hash table associated with interpreter
--- 489,494 ----
*************** pltcl_init_interp(pltcl_interp_desc *int
*** 490,505 ****
pltcl_SPI_execute_plan, NULL, NULL);
Tcl_CreateObjCommand(interp, "spi_lastoid",
pltcl_SPI_lastoid, NULL, NULL);
}
/**********************************************************************
* pltcl_fetch_interp() - fetch the Tcl interpreter to use for a function
*
* This also takes care of any on-first-use initialization required.
- * Note: we assume caller has already connected to SPI.
**********************************************************************/
static pltcl_interp_desc *
! pltcl_fetch_interp(bool pltrusted)
{
Oid user_id;
pltcl_interp_desc *interp_desc;
--- 516,550 ----
pltcl_SPI_execute_plan, NULL, NULL);
Tcl_CreateObjCommand(interp, "spi_lastoid",
pltcl_SPI_lastoid, NULL, NULL);
+
+ /************************************************************
+ * Call the appropriate start_proc, if there is one.
+ *
+ * We must set interp_desc->interp before the call, else the start_proc
+ * won't find the interpreter it's supposed to use. But, if the
+ * start_proc fails, we want to abandon use of the interpreter.
+ ************************************************************/
+ PG_TRY();
+ {
+ interp_desc->interp = interp;
+ call_pltcl_start_proc(prolang, pltrusted);
+ }
+ PG_CATCH();
+ {
+ interp_desc->interp = NULL;
+ Tcl_DeleteInterp(interp);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
}
/**********************************************************************
* pltcl_fetch_interp() - fetch the Tcl interpreter to use for a function
*
* This also takes care of any on-first-use initialization required.
**********************************************************************/
static pltcl_interp_desc *
! pltcl_fetch_interp(Oid prolang, bool pltrusted)
{
Oid user_id;
pltcl_interp_desc *interp_desc;
*************** pltcl_fetch_interp(bool pltrusted)
*** 515,527 ****
HASH_ENTER,
&found);
if (!found)
! pltcl_init_interp(interp_desc, pltrusted);
return interp_desc;
}
/**********************************************************************
* pltcl_call_handler - This is the only visible function
* of the PL interpreter. The PostgreSQL
* function manager and trigger manager
--- 560,677 ----
HASH_ENTER,
&found);
if (!found)
! interp_desc->interp = NULL;
!
! /* If we haven't yet successfully made an interpreter, try to do that */
! if (!interp_desc->interp)
! pltcl_init_interp(interp_desc, prolang, pltrusted);
return interp_desc;
}
/**********************************************************************
+ * call_pltcl_start_proc() - Call user-defined initialization proc, if any
+ **********************************************************************/
+ static void
+ call_pltcl_start_proc(Oid prolang, bool pltrusted)
+ {
+ char *start_proc;
+ const char *gucname;
+ ErrorContextCallback errcallback;
+ List *namelist;
+ Oid fargtypes[1]; /* dummy */
+ Oid procOid;
+ HeapTuple procTup;
+ Form_pg_proc procStruct;
+ AclResult aclresult;
+ FmgrInfo finfo;
+ FunctionCallInfoData fcinfo;
+ PgStat_FunctionCallUsage fcusage;
+
+ /* select appropriate GUC */
+ start_proc = pltrusted ? pltcl_start_proc : pltclu_start_proc;
+ gucname = pltrusted ? "pltcl.start_proc" : "pltclu.start_proc";
+
+ /* Nothing to do if it's empty or unset */
+ if (start_proc == NULL || start_proc[0] == '\0')
+ return;
+
+ /* Set up errcontext callback to make errors more helpful */
+ errcallback.callback = start_proc_error_callback;
+ errcallback.arg = (void *) gucname;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
+
+ /* Parse possibly-qualified identifier and look up the function */
+ namelist = stringToQualifiedNameList(start_proc);
+ procOid = LookupFuncName(namelist, 0, fargtypes, false);
+
+ /* Current user must have permission to call function */
+ aclresult = pg_proc_aclcheck(procOid, GetUserId(), ACL_EXECUTE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_PROC, start_proc);
+
+ /* Get the function's pg_proc entry */
+ procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(procOid));
+ if (!HeapTupleIsValid(procTup))
+ elog(ERROR, "cache lookup failed for function %u", procOid);
+ procStruct = (Form_pg_proc) GETSTRUCT(procTup);
+
+ /* It must be same language as the function we're currently calling */
+ if (procStruct->prolang != prolang)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("function \"%s\" is in the wrong language",
+ start_proc)));
+
+ /*
+ * It must not be SECURITY DEFINER, either. This together with the
+ * language match check ensures that the function will execute in the same
+ * Tcl interpreter we just finished initializing.
+ */
+ if (procStruct->prosecdef)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("function \"%s\" must not be SECURITY DEFINER",
+ start_proc)));
+
+ /* A-OK */
+ ReleaseSysCache(procTup);
+
+ /*
+ * Call the function using the normal SQL function call mechanism. We
+ * could perhaps cheat and jump directly to pltcl_handler(), but it seems
+ * better to do it this way so that the call is exposed to, eg, call
+ * statistics collection.
+ */
+ InvokeFunctionExecuteHook(procOid);
+ fmgr_info(procOid, &finfo);
+ InitFunctionCallInfoData(fcinfo, &finfo,
+ 0,
+ InvalidOid, NULL, NULL);
+ pgstat_init_function_usage(&fcinfo, &fcusage);
+ (void) FunctionCallInvoke(&fcinfo);
+ pgstat_end_function_usage(&fcusage, true);
+
+ /* Pop the error context stack */
+ error_context_stack = errcallback.previous;
+ }
+
+ /*
+ * Error context callback for errors occurring during start_proc processing.
+ */
+ static void
+ start_proc_error_callback(void *arg)
+ {
+ const char *gucname = (const char *) arg;
+
+ /* translator: %s is "pltcl.start_proc" or "pltclu.start_proc" */
+ errcontext("processing %s parameter", gucname);
+ }
+
+
+ /**********************************************************************
* pltcl_call_handler - This is the only visible function
* of the PL interpreter. The PostgreSQL
* function manager and trigger manager
*************** compile_pltcl_function(Oid fn_oid, Oid t
*** 1319,1325 ****
/************************************************************
* Identify the interpreter to use for the function
************************************************************/
! prodesc->interp_desc = pltcl_fetch_interp(prodesc->lanpltrusted);
interp = prodesc->interp_desc->interp;
/************************************************************
--- 1469,1476 ----
/************************************************************
* Identify the interpreter to use for the function
************************************************************/
! prodesc->interp_desc = pltcl_fetch_interp(procStruct->prolang,
! prodesc->lanpltrusted);
interp = prodesc->interp_desc->interp;
/************************************************************
diff --git a/src/pl/tcl/sql/pltcl_start_proc.sql b/src/pl/tcl/sql/pltcl_start_proc.sql
index ...7a8e68e2663e19816c0f9214d2782c7ca56561ac .
*** a/src/pl/tcl/sql/pltcl_start_proc.sql
--- b/src/pl/tcl/sql/pltcl_start_proc.sql
***************
*** 0 ****
--- 1,21 ----
+ --
+ -- Test start_proc execution
+ --
+
+ SET pltcl.start_proc = 'no_such_function';
+
+ select tcl_int4add(1, 2);
+ select tcl_int4add(1, 2);
+
+ create function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl SECURITY DEFINER;
+
+ SET pltcl.start_proc = 'public.tcl_initialize';
+
+ select tcl_int4add(1, 2); -- fail
+
+ create or replace function tcl_initialize() returns void as
+ $$ elog NOTICE "in tcl_initialize" $$ language pltcl;
+
+ select tcl_int4add(1, 2);
+ select tcl_int4add(1, 2);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 2, 2017 at 11:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > After thinking about that for awhile, it seemed like the most useful thing > to do is to set up an errcontext callback that will be active throughout > execution of the start_proc. That will cover both setup failures like > the above, and errors occurring within the start_proc, which could be > equally confusing if you think they apply to the function you initially > tried to call. v2 patch attached that does it like that. +1 for that approach. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company