polymorphic arguments and return type for PL/pgSQL

Joe Conway <mail@joeconway.com> writes:

The attached patch enables PL/pgSQL functions (but not triggers) to
accept and return polymorphic types. It is careful to return false from
func_up_to_date() if any of the polymorphic types change from
call-to-call.

I don't think you can usefully do it that way. Suppose the same
function is being invoked in two places in a query, with two different
actual argument types at the two spots. Won't this setup result in
dropping and rebuilding the function cache twice per row? You've really
got to arrange for there to be a separate function cache entry for each
set of argument types --- in other words, the actual arg types have to
be part of the cache key. (It might be time to change the cache lookup
into a hashtable instead of a simple linear list search...)

It also falls back to the pg_proc declared types if the
caller didn't setup the FuncExpr node.

This will result in "plpgsql functions cannot return type anyarray"
which is at best misleading. It'd be better to have a specific error
message, say "could not determine actual return type for polymorphic
function %s".

regards, tom lane

Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

The attached patch enables PL/pgSQL functions (but not triggers) to
accept and return polymorphic types. It is careful to return false from
func_up_to_date() if any of the polymorphic types change from
call-to-call.

I don't think you can usefully do it that way. Suppose the same
function is being invoked in two places in a query, with two different
actual argument types at the two spots. Won't this setup result in
dropping and rebuilding the function cache twice per row?

Actually, no. Every compile of the function gets added to a linked list,
and a subsequent call checks the list for a compiled version matching
the funcoid and argument/return types; from plpgsql_call_handler():

<snip>
if (func == NULL)
{
/*
* Check if we already compiled this function for another caller
*/
for (func = compiled_functions; func != NULL; func = func->next)
{
if (funcOid == func->fn_oid && func_up_to_date(func, fcinfo))
break;
}

/*
* If not, do so and add it to the compiled ones
*/
if (func == NULL)
{
func = plpgsql_compile(funcOid,
isTrigger ? T_TRIGGER : T_FUNCTION,
fcinfo);
func->next = compiled_functions;
compiled_functions = func;
}
</snip>

You've really got to arrange for there to be a separate function cache
entry for each set of argument types --- in other words, the actual arg
types have to be part of the cache key.

See above; it already does that. Actually, that's not even new, but it
did work out nice for this purpose.

(It might be time to change the cache lookup into a hashtable instead of
a simple linear list search...)

I could do that if you want, but do you really think it's worth it? How
long does a linked list have to get before a hash table starts to be a
win (this is something I've always wondered about anyway)?

It also falls back to the pg_proc declared types if the
caller didn't setup the FuncExpr node.

This will result in "plpgsql functions cannot return type anyarray"
which is at best misleading. It'd be better to have a specific error
message, say "could not determine actual return type for polymorphic
function %s".

OK, that's an easy change.

Thanks,

Joe

Joe Conway wrote:

Tom Lane wrote:

(It might be time to change the cache lookup into a hashtable instead of
a simple linear list search...)

I could do that if you want, but do you really think it's worth it? How
long does a linked list have to get before a hash table starts to be a
win (this is something I've always wondered about anyway)?

I was about to start looking at the hash table implementation and have a
question. Do you think it would be better to do

1) a hash lookup by function oid to a linked list of different compiled
versions (for each set of argument/return types)
-or-
2) create hash key using a new structure that includes function oid,
return type, and argument types, and use that for direct lookup.

#1 looks easier, and given the waning hours might be more likely to get
finished. #2 is probably preferable, but a bit more work.

Thoughts?

Thanks,

Joe

Do you think it would be better to do

1) a hash lookup by function oid to a linked list of different compiled
versions (for each set of argument/return types)
-or-
2) create hash key using a new structure that includes function oid,
return type, and argument types, and use that for direct lookup.

The latter. By the time you pay the price of a hash lookup, a slightly
longer key is nearly free. (Maybe entirely free, since it might produce
better-distributed hash values.)

dynahash only supports fixed-length keys, so don't forget to zero out
unused positions in the argument type vector.

BTW, I can't see any need to include the return type in the hash key ---
wouldn't it be predetermined given the argument types?

regards, tom lane

Tom Lane wrote:

