To: Josh Berkus <josh@agliodbs.com>
Cc: Roberto Mello <rmello@cc.usu.edu>, pgsql-sql@postgresql.org
Subject: Re: Return type of triger functions from OPAQUE to TRIGGER in 7.3 
In-reply-to: <200302251642.35425.josh@agliodbs.com> 
References: <20030225192043.GA30324@cc.usu.edu>
	<200302251642.35425.josh@agliodbs.com>
Comments: In-reply-to Josh Berkus <josh@agliodbs.com>
	message dated "Tue, 25 Feb 2003 16:42:35 -0800"
Date: Tue, 25 Feb 2003 16:10:14 -0500
Message-ID: <12893.1046207414@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>

Josh Berkus <josh@agliodbs.com> writes:
> Yes.  In < 7.3, OPAQUE actually referred to 3 different types of non-parsable
> return values.   Differntiating the different types should be an improvement 
> in both performance and error prevention.

Actually, there were at least half a dozen distinguishable meanings of
"OPAQUE" in older releases :-(.  The failure to make these distinctions
led to such infamous security holes as the "select cash_out(2)" bug.
I don't think there's any real speed gain involved ... but not having
the backend dump core when you pass something the wrong kind of argument
is a big improvement IMHO.

>> I would classify this as a bug, because it defeats the purpose of CREATE
>> OR REPLACE, at least for triggers. If the behavior of CREATE FUNCTION is to
>> change OPAQUE to TRIGGER, then CREATE OR REPLACE FUNCTION should be
>> consistent with that behavior.

But that *isn't* the behavior of CREATE FUNCTION.  There are some hacks
(temporary ones, I hope) in CREATE TRIGGER and other places to adjust
the type of an existing function rather than declaring error.  We
intended those hacks only to serve to let existing pre-7.3 pg_dump scripts
be loaded into 7.3; not as an indefinite equivalencing of OPAQUE with
other types, because that would defeat the purpose of introducing the
other types to begin with.

What you're essentially suggesting is that "CREATE OR REPLACE FUNCTION
foo() RETURNS OPAQUE" shouldn't fail if it finds an existing function
returning trigger --- nor, presumably, any of the several other
pseudotypes that replaced OPAQUE.  We could do that, I suppose, but it
strikes me as opening a big hole for the same kinds of mistakes that we
are trying to prevent.  Who's to say whether the function definition
you're supplying is actually meant as a trigger, or a language handler,
or the other possibilities?

Also, it would be inconsistent with the behavior in the cases where
an OPAQUE input parameter gets adjusted.  There's no chance of "CREATE
OR REPLACE FUNCTION foo(opaque) ..." even realizing which function you
meant to replace, if the existing function's input type has been changed
to cstring or something else entirely.

>> So right now we are stuck on either changing the return type of every
>> trigger for TRIGGER (which means no 7.2 compatibility) or stay stuck in
>> 7.2-land.

It's hard to believe that this is the only thing standing between you
and having SQL code that works in both 7.2 and 7.3.  That would mean
that you have no intention of adopting *any* 7.3 improvements.  Is that
really your plan?

			regards, tom lane