Thread: [BUGS] BUG #14706: Dependencies not recorded properly for base types
[BUGS] BUG #14706: Dependencies not recorded properly for base types
From
khuddleston@pivotal.io
Date:
The following bug has been logged on the website: Bug reference: 14706 Logged by: Karen Huddleston Email address: khuddleston@pivotal.io PostgreSQL version: 9.5.7 Operating system: macOS Sierra Description: Repro ```CREATE FUNCTION base_fn_in(cstring) returns opaque as 'boolin' language internal; CREATE FUNCTION base_fn_out(opaque) returns opaque as 'boolout' language internal; CREATE TYPE base_type(input=base_fn_in, output=base_fn_out); \df List of functionsSchema | Name | Result data type | Argument data types | Type --------+--------------+------------------+---------------------+--------public | base_fn_in | base_type | cstring | normalpublic | base_fn_out | cstring | base_type | normal DROP TYPE base_type; \df ERROR: cache lookup failed for type 22241 (format_type.c:137) ``` When I first create the functions, they are not dependent on other objects. However, when I create base_type, the functions are modified to use the new type. Then, dropping the type first causes the functions to have a cache lookup error because they are missing a new dependency. It is possible to clean up the state if you run DROP FUNCTION base_fn_out(base_type) CASCADE; and then DROP FUNCTION base_fn_in(cstring);, but if you run DROP TYPE base_type; first, it is impossible to drop base_fn_out. Base types should make users run CASCADE to clean up dependencies in the other direction. Additionally, the documentation claims that running DROP TYPE ... CASCADE "Automatically drops objects that depend on the type (such as table columns, functions, operators).", but it doesn't seem to do anything. -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
khuddleston@pivotal.io writes:
> CREATE FUNCTION base_fn_in(cstring) returns opaque as 'boolin' language
> internal;
> CREATE FUNCTION base_fn_out(opaque) returns opaque as 'boolout' language
> internal;
> CREATE TYPE base_type(input=base_fn_in, output=base_fn_out);
You realize of course that this is long-deprecated style.
> DROP TYPE base_type;
> \df
> ERROR: cache lookup failed for type 22241 (format_type.c:137)
... but yeah, as long as we're supporting it at all, that shouldn't
happen. Thanks for the report!
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Karen Huddleston <khuddleston@pivotal.io> writes:
> I didn't actually realize it was deprecated style. I'm not super familiar
> with the syntax for creating types and functions. Which part is deprecated
> and what should it be instead?
The way you're supposed to make a new type nowadays is
CREATE TYPE base_type; -- make a shell type
CREATE FUNCTION base_fn_in(cstring) returns base_type as ...
CREATE FUNCTION base_fn_out(base_type) returns cstring as ...
-- convert shell to real type
CREATE TYPE base_type(input=base_fn_in, output=base_fn_out);
In this way the function signatures are legal from the get-go,
and the dependencies are right too. The business with "opaque"
as a placeholder has been deprecated for circa 15 years; did
you find that example in any modern documentation?
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Thu, Jun 15, 2017 at 9:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > khuddleston@pivotal.io writes: >> CREATE FUNCTION base_fn_in(cstring) returns opaque as 'boolin' language >> internal; >> CREATE FUNCTION base_fn_out(opaque) returns opaque as 'boolout' language >> internal; >> CREATE TYPE base_type(input=base_fn_in, output=base_fn_out); > > You realize of course that this is long-deprecated style. > >> DROP TYPE base_type; >> \df >> ERROR: cache lookup failed for type 22241 (format_type.c:137) > > ... but yeah, as long as we're supporting it at all, that shouldn't > happen. Thanks for the report! The return type of the input function and argument type of the output function get updated when the type is created via SetFunctionReturnType(). This code is actually missing the fact that at the same time to add dependencies in pg_depend which should add a link between the function as objid to the type as refobjid. The reversed dependency is tracked though. Tom, are you working on a patch? At quick glance, this is just missing a call to recordDependencyOn() in SetFunctionReturnType(). -- Michael -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Jun 15, 2017 at 9:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... but yeah, as long as we're supporting it at all, that shouldn't
>> happen. Thanks for the report!
> Tom, are you working on a patch? At quick glance, this is just missing a
> call to recordDependencyOn() in SetFunctionReturnType().
No, I'm going to bed soon. If you want to fix this, have at it.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #14706: Dependencies not recorded properly for basetypes
From
Heikki Linnakangas
Date:
On 06/15/2017 04:02 AM, Tom Lane wrote: > The business with "opaque" as a placeholder has been deprecated for > circa 15 years; did you find that example in any modern > documentation? I wonder if we should remove the support for this, in master. It's been deprecated for long enough that no-one should miss it when it's gone. Would be one less hack to maintain. - Heikki -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
On Thu, Jun 15, 2017 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Thu, Jun 15, 2017 at 9:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> ... but yeah, as long as we're supporting it at all, that shouldn't
>>> happen. Thanks for the report!
>
>> Tom, are you working on a patch? At quick glance, this is just missing a
>> call to recordDependencyOn() in SetFunctionReturnType().
>
> No, I'm going to bed soon. If you want to fix this, have at it.
Okay, here you do. The other two call sites of SetFunctionReturnType
involve triggers and PLs, but those are able to consider correctly
dependencies. For PLs:
=# CREATE FUNCTION mylang_call_handler() RETURNS opaque language c AS
'$libdir/plpgsql', $function$plpgsql_call_handler$function$;
CREATE FUNCTION
=# CREATE language mylang HANDLER mylang_call_handler;
WARNING: 42809: changing return type of function mylang_call_handler
from opaque to language_handler
LOCATION: CreateProceduralLanguage, proclang.c:283
CREATE LANGUAGE
=# drop function mylang_call_handler ();
ERROR: 2BP01: cannot drop function mylang_call_handler() because
other objects depend on it
DETAIL: language mylang depends on function mylang_call_handler()
HINT: Use DROP ... CASCADE to drop the dependent objects too.
LOCATION: reportDependentObjects, dependency.c:963
For triggers:
=# CREATE FUNCTION fn_x_before () RETURNS opaque AS $$
BEGIN
return NEW;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
=# CREATE TABLE aa (a int);
CREATE TABLE
=# CREATE TRIGGER aa_before BEFORE INSERT ON aa FOR EACH ROW EXECUTE
PROCEDURE fn_x_before();
WARNING: 01000: changing return type of function fn_x_before from
opaque to trigger
LOCATION: CreateTrigger, trigger.c:551
CREATE TRIGGER
=# DROP FUNCTION fn_x_before ;
ERROR: 2BP01: cannot drop function fn_x_before() because other
objects depend on it
DETAIL: trigger aa_before on table aa depends on function fn_x_before()
HINT: Use DROP ... CASCADE to drop the dependent objects too.
LOCATION: reportDependentObjects, dependency.c:963
Attached is an idea of patch, inputs welcome. Another idea would be to
deprecate things on HEAD and cause an ERROR when doing such things.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
On Thu, Jun 15, 2017 at 2:59 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 06/15/2017 04:02 AM, Tom Lane wrote: >> >> The business with "opaque" as a placeholder has been deprecated for >> circa 15 years; did you find that example in any modern >> documentation? > > > I wonder if we should remove the support for this, in master. It's been > deprecated for long enough that no-one should miss it when it's gone. Would > be one less hack to maintain. Yeah, the idea has crossed my mind. And do that as well for PLs and triggers. -- Michael -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
On Thu, Jun 15, 2017 at 3:05 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Attached is an idea of patch, inputs welcome. Another idea would be to > deprecate things on HEAD and cause an ERROR when doing such things. Not completely bullet-proof actually. As SetFunctionArgType() can be called, arguments of a function could be changed, leading to lookup errors. Attached is an updated patch with more regression tests. -- Michael -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
Re: [BUGS] BUG #14706: Dependencies not recorded properly for basetypes
From
Heikki Linnakangas
Date:
On 06/15/2017 09:51 AM, Michael Paquier wrote: > On Thu, Jun 15, 2017 at 3:05 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Attached is an idea of patch, inputs welcome. Another idea would be to >> deprecate things on HEAD and cause an ERROR when doing such things. > > Not completely bullet-proof actually. As SetFunctionArgType() can be > called, arguments of a function could be changed, leading to lookup > errors. Attached is an updated patch with more regression tests. Hmm. Strictly speaking there's no need to update the dependency when changing opaque into cstring. Because cstring is a pinned type, recordDependency will do nothing for it. But in any case, I think it'd be better and simpler to fix the dependency in SetFunctionArgType() and SetFunctionReturnType() functions themselves. They're the ones that change the type, they ought to be responsible for fixing the dependency too. See attached. - Heikki -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 06/15/2017 04:02 AM, Tom Lane wrote:
>> The business with "opaque" as a placeholder has been deprecated for
>> circa 15 years; did you find that example in any modern
>> documentation?
> I wonder if we should remove the support for this, in master. It's been
> deprecated for long enough that no-one should miss it when it's gone.
> Would be one less hack to maintain.
I thought about that too, but I think it's a bit late for v10.
Wouldn't object to doing it in the v11 cycle. In the meantime,
we'd need a fix for the back branches anyway.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Hmm. Strictly speaking there's no need to update the dependency when
> changing opaque into cstring. Because cstring is a pinned type,
> recordDependency will do nothing for it.
> But in any case, I think it'd be better and simpler to fix the
> dependency in SetFunctionArgType() and SetFunctionReturnType() functions
> themselves. They're the ones that change the type, they ought to be
> responsible for fixing the dependency too. See attached.
Yeah, Heikki's version looks good to me.
Heikki, are you going to commit/backpatch this, or do you want me to?
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #14706: Dependencies not recorded properly for basetypes
From
Heikki Linnakangas
Date:
On 06/15/2017 07:31 PM, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Hmm. Strictly speaking there's no need to update the dependency when >> changing opaque into cstring. Because cstring is a pinned type, >> recordDependency will do nothing for it. > >> But in any case, I think it'd be better and simpler to fix the >> dependency in SetFunctionArgType() and SetFunctionReturnType() functions >> themselves. They're the ones that change the type, they ought to be >> responsible for fixing the dependency too. See attached. > > Yeah, Heikki's version looks good to me. > > Heikki, are you going to commit/backpatch this, or do you want me to? I can do it, thanks. - Heikki -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
On Fri, Jun 16, 2017 at 1:38 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 06/15/2017 07:31 PM, Tom Lane wrote: >> >> Heikki Linnakangas <hlinnaka@iki.fi> writes: >>> >>> Hmm. Strictly speaking there's no need to update the dependency when >>> changing opaque into cstring. Because cstring is a pinned type, >>> recordDependency will do nothing for it. >> >>> But in any case, I think it'd be better and simpler to fix the >>> dependency in SetFunctionArgType() and SetFunctionReturnType() functions >>> themselves. They're the ones that change the type, they ought to be >>> responsible for fixing the dependency too. See attached. >> >> Yeah, Heikki's version looks good to me. >> >> Heikki, are you going to commit/backpatch this, or do you want me to? > > I can do it, thanks. Fine for me. My first quick guess was better than what I coded at the end, the idea being to wait after TypeCreate() to get the ObjectAdress of the new type. -- Michael -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #14706: Dependencies not recorded properly for basetypes
From
Heikki Linnakangas
Date:
On 06/16/2017 04:54 AM, Michael Paquier wrote: > On Fri, Jun 16, 2017 at 1:38 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> On 06/15/2017 07:31 PM, Tom Lane wrote: >>> >>> Heikki Linnakangas <hlinnaka@iki.fi> writes: >>>> >>>> Hmm. Strictly speaking there's no need to update the dependency when >>>> changing opaque into cstring. Because cstring is a pinned type, >>>> recordDependency will do nothing for it. >>> >>>> But in any case, I think it'd be better and simpler to fix the >>>> dependency in SetFunctionArgType() and SetFunctionReturnType() functions >>>> themselves. They're the ones that change the type, they ought to be >>>> responsible for fixing the dependency too. See attached. >>> >>> Yeah, Heikki's version looks good to me. >>> >>> Heikki, are you going to commit/backpatch this, or do you want me to? >> >> I can do it, thanks. > > Fine for me. My first quick guess was better than what I coded at the > end, the idea being to wait after TypeCreate() to get the ObjectAdress > of the new type. Ok, committed and backported, thanks! - Heikki -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs