CVSROOT:	/cvsroot
Module name:	pgsql-server
Changes by:	tgl@svr1.postgresql.org	03/10/03 16:26:49

Modified files:
	doc/src/sgml   : runtime.sgml 
	src/backend/catalog: pg_proc.c 
	src/backend/utils/misc: guc.c postgresql.conf.sample 

Log message:
	Add GUC parameter check_function_bodies to control whether validation
	of function bodies is done at CREATE FUNCTION time.  This is normally
	true but can be set false to avoid problems with forward references,
	wrong schema search path, etc.  This is just the backend patch, still
	need to adjust pg_dump to make use of it.


Fri,  3 Oct 2003 16:26:49 -0300 (ADT)
tgl@svr1.postgresql.org (Tom Lane)
pgsql-server/ oc/src/sgml/runtime.sgml rc/back ...
Tom Lane writes:

> 	Add GUC parameter check_function_bodies to control whether validation
> 	of function bodies is done at CREATE FUNCTION time.  This is normally
> 	true but can be set false to avoid problems with forward references,
> 	wrong schema search path, etc.  This is just the backend patch, still
> 	need to adjust pg_dump to make use of it.

If it's only honored by SQL functions, then it should probably be called
check_sql_function_bodies.

-- 
Peter Eisentraut   peter_e@gmx.net


Sat, 4 Oct 2003 02:15:20 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ...
Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>> Add GUC parameter check_function_bodies to control whether validation
>> of function bodies is done at CREATE FUNCTION time.

> If it's only honored by SQL functions, then it should probably be called
> check_sql_function_bodies.

I thought about that while I was making the patch, but decided that it
would be a very un-forward-looking name.  Someday we will probably have
syntax-checking validators for plpgsql, etc.

The original version of the patch actually suppressed calling the
validator altogether, but I soon realized that wouldn't do --- it
would allow you to create SQL functions with unsupported pseudotype
arguments or results, for example.  Further thought led me to decide
explicitly not to suspend checking of internal and C function
references, on the grounds that if they are broken they'd still be
broken at the completion of the restore script, and so we'd only
be losing the ability to report the problem.

So the fact that it only affects SQL functions at the moment is IMHO
just happenstance; the scope of what it does will change as we add more
validation capability.

			regards, tom lane

Fri, 03 Oct 2003 23:55:42 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ... 
Tom Lane writes:

> > If it's only honored by SQL functions, then it should probably be called
> > check_sql_function_bodies.
>
> I thought about that while I was making the patch, but decided that it
> would be a very un-forward-looking name.  Someday we will probably have
> syntax-checking validators for plpgsql, etc.

The point of this feature is to avoid failures because of forward
references in SQL code.  A syntax-checking validator in anything but
possibly plpgsql will not even look at SQL code, so a validator for
a different language will only gain pain and confusion by respecting this
parameter.  Perhaps it needs to different name altogether, along the lines
of "do not check SQL code in functions".

-- 
Peter Eisentraut   peter_e@gmx.net


Sun, 5 Oct 2003 01:01:35 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ...
Peter Eisentraut <peter_e@gmx.net> writes:
> The point of this feature is to avoid failures because of forward
> references in SQL code.  A syntax-checking validator in anything but
> possibly plpgsql will not even look at SQL code, so a validator for
> a different language will only gain pain and confusion by respecting this
> parameter.  Perhaps it needs to different name altogether, along the lines
> of "do not check SQL code in functions".

Well, I'm certainly not wedded to the name "check_function_bodies", but
I still feel that you're taking an overly narrow view of the future
functionality of this switch.  The existing reason for having the switch
is not only to avoid forward-reference problems; it also is needed to
avoid search-path problems (eg, if the function assumes "public" is in
the search path, which it won't be during pg_restore).  I think that
issues like these are likely to arise for other sorts of checks than
those on embedded SQL code.  For example, it probably wouldn't be
unreasonable for a validator for brand-X-language to try to check the
existence of referenced functions, even if those functions are called
from code that doesn't look much like SQL.

We could use "check_sql_code" or some such, and it might be an accurate
description of what the switch does today, but I think we'd end up
having to either change the switch name or add more switches of the same
kind in future to cover other checks that turn out to be not always
applicable.  I'd prefer to use a deliberately somewhat vague name to
improve the odds that we don't have to do that.

Would you like it better if the switch were called
do_all_the_right_things_for_pg_dump?  (That name is a bit facetious,
but in terms of long-term behavior that's pretty much what I'm after.)

			regards, tom lane

Sat, 04 Oct 2003 19:44:40 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ... 
Tom Lane writes:

> I think that issues like these are likely to arise for other sorts of
> checks than those on embedded SQL code.  For example, it probably
> wouldn't be unreasonable for a validator for brand-X-language to try to
> check the existence of referenced functions, even if those functions are
> called from code that doesn't look much like SQL.

Given that new languages don't tend to appear out of the blue, I think
it's reasonable to design the feature considering the languages currently
available.  We have sql, plpgsql, pltcl, plpython, plperl, plruby, plsh,
pljava, maybe something Scheme-based.  None of these languages except the
first two have anything to gain, but everything to lose, if they were
asked not to check the function body during a dump restore.  So do you
have anything more particular in mind?

> Would you like it better if the switch were called
> do_all_the_right_things_for_pg_dump?  (That name is a bit facetious, but
> in terms of long-term behavior that's pretty much what I'm after.)

Would that include altering all sorts of other behaviors, beyond the issue
of function bodies, to facilitate restoring dumps?  That might not be the
worst of ideas, but I'd rather see us improving pg_dump and keep the
relaxed behavior constrained to very well defined areas.

-- 
Peter Eisentraut   peter_e@gmx.net


Sun, 5 Oct 2003 16:26:06 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ...
Peter Eisentraut wrote:
> Given that new languages don't tend to appear out of the blue, I think
> it's reasonable to design the feature considering the languages currently
> available.  We have sql, plpgsql, pltcl, plpython, plperl, plruby, plsh,
> pljava, maybe something Scheme-based.  None of these languages except the
> first two have anything to gain, but everything to lose, if they were
> asked not to check the function body during a dump restore.  So do you
> have anything more particular in mind?
> 
> > Would you like it better if the switch were called
> > do_all_the_right_things_for_pg_dump?  (That name is a bit facetious, but
> > in terms of long-term behavior that's pretty much what I'm after.)
> 
> Would that include altering all sorts of other behaviors, beyond the issue
> of function bodies, to facilitate restoring dumps?  That might not be the
> worst of ideas, but I'd rather see us improving pg_dump and keep the
> relaxed behavior constrained to very well defined areas.

Once we put a GUC value in a dump, we have to keep that parameter valid
almost forever.  I think a general restore GUC setting will be much more
valuable in the future.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 5 Oct 2003 10:32:14 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ...
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Peter Eisentraut wrote:
>> Given that new languages don't tend to appear out of the blue, I think
>> it's reasonable to design the feature considering the languages currently
>> available.

I think that position is sufficiently rebutted by Bruce's observation:

> Once we put a GUC value in a dump, we have to keep that parameter valid
> almost forever.

Since we are inventing this thing specifically to put it in dump files,
we had better take a very long-term view of its purposes.

>> None of these languages except the
>> first two have anything to gain, but everything to lose, if they were
>> asked not to check the function body during a dump restore.

That's why the code leaves it up to the individual validator routine how
much to check or not check depending on the flag setting.  I have no
problem with an individual language deciding that it should or shouldn't
do a particular check.  I do think that we'd be foolish to make advance
judgements about what those decisions will be.

Bottom line is that I wouldn't object to changing the switch name to
something more general ("restore_validation_mode", maybe?) but I think
that changing it to something more specific would be a mistake in the
long run.

			regards, tom lane

Sun, 05 Oct 2003 11:45:40 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/ oc/src/sgml/runtime.sgml rc/back ... 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Peter Eisentraut wrote:
> >> Given that new languages don't tend to appear out of the blue, I think
> >> it's reasonable to design the feature considering the languages currently
> >> available.
> 
> I think that position is sufficiently rebutted by Bruce's observation:
> 
> > Once we put a GUC value in a dump, we have to keep that parameter valid
> > almost forever.
> 
> Since we are inventing this thing specifically to put it in dump files,
> we had better take a very long-term view of its purposes.
> 
> >> None of these languages except the
> >> first two have anything to gain, but everything to lose, if they were
> >> asked not to check the function body during a dump restore.
> 
> That's why the code leaves it up to the individual validator routine how
> much to check or not check depending on the flag setting.  I have no
> problem with an individual language deciding that it should or shouldn't
> do a particular check.  I do think that we'd be foolish to make advance
> judgements about what those decisions will be.
> 
> Bottom line is that I wouldn't object to changing the switch name to
> something more general ("restore_validation_mode", maybe?) but I think
> that changing it to something more specific would be a mistake in the
> long run.

[ Moved to hackers.]

I think we should change the "check_function_bodies" to something more
general.  I like "restore_validation_mode" because it could also be used
to disable foreign key checks which we are discussing.  An even more
general idea would be to have something like "restore_mode", and perhaps
could handle cases like allowing a larger sort_mem or other
optimizations during restore.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 6 Oct 2003 18:52:44 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Disabling function validation
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think we should change the "check_function_bodies" to something more
> general.  I like "restore_validation_mode" because it could also be used
> to disable foreign key checks which we are discussing.

I think I'd prefer to keep foreign key check disabling separate.  Or at
least make it separately selectable.  Maybe validation_mode could have
multiple levels ("off", "safe", "risky")?

			regards, tom lane

Mon, 06 Oct 2003 19:13:30 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Disabling function validation 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I think we should change the "check_function_bodies" to something more
> > general.  I like "restore_validation_mode" because it could also be used
> > to disable foreign key checks which we are discussing.
> 
> I think I'd prefer to keep foreign key check disabling separate.  Or at
> least make it separately selectable.  Maybe validation_mode could have
> multiple levels ("off", "safe", "risky")?

Makes sense.  Function bodies will be validated anyway when they are
run, while foreign keys would not.  Shame we can't disable fsync per
backend.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 6 Oct 2003 19:17:22 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Disabling function validation
Bruce Momjian writes:

> I think we should change the "check_function_bodies" to something more
> general.  I like "restore_validation_mode" because it could also be used
> to disable foreign key checks which we are discussing.  An even more
> general idea would be to have something like "restore_mode", and perhaps
> could handle cases like allowing a larger sort_mem or other
> optimizations during restore.

I also like this approach (independent of whether foreign keys should be
one of its applications).  It gives us more freedom to open and close
these types of holes when new issues arise or pg_dump improves.

-- 
Peter Eisentraut   peter_e@gmx.net


Tue, 7 Oct 2003 18:39:43 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: Disabling function validation
Peter Eisentraut wrote:
> Bruce Momjian writes:
> 
> > I think we should change the "check_function_bodies" to something more
> > general.  I like "restore_validation_mode" because it could also be used
> > to disable foreign key checks which we are discussing.  An even more
> > general idea would be to have something like "restore_mode", and perhaps
> > could handle cases like allowing a larger sort_mem or other
> > optimizations during restore.
> 
> I also like this approach (independent of whether foreign keys should be
> one of its applications).  It gives us more freedom to open and close
> these types of holes when new issues arise or pg_dump improves.

Should we add a variable that is set from the dump filew that identifies
the version of PostgreSQL that generated the dump?

	SET dumped_version = 7.3

or something like that.  We could test it in the PostgreSQL backend code
in case we have to handle something differently.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tue, 7 Oct 2003 12:58:24 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Disabling function validation

> Should we add a variable that is set from the dump filew that identifies
> the version of PostgreSQL that generated the dump?
> 
> 	SET dumped_version = 7.3

With something like that, does it have to be reissued after every 
\connect in the dump?

Chris


Wed, 08 Oct 2003 09:31:24 +0800
Christopher Kings-Lynne <chriskl@familyhealth.com.au>
Re: Disabling function validation
On Tue, 2003-10-07 at 21:31, Christopher Kings-Lynne wrote:
> > Should we add a variable that is set from the dump filew that identifies
> > the version of PostgreSQL that generated the dump?
> > 
> > 	SET dumped_version = 7.3
> 
> With something like that, does it have to be reissued after every 
> \connect in the dump?

Heh.. what \connect ;)

