Purpose
========
Better test coverage of functions. On first call of a function, all sql
statements will be prepared, even those not directly called. Think:
create function test() returns void as $$
begin
if false then
select badcolumn from badtable;
end if;
end; $$ language plpgsql;
At first I thought this patch would check sql on create, but that would create
a dependency, which would be bad.
Before, if you called this function, you'd get no error. With this patch, and
with postgresql.conf settings enabled, you get:
select * from test();
ERROR: relation "badtable" does not exist
LINE 1: select badcolumn from badtable
^
QUERY: select badcolumn from badtable
CONTEXT: PL/pgSQL function "test" line 4 at SQL statement
The Patch
=========
Applied ok, compile and make check ran ok. It seems to add/edit regression
tests, but no documentation.
I tried several different things I could think of, but it always found my bugs.
Its disabled by default so wont cause unexpected changes. Its easy to enable,
and to have individual functions exempt themselves.
Performance
===========
No penalty. At first I was concerned every function call would have overhead
of extra preparing, but that is not the case. It's prepared on first call but
not subsequent calls. But that made me worry that the prepare would exist too
long and use old outdated stats, that as well is not a problem. I was able to
setup a test where a bad index was chosen. I used two different psql sessions.
In one I started a transaction, and selected from my function several times
(and it was slow because it was using a bad index). In the other psql session
I ran analyze on my table. Back in my first psql session, I just waited,
running my function ever once and a while. Eventually it picked up the new
stats and start running quick again.
Code Review
===========
I am not qualified
Problems
========
I like the idea of this patch. I think it will help catch more bugs in
functions sooner. However, a function like:
create function test5() returns integer as $$
begin
create temp table junk(id integer);
insert into junk(id) values(100);
drop table temp;
return 1;
end;
$$ language plpgsql;
Will always throw an error because at prepare time, the temp junk table wont
exist. This patch implements new syntax to disable the check:
create function test5() returns integer as $$
#prepare_plans on_demand
begin
...
Was it Tom Lane that said, "if we add new syntax, we have to support it forever"? As a helpful
feature I can see people (myself included) enabling this system wide. So what happens to all the plpgsql on
pgxn that this happens to break? Well it needs updated, no problem, but the fix will be to add
"#prepare_plans on_demand" in the magic spot. That breaks it for prior versions of PG. Is this
the syntax we want? What if we add more "compiler flags" in the future:
create function test5() returns integer as $$
#prepare_plans on_demand
#disable_xlog
#work_mem 10MB
begin
create temp table junk(id integer);
insert into junk(id) values(100);
drop table temp;
return 1;
end;
$$ language plpgsql;
I don't have an answer to that. Other sql implement via OPTION(...).
One option I'd thought about, was to extended ANALYZE to support functions. It
would require no additional plpgsql syntax changes, no postgresql.conf
settings. If you wanted to prepare (prepare for a testing purpose, not a
performance purpose) all the sql inside your function, youd:
analyze test5();
I'd expect to get errors from that, because the junk table doesn't exist. I'd
expect it, and just never analyze it.
Summary
=======
Its a tough one. I see benefit here. I can see myself using it. If I had to
put my finger on it, I'm not 100% sold on the syntax. It is usable though, it
does solve problems, so I'd use it. (I'm not 100% sure ANALYZE is better,
either).
I'm going to leave this patch as "needs review", I think more eyes might be
helpful.
-Andy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
