Re: Patch: plan invalidation vs stored procedures

This is a followup for thread "plan invalidation vs stored procedures".

The background is that it is impossible to change function return type without
dropping and recreating. Unfortunately dropping a function ruins all of the
prepared statements that reference that function (including other functions).
To make matters worse the ruined plans are never invalidated and keep returning
"cache lookup failed" error until replanned (requires admin intervention). Also
the DBA that dropped the function probably has no clue that something is wrong
- not before looking at the server logs at least. This is NOT good, especially
if the database is supporting paid services.

I have prepared proof of concept patch to support plan invalidation on function
DROP (will add ALTER, REPLACE, etc. later). Currently the invalidation is
handled by just dropping all the plans when invalidation message is received.
The desired behaviour would be of course to drop only the affected plans. This
needs function oid list to be present in PlannedStmt -- will look into this later.
Probably a job for the planner.

Changing statement result type is also currently prohibited in
StorePreparedStatement. There maybe good reasons for this, but for the
invalidation to be really useful, it should be enabled. Right now the attempt
to change type renders the plan unusable -- "ERROR: cached plan must not
change result type". Besides that, the patch could already be useful in some
environments - if you are willing to trade the errors for some extra planning
CPU.

There are probably a lot of details that I have overlooked. I'd be really
thankful for some constructive comments and criticism. Especially, what needs
to be done to have this in the core. Feedback appreciated.

regards,
Martin

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)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(dot)pihlak(at)gmail(dot)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(dot)pihlak(at)gmail(dot)com>wrote:

> Tom Lane wrote:
> > Martin Pihlak <martin(dot)pihlak(at)gmail(dot)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
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

"Asko Oja" <ascoja(at)gmail(dot)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(at)gmail(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)gmail(dot)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(at)hi-media(dot)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?

> Regards,
> --
> dim
>

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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(at)2ndquadrant(dot)com>:
> On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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
>
> ---------
> Hannu
>
>

> 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(dot)stehule(at)gmail(dot)com>wrote:

> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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?
>
> > Regards,
> > --
> > dim
> >
>

"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

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
>

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
> 2008/8/18 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> >> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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(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.

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(at)2ndquadrant(dot)com>:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> 2008/8/18 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
>> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> >> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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

>
> 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
>
>
>
>

2008/8/19 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> 2008/8/18 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
>> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> >> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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

> -----
> Hannu
>
>
>
>

On Tue, 2008-08-19 at 12:42 +0200, Pavel Stehule wrote:
> 2008/8/19 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
> > On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
> >> 2008/8/18 Hannu Krosing <hannu(at)2ndquadrant(dot)com>:
> >> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> >> >> 2008/8/18 Dimitri Fontaine <dfontaine(at)hi-media(dot)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.

> Pavel
>
> >
> > 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
> >
> >
> >
> >
>

"Hannu Krosing" <hannu(at)2ndQuadrant(dot)com> writes:

> 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 ?

Now who's trolling :)

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> I am sorry, but it's really new feature and not bug fix

> Could you please explain why you think so ?

For the same reasons that plan invalidation itself was a new feature
and not a bug fix; notably, risk vs reward tradeoff and not wanting
to change long-established behavior in stable branches.

regards, tom lane

Le mardi 19 août 2008, Tom Lane a écrit :
> For the same reasons that plan invalidation itself was a new feature
> and not a bug fix;

I'm sorry but that doesn't help me a dime to understand current situation. It
could well be just me, but... here's how I see it:

- plan invalidation is a new feature in 8.3
- previous releases are out of business: new feature against stable code
- now, we have found a bug in plan invalidation
- HEAD (to be 8.4) will get some new code to fix it

But 8.3 won't benefit from this bugfix? On the grounds that the feature which
is now deployed on the field should *maybe* not get used the way it *is*?

Sorry again, I really don't get it.
--
dim

On Tue, 19 Aug 2008 12:48:06 +0100
Gregory Stark <stark(at)enterprisedb(dot)com> wrote:

> "Hannu Krosing" <hannu(at)2ndQuadrant(dot)com> writes:
>
> > 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 ?
>
> Now who's trolling :)

Although I read his remark as sarcastic after reading the entire
thread I have to say it may be a good idea to have the something in
the docs about the limitation. I never think about it anymore
because I am used to the behavior. I can see where and entity
like skype who has I am sure thousands of procedures would have this
as a constant irritant.

Do I think it should be pushed back to 8.3.x; no. It is a feature. I
don't consider the existing behavior a bug. I consider it a limitation
and we don't back patch fixes for limitations.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
> - now, we have found a bug in plan invalidation

[ shrug... ] You have not found a bug in plan invalidation. You have
found omitted functionality --- functionality that was *intentionally*
omitted from the 8.3 version.

regards, tom lane

Le mardi 19 août 2008, Tom Lane a écrit :
> [ shrug... ] You have not found a bug in plan invalidation. You have
> found omitted functionality --- functionality that was *intentionally*
> omitted from the 8.3 version.

Thanks a lot for this clarification, now I understand you viewpoint.

So, the 8.3 "fix" would be about documenting this intentionnal omit in the
great manual, maybe in a Limits section of the sql-createfunction page?

Another thing I do not understand well is how people are expected to work in
8.3 with a function based API, without hitting Skype problems. I'm having a
project here where the project manager wants a database function API to keep
data logic at serverside, should I tell him to reconsider this while 8.4 is
not ready?
We would then have to go live with an 8.3 based solution containing middleware
code, then port it again to SQL functions when 8.4 is out & stable. Not
appealing, but I sure understand the no new feature in stable code base
argument here.

Regards,
--
dim

Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
> Another thing I do not understand well is how people are expected to work in
> 8.3 with a function based API, without hitting Skype problems.

I could understand this level of complaining if this were a new problem
that'd appeared in 8.3. But *every PG version that we've ever released*
behaves the same way with respect to function drop/recreate. If the
Skype folk have developed a way of working that is guaranteed not to
work with any released version, one has to wonder what they were
thinking.

If you need to DROP rather than CREATE OR REPLACE functions, then 8.3
doesn't make things better for you than prior releases did, but it
does't make them worse either. Making things better for that case is
unequivocally a new feature. And it's rather a corner case at that,
else there would have been enough prior complaints to put it on the
radar screen for 8.3.

What we've got at this point is a submitted patch for a new feature
that hasn't even been accepted into HEAD yet. Lobbying to get it
back-patched is entirely inappropriate IMHO.

regards, tom lane

Polite answers lead to polite discussions. Caling other people names lead to
flame wars.
It's perfectly ok for Skype to keep our own build of 8.3 with given patch
and make it available for whoever might want it. At least now there is
almost good enough description why the patch was needed althou it would have
been more pleasant if the discussion had been constructive.
We didn't keep close enough watch on the list when 8.3 plan invalidation was
discussed and it came as bad surprise to us that some parts important to us
were left out.

By the way it's real nice what you are doing with in and exists
improvements. Thanks.

regards
Asko

On Tue, Aug 19, 2008 at 8:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
> > Another thing I do not understand well is how people are expected to work
> in
> > 8.3 with a function based API, without hitting Skype problems.
>
> I could understand this level of complaining if this were a new problem
> that'd appeared in 8.3. But *every PG version that we've ever released*
> behaves the same way with respect to function drop/recreate. If the
> Skype folk have developed a way of working that is guaranteed not to
> work with any released version, one has to wonder what they were
> thinking.
>
> If you need to DROP rather than CREATE OR REPLACE functions, then 8.3
> doesn't make things better for you than prior releases did, but it
> does't make them worse either. Making things better for that case is
> unequivocally a new feature. And it's rather a corner case at that,
> else there would have been enough prior complaints to put it on the
> radar screen for 8.3.
>
> What we've got at this point is a submitted patch for a new feature
> that hasn't even been accepted into HEAD yet. Lobbying to get it
> back-patched is entirely inappropriate IMHO.
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

2008/8/19 Dimitri Fontaine <dfontaine(at)hi-media(dot)com>:
> Le mardi 19 août 2008, Tom Lane a écrit :
>> [ shrug... ] You have not found a bug in plan invalidation. You have
>> found omitted functionality --- functionality that was *intentionally*
>> omitted from the 8.3 version.
>
> Thanks a lot for this clarification, now I understand you viewpoint.
>
> So, the 8.3 "fix" would be about documenting this intentionnal omit in the
> great manual, maybe in a Limits section of the sql-createfunction page?
>
> Another thing I do not understand well is how people are expected to work in
> 8.3 with a function based API, without hitting Skype problems. I'm having a
> project here where the project manager wants a database function API to keep
> data logic at serverside, should I tell him to reconsider this while 8.4 is
> not ready?

You could to use patched 8.3.

> We would then have to go live with an 8.3 based solution containing middleware
> code, then port it again to SQL functions when 8.4 is out & stable. Not
> appealing, but I sure understand the no new feature in stable code base
> argument here.

This problem isn't too hard without pooling. Not all systems are
global - so usually is possible to find some window and recreate
functions and close all user connections.

Regards
Pavel Stehule

>
> Regards,
> --
> dim
>

> Another thing I do not understand well is how people are expected to work
in
> 8.3 with a function based API, without hitting Skype problems.
People are expected to use same workarounds as Skype is using. For us
another unneccessary downtime week ago was what set us moving/thinking :).
When you use software with limitations then you learn to live with them.
Good thing about postgres you can do something yourself to get some of the
limitations removed.
As Pavel said you are probably using your own build anyway so one more patch
should not be a problem.

regards
Asko

On Tue, Aug 19, 2008 at 8:48 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

> 2008/8/19 Dimitri Fontaine <dfontaine(at)hi-media(dot)com>:
> > Le mardi 19 août 2008, Tom Lane a écrit :
> >> [ shrug... ] You have not found a bug in plan invalidation. You have
> >> found omitted functionality --- functionality that was *intentionally*
> >> omitted from the 8.3 version.
> >
> > Thanks a lot for this clarification, now I understand you viewpoint.
> >
> > So, the 8.3 "fix" would be about documenting this intentionnal omit in
> the
> > great manual, maybe in a Limits section of the sql-createfunction page?
> >
> > Another thing I do not understand well is how people are expected to work
> in
> > 8.3 with a function based API, without hitting Skype problems. I'm having
> a
> > project here where the project manager wants a database function API to
> keep
> > data logic at serverside, should I tell him to reconsider this while 8.4
> is
> > not ready?
>
> You could to use patched 8.3.
>
> > We would then have to go live with an 8.3 based solution containing
> middleware
> > code, then port it again to SQL functions when 8.4 is out & stable. Not
> > appealing, but I sure understand the no new feature in stable code base
> > argument here.
>
> This problem isn't too hard without pooling. Not all systems are
> global - so usually is possible to find some window and recreate
> functions and close all user connections.
>
> Regards
> Pavel Stehule
>
> >
> > Regards,
> > --
> > dim
> >
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Joshua Drake wrote:
> On Tue, 19 Aug 2008 12:48:06 +0100
> Gregory Stark <stark(at)enterprisedb(dot)com> wrote:
>
> > "Hannu Krosing" <hannu(at)2ndQuadrant(dot)com> writes:
> >
> > > 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 ?
> >
> > Now who's trolling :)
>
> Although I read his remark as sarcastic after reading the entire
> thread I have to say it may be a good idea to have the something in
> the docs about the limitation. I never think about it anymore
> because I am used to the behavior. I can see where and entity
> like skype who has I am sure thousands of procedures would have this
> as a constant irritant.
>
> Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> don't consider the existing behavior a bug. I consider it a limitation
> and we don't back patch fixes for limitations.

The bottom line here is that we don't have the time to explain or
justify our backpatch policy every time someone shows up with a bug that
needs to be fixed.

If you want to create your own version of Postgres, go ahead; no one is
stopping you. But if we backpatched everything and we introduced bugs
or change behavior, more people would complain.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Tue, 19 Aug 2008 14:29:52 -0400 (EDT)
Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> > Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> > don't consider the existing behavior a bug. I consider it a
> > limitation and we don't back patch fixes for limitations.
>
> The bottom line here is that we don't have the time to explain or
> justify our backpatch policy every time someone shows up with a bug
> that needs to be fixed.

Is our backpatch policy documented? It does not appear to be in
developer FAQ.

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua Drake wrote:
> On Tue, 19 Aug 2008 14:29:52 -0400 (EDT)
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
>
> > > Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> > > don't consider the existing behavior a bug. I consider it a
> > > limitation and we don't back patch fixes for limitations.
> >
> > The bottom line here is that we don't have the time to explain or
> > justify our backpatch policy every time someone shows up with a bug
> > that needs to be fixed.
>
> Is our backpatch policy documented? It does not appear to be in
> developer FAQ.

Seems we need to add it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

>>> Joshua Drake <jd(at)commandprompt(dot)com> wrote:

> Is our backpatch policy documented? It does not appear to be in
> developer FAQ.

It's mentioned here:

http://www.postgresql.org/support/versioning

"PostgreSQL minor releases fix only frequently-encountered, security,
and data corruption bugs to reduce the risk of upgrading."

-Kevin

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Joshua Drake wrote:
>> Is our backpatch policy documented? It does not appear to be in
>> developer FAQ.

> Seems we need to add it.

I'm not sure that I *want* a formal written-down backpatch policy.
Whether (and how far) to backpatch has always been a best-judgment call
in the past, and we've gotten along fine with that. I think having a
formal policy is just likely to lead to even more complaints: either
patching or not patching could result in second-guessing by someone
who feels he can construe the policy to match the result he prefers.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Joshua Drake wrote:
> >> Is our backpatch policy documented? It does not appear to be in
> >> developer FAQ.
>
> > Seems we need to add it.
>
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment call
> in the past, and we've gotten along fine with that. I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

OK, agreed.
--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Asko Oja escribió:
> > Another thing I do not understand well is how people are expected to work
> in
> > 8.3 with a function based API, without hitting Skype problems.
> People are expected to use same workarounds as Skype is using. For us
> another unneccessary downtime week ago was what set us moving/thinking :).
> When you use software with limitations then you learn to live with them.
> Good thing about postgres you can do something yourself to get some of the
> limitations removed.

Make sure you do not live with patches forever, i.e. that it gets into
8.4. Otherwise it's going to be a pain for everyone.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 19 août 08 à 19:06, Tom Lane a écrit :
> Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
>> Another thing I do not understand well is how people are expected
>> to work in
>> 8.3 with a function based API, without hitting Skype problems.
>
> What we've got at this point is a submitted patch for a new feature
> that hasn't even been accepted into HEAD yet. Lobbying to get it
> back-patched is entirely inappropriate IMHO.