I believe those have been replaced with SET SESSION_AUTHORIZATION...
Tue, 07 Oct 2003 21:36:33 -0400
Rod Taylor <rbt@rbt.ca>
Re: Disabling function validation
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Should we add a variable that is set from the dump filew that identifies
> the version of PostgreSQL that generated the dump?
> 	SET dumped_version = 7.3

Is that identifying the backend version, or the pg_dump version?

Without a solid rationale for this, I'd rather not do it.

			regards, tom lane

Tue, 07 Oct 2003 23:03:13 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Disabling function validation 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Should we add a variable that is set from the dump filew that identifies
> > the version of PostgreSQL that generated the dump?
> > 	SET dumped_version = 7.3
> 
> Is that identifying the backend version, or the pg_dump version?
> 
> Without a solid rationale for this, I'd rather not do it.

Uh, not sure --- whichever one is more useful, I guess.  Just an idea.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tue, 7 Oct 2003 23:17:01 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Disabling function validation
Tom Lane wrote:

>Bruce Momjian <pgman@candle.pha.pa.us> writes:
>  
>
>>Should we add a variable that is set from the dump filew that identifies
>>the version of PostgreSQL that generated the dump?
>>	SET dumped_version = 7.3
>>    
>>
>
>Is that identifying the backend version, or the pg_dump version?
>
>Without a solid rationale for this, I'd rather not do it.
>
>  
>
Why not both?  I would also think this info could be used in pg_restore 
in some way at some point.  Even if if can't, wouldn't it be worth it 
just for debugging purposes.  Knowing for sure what the backend and 
pg_dump versions were could be helpful.


Wed, 08 Oct 2003 10:52:25 -0400
"Matthew T. O'Connor" <matthew@zeut.net>
Re: Disabling function validation
Matthew T. O'Connor wrote:
> Tom Lane wrote:
> 
> >Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >  
> >
> >>Should we add a variable that is set from the dump filew that identifies
> >>the version of PostgreSQL that generated the dump?
> >>	SET dumped_version = 7.3
> >>    
> >>
> >
> >Is that identifying the backend version, or the pg_dump version?
> >
> >Without a solid rationale for this, I'd rather not do it.
> >
> >  
> >
> Why not both?  I would also think this info could be used in pg_restore 
> in some way at some point.  Even if if can't, wouldn't it be worth it 
> just for debugging purposes.  Knowing for sure what the backend and 
> pg_dump versions were could be helpful.

My guess was that this information would allow us to change the behavior
of PostgreSQL in the future but allow older dumps to still load.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Wed, 8 Oct 2003 10:57:31 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Disabling function validation