Lookup penalty for VARIADIC patch

2008/7/15 Tom Lane <tgl@sss.pgh.pa.us>:

The proposed variadic-functions patch inserts some none-too-cheap code
into FuncnameGetCandidates (it's deconstructing the proargmodes column
to see if the function is variadic or not) which gets executed whether
or not there are any variadic functions involved. I checked whether
this would cause a noticeable slowdown in practice, and got a
discouraging answer:

$ cat functest.sql
select sin(5), cos(45);
$ pgbench -c 1 -t 10000 -n -f functest.sql regression
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 1
number of transactions per client: 10000
number of transactions actually processed: 10000/10000
tps = 927.418555 (including connections establishing)
tps = 928.953281 (excluding connections establishing)

That's with the patch. CVS HEAD gets
tps = 1017.901218 (including connections establishing)
tps = 1019.724948 (excluding connections establishing)

so that code is adding about 10% to the total round-trip execution time
for the select --- considering all the other overhead involved there,
that means the actual cost of FuncnameGetCandidates has gone up probably
by an order of magnitude. And that's for the *best* case, where
proargmodes is null so SysCacheGetAttr will fall out without returning
an array to examine. This doesn't seem acceptable to me.

What I'm thinking of doing is adding a column to pg_proc that provides
the needed info in a trivial-to-get-at format. There are two ways we
could do it: a bool column that is TRUE if the function is variadic,
or an oid column that is the variadic array's element type, or zero
if the function isn't variadic. The second would take more space but
would avoid having to do a catalog lookup to get the element type in
the case that the function is indeed variadic. I'm leaning to the
second way but wanted to know if anyone objected?

Also, it occurs to me that we could buy back a good part of the extra
space if we allowed pg_proc.probin to be NULL for internal functions.
Right now it's always "-" in that case, which is useless ...

probin is used in some unofficial pl hacks, so this space its some
times used. I vote for special column that containst variadic element
type

Regards
Pavel Stehule

"Pavel Stehule" <pavel.stehule@gmail.com> writes:

2008/7/15 Tom Lane <tgl@sss.pgh.pa.us>:

Also, it occurs to me that we could buy back a good part of the extra
space if we allowed pg_proc.probin to be NULL for internal functions.
Right now it's always "-" in that case, which is useless ...

probin is used in some unofficial pl hacks, so this space its some
times used.

Sure, if you want to use it you can. I'm just saying we should allow it
to be really NULL, instead of a dummy value, when it isn't being used.

regards, tom lane

On Jul 15, 2008, at 4:58 PM, Tom Lane wrote:

There are two ways we
could do it: a bool column that is TRUE if the function is variadic,
or an oid column that is the variadic array's element type, or zero
if the function isn't variadic. The second would take more space but
would avoid having to do a catalog lookup to get the element type in
the case that the function is indeed variadic. I'm leaning to the
second way but wanted to know if anyone objected?

If you go the second route, I'd vote for it being NULL if the
function isn't variadic, unless that would play hell with the C side
of the catalog code...

Also, it occurs to me that we could buy back a good part of the extra
space if we allowed pg_proc.probin to be NULL for internal functions.
Right now it's always "-" in that case, which is useless ...

I'd vote for that being NULL in any case... magic values should be
avoided when possible.
--
Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

Decibel! <decibel@decibel.org> writes:

On Jul 15, 2008, at 4:58 PM, Tom Lane wrote:

There are two ways we
could do it: a bool column that is TRUE if the function is variadic,
or an oid column that is the variadic array's element type, or zero
if the function isn't variadic. The second would take more space but
would avoid having to do a catalog lookup to get the element type in
the case that the function is indeed variadic. I'm leaning to the
second way but wanted to know if anyone objected?

If you go the second route, I'd vote for it being NULL if the
function isn't variadic, unless that would play hell with the C side
of the catalog code...

Getting rid of the check for null is *exactly* the point here --- AFAICT
that's what's eating all the time in the existing code.

regards, tom lane