Horton

Thread

Commits

  1. Fix dependency, when changing a function's argument/return type.

  1. BUG #14706: Dependencies not recorded properly for base types

    Karen Huddleston <khuddleston@pivotal.io> — 2017-06-14T23:22:59Z

    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 functions
 Schema |     Name     | Result data type | Argument data types |  Type
--------+--------------+------------------+---------------------+--------
 public | base_fn_in  | base_type        | cstring             | normal
 public | 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.

  2. Re: BUG #14706: Dependencies not recorded properly for base types

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T00:42:16Z

    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

  3. Re: BUG #14706: Dependencies not recorded properly for base types

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T01:02:28Z

    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

  4. Re: BUG #14706: Dependencies not recorded properly for base types

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-15T04:52:01Z

    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

  5. Re: BUG #14706: Dependencies not recorded properly for base types

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T04:53:54Z

    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

  6. Re: BUG #14706: Dependencies not recorded properly for base types

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-15T05:59:21Z

    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

  7. Re: BUG #14706: Dependencies not recorded properly for base types

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-15T06:05:01Z

    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

  8. Re: BUG #14706: Dependencies not recorded properly for base types

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-15T06:07:53Z

    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

  9. Re: BUG #14706: Dependencies not recorded properly for base types

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-15T06:51:30Z

    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

  10. Re: BUG #14706: Dependencies not recorded properly for base types

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-15T07:49:08Z

    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

  11. Re: BUG #14706: Dependencies not recorded properly for base types

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T13:20:35Z

    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

  12. Re: BUG #14706: Dependencies not recorded properly for base types

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T16:31:40Z

    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

  13. Re: BUG #14706: Dependencies not recorded properly for base types

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-15T16:38:24Z

    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

  14. Re: BUG #14706: Dependencies not recorded properly for base types

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-16T01:54:25Z

    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

  15. Re: BUG #14706: Dependencies not recorded properly for base types

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-16T08:49:48Z

    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