Patch: plan invalidation vs stored procedures

Martin Pihlak <martin.pihlak@gmail.com> writes:

Changing statement result type is also currently prohibited in
StorePreparedStatement. There maybe good reasons for this,

How about "the SQL spec says so"?

Admittedly, it's a bit of a jump from views to prepared statements,
but the spec is perfectly clear that altering a table doesn't alter
any views dependent on it: SQL99 11.11 <add column definition> saith

NOTE 189 - The addition of a column to a table has no effect on
any existing <query expression> included in a view descriptor,
<triggered action> included in a trigger descriptor, or <search
condition> included in a constraint descriptor because any
implicit column references in these descriptor elements are
syntactically substituted by explicit column references under
the Syntax Rules of Subclause 7.11, "<query specification>".
Furthermore, by implication (from the lack of any General Rules
to the contrary), the meaning of a column reference is never
retroactively changed by the addition of a column subsequent
to the invocation of the <SQL schema statement> containing that
column reference.

and there was a comparable restriction in SQL92. You'd need to make a
pretty strong argument why prepared statements should behave differently
from views to convince me that changing this is a good idea.

regards, tom lane

Changing statement result type is also currently prohibited in
StorePreparedStatement. There maybe good reasons for this,

How about "the SQL spec says so"?

Prepare time is often also the time when you bind the result, or more
generally set up the code to handle the result.

Generally I argue, that a mode of operation must exist where a change in
return type throws an error, so the client can readjust to the change.

We are only allowed to silently replan when it is clear that
the caller is agnostic to the change.
e.g. because the caller only accesses explicit columns of the return type/result set,
or does not supply a new parameter with a default, (or because he set some
parameter that tells us he can cope).

Certainly a new prepare must be able to cope with the change though,
which currently does not seem to be the case when an SP calls another
one that was dropped (and recreated)?

Andreas

Tom Lane wrote:

Martin Pihlak <martin.pihlak@gmail.com> writes:

Changing statement result type is also currently prohibited in
StorePreparedStatement. There maybe good reasons for this,

How about "the SQL spec says so"?

Admittedly, it's a bit of a jump from views to prepared statements,
but the spec is perfectly clear that altering a table doesn't alter
any views dependent on it: SQL99 11.11 <add column definition> saith

As you said it is a bit of a jump ... For one thing view definitions are
persistent whereas statements are bound to be replanned sooner or later -
reconnects etc. Disallowing replanning after invalidation just postpones
it and meanwhile the cached plans are left unusable ("cached plan must not
change result"). IMHO the problem should be left for the application to handle.
Because this is where it will end up anyway.

Attached is a patch that implements plan invalidation on function DROP,
REPLACE and ALTER. Function oids used by the query are collected in analyze phase
and stored in PlannedStmt. Only plans that reference the altered function are
invalidated. The patch also enables replanning on result set change.

regards,
Martin

Hi

We need plan invalidation fix in 8.3 also at least it would make migrating
from 8.2 to 8.3 much more attractive.
Currenlty we are having problems related to plan invalidation couple of
times per week (mainly we have to let developers change their code before we
release it into live databases but it feels like sitting on ticking bomb
after previous downtime).
Is it possible to get it into some official 8.3.x release or should we do it
in house?
Who should add it into september commitfest?

Asko

On Fri, Aug 15, 2008 at 2:13 PM, Martin Pihlak <martin.pihlak@gmail.com>wrote:

"Asko Oja" <ascoja@gmail.com> writes:

Is it possible to get it into some official 8.3.x release

This is not the kind of patch we put into stable branches.

regards, tom lane

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

"Asko Oja" <ascoja@gmail.com> writes:

Is it possible to get it into some official 8.3.x release

This is not the kind of patch we put into stable branches.

Does this really count as a user-visible change, except in the sense
that they won't see things erroring out? It doesn't add new syntax,
as far as I can tell.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

"Asko Oja" <ascoja@gmail.com> writes:

Is it possible to get it into some official 8.3.x release

This is not the kind of patch we put into stable branches.

Does this really count as a user-visible change, except in the sense
that they won't see things erroring out? It doesn't add new syntax,
as far as I can tell.

So what? That is not the only criterion for backpatching.

The bigger the change the more resistance there will be to backpatching
it. Code stability is a major concern.

cheers

andrew

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Regards,
--
dim

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries? It is protection to server's hang?

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

---------
Hannu

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Pavel

Does it change of result some queries?

Patch in itself is not changing what the queries return. It just gets rid of
error condition from which Postgres itself is not able to recover.

It is protection to server's hang?
For users of stored procedures it is protection from downtime. For Skype it
has been around 20% of databse related downtime this year.

On Mon, Aug 18, 2008 at 12:05 PM, Pavel Stehule <pavel.stehule@gmail.com>wrote:

"Asko Oja" <ascoja@gmail.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

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@sss.pgh.pa.us> wrote:

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Could you please explain why you think so ?

As I see it, the patch does not change visible behaviour, except
removing some sonditions where client becomes unusable after some other
backend does some legitimate changes.

Is the current behavior planned or even defined by spec ?

I agree, that the bug (if it is a bug) could also be circumvented by the
calling function by detecting a failed cache lookup and doing
replan/requery itself, but this would require all PL implementations and
other functions with stored plans to do it independently.

-----
Hannu

On Mon, 2008-08-18 at 20:29 -0400, Tom Lane wrote:

"Asko Oja" <ascoja@gmail.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.

Why not suggest they just should stop using functions and move all
business logic into client or "3rd tier" ?

(Actually I would not recommend that as functions are very good way to
abstract database access AND provide better security AND speed up
queries)

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.

Maybe there should be something in postgreSQL docs that warns users against
using functions in any non-trivial circumstances, as functions are not
expected to behave like the rest of postgreSQL features and there is
not plan to fix that ?

----------------
Hannu

2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Could you please explain why you think so ?

As I see it, the patch does not change visible behaviour, except
removing some sonditions where client becomes unusable after some other
backend does some legitimate changes.

Are you sure, so this behave hasn't any secondary effect? So this
change doesn't breaks any application?

Pavel

2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Could you please explain why you think so ?

As I see it, the patch does not change visible behaviour, except
removing some sonditions where client becomes unusable after some other
backend does some legitimate changes.

Is the current behavior planned or even defined by spec ?

I agree, that the bug (if it is a bug) could also be circumvented by the
calling function by detecting a failed cache lookup and doing
replan/requery itself, but this would require all PL implementations and
other functions with stored plans to do it independently.

I am not against to this patch or this feature. But I am sure, so
isn't well to do not necessary changes in stable version.

Pavel

On Tue, 2008-08-19 at 12:42 +0200, Pavel Stehule wrote:

2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:

This is not the kind of patch we put into stable branches.

So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Does it change of result some queries?

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Could you please explain why you think so ?

As I see it, the patch does not change visible behaviour, except
removing some sonditions where client becomes unusable after some other
backend does some legitimate changes.

Are you sure, so this behave hasn't any secondary effect? So this
change doesn't breaks any application?

I can't think of any.

What it does, is it makes the changed function usable right after
redefining the new function.

Current behaviour is to make the calling function unusable until the
backend is restarted, after which it still will use the new version of
the function.