Обсуждение: CREATE OR REPLACE FUNCTION vs ownership

Whilst fooling with the default ACLs patch I noticed that there's a
pre-existing bug in CREATE OR REPLACE FUNCTION.  It will let you
replace a function if pg_proc_ownercheck passes, which these days
does *not* mean that you are exactly the role mentioned in
pg_proc.proowner; it only means you are some member of that role.
It then proceeds with the function replacement, while keeping the
old proowner value.  It then proceeds to wipe out the old dependency
info and record *you* as the owner in pg_shdepend.  So pg_shdepend
will be inconsistent with proowner if you're a member of the owning
role but not running with SET ROLE to the owning role.

(I was led to this after noticing that the patch similarly bollixes
the grant dependencies ...)

My inclination is to think that the right behavior for REPLACE FUNCTION
is to keep the old proowner and proacl values, because that's what it
always has done and nobody's complained.  But I suppose a case could
be made that you're completely replacing the function and so you should
replace its ownership/permissions too.  The CREATE FUNCTION reference
page fails to specify either way, which is a documentation bug as well.

Comments?

Whichever way you think it should work, there's a bug here that goes
back several versions, and I rather suspect we may have the same issue
for other REPLACE-type commands ...
        regards, tom lane

On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:

> My inclination is to think that the right behavior for REPLACE  
> FUNCTION
> is to keep the old proowner and proacl values, because that's what it
> always has done and nobody's complained.  But I suppose a case could
> be made that you're completely replacing the function and so you  
> should
> replace its ownership/permissions too.  The CREATE FUNCTION reference
> page fails to specify either way, which is a documentation bug as  
> well.
>
> Comments?

The latter, I think. If I replace a function, I should be the new  
owner. To me it makes no sense for someone else to own it.

Best,

David

David E. Wheeler escreveu:
> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
> 
>> My inclination is to think that the right behavior for REPLACE FUNCTION
>> is to keep the old proowner and proacl values, because that's what it
>> always has done and nobody's complained.  But I suppose a case could
>> be made that you're completely replacing the function and so you should
>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>> page fails to specify either way, which is a documentation bug as well.
>>
>> Comments?
> 
> The latter, I think. If I replace a function, I should be the new owner.
> To me it makes no sense for someone else to own it.
> 
Hmm... Using the same logic, if I add a new column should I be the table
owner? If you're changing the function that is because you have permission.

IMHO the owner should be preserved. In my mind, REPLACE is for changing the
content and not the properties (name, owner, etc).


--  Euler Taveira de Oliveira http://www.timbira.com/

On Thu, Oct 1, 2009 at 8:52 PM, Euler Taveira de Oliveira
<euler@timbira.com> wrote:
> David E. Wheeler escreveu:
>> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
>>
>>> My inclination is to think that the right behavior for REPLACE FUNCTION
>>> is to keep the old proowner and proacl values, because that's what it
>>> always has done and nobody's complained.  But I suppose a case could
>>> be made that you're completely replacing the function and so you should
>>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>>> page fails to specify either way, which is a documentation bug as well.
>>>
>>> Comments?
>>
>> The latter, I think. If I replace a function, I should be the new owner.
>> To me it makes no sense for someone else to own it.
>>
> Hmm... Using the same logic, if I add a new column should I be the table
> owner? If you're changing the function that is because you have permission.
>
> IMHO the owner should be preserved. In my mind, REPLACE is for changing the
> content and not the properties (name, owner, etc).

I disagree.  I think David has this one right.  I expect the results
of CREATE OR REPLACE to be the same as the result of CREATE would have
been had the object not existed.

...Robert

Robert Haas wrote:
> On Thu, Oct 1, 2009 at 8:52 PM, Euler Taveira de Oliveira
> <euler@timbira.com> wrote:
>> David E. Wheeler escreveu:
>>> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
>>>
>>>> My inclination is to think that the right behavior for REPLACE FUNCTION
>>>> is to keep the old proowner and proacl values, because that's what it
>>>> always has done and nobody's complained.  But I suppose a case could
>>>> be made that you're completely replacing the function and so you should
>>>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>>>> page fails to specify either way, which is a documentation bug as well.
>>>>
>>>> Comments?
>>> The latter, I think. If I replace a function, I should be the new owner.
>>> To me it makes no sense for someone else to own it.
>>>
>> Hmm... Using the same logic, if I add a new column should I be the table
>> owner? If you're changing the function that is because you have permission.
>>
>> IMHO the owner should be preserved. In my mind, REPLACE is for changing the
>> content and not the properties (name, owner, etc).