Well, there's a misunderstanding here. I certainly were lobbying for
considering a backpatch as I saw it as a bugfix. You told me it's a
new feature, I say ok for not backpatching, obviously.

This mail was a real attempt at learning some tips to be able to push
the functions usage as far as Skype is doing, in 8.3 release, and
avoiding the trap which has always existed in released PostgreSQL
version. This certainly was a bad attempt at it.

Now, my understanding is that rolling out new versions of functions
requires forcing dropping all current opened sessions as soon as
PostgreSQL considers you need to drop any function. I'll think about
it in next project design meetings.

Regards,
- --
dim

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Darwin)

iEYEARECAAYFAkirHlEACgkQlBXRlnbh1bk4YQCgswDS1bu+P+N7yKJvwnRAWnL3
FYkAnRZQzqbEoahShh/Qz9mnrIm1e99y
=hIBt
-----END PGP SIGNATURE-----

Le 19 août 08 à 20:47, Tom Lane a écrit :
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment
> call
> in the past, and we've gotten along fine with that. I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

Agreed.
The problem here (at least for me) was to understand why this (yet to
be reviewed) patch is about implementing a new feature and not about
bugfixing an existing one. So we're exactly in the fog around the
informal backpatch policy, and as long as we're able to continue
talking nicely about it, this seems the finest solution :)

Keep up the amazing work, regards,
--
dim

On Tue, 19 Aug 2008 14:47:13 -0400
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Joshua Drake wrote:
> >> Is our backpatch policy documented? It does not appear to be in
> >> developer FAQ.
>
> > Seems we need to add it.
>
> I'm not sure that I *want* a formal written-down backpatch policy.

Then we write a formal guideline. It really isn't fair to new developers
to not have any idea how they are going to be able to get a patch
applied to older branches. Something like:

Generally speaking we adhere to the following guideline for patches.
* Security fixes are applied to all applicable branches.
* Bugfixes are applied to all applicable branches
* Note: A patch that addresses a known limitation is generally
not backpatched
* New features are always applied to -HEAD only.

This is not a policy as much as a legend for developers to consider
before they submit their patch.

If we do this, we have the opportunity to just point to the FAQ when
there is no ambiguity. It also increases transparency of the process;
which is always a good thing.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

On Tue, Aug 19, 2008 at 02:47:13PM -0400, Tom Lane wrote:

> Whether (and how far) to backpatch has always been a best-judgment call
> in the past, and we've gotten along fine with that. I think having a
> formal policy is just likely to lead to even more complaints:

I completely agree with this. If you formalise the back-patch policy,
then it will be necessary to invent classifications for bug severity
to determine whether to back patch. This will inevitably lead to some
sort of false objectivity measure, where bugs get a "severity number"
that actually just means "we have already decided to back-patch".

A

--
Andrew Sullivan
ajs(at)commandprompt(dot)com
+1 503 667 4564 x104
http://www.commandprompt.com/

> Another thing I do not understand well is how people are expected to work in
> 8.3 with a function based API, without hitting Skype problems. I'm having a

All database-driven applications have this problem. Any time you have
a database on the backend and interface code on the front-end, you
need to keep in mind that it won't necessarily be possible to update
the two of them simultaneously, especially if you have multiple
back-ends and multiple front-ends, as you almost certainly do. Even
if PostgreSQL invalidated plans in the particular situation you're
discussing, there would still be other problems. You could add a new,
non-NULLable column to a table before updating the code that insert
into that table, or drop an old column that the code still counts on
being able to access.

I handle these problems all the time by ordering the changes
carefully. If I need to change a function API in an incompatible way,
I change the NAME of the function as well as the type signature (eg.
do_important_thing -> do_important_thing_v2). Then I change the code.
Then I remove the old function once everything that relies on it is
dead.

Maybe in your particular environment plan invalidation for functions
will solve most of the cases you care about, but I respectfully submit
that there's no substitute for good release engineering. If you don't
know exactly what functions are going to be created, modified, or
dropped on your production servers during each release before you
actually roll that release out... you probably need to improve your
internal documentation.

...Robert

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 19 août 08 à 22:03, Robert Haas a écrit :
> All database-driven applications have this problem. Any time you have
> a database on the backend and interface code on the front-end, you
> need to keep in mind that it won't necessarily be possible to update
> the two of them simultaneously, especially if you have multiple
> back-ends and multiple front-ends, as you almost certainly do. Even
> if PostgreSQL invalidated plans in the particular situation you're
> discussing, there would still be other problems. You could add a new,
> non-NULLable column to a table before updating the code that insert
> into that table, or drop an old column that the code still counts on
> being able to access.

Using functions the way Skype uses them means not issuing a single
insert, update or delete directly from your code, but calling a
function which takes care about it.
So you use PostgreSQL transactionnal DDL to roll-out new function
versions at the same time you push the schema modifications, and
commit it all in one go.

> Maybe in your particular environment plan invalidation for functions
> will solve most of the cases you care about

When the code only is a client to an SQL functions API, which
effectively replaces SQL as the way to interact between code and
database, then I believe plan invalidation at function change is the
missing piece.

> , but I respectfully submit
> that there's no substitute for good release engineering. If you don't
> know exactly what functions are going to be created, modified, or
> dropped on your production servers during each release before you
> actually roll that release out... you probably need to improve your
> internal documentation.

Agreed :)
- --
dim

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Darwin)

iEYEARECAAYFAkirK2kACgkQlBXRlnbh1bmxvQCgmowpfnZ5nFRml0mNfj2HRE+3
HJEAnR3G6Lhnb7R4+iSze8xGACwyk4D7
=of1o
-----END PGP SIGNATURE-----

On Tue, Aug 19, 2008 at 12:42:29PM -0700, Joshua Drake wrote:
> Generally speaking we adhere to the following guideline for patches.
> * Security fixes are applied to all applicable branches.
> * Bugfixes are applied to all applicable branches
> * Note: A patch that addresses a known limitation is generally
> not backpatched
> * New features are always applied to -HEAD only.
>
> This is not a policy as much as a legend for developers to consider
> before they submit their patch.

But it's meaningless. "Bugfixes are applied to all applicable
branches," is either false or trivially true. It's trivially true if
you interpret "applicable branches" to mean "the ones that get the
patch". It's false if you mean "bugfix" to mean "every patch that
fixes a bug". I can think of bugs that we have lived with in older
releases because fixing them was too risky or because the bug was so
tiny or unusual as to make the risk greater than the reward.

A formal policy that's any more detailed than what's in the FAQ today
is a solution in search of a problem.

A

--
Andrew Sullivan
ajs(at)commandprompt(dot)com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Tue, 2008-08-19 at 21:26 +0200, Dimitri Fontaine wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> Le 19 août 08 à 19:06, Tom Lane a écrit :
> > Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
> >> Another thing I do not understand well is how people are expected
> >> to work in
> >> 8.3 with a function based API, without hitting Skype problems.
> >
> > What we've got at this point is a submitted patch for a new feature
> > that hasn't even been accepted into HEAD yet. Lobbying to get it
> > back-patched is entirely inappropriate IMHO.
>
> Well, there's a misunderstanding here. I certainly were lobbying for
> considering a backpatch as I saw it as a bugfix. You told me it's a
> new feature, I say ok for not backpatching, obviously.
>
> This mail was a real attempt at learning some tips to be able to push
> the functions usage as far as Skype is doing, in 8.3 release, and
> avoiding the trap which has always existed in released PostgreSQL
> version. This certainly was a bad attempt at it.
>
> Now, my understanding is that rolling out new versions of functions
> requires forcing dropping all current opened sessions as soon as
> PostgreSQL considers you need to drop any function. I'll think about
> it in next project design meetings.