The latter. By the time you pay the price of a hash lookup, a slightly
longer key is nearly free. (Maybe entirely free, since it might produce
better-distributed hash values.)

OK -- I figured that's what you'd say, so I've already started down that
road.

dynahash only supports fixed-length keys, so don't forget to zero out
unused positions in the argument type vector.

Important safety tip! Thanks.

BTW, I can't see any need to include the return type in the hash key ---
wouldn't it be predetermined given the argument types?

Yup, very true.

Thanks,

Joe

Tom Lane wrote:

Joe Conway wrote:

2) create hash key using a new structure that includes function oid,
return type, and argument types, and use that for direct lookup.

The latter. By the time you pay the price of a hash lookup, a slightly
longer key is nearly free. (Maybe entirely free, since it might produce
better-distributed hash values.)

dynahash only supports fixed-length keys, so don't forget to zero out
unused positions in the argument type vector.

BTW, I can't see any need to include the return type in the hash key ---
wouldn't it be predetermined given the argument types?

The attached implements a compiled function hash in addition to the
earlier changes to support PL/pgSQL polymorphism. It also includes the
ealier requested change wrt generating an ERROR when faced with
polymorphic arguments or return type and no FuncExpr node available.

The compiled function hash uses the following key:
typedef struct PLpgSQL_func_key
{
Oid funcOid;
Oid argtypes[FUNC_MAX_ARGS];
} PLpgSQL_func_key;

I did a simple test to check performance impact using the ealier sample
function and table:

CREATE OR REPLACE FUNCTION tst(anyelement) returns anyarray as '
begin
if $1 is of (int2, int4, int8, float4, float8, numeric) then
return array[$1 * 2];
elsif $1 is of (text) then
return array[$1 || $1];
else
return array[$1];
end if;
end;
' language 'plpgsql';

create table plpgsql(f1 int, f2 float8, f3 text, f4 oid);
insert into plpgsql values(1, 1.1, 'a', 1);
insert into plpgsql values(2, 2.2, 'b', 2);

---------------------------------------------------------
with original patch (linked list compiled function cache)
---------------------------------------------------------
psql regression
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 3.73 msec
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 1.89 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 1.36 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.70 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.21 msec
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 0.18 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 0.18 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.18 msec
\q

----------------------------------------------------
with this patch (hash table compiled function cache)
----------------------------------------------------
psql regression
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 2.93 msec
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 1.64 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 0.18 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 1.05 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.69 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f1) from plpgsql; Total runtime: 0.19 msec
explain analyze SELECT tst(f2) from plpgsql; Total runtime: 0.18 msec
explain analyze SELECT tst(f3) from plpgsql; Total runtime: 0.21 msec
explain analyze SELECT tst(f4) from plpgsql; Total runtime: 0.22 msec
\q

No difference worth caring about. In more complex scenarios, the hash
table cache should win hands down, I'd think.

Compiles clean, and passes all regression tests. I'll look to update the
docs and regression tests as part of my post freeze array/polymorphic
function cleanup.

If there are no objections, please apply.

Thanks,

Joe

Joe Conway wrote:

Compiles clean, and passes all regression tests. I'll look to update the
docs and regression tests as part of my post freeze array/polymorphic
function cleanup.

Oh, almost forgot. I had one question, seen here in
get_function_by_signature()

+ if (!entry_valid)
+ {
+ 	plpgsql_HashTableDelete(function);
+
+ 	/* XXX: is it worth worrying about the leaked function struct? */
+ 	function = NULL;

With the current linked list, the function is effectively leaked to the
end of the session anyway, so I don't think this is any worse. Thoughts?

Thanks,

Joe

Joe Conway <mail@joeconway.com> writes:

With the current linked list, the function is effectively leaked to the
end of the session anyway, so I don't think this is any worse. Thoughts?

Don't worry about it now. If a plpgsql function cache entry is
discarded, we leak far more than just the function struct :-( ... and
most of it has been malloc'd, not palloc'd, so it's very hard to get
back.

Someday plpgsql should be rewritten so that it never malloc's anything,
and all its parse data structures are palloc'd into a memory context
associated with the current function cache entry. Then it would be
practical to recover the memory associated with an obsoleted cache
entry. But we've lived without this so far, so I guess it's not causing
many people problems.

regards, tom lane