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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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