I think that another option is to manipulate pg_proc - just do a no-op
update to advance xmin for all functions that may have cached plans.

UPDATE pg_proc SET proname = proname;

then make sure that pg_proc is vacuumed often enough.

It's a bit wasteful, as it forces re-planning of all functions, but
should have similar effect than the patch.

It's also possible that updating pg_proc in bulk introduces some race
conditions which lock up the database.

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

On Tue, 19 Aug 2008 16:22:43 -0400
Andrew Sullivan <ajs(at)commandprompt(dot)com> wrote:

> A formal policy that's any more detailed than what's in the FAQ today
> is a solution in search of a problem.

Odd that the problem continues to rear its head though isn't it? This
certainly isn't the first time it has come up. I have however made my
argument. I also tried to help solve the problem. If we aren't
interested in a solution, oh well. It doesn't make my life any harder.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Dimitri Fontaine escribió:

> The problem here (at least for me) was to understand why this (yet to be
> reviewed) patch is about implementing a new feature and not about
> bugfixing an existing one. So we're exactly in the fog around the
> informal backpatch policy, and as long as we're able to continue talking
> nicely about it, this seems the finest solution :)

The actual criterion is not really "new user-visible feature" versus
"bug fix". It's more an attempt at measuring how large a potential
impact the change has. The patch I saw was introducing a whole new
message type to go through the shared invalidation queue, which is not
something to be taken lightly (consider that there are three message
types of messages currently.)

It's possible that for the Skype usage this patch introduces the
behavior they want. But for other people, perhaps this kind of
invalidation causes secondary effects that are completely unforeseen --
what if it breaks their apps and they must carry out a week's work to
fix it? What if a serious security problem is discovered tomorrow and
they can't update because we've broken backwards compatibility for them?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> The actual criterion is not really "new user-visible feature" versus
> "bug fix". It's more an attempt at measuring how large a potential
> impact the change has. The patch I saw was introducing a whole new
> message type to go through the shared invalidation queue, which is not
> something to be taken lightly (consider that there are three message
> types of messages currently.)

I hadn't read it yet, but that makes it wrong already. There's no need
for any new inval traffic --- the existing syscache inval messages on
pg_proc entries should serve fine.

More generally, if we are to try to invalidate on the strength of
pg_proc changes, what of other DDL changes? Operators, operator
classes, maybe? How about renaming a schema? I would like to see a
line drawn between things we find worth trying to track and things we
don't. If there is no such line, we're going to need a patch a lot
larger than this one.

regards, tom lane

On Tue, 2008-08-19 at 16:03 -0400, Robert Haas wrote:
> > Another thing I do not understand well is how people are expected to work in
> > 8.3 with a function based API, without hitting Skype problems. I'm having a
>
> All database-driven applications have this problem. Any time you have
> a database on the backend and interface code on the front-end, you
> need to keep in mind that it won't necessarily be possible to update
> the two of them simultaneously, especially if you have multiple
> back-ends and multiple front-ends, as you almost certainly do. Even
> if PostgreSQL invalidated plans in the particular situation you're
> discussing, there would still be other problems. You could add a new,
> non-NULLable column to a table before updating the code that insert
> into that table, or drop an old column that the code still counts on
> being able to access.
>
> I handle these problems all the time by ordering the changes
> carefully. If I need to change a function API in an incompatible way,
> I change the NAME of the function as well as the type signature (eg.
> do_important_thing -> do_important_thing_v2). Then I change the code.
> Then I remove the old function once everything that relies on it is
> dead.

Not having plan invalidation forces you to have do_important_thing_v2
for do_important_thing even with no changes in source code, just for the
fact that do_part_of_important_thing() which it calls has changed.

An example -

you have functions

A) caller1() to callerN() which includes call to called1()

B) one of these functions, say callerM() needs one more field returned
from called1(), so you either write a completely new function
called1_v2() with one more field and then update callerM() to call
called1_v2()

C) now, to get rid of called1() you have to replace called1 with
called1_v2 also in all other functions caller1() to callerN()

D) then you can drop called1()

if you missed one of callerx() functions (you can drop called1() even if
it is used, as postgreSQL does not check dependencies in functions) then
you have a non-functioning database, where even client reconnect won't
help, only putting called1() back.

If there is plan invalidation then you just change called1() to return
one more field and that's it - no juggling with C) and D) and generally
less things that can go wrong.

> Maybe in your particular environment plan invalidation for functions
> will solve most of the cases you care about, but I respectfully submit
> that there's no substitute for good release engineering.

Nope, but the amount of release engineering (and deployment-time work)
you need to do depends a lot on fragility of the system.

The more arcane and fragile the system is, the more you need to rely on
external systems and procedures to keep it working.

Imagine how much harder it would be, if there were no transactions and
you had to ensure the right ordering of all changes by release process
only. You probably would end up doing several times more work and
temporary hacks and you would still be out of luck doing _any_
nontrivial updates while the systems are running 24/7.

> If you don't
> know exactly what functions are going to be created, modified, or
> dropped on your production servers during each release before you
> actually roll that release out...

this is not about knowing this at all - this is about needing to change
less, about optimizing on work that does not need to be done if system
is smarter.

> you probably need to improve your internal documentation.

or improve the database system you use.

if you need to change less functions, you also need less documentation
about changes. if you can prove that "select a,b from f()" always
returns the same data as "select a,b from f_b2()" then you don't need to
write f_b2() at all, you just redefine f() and can also skip migrating
all callers of f() to f_v2() just to satisfy your databases quirks.

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

On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > The actual criterion is not really "new user-visible feature" versus
> > "bug fix". It's more an attempt at measuring how large a potential
> > impact the change has. The patch I saw was introducing a whole new
> > message type to go through the shared invalidation queue, which is not
> > something to be taken lightly (consider that there are three message
> > types of messages currently.)
>
> I hadn't read it yet, but that makes it wrong already. There's no need
> for any new inval traffic --- the existing syscache inval messages on
> pg_proc entries should serve fine.
>
> More generally, if we are to try to invalidate on the strength of
> pg_proc changes, what of other DDL changes? Operators, operator
> classes, maybe? How about renaming a schema? I would like to see a
> line drawn between things we find worth trying to track and things we
> don't. If there is no such line, we're going to need a patch a lot
> larger than this one.

Or maybe a simpler and smaller patch - just invalidate everything on
every schema change :)

It will have a momentary impact on performance at DDL time, but
otherways might be more robust and easier to check for errors.

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

On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > The actual criterion is not really "new user-visible feature" versus
> > "bug fix". It's more an attempt at measuring how large a potential
> > impact the change has. The patch I saw was introducing a whole new
> > message type to go through the shared invalidation queue, which is not
> > something to be taken lightly (consider that there are three message
> > types of messages currently.)
>
> I hadn't read it yet, but that makes it wrong already. There's no need
> for any new inval traffic --- the existing syscache inval messages on
> pg_proc entries should serve fine.

I have'nt looke at the patch either, but I suspect that what goes
through shared mem is the registration for invalidation, as dependent
function OIDs are only learned while compiling functions

