Re: Patch: plan invalidation vs stored procedures

Hi

> The reason that this case wasn't covered in 8.3 is that there didn't
> seem to be a use-case that justified doing the extra work. I still
> haven't seen one.
You just stopped reading the thread where it was discussed after your troll
remark?

> Other than inline-able SQL functions there is no reason to invalidate a
stored plan
> based on the fact that some function it called changed contents.
Isn't it reason enough for this patch?
ERROR: cache lookup failed for function is normal and good behaviour and
should not be recoverd from because it never happen if you PostgreSQL right
:)

Usecase 1: Inlined functions
postgres=# create or replace function salary_without_income_tax(i_salary in
numeric, salary out numeric ) returns numeric as $$ select $1 * 0.76 as
salary $$ language sql;
postgres=# prepare c2 as select salary, salary_without_income_tax(salary)
from salaries;
postgres=# execute c2;
salary | salary_without_income_tax
--------+---------------------------
10000 | 7600.00
postgres=# create or replace function salary_without_income_tax(i_salary in
numeric, salary out numeric ) returns numeric as $$ select $1 * 0.74 as
salary $$ language sql;
postgres=# execute c2; salary | salary_without_income_tax
--------+---------------------------
10000 | 7600.00

Use case 2: While rewriting existing modules due to changes in business
requirements then in addition to new code we have to refactor lots of old
functions one natural thing to do would be to get rid of return types as
they are even more inconvenient to use than out parameters. Another reason
is keep coding style consistent over modules so future maintenace will be
less painful in the assholes.
postgres=# create type public.ret_status as ( status integer, status_text
text);
CREATE TYPE
postgres=# create or replace function x ( i_param text ) returns
public.ret_status as $$ select 200::int, 'ok'::text; $$ language sql;
CREATE FUNCTION
postgres=# create or replace function x ( i_param text, status OUT int,
status_text OUT text ) returns record as $$ select 200::int, 'ok'::text; $$
language sql;
ERROR: cannot change return type of existing function
HINT: Use DROP FUNCTION first

Usecase 3.: Extra out parameters are needed in existing functions. I assure
you if you have 5 years of legacy code that is constantly changing it does
happen (often).
postgres=# create or replace function xl ( i_param text, status OUT int,
status_text OUT text, more_text OUT text ) returns record as $$ select
200::int, 'ok'::text, 'cat'::text; $$ language sql;
ERROR: cannot change return type of existing function
DETAIL: Row type defined by OUT parameters is different.
HINT: Use DROP FUNCTION first.

Usecase 4: Things are even worse when you need to change the type that is
used in functions. You have to drop and recreate the type and all the
functions that are using it. Sometimes type is used in several functions and
only some of them need changes.
postgres=# create type public.ret_status_ext as ( status integer,
status_text text, more_money numeric);

CREATE TYPE
postgres=# create or replace function x ( i_param text ) returns
public.ret_status_ext as $$ select 200::int, 'ok'::text; $$ language sql;
ERROR: cannot change return type of existing function
HINT: Use DROP FUNCTION first.

And whenever we do drop and create as hinted then we receive error flood
that won't stop until something is manually done to get rid of it
postgres=# drop function x(text);
DROP FUNCTION
postgres=# create or replace function x ( i_param text ) returns
public.ret_status_ext as $$ select 200::int, $1, 2.3 $$ language sql;
CREATE FUNCTION
postgres=# execute c;
ERROR: cache lookup failed for function 24598

I hope i have answered your question "Why do you not use CREATE OR REPLACE
FUNCTION?" That leaves us to deal with functions in our usual bad, wrong and
stupid ways.
* We create a function with new name and redirect all the calls to it.
(stupid as it creates extra development, testing, code reviewing and
releasing work and leaves around old code).
* We pause pgBouncer and after release let it reconnect all connections (bad
as it creates downtime).
* We invalidate all procedures using update to pg_proc (simply wrong way to
do it but still our best workaround short of restarting postgres).
postgres=# update pg_proc set proname = proname;
UPDATE 2152
postgres=# execute c2;
salary | salary_without_income_tax
--------+---------------------------
10000 | 7400.00

> Perhaps Skype needs to rethink how they are modifying functions.
We have had to change the way we use functions to suit PostgreSQL for 5
years now. That creates us quite a lot of extra work both on development
side and DBA side plus the constantly hanging danger of downtime. Our DBA
teams job is to reduce all possible causes for downtime and this patch is
solution to one of them. Sadly we just get trolled into the ground :)

All in all it's not the job of PostgreSQL to tell the user what we he can or
can't replace in functions. It should be up to the user to decide what and
how to replace so that all surrounding applications will say working.

regards
Asko

PS: It all confuses poor developers "Hi Asko, I work on web backend and am
in the process of changing infodb.eurorates_exchange() and
infodb._eurorates_lookup db functions found in dbs. Problem is the
_eurorates_lookup function did not use IN OUT params but a type, we have
been told we should update all functions to use IN OUT params. In doing so I
will also need to drop the function when it comes to deployment. This
function is in the accounts partition and I know we are not supposed to drop
functions in partitions due to cache problems it creates. Could you tell me
what I should do? Thanks"

On Tue, Aug 19, 2008 at 3:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Asko Oja" <ascoja(at)gmail(dot)com> writes:
> > For users of stored procedures it is protection from downtime. For Skype
> it
> > has been around 20% of databse related downtime this year.
>
> Perhaps Skype needs to rethink how they are modifying functions.
>
> The reason that this case wasn't covered in 8.3 is that there didn't
> seem to be a use-case that justified doing the extra work. I still
> haven't seen one. Other than inline-able SQL functions there is no
> reason to invalidate a stored plan based on the fact that some function
> it called changed contents.
>
> regards, tom lane
>