If so, it seems to me CREATE OR REPLACE is equivalent to ALTER FUNCTION
with currently unsupported option. In this case, it is not necessary to
check CREATE privilege on the namespace because it does not affect to
its name/schema.

> I disagree.  I think David has this one right.  I expect the results
> of CREATE OR REPLACE to be the same as the result of CREATE would have
> been had the object not existed.

If so, it seems to me CREATE OR REPLACE is equivalent to a pair of
actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.

I think the current implementation intend the later perspective.
(It also checks ownership of the older definition, only if exists.)

However, I'm not sure which is the better way, currently.

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2009/10/1 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Robert Haas wrote:
>> On Thu, Oct 1, 2009 at 8:52 PM, Euler Taveira de Oliveira
>> <euler@timbira.com> wrote:
>>> David E. Wheeler escreveu:
>>>> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
>>>>
>>>>> My inclination is to think that the right behavior for REPLACE FUNCTION
>>>>> is to keep the old proowner and proacl values, because that's what it
>>>>> always has done and nobody's complained.  But I suppose a case could
>>>>> be made that you're completely replacing the function and so you should
>>>>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>>>>> page fails to specify either way, which is a documentation bug as well.
>>>>>
>>>>> Comments?
>>>> The latter, I think. If I replace a function, I should be the new owner.
>>>> To me it makes no sense for someone else to own it.
>>>>
>>> Hmm... Using the same logic, if I add a new column should I be the table
>>> owner? If you're changing the function that is because you have permission.
>>>
>>> IMHO the owner should be preserved. In my mind, REPLACE is for changing the
>>> content and not the properties (name, owner, etc).
>
> If so, it seems to me CREATE OR REPLACE is equivalent to ALTER FUNCTION
> with currently unsupported option. In this case, it is not necessary to
> check CREATE privilege on the namespace because it does not affect to
> its name/schema.

Right - so the subtle point here is that ALTER means something
different from CREATE OR REPLACE.  "ALTER" means to make a
modification to something; to change it; to adjust one particular
property of the object without disturbing the others.  On the other
hand, "REPLACE" means to get rid of something and replace it with an
entirely new thing.  I think that is exactly why we have ALTER TABLE
but CREATE OR REPLACE FUNCTION.

Now, if we want to have an ALTER FUNCTION that replaces the function
definition and leaves the owner intact - fine!  But that is not what
REPLACE means.

>> I disagree.  I think David has this one right.  I expect the results
>> of CREATE OR REPLACE to be the same as the result of CREATE would have
>> been had the object not existed.
>
> If so, it seems to me CREATE OR REPLACE is equivalent to a pair of
> actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.

Except that you don't have to drop and recreate the dependencies, if any.

...Robert

Robert Haas wrote:
> 2009/10/1 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> Robert Haas wrote:
>>> On Thu, Oct 1, 2009 at 8:52 PM, Euler Taveira de Oliveira
>>> <euler@timbira.com> wrote:
>>>> David E. Wheeler escreveu:
>>>>> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
>>>>>
>>>>>> My inclination is to think that the right behavior for REPLACE FUNCTION
>>>>>> is to keep the old proowner and proacl values, because that's what it
>>>>>> always has done and nobody's complained.  But I suppose a case could
>>>>>> be made that you're completely replacing the function and so you should
>>>>>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>>>>>> page fails to specify either way, which is a documentation bug as well.
>>>>>>
>>>>>> Comments?
>>>>> The latter, I think. If I replace a function, I should be the new owner.
>>>>> To me it makes no sense for someone else to own it.
>>>>>
>>>> Hmm... Using the same logic, if I add a new column should I be the table
>>>> owner? If you're changing the function that is because you have permission.
>>>>
>>>> IMHO the owner should be preserved. In my mind, REPLACE is for changing the
>>>> content and not the properties (name, owner, etc).
>> If so, it seems to me CREATE OR REPLACE is equivalent to ALTER FUNCTION
>> with currently unsupported option. In this case, it is not necessary to
>> check CREATE privilege on the namespace because it does not affect to
>> its name/schema.
> 
> Right - so the subtle point here is that ALTER means something
> different from CREATE OR REPLACE.  "ALTER" means to make a
> modification to something; to change it; to adjust one particular
> property of the object without disturbing the others.  On the other
> hand, "REPLACE" means to get rid of something and replace it with an
> entirely new thing.  I think that is exactly why we have ALTER TABLE
> but CREATE OR REPLACE FUNCTION.
> 
> Now, if we want to have an ALTER FUNCTION that replaces the function
> definition and leaves the owner intact - fine!  But that is not what
> REPLACE means.
> 
>>> I disagree.  I think David has this one right.  I expect the results
>>> of CREATE OR REPLACE to be the same as the result of CREATE would have
>>> been had the object not existed.
>> If so, it seems to me CREATE OR REPLACE is equivalent to a pair of
>> actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.
> 
> Except that you don't have to drop and recreate the dependencies, if any.

Indeed, but here is one other issue from the perspective of security.

For example, a superuser can define a new type which has input/output
handler using user defined functions. Its ownership is not limited to
superuser, so it means non-privilege user can replace the type handler
owned by himself later.

If we also rebuild dependencies on the CREATE OR REPLACE FUNCTION,
we can prevent other user implicitly invokes replaced malicaious
function (it may perform as a trojan-horse), because dependency
mechanism abort this peudo DROP FUNCTION.

However, similar issue can be happen on ALTER FUNCTION OWNER TO.
IMO, we need a mechanism to prevent ALTER or REPLACE functions
which are used for other stuff without permission checks to
execute it.

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2009/10/1 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Robert Haas wrote:
>> 2009/10/1 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>>> Robert Haas wrote:
>>>> On Thu, Oct 1, 2009 at 8:52 PM, Euler Taveira de Oliveira
>>>> <euler@timbira.com> wrote:
>>>>> David E. Wheeler escreveu:
>>>>>> On Oct 1, 2009, at 3:42 PM, Tom Lane wrote:
>>>>>>
>>>>>>> My inclination is to think that the right behavior for REPLACE FUNCTION
>>>>>>> is to keep the old proowner and proacl values, because that's what it
>>>>>>> always has done and nobody's complained.  But I suppose a case could
>>>>>>> be made that you're completely replacing the function and so you should
>>>>>>> replace its ownership/permissions too.  The CREATE FUNCTION reference
>>>>>>> page fails to specify either way, which is a documentation bug as well.
>>>>>>>
>>>>>>> Comments?
>>>>>> The latter, I think. If I replace a function, I should be the new owner.
>>>>>> To me it makes no sense for someone else to own it.
>>>>>>
>>>>> Hmm... Using the same logic, if I add a new column should I be the table
>>>>> owner? If you're changing the function that is because you have permission.
>>>>>
>>>>> IMHO the owner should be preserved. In my mind, REPLACE is for changing the
>>>>> content and not the properties (name, owner, etc).
>>> If so, it seems to me CREATE OR REPLACE is equivalent to ALTER FUNCTION
>>> with currently unsupported option. In this case, it is not necessary to
>>> check CREATE privilege on the namespace because it does not affect to
>>> its name/schema.
>>
>> Right - so the subtle point here is that ALTER means something
>> different from CREATE OR REPLACE.  "ALTER" means to make a
>> modification to something; to change it; to adjust one particular
>> property of the object without disturbing the others.  On the other
>> hand, "REPLACE" means to get rid of something and replace it with an
>> entirely new thing.  I think that is exactly why we have ALTER TABLE
>> but CREATE OR REPLACE FUNCTION.
>>
>> Now, if we want to have an ALTER FUNCTION that replaces the function
>> definition and leaves the owner intact - fine!  But that is not what
>> REPLACE means.
>>
>>>> I disagree.  I think David has this one right.  I expect the results
>>>> of CREATE OR REPLACE to be the same as the result of CREATE would have
>>>> been had the object not existed.
>>> If so, it seems to me CREATE OR REPLACE is equivalent to a pair of
>>> actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.
>>
>> Except that you don't have to drop and recreate the dependencies, if any.
>
> Indeed, but here is one other issue from the perspective of security.
>
> For example, a superuser can define a new type which has input/output
> handler using user defined functions. Its ownership is not limited to
> superuser, so it means non-privilege user can replace the type handler
> owned by himself later.
>
> If we also rebuild dependencies on the CREATE OR REPLACE FUNCTION,
> we can prevent other user implicitly invokes replaced malicaious
> function (it may perform as a trojan-horse), because dependency
> mechanism abort this peudo DROP FUNCTION.
>
> However, similar issue can be happen on ALTER FUNCTION OWNER TO.
> IMO, we need a mechanism to prevent ALTER or REPLACE functions
> which are used for other stuff without permission checks to
> execute it.

Good point.

...Robert

<font face="Calibri, Verdana, Helvetica, Arial"><span style="font-size:11pt">On 10/1/09 9:26 PM, "Robert Haas" <<a
href="robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br /><br /></span></font><blockquote><font
face="Calibri,Verdana, Helvetica, Arial"><span style="font-size:11pt">2009/10/1 KaiGai Kohei <<a
href="kaigai@ak.jp.nec.com">kaigai@ak.jp.nec.com</a>>:<br/> > Robert Haas wrote:<br /> >> On Thu, Oct 1,
2009at 8:52 PM, Euler Taveira de Oliveira<br /> >> <<a href="euler@timbira.com">euler@timbira.com</a>>
wrote:<br/> >>> David E. Wheeler escreveu:<br /> >>>> On Oct 1, 2009, at 3:42 PM, Tom Lane
wrote:<br/> >>>><br /> >>>>> My inclination is to think that the right behavior for REPLACE
FUNCTION<br/> >>>>> is to keep the old proowner and proacl values, because that's what it<br />
>>>>>always has done and nobody's complained.  But I suppose a case could<br /> >>>>> be
madethat you're completely replacing the function and so you should<br /> >>>>> replace its
ownership/permissionstoo.  The CREATE FUNCTION reference<br /> >>>>> page fails to specify either way,
whichis a documentation bug as well.<br /> >>>>><br /> >>>>> Comments?<br />
>>>>The latter, I think. If I replace a function, I should be the new owner.<br /> >>>> To me
itmakes no sense for someone else to own it.<br /> >>>><br /> >>> Hmm... Using the same logic, if
Iadd a new column should I be the table<br /> >>> owner? If you're changing the function that is because you
havepermission.<br /> >>><br /> >>> IMHO the owner should be preserved. In my mind, REPLACE is for
changingthe<br /> >>> content and not the properties (name, owner, etc).<br /> ><br /> > If so, it seems
tome CREATE OR REPLACE is equivalent to ALTER FUNCTION<br /> > with currently unsupported option. In this case, it
isnot necessary to<br /> > check CREATE privilege on the namespace because it does not affect to<br /> > its
name/schema.<br/><br /> Right - so the subtle point here is that ALTER means something<br /> different from CREATE OR
REPLACE. "ALTER" means to make a<br /> modification to something; to change it; to adjust one particular<br /> property
ofthe object without disturbing the others.  On the other<br /> hand, "REPLACE" means to get rid of something and
replaceit with an<br /> entirely new thing.  I think that is exactly why we have ALTER TABLE<br /> but CREATE OR
REPLACEFUNCTION.<br /><br /> Now, if we want to have an ALTER FUNCTION that replaces the function<br /> definition and
leavesthe owner intact - fine!  But that is not what<br /> REPLACE means.<br /><br /></span></font></blockquote><font
face="Calibri,Verdana, Helvetica, Arial"><span style="font-size:11pt">By this argument CREATE OR REPLACE FUNCTION
shouldbe able to change the <br /> return type of the function; which it can't.<br /></span></font><blockquote><font
face="Calibri,Verdana, Helvetica, Arial"><span style="font-size:11pt"><br /> >> I disagree.  I think David has
thisone right.  I expect the results<br /> >> of CREATE OR REPLACE to be the same as the result of CREATE would
have<br/> >> been had the object not existed.<br /> ><br /> > If so, it seems to me CREATE OR REPLACE is
equivalentto a pair of<br /> > actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.<br /><br /> Except that
youdon't have to drop and recreate the dependencies, if any.<br /><br /> ...Robert<br
/></span></font></blockquote><fontface="Calibri, Verdana, Helvetica, Arial"><span style="font-size:11pt"><br /> And
thereare things that you can do with a real drop/create that you cannot<br /> do with create/replace.   In other words
Iagree with others that this is more<br /> of an ALTER operation.<br /><br /> -Caleb</span></font> 

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> Robert Haas wrote:
>> I disagree.  I think David has this one right.  I expect the results
>> of CREATE OR REPLACE to be the same as the result of CREATE would have
>> been had the object not existed.

> If so, it seems to me CREATE OR REPLACE is equivalent to a pair of
> actions: 1) DROP FUNCTION (if exist) and 2) CREATE FUNCTION.

But in fact CREATE OR REPLACE is *not* meant to be the same as DROP
followed by CREATE.  What it is meant to do is allow you to replace the
implementation of the function while existing callers see it as still
being the same function.  Thus we prevent you from changing the name,
arguments, or result type of the function.  If we were to replace the
permissions that would result in a more insidious but still real reason
why former callers would suddenly stop working.  In effect, permissions
are part of the function's API.

Another big reason not to change it is that it's worked like this ever
since we had function permissions at all.  It seems highly likely that
we could introduce silent security breakages into applications that have
been depending on the old behavior.

I think the original reasoning for the behavior may have been that
ownership/permissions are the only properties of the function that
cannot be specified in the CREATE OR REPLACE FUNCTION syntax.  It makes
sense to leave them alone, because that is more likely to be right than
reverting to default.
        regards, tom lane

On Oct 2, 2009, at 7:49 AM, Tom Lane wrote:

> But in fact CREATE OR REPLACE is *not* meant to be the same as DROP
> followed by CREATE.  What it is meant to do is allow you to replace  
> the
> implementation of the function while existing callers see it as still
> being the same function.  Thus we prevent you from changing the name,
> arguments, or result type of the function.  If we were to replace the
> permissions that would result in a more insidious but still real  
> reason
> why former callers would suddenly stop working.  In effect,  
> permissions
> are part of the function's API.

Okay, this convinces me otherwise. But is it not in fact the case that  
CREATE OR REPLACE FUNCTION doesn't expire the old version of the  
function in the cache of other processes? Don't those processes have  
to reconnect in order to see the new function?

Best,

David

On Fri, Oct 2, 2009 at 10:25 AM, Caleb Welton <cwelton@greenplum.com> wrote:
> Right - so the subtle point here is that ALTER means something
> different from CREATE OR REPLACE.  "ALTER" means to make a
> modification to something; to change it; to adjust one particular
> property of the object without disturbing the others.  On the other
> hand, "REPLACE" means to get rid of something and replace it with an
> entirely new thing.  I think that is exactly why we have ALTER TABLE
> but CREATE OR REPLACE FUNCTION.
>
> Now, if we want to have an ALTER FUNCTION that replaces the function
> definition and leaves the owner intact - fine!  But that is not what
> REPLACE means.
>
> By this argument CREATE OR REPLACE FUNCTION should be able to change the
> return type of the function; which it can't.

No, because when we REPLACE we (rightly) prohibit a replacement that
is incompatible with the existing uses of the function.

...Robert

"David E. Wheeler" <david@kineticode.com> writes:
> Okay, this convinces me otherwise. But is it not in fact the case that  
> CREATE OR REPLACE FUNCTION doesn't expire the old version of the  
> function in the cache of other processes?

It is not.

> Don't those processes have  
> to reconnect in order to see the new function?

The ideal is that backends will start using the new function
implementation on the next call after the REPLACE commits (but any
evaluations already in progress must of course continue with the text
they have).  We have been gradually getting closer to that ideal over
the years, although I think there are still cases where it will take a
little longer --- for instance if a SQL function gets inlined I think
the inlined code will persist for the duration of the query's execution.
I don't believe there are still any cases where you actually have to
reconnect to get it to notice the update.

(At least this is true for plpgsql --- not sure if all the other PLs
are equally up to speed.)
        regards, tom lane

On Oct 2, 2009, at 8:49 AM, Tom Lane wrote:

> The ideal is that backends will start using the new function
> implementation on the next call after the REPLACE commits (but any
> evaluations already in progress must of course continue with the text
> they have).  We have been gradually getting closer to that ideal over
> the years, although I think there are still cases where it will take a
> little longer --- for instance if a SQL function gets inlined I think
> the inlined code will persist for the duration of the query's
> execution.
> I don't believe there are still any cases where you actually have to
> reconnect to get it to notice the update.
>
> (At least this is true for plpgsql --- not sure if all the other PLs
> are equally up to speed.)

Ah, good to know. Perhaps an audit is in order…

Best,

David

I wrote:
> Whichever way you think it should work, there's a bug here that goes
> back several versions, and I rather suspect we may have the same issue
> for other REPLACE-type commands ...

BTW, I looked around for related problems and don't see any.  We only
have CREATE OR REPLACE for functions, rules, and views.  Rules don't
have any permissions or shared dependencies of their own.  CREATE OR
REPLACE VIEW really does work like an ALTER --- it optionally
adds some columns, and then does a REPLACE RULE on the view rule.

I think we do have a documentation problem for CREATE OR REPLACE VIEW
too, in that it ought to mention explicitly that permissions and
non-SELECT rules for the view remain in place.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> My inclination is to think that the right behavior for REPLACE FUNCTION
> is to keep the old proowner and proacl values, because that's what it
> always has done and nobody's complained.

+1.
Stephen

2009/10/2 Stephen Frost <sfrost@snowman.net>:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> My inclination is to think that the right behavior for REPLACE FUNCTION
>> is to keep the old proowner and proacl values, because that's what it
>> always has done and nobody's complained.
>
> +1.

+1
Pavel

>
>        Stephen
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkrGMm4ACgkQrzgMPqB3kiiA5wCgis9FDnbm3wQ2cktKDxOK2hjL
> ZqQAnRHl3rnXTki4WUBcS+iiZRWuzvSU
> =Uq6m
> -----END PGP SIGNATURE-----
>
>