so when f_caller() learns that it caches plan f_called() then it
registers through shared mem message its wish to invalidate this plan if
f_called() is dropped or redefined.

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

> you have functions
>
> A) caller1() to callerN() which includes call to called1()
>
> B) one of these functions, say callerM() needs one more field returned
> from called1(), so you either write a completely new function
> called1_v2() with one more field and then update callerM() to call
> called1_v2()
>
> C) now, to get rid of called1() you have to replace called1 with
> called1_v2 also in all other functions caller1() to callerN()
>
> D) then you can drop called1()

True. I complained about this same problem in the context of views -
you can add a column to a table in place but not to a view, or even a
type created via CREATE TYPE. I even went so far as to develop a
patch[1] to improve the situation, which to my sadness was not met
with wild enthusiasm.

[1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00272.php

Does it help to do CREATE OR REPLACE FUNCTION on callerX() after
dropping and recreating called1()? If so, you might want to just
recreate all of your functions every time you do a release. I wrote a
perl script that does this and it's worked pretty well for me.
Besides possibly avoiding this problem, it means that I don't really
need to worry about which functions I've modified in this release
quite as much, since I'm just going to push out the most-current
definition for all of them.

> Nope, but the amount of release engineering (and deployment-time work)
> you need to do depends a lot on fragility of the system.

Also true, but I think comparing plan invalidation to transactional
semantics is quite unfair. There's basically no amount of user code
which will compensate for the lack of an ACID-compliant database. On
the other hand, working around the lack of plan invalidation (or the
inability to add columns to views without recreating them) just
requires being careful to catch all of the stray references in your
DDL and testing thoroughly before you roll out to production, which
are good things to do anyway. That's not to say that we shouldn't
have plan invalidation, just that I don't think it's anywhere close to
the same level of broken.

...Robert

On Wed, 2008-08-20 at 00:11 +0300, Hannu Krosing wrote:
> On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> >
> > More generally, if we are to try to invalidate on the strength of
> > pg_proc changes, what of other DDL changes? Operators, operator
> > classes, maybe? How about renaming a schema? I would like to see a
> > line drawn between things we find worth trying to track and things we
> > don't. If there is no such line, we're going to need a patch a lot
> > larger than this one.
>
> Or maybe a simpler and smaller patch - just invalidate everything on
> every schema change :)
>
> It will have a momentary impact on performance at DDL time, but
> otherways might be more robust and easier to check for errors.

I think Tom's main question is the right one: how much to invalidate?

ISTM that there must be some user defined control over how this occurs.
We have cascade and restrict as options in other places.

Being able to force replanning of everything when you know its the right
thing to do sounds sensible and useful. Being able to avoid it when you
know it isn't needed also sounds sensible and useful.

It would be useful to have an impact assessment tool, so we could say
"if I made this change, how many plans would it effect?". We can't do
that because the plans aren't shared. Perhaps that is a good argument
for a shared plan cache, or at least some way of defining whether some
plans are shared, some not.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Joshua Drake <jd(at)commandprompt(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not sure that I *want* a formal written-down backpatch policy.

> Then we write a formal guideline. It really isn't fair to new developers
> to not have any idea how they are going to be able to get a patch
> applied to older branches. Something like:

> Generally speaking we adhere to the following guideline for patches.
> * Security fixes are applied to all applicable branches.
> * Bugfixes are applied to all applicable branches
> * Note: A patch that addresses a known limitation is generally
> not backpatched
> * New features are always applied to -HEAD only.

That just begs the question of what's the difference between a "bug" and
a "limitation". AFAICS, having such a policy/guideline/whatchacallit
in place wouldn't have done a single thing to stop the current flamewar,
because the people who want this thing back-patched are insisting that
it's a bug, while those who don't are saying it's a long-known
limitation.

Also, there are a whole lot more considerations in a backpatch decision
than just "is it a bug". The (estimated) risk of creating new bugs and
the extent to which the patch will change behavior that apps might be
relying on are two big reasons why we might choose not to back-patch
a bug fix.

regards, tom lane

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> If there is plan invalidation then you just change called1() to return
> one more field and that's it - no juggling with C) and D) and generally
> less things that can go wrong.

That is a pure flight of fancy. Adjusting a function's API generally
requires source-code changes on the caller side too. There might be
a few limited cases where you can avoid that, but that doesn't leave
you with much of an argument that this is a critical bug fix. It's
a corner case and little more.

FWIW, given that there will probably always be corner cases. I can see
the attraction in Simon's suggestion of providing a way to manually
issue a system-wide forced plan flush.

regards, tom lane

Tom Lane wrote:
> Also, there are a whole lot more considerations in a backpatch decision
> than just "is it a bug". The (estimated) risk of creating new bugs and
> the extent to which the patch will change behavior that apps might be
> relying on are two big reasons why we might choose not to back-patch
> a bug fix.
>
>
>

Right. And even if it is a bug the question might be "what sort of bug
is it?" We might well be prepared to take some risks with code stability
to plug security or data corruption bugs, a lot more than we would for
other sorts of bugs. Even if this were considered a bug instead of a
limitation, it doesn't come into the class of things we should be
rushing to fix in the stable branches, unless the fix is fairly obvious
and of limited impact, which is clearly not the case.

cheers

andrew

On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> > If there is plan invalidation then you just change called1() to
> > return one more field and that's it - no juggling with C) and D)
> > and generally less things that can go wrong.
>
> That is a pure flight of fancy. Adjusting a function's API
> generally requires source-code changes on the caller side too.
> There might be a few limited cases where you can avoid that, but
> that doesn't leave you with much of an argument that this is a
> critical bug fix. It's a corner case and little more.
>
> FWIW, given that there will probably always be corner cases. I can
> see the attraction in Simon's suggestion of providing a way to
> manually issue a system-wide forced plan flush.

Would that require a system-wide plan cache to implement?

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

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

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Right. And even if it is a bug the question might be "what sort of bug
> is it?" We might well be prepared to take some risks with code stability
> to plug security or data corruption bugs, a lot more than we would for
> other sorts of bugs.

As indeed we have done, and lost the bet more than once :-(. Rev 8.2.2
and siblings being the most recent example. A quick review of the
release history will show other cases where well-intentioned, seemingly
safe back-patches broke things.

Now security patches are the worst-case scenario for this, because they
typically go out with no significant public review. But even a regular
bug-fix patch doesn't get all that much testing in the back branches
before it hits the streets as a supposedly-stable update. By and large,
if we commit something into REL8_3_STABLE today, it's going to appear
in 8.3.4 with nothing more than buildfarm testing. That is a sobering
prospect, and not one that makes me want to put nontrivial patches in
there except at great need.

regards, tom lane

David Fetter <david(at)fetter(dot)org> writes:
> On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
>> FWIW, given that there will probably always be corner cases. I can
>> see the attraction in Simon's suggestion of providing a way to
>> manually issue a system-wide forced plan flush.

> Would that require a system-wide plan cache to implement?

No, just a function that can issue a suitable sinval message.

plancache.c would already respond in the desired way to a relcache inval
message with OID = 0, though likely it'll be cleaner to invent an sinval
message type specifically for the purpose.

One thing to think about is whether the flush should be truly
system-wide or just database-wide. I can see a lot more uses for the
latter than the former --- I don't think there's a reason for cached
plans to depend on any contents of the shared catalogs.

regards, tom lane

Every thread we are concerned in turns into something strange thing that is
almost entirely differnet from the original intention. First thread we
started was with the intention to discuss how we should handle the problem.
Instead of discussion it was trolled into oblivion. Then we thought so what
if no discussion we will submit a patch maybe people will understand we are
serious. Nothing relevant came up. Spent week more to refine patch into
something that looks good enough. And now we are having discusion what is
bug and what s not in this thread.

In the first message Martin asked
"There are probably a lot of details that I have overlooked. I'd be really
thankful for some constructive comments and criticism. Especially, what
needs
to be done to have this in the core. Feedback appreciated."

Can we get back to the topic?

PS: We have 10000+ functions (including lots of duplicates)
PS: We are able to be as arrogant as any of you but we can get more things
done with constructive comments.

On Wed, Aug 20, 2008 at 2:53 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
>
> Tom Lane wrote:
>
>> Also, there are a whole lot more considerations in a backpatch decision
>> than just "is it a bug". The (estimated) risk of creating new bugs and
>> the extent to which the patch will change behavior that apps might be
>> relying on are two big reasons why we might choose not to back-patch
>> a bug fix.
>>
>>
>>
>>
>
> Right. And even if it is a bug the question might be "what sort of bug is
> it?" We might well be prepared to take some risks with code stability to
> plug security or data corruption bugs, a lot more than we would for other
> sorts of bugs. Even if this were considered a bug instead of a limitation,
> it doesn't come into the class of things we should be rushing to fix in the
> stable branches, unless the fix is fairly obvious and of limited impact,
> which is clearly not the case.
>
> cheers
>
> andrew
>

On Tue, Aug 19, 2008 at 09:50:53PM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
> > On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
> >> FWIW, given that there will probably always be corner cases. I can
> >> see the attraction in Simon's suggestion of providing a way to
> >> manually issue a system-wide forced plan flush.
>
> > Would that require a system-wide plan cache to implement?
>
> No, just a function that can issue a suitable sinval message.
>
> plancache.c would already respond in the desired way to a relcache inval
> message with OID = 0, though likely it'll be cleaner to invent an sinval
> message type specifically for the purpose.
>
> One thing to think about is whether the flush should be truly
> system-wide or just database-wide. I can see a lot more uses for the
> latter than the former --- I don't think there's a reason for cached
> plans to depend on any contents of the shared catalogs.

They might during an on-line upgrade.

Zdenek?

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

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

Am 19.08.2008 um 20:47 schrieb Tom Lane:

> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Joshua Drake wrote:
>>> Is our backpatch policy documented? It does not appear to be in
>>> developer FAQ.
>
>> Seems we need to add it.
>
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment
> call
> in the past, and we've gotten along fine with that. I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

Agreeing to you and some later posters in this thread, I would not
vote for a formal policy either. But IMHO there should be a general,
informal note about backpatching in developer docs/faqs. A place where
you can point to, and a chance for new people to read about the
postgres way of handling backpatching.

Btw., how backpatching is handled here is one of the reasons I trust
my data to postgres.

Best Regards
Michael

Le mercredi 20 août 2008, Tom Lane a écrit :
> That just begs the question of what's the difference between a "bug" and
> a "limitation". AFAICS, having such a policy/guideline/whatchacallit
> in place wouldn't have done a single thing to stop the current flamewar,
> because the people who want this thing back-patched are insisting that
> it's a bug, while those who don't are saying it's a long-known
> limitation.

As a person who previously insisted it was a bug, I'd like to take the
opportunity to claim that I didn't realize this was a limitation of the
design of plan invalidation, which now seems related to DDL operations.
Realizing this earlier would have resulted in no mail at all on this thread
from here.

There's certainly a balance between -hackers readers not doing their homework
and people in the know choosing not to re-estate known things...

> Also, there are a whole lot more considerations in a backpatch decision
> than just "is it a bug". The (estimated) risk of creating new bugs and
> the extent to which the patch will change behavior that apps might be
> relying on are two big reasons why we might choose not to back-patch
> a bug fix.

And this way the project works is what leads its users not to fear minor
upgrades, which is something I (we all?) highly value.

Regards,
--
dim

On Tue, 2008-08-19 at 19:45 -0400, Tom Lane wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> > If there is plan invalidation then you just change called1() to return
> > one more field and that's it - no juggling with C) and D) and generally
> > less things that can go wrong.
>
> That is a pure flight of fancy.

Nope, this is description of real situation when you have to maintain
lots and lots of functions.

> Adjusting a function's API generally
> requires source-code changes on the caller side too.

Adding a column to table does not (even generally) require changing all
queries accessing that table, why should adding a column to functions
return type do ?

> There might be
> a few limited cases where you can avoid that, but that doesn't leave
> you with much of an argument that this is a critical bug fix. It's
> a corner case and little more.

It is a corner case if you don't have a dynamic system, evolving over
time, which relies heavily on functions .

It is a complete non-issue if you don't use functions at all.

> FWIW, given that there will probably always be corner cases. I can see
> the attraction in Simon's suggestion of providing a way to manually
> issue a system-wide forced plan flush.

That was also what I suggested as one blanket way of solving the bigger
issue you brought up, that of not knowing where to stop tracking
dependencies for plan invalidation.

My thinking was, that this trades one-time inefficiency (replanning all
stored plans) against more general but spread in time inefficiency of
current patch (sending registration messages around for each function
OID you depend on at each time you plan ).

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

The lack of plan invalidation is limitation that also has two bugs attached
to it.
I agree that full fledged patch to fix all the isssues should not be done in
8.3.
I can't agree that effort to get the bugs fixed already in 8.3 should not be
made.
I can understand that hackers here have learned to live with these bugs and
limitations but not all the users are reading these flame wars here and most
of them are not even aware of these bugs until they are hit by them.

Sql function bug is such that users probably won't even understand what hit
them and how the data got mangled.
- If there is nothing that can be done in 8.3 at least warning should be
added into the documentation. It will be just one more don't in our long
list don'ts for our developers.

ERROR: cache lookup failed for function.
- Could the plan be marked as invalid so it would fail only once so the next
call to the function would get replanned and work again. At least it would
be better than losing parts of application for indeterminate time.
- Should update pg_proc set proname = proname; be the current solution to
the problem or has someone something better to offer. We could scan released
code for DROP FUNCTION and generate plan invalidation statement as last item
of transaction releasing the code.
- Could some less dangerous looking mechanism be added to 8.3 that wouldn't
make users not used to PostgreSQL limitations gasp for air when they see the
workarounds :)
Calling the problem limitation will not make it go away. I am quite sure
that new users consider it a bug until thay are converted to perceive it as
lmitation.

No matter how many time the usage of functions in database is called corner
case it does not make it a corner case. In my experience it is quite common
practice on all the database systems i have worked with. I do get the
impression that Tom who would prefer to get all the pl's out of PostgreSQL
and live happily ever after with pure SQL standard.

On Wed, Aug 20, 2008 at 11:27 AM, Dimitri Fontaine
<dfontaine(at)hi-media(dot)com>wrote:

> Le mercredi 20 août 2008, Tom Lane a écrit :
> > That just begs the question of what's the difference between a "bug" and
> > a "limitation". AFAICS, having such a policy/guideline/whatchacallit
> > in place wouldn't have done a single thing to stop the current flamewar,
> > because the people who want this thing back-patched are insisting that
> > it's a bug, while those who don't are saying it's a long-known
> > limitation.
>
> As a person who previously insisted it was a bug, I'd like to take the
> opportunity to claim that I didn't realize this was a limitation of the
> design of plan invalidation, which now seems related to DDL operations.
> Realizing this earlier would have resulted in no mail at all on this thread
> from here.
>
> There's certainly a balance between -hackers readers not doing their
> homework
> and people in the know choosing not to re-estate known things...
>
> > Also, there are a whole lot more considerations in a backpatch decision
> > than just "is it a bug". The (estimated) risk of creating new bugs and
> > the extent to which the patch will change behavior that apps might be
> > relying on are two big reasons why we might choose not to back-patch
> > a bug fix.
>
> And this way the project works is what leads its users not to fear minor
> upgrades, which is something I (we all?) highly value.
>
> Regards,
> --
> dim
>

Asko Oja wrote:
> I do get the impression that Tom who would prefer to get all the pl's
> out of PostgreSQL and live happily ever after with pure SQL standard.
>
>

I have not seen the slightest evidence of this, and don't believe it for
a minute.

I understand some of the frustration you are feeling, but statements
like this don't help anything.

(And yes, I too have recently been bitten nastily by cached plan
problems, and want to see them fixed. I rather like Simon's suggestion
of a command or function that would clear the plan cache.)

cheers

andrew

On Wed, Aug 20, 2008 at 03:12:43PM +0300, Asko Oja wrote:

> - If there is nothing that can be done in 8.3 at least warning should be
> added into the documentation. It will be just one more don't in our long
> list don'ts for our developers.

I am in favour of that change in the 8.3 branch.

>
> ERROR: cache lookup failed for function.
> - Could the plan be marked as invalid so it would fail only once so the next
> call to the function would get replanned and work again. At least it would
> be better than losing parts of application for indeterminate time.

That seems to me to be a behaviour change, not a bug fix. I agree
that the current behaviour is pretty annoying. That is not the same
thing as "a bug" except in the loosest sense. The system works as
specified, and therefore it's not a bug. If the specification is
wrong, you need a new specification; that's a "bug fix" that is
usually pronounced "major release".

> - Could some less dangerous looking mechanism be added to 8.3 that wouldn't
> make users not used to PostgreSQL limitations gasp for air when they see the
> workarounds :)

I think it a very bad idea even to suggest that we start undertaking
things like adding mechanisms to minor releases, even with smileys at
the end of the sentence. I appreciate (possibly more than many
hackers) the limitations that are imposed on users by some of the
decisions historically taken by developers in some of the previous
major releases. But I very strongly agree with Dimitri: the
super-conservative approach to maintenance releases that this project
takes is a really big benefit to users, and is ultra important in
"mission critical" environments. Otherwise, it becomes practically
impossible to get minor releases into production. If you have to
worry about the possibility of major changes between minor versions,
you will have to treat every release as a major release.

I don't think we have sufficient commercial integration support yet
that we can follow the lead of the Linux kernel, where the system
vendor has the effective obligation to make sure your kernel actually
works.

In addition, if someone wants to develop back-patches for 8.3 that
give it new functionality otherwise planned for 8.4, I see nothing
wrong with them doing so. That's the advantage offered by having the
source. But the idea that the new functionality should be patched
back by the project because one is impatient is not on.

A

--
Andrew Sullivan
ajs(at)commandprompt(dot)com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Wed, Aug 20, 2008 at 09:16:56AM -0400, Andrew Sullivan wrote:
> On Wed, Aug 20, 2008 at 03:12:43PM +0300, Asko Oja wrote:
>
> > - If there is nothing that can be done in 8.3 at least warning should be
> > added into the documentation. It will be just one more don't in our long
> > list don'ts for our developers.
>
> I am in favour of that change in the 8.3 branch.
+1

>
> >
> > ERROR: cache lookup failed for function.
> > - Could the plan be marked as invalid so it would fail only once so the next
> > call to the function would get replanned and work again. At least it would
> > be better than losing parts of application for indeterminate time.
>
> That seems to me to be a behaviour change, not a bug fix. I agree
> that the current behaviour is pretty annoying. That is not the same
> thing as "a bug" except in the loosest sense. The system works as
> specified, and therefore it's not a bug. If the specification is
> wrong, you need a new specification; that's a "bug fix" that is
> usually pronounced "major release".
>
> > - Could some less dangerous looking mechanism be added to 8.3 that wouldn't
> > make users not used to PostgreSQL limitations gasp for air when they see the
> > workarounds :)
>
> I think it a very bad idea even to suggest that we start undertaking
> things like adding mechanisms to minor releases, even with smileys at
> the end of the sentence. I appreciate (possibly more than many
> hackers) the limitations that are imposed on users by some of the
> decisions historically taken by developers in some of the previous
> major releases. But I very strongly agree with Dimitri: the
> super-conservative approach to maintenance releases that this project
> takes is a really big benefit to users, and is ultra important in
> "mission critical" environments. Otherwise, it becomes practically
> impossible to get minor releases into production. If you have to
> worry about the possibility of major changes between minor versions,
> you will have to treat every release as a major release.
>
+10

This policy has allowed us to upgrade to new minor releases with a
minimum of testing for critical systems and basically none for non-
critical systems. We would never upgrade for minor releases if this
changes. We do not have the resources to perform full regression
tests without having a very big carrot such as the new features a
major release contains.

Cheers,
Ken

On Wed, 2008-08-20 at 08:50 -0400, Andrew Dunstan wrote:
>
> Asko Oja wrote:
> > I do get the impression that Tom who would prefer to get all the pl's
> > out of PostgreSQL and live happily ever after with pure SQL standard.
> >
> >
>
> I have not seen the slightest evidence of this, and don't believe it for
> a minute.
>
> I understand some of the frustration you are feeling, but statements
> like this don't help anything.

Claiming that problems with functions are a "corner case" seems to
indicate that kind of attitude.

OTOH, it may still be, that building really large and complex live
(evolving) databases using postgreSQL is also still a "corner case", so
any bug/limitation that manifests itself when doing DDL under 24/7
database carrying big loads is a "corner case"

> (And yes, I too have recently been bitten nastily by cached plan
> problems, and want to see them fixed. I rather like Simon's suggestion
> of a command or function that would clear the plan cache.)

I guess this would be more robust.

Mostly we use _dependencies_ to forbid stuff or to do DROP CASCADE, that
is, to enforce user-visible behaviour.

Cache invalidation seems much lighter and safer operations.

We could even add an option to do a global cache invalidation at the end
of any transaction which does DDL. That would of course need automatic
re-planning the invalidated queries and keeping some intermediate form
of query (with original * expanded to col lists, maybe something else,
basically the same as is currently saved for view's) in order to do so.

-----
Hannu

Asko Oja escribió:

> In the first message Martin asked
> "There are probably a lot of details that I have overlooked. I'd be really
> thankful for some constructive comments and criticism. Especially, what
> needs
> to be done to have this in the core. Feedback appreciated."
>
> Can we get back to the topic?

This is where the interesting questions are:

http://archives.postgresql.org/message-id/10333.1219179364%40sss.pgh.pa.us

I think the efforts to get the patch in 8.3 are wasted time. Better
concentrate on getting something good for everyone in 8.4.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Thanks for a nice replay Andrew.

So best solution for 8.3 is update pg_proc set proname = proname; whenever
you need to drop and create functions or some in house patch.

Lets get on with 8.4

Asko

On Wed, Aug 20, 2008 at 4:16 PM, Andrew Sullivan <ajs(at)commandprompt(dot)com>wrote:

> On Wed, Aug 20, 2008 at 03:12:43PM +0300, Asko Oja wrote:
>
> > - If there is nothing that can be done in 8.3 at least warning should be
> > added into the documentation. It will be just one more don't in our long
> > list don'ts for our developers.
>
> I am in favour of that change in the 8.3 branch.
>
> >
> > ERROR: cache lookup failed for function.
> > - Could the plan be marked as invalid so it would fail only once so the
> next
> > call to the function would get replanned and work again. At least it
> would
> > be better than losing parts of application for indeterminate time.
>
> That seems to me to be a behaviour change, not a bug fix. I agree
> that the current behaviour is pretty annoying. That is not the same
> thing as "a bug" except in the loosest sense. The system works as
> specified, and therefore it's not a bug. If the specification is
> wrong, you need a new specification; that's a "bug fix" that is
> usually pronounced "major release".
>
> > - Could some less dangerous looking mechanism be added to 8.3 that
> wouldn't
> > make users not used to PostgreSQL limitations gasp for air when they see
> the
> > workarounds :)
>
> I think it a very bad idea even to suggest that we start undertaking
> things like adding mechanisms to minor releases, even with smileys at
> the end of the sentence. I appreciate (possibly more than many
> hackers) the limitations that are imposed on users by some of the
> decisions historically taken by developers in some of the previous
> major releases. But I very strongly agree with Dimitri: the
> super-conservative approach to maintenance releases that this project
> takes is a really big benefit to users, and is ultra important in
> "mission critical" environments. Otherwise, it becomes practically
> impossible to get minor releases into production. If you have to
> worry about the possibility of major changes between minor versions,
> you will have to treat every release as a major release.
>
> I don't think we have sufficient commercial integration support yet
> that we can follow the lead of the Linux kernel, where the system
> vendor has the effective obligation to make sure your kernel actually
> works.
>
> In addition, if someone wants to develop back-patches for 8.3 that
> give it new functionality otherwise planned for 8.4, I see nothing
> wrong with them doing so. That's the advantage offered by having the
> source. But the idea that the new functionality should be patched
> back by the project because one is impatient is not on.
>
> A
>
> --
> Andrew Sullivan
> ajs(at)commandprompt(dot)com
> +1 503 667 4564 x104
> http://www.commandprompt.com/
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> This is where the interesting questions are:
> http://archives.postgresql.org/message-id/10333.1219179364%40sss.pgh.pa.us

Upthread, someone speculated about solving the problem by forcing plan
cache flush on *any* catalog change. I think that's probably not
acceptable from an efficiency standpoint. But maybe it'd be a good idea
to special-case common cases and fall back to a stupid flush for less
common cases, rather than invest all the work that'd be needed to track
every direct and indirect dependency of every plan. My first thought
along these lines is:

* track table dependencies exactly (important for efficiency, plus we've
got the code already)

* track function dependencies exactly (seems function definitions might
change often enough to make it important for efficiency; maybe only
track PL function dependencies??)

* brute-force flush for any other catalog change that could affect plans

However I have no hard evidence to back up drawing the line there rather
than somewhere else. Anyone have data on what sort of DDL changes are
common in their applications?

regards, tom lane

David Fetter napsal(a):
> On Tue, Aug 19, 2008 at 09:50:53PM -0400, Tom Lane wrote:
>> David Fetter <david(at)fetter(dot)org> writes:
>>> On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
>>>> FWIW, given that there will probably always be corner cases. I can
>>>> see the attraction in Simon's suggestion of providing a way to
>>>> manually issue a system-wide forced plan flush.
>>> Would that require a system-wide plan cache to implement?
>> No, just a function that can issue a suitable sinval message.
>>
>> plancache.c would already respond in the desired way to a relcache inval
>> message with OID = 0, though likely it'll be cleaner to invent an sinval
>> message type specifically for the purpose.
>>
>> One thing to think about is whether the flush should be truly
>> system-wide or just database-wide. I can see a lot more uses for the
>> latter than the former --- I don't think there's a reason for cached
>> plans to depend on any contents of the shared catalogs.
>
> They might during an on-line upgrade.
>

At this moment we have offline catalog upgrade. On-line old catalog
processing is nice idea but amount of work and impact is too high to do
it. Catalog is usually small and its offline upgrade is fast.

Zdenek

On Wed, Aug 20, 2008 at 05:03:19PM +0300, Asko Oja wrote:
>
> Lets get on with 8.4

Oh, I shoulda mentioned that, too -- I completely support doing this
work for 8.4. (I can think of more than one case where this feature
alone would be worth the upgrade.)

A

--
Andrew Sullivan
ajs(at)commandprompt(dot)com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Aug 20, 2008, at 9:18 AM, Tom Lane wrote:
> However I have no hard evidence to back up drawing the line there
> rather
> than somewhere else. Anyone have data on what sort of DDL changes are
> common in their applications?

I've worked in environments where we used stored functions
extensively and where we didn't. Table DDL is generally fairly common
in both cases, and if stored functions or views are used, it's very
common for table DDL to trigger updates in views and functions. It's
fairly common to have to update just functions to kill bugs or change
functionality. Trigger changes are a bit less frequent, and views are
probably the least frequent.
--
Decibel!, aka Jim C. Nasby, Database Architect decibel(at)decibel(dot)org
Give your computer some brain candy! www.distributed.net Team #1828

Tom Lane wrote:
> I hadn't read it yet, but that makes it wrong already. There's no need
> for any new inval traffic --- the existing syscache inval messages on
> pg_proc entries should serve fine.

Yes, creating a new message type was a bit short sighted -- attached is a patch
that uses syscache invalidation messages instead. This also adds additional
tupleId field to SharedInvalCatcacheMsg. This is used to identify the
invalidated tuple in PROC messages, for now others still pass InvalidOid.

> More generally, if we are to try to invalidate on the strength of
> pg_proc changes, what of other DDL changes? Operators, operator
> classes, maybe? How about renaming a schema? I would like to see a
> line drawn between things we find worth trying to track and things we
> don't. If there is no such line, we're going to need a patch a lot
> larger than this one.

The attached patch registers callbacks for namespace, operator and op family
catalog changes. PlanCacheCallback now takes catalog id as arg and can
take actions depending on the catalog type. Adding new catalogs is just a
matter of registering the callback in InitPlanCache. Of course, only tables
and functions have exact tracking -- other changes just invalidate all.

I'm wondering if the list of catalogs to be tracked should be fixed at all.
Maybe it would be better to call PlanCacheCallback directly on any syscache
entry invalidation? This way no catalog would be overlooked and the
cache_callback_list could be kept nice and short. PlanCacheCallback would
receive the catalog id and OID of the invalidated tuple and could then
decide whether it can do precise invalidation, flush the cache or just
skip the event. What do you think?

regards,
Martin

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> Yes, creating a new message type was a bit short sighted -- attached is a patch
> that uses syscache invalidation messages instead. This also adds additional
> tupleId field to SharedInvalCatcacheMsg. This is used to identify the
> invalidated tuple in PROC messages, for now others still pass InvalidOid.

Applied after rather heavy revision. Aside from the gripes I had
yesterday, I found out on closer inspection that the patch did things
all wrong for the case of a not-fully-planned cache item. I ended up
discarding the existing code for that and instead using the planner
machinery to extract dependencies of a parsed querytree.

regards, tom lane