Add notification on BEGIN ATOMIC SQL functions using temp relations

Hi

ne 21. 9. 2025 v 13:49 odesílatel Jim Jones <jim.jones@uni-muenster.de>
napsal:

Hi,

While reviewing a patch I noticed that SQL functions defined with BEGIN
ATOMIC can reference temporary relations, and such functions are
(rightfully) dropped at session end --- but without any notification to
the user:

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1

postgres=# CREATE FUNCTION tmpval_atomic()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
CREATE FUNCTION

postgres=# \df
List of functions
Schema | Name | Result data type | Argument data types | Type
--------+---------------+------------------+---------------------+------
public | tmpval_atomic | integer | | func
(1 row)

postgres=# \q

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# \df
List of functions
Schema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------
(0 rows)

Although this behaviour is expected, it can be surprising. A NOTICE or
WARNING at CREATE FUNCTION time could save some head-scratching later.
We already have a precedent. When creating a view that depends on a
temporary relation, postgres automatically makes it a temporary view and
emits a NOTICE:

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1

postgres=# CREATE VIEW v AS SELECT * FROM tmp;
NOTICE: view "v" will be a temporary view
CREATE VIEW

postgres=# \d
List of relations
Schema | Name | Type | Owner
------------+------+-------+-------
pg_temp_74 | tmp | table | jim
pg_temp_74 | v | view | jim
(2 rows)

postgres=# \q

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# \d
Did not find any relations.

Attached a PoC that issues a WARNING if a BEGIN ATOMIC function is
created using temporary objects:

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1

postgres=# CREATE FUNCTION tmpval_atomic()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
WARNING: function defined with BEGIN ATOMIC depends on temporary
relation "tmp"
DETAIL: the function will be dropped automatically at session end.
CREATE FUNCTION

This PoC adds a parameter to check_sql_fn_statements() and
check_sql_fn_statement(), so I’m not entirely sure if that’s the best
approach. I’m also not sure whether a NOTICE would be a better fit than
a WARNING here. Feedback is welcome.

Any thoughts?

i understand your motivation, but with this warning temp tables cannot be
used in SQL function due log overhead.

Regards

Pavel

Hi Pavel

On 9/21/25 14:33, Pavel Stehule wrote:

i understand your motivation, but with this warning temp tables cannot
be used in SQL function due log overhead.

My intention was not to warn on every function call. The WARNING is only
emitted once at CREATE FUNCTION time, similar to how CREATE VIEW warns
if a view depends on a temporary relation. After creation, the function
can still be used normally without additional log overhead. Is there a
side effect I might be overlooking here?

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1

postgres=# CREATE FUNCTION tmpval_atomic()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
WARNING: function defined with BEGIN ATOMIC depends on temporary
relation "tmp"
DETAIL: the function will be dropped automatically at session end.
CREATE FUNCTION

postgres=# SELECT tmpval_atomic();
tmpval_atomic
---------------
42
(1 row)

Best regards, Jim

On Sunday, September 21, 2025, Jim Jones <jim.jones@uni-muenster.de> wrote:

Hi Pavel

On 9/21/25 14:33, Pavel Stehule wrote:

i understand your motivation, but with this warning temp tables cannot
be used in SQL function due log overhead.

My intention was not to warn on every function call. The WARNING is only
emitted once at CREATE FUNCTION time, similar to how CREATE VIEW warns
if a view depends on a temporary relation. After creation, the function
can still be used normally without additional log overhead. Is there a
side effect I might be overlooking here?

I’m surprised that this is how the system works and I agree that either we
should add this notice or remove the one for create view. Even more
because there is no syntax for directly creating a temporary function -
this is basically executing drop…cascade on the temporary relation. Which
then leads me to wonder whether selecting from a temporary view is is
detected here.

One argument for leaving the status quote, which is a decent one, is that
one can prevent the create view from emitting the notice via adding
temporary to the SQL command - there is no such ability for create function.

If added this should be a notice, not a warning, so least min messages can
be used to ignore it reasonably.

I’d rather we take this one step further and add “temp” to “create
function” and make this behave exactly identical to “create [temp] view”
than put in this half-measure where you get a notice without any way to
suppress it specifically.

David J.

On 21/09/2025 13:49, Jim Jones wrote:

WARNING: function defined with BEGIN ATOMIC depends on temporary
relation "tmp"
DETAIL: the function will be dropped automatically at session end.
CREATE FUNCTION

In addition to what others have said, this DETAIL line needs to be
contextual.  The temporary table could have been declared as ON COMMIT
DROP in which case the function will only last until transaction end.

--

Vik Fearing

"David G. Johnston" <david.g.johnston@gmail.com> writes:

I’m surprised that this is how the system works and I agree that either we
should add this notice or remove the one for create view. Even more
because there is no syntax for directly creating a temporary function -

It is possible to do

CREATE FUNCTION pg_temp.foo() ...

However, then it's not in your search path and you have to write
"pg_temp.foo" to call it, so this is far from transparent.

The fact that you can't call a temporary function without explicit
schema qualification is a security decision that is very unlikely
to get relaxed. But because of that, temp functions aren't really
first-class objects, and so I wouldn't be in favor of inventing
CREATE TEMP FUNCTION.

There's a larger issue here though: a function such as Jim shows
is a normal function, probably stored in the public schema, and
by default other sessions will be able to call it. But it will
certainly not work as desired for them, since they can't access
the creating session's temp tables. It would likely bollix
a concurrent pg_dump too. I wonder if we'd be better off to
forbid creation of such a function altogether.

regards, tom lane

ne 21. 9. 2025 v 16:59 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

I’m surprised that this is how the system works and I agree that either

we

should add this notice or remove the one for create view. Even more
because there is no syntax for directly creating a temporary function -

It is possible to do

CREATE FUNCTION pg_temp.foo() ...

However, then it's not in your search path and you have to write
"pg_temp.foo" to call it, so this is far from transparent.

The fact that you can't call a temporary function without explicit
schema qualification is a security decision that is very unlikely
to get relaxed. But because of that, temp functions aren't really
first-class objects, and so I wouldn't be in favor of inventing
CREATE TEMP FUNCTION.

There's a larger issue here though: a function such as Jim shows
is a normal function, probably stored in the public schema, and
by default other sessions will be able to call it. But it will
certainly not work as desired for them, since they can't access
the creating session's temp tables. It would likely bollix
a concurrent pg_dump too. I wonder if we'd be better off to
forbid creation of such a function altogether.

+1

Pavel

On 9/21/25 16:59, Tom Lane wrote:

There's a larger issue here though: a function such as Jim shows
is a normal function, probably stored in the public schema, and
by default other sessions will be able to call it. But it will
certainly not work as desired for them, since they can't access
the creating session's temp tables. It would likely bollix
a concurrent pg_dump too. I wonder if we'd be better off to
forbid creation of such a function altogether.

That's indeed a much larger problem. Calling it from a session silently
delivers a "wrong" result --- I was expecting an error.

== Session 1 ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=#
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# CREATE FUNCTION f()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
CREATE FUNCTION
postgres=# SELECT f();
f
----
42
(1 row)

== Session 2 (concurrent) ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# SELECT f();
f
---

(1 row)

In that light, forbidding creation of functions that depend on temporary
objects might be the safer and more consistent approach.

Best regards, Jim

On 9/21/25 17:37, Jim Jones wrote:

On 9/21/25 16:59, Tom Lane wrote:

There's a larger issue here though: a function such as Jim shows
is a normal function, probably stored in the public schema, and
by default other sessions will be able to call it. But it will
certainly not work as desired for them, since they can't access
the creating session's temp tables. It would likely bollix
a concurrent pg_dump too. I wonder if we'd be better off to
forbid creation of such a function altogether.

That's indeed a much larger problem. Calling it from a session silently
delivers a "wrong" result --- I was expecting an error.

== Session 1 ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=#
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# CREATE FUNCTION f()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
CREATE FUNCTION
postgres=# SELECT f();
f
----
42
(1 row)

== Session 2 (concurrent) ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# SELECT f();
f
---

(1 row)

In that light, forbidding creation of functions that depend on temporary
objects might be the safer and more consistent approach.

As Tom pointed out, pg_dump produces strange output in this case: it
shows a reference to a temporary table that shouldn’t even be visible:

...

--
-- Name: f(); Type: FUNCTION; Schema: public; Owner: jim
--

CREATE FUNCTION public.f() RETURNS integer
LANGUAGE sql
BEGIN ATOMIC
SELECT tmp.val
FROM pg_temp_3.tmp;
END;

...

This seems to confirm that allowing such functions leads to more than
just user confusion --- it creates broken dump/restore behaviour.

Given that, I agree forbidding functions from referencing temporary
relations is probably the right fix. If there's consensus, I can rework
my PoC in that direction.

Best regards, Jim

ne 21. 9. 2025 v 18:42 odesílatel Jim Jones <jim.jones@uni-muenster.de>
napsal:

On 9/21/25 17:37, Jim Jones wrote:

On 9/21/25 16:59, Tom Lane wrote:

There's a larger issue here though: a function such as Jim shows
is a normal function, probably stored in the public schema, and
by default other sessions will be able to call it. But it will
certainly not work as desired for them, since they can't access
the creating session's temp tables. It would likely bollix
a concurrent pg_dump too. I wonder if we'd be better off to
forbid creation of such a function altogether.

That's indeed a much larger problem. Calling it from a session silently
delivers a "wrong" result --- I was expecting an error.

== Session 1 ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=#
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# CREATE FUNCTION f()
RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM tmp;
END;
CREATE FUNCTION
postgres=# SELECT f();
f
----
42
(1 row)

== Session 2 (concurrent) ==

$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.

postgres=# SELECT f();
f
---

(1 row)

In that light, forbidding creation of functions that depend on temporary
objects might be the safer and more consistent approach.

As Tom pointed out, pg_dump produces strange output in this case: it
shows a reference to a temporary table that shouldn’t even be visible:

...

--
-- Name: f(); Type: FUNCTION; Schema: public; Owner: jim
--

CREATE FUNCTION public.f() RETURNS integer
LANGUAGE sql
BEGIN ATOMIC
SELECT tmp.val
FROM pg_temp_3.tmp;
END;

...

This seems to confirm that allowing such functions leads to more than
just user confusion --- it creates broken dump/restore behaviour.

Given that, I agree forbidding functions from referencing temporary
relations is probably the right fix. If there's consensus, I can rework
my PoC in that direction.

only when the function is not created in pg_temp schema - I think

Pavel

Jim Jones <jim.jones@uni-muenster.de> writes:

That's indeed a much larger problem. Calling it from a session silently
delivers a "wrong" result --- I was expecting an error.

Yeah, me too. See

/messages/by-id/2736425.1758475979@sss.pgh.pa.us

regards, tom lane

On 9/21/25 19:34, Tom Lane wrote:

Jim Jones <jim.jones@uni-muenster.de> writes:

That's indeed a much larger problem. Calling it from a session silently
delivers a "wrong" result --- I was expecting an error.

Yeah, me too. See

/messages/by-id/2736425.1758475979@sss.pgh.pa.us

The attached PoC now raises an ERROR instead of a WARNING.

A boolean is now computed in fmgr_sql_validator(), set to true if the
function has a prosqlbody (BEGIN ATOMIC) and is defined in a
non-temporary schema. This flag is then used to call
check_sql_fn_statements().

In check_sql_fn_statements(): if the new flag is true, it scans the
function body and raises an error if any temporary relations are found;
if it's false, it skips that check.

In returning.sql there was a query that creates a BEGIN ATOMIC function
using on a temporary table. I changed the table to permanent.

Best regards, Jim

rebased

Jim

Jim Jones <jim.jones@uni-muenster.de> writes:

[ v3-0001-Disallow-ATOMIC-functions-depending-on-temp-relat.patch ]

Got around to reading the patch finally. I don't like anything
about this implementation. It introduces yet another place that
(thinks it) knows how to find all the dependencies in a query
tree, requiring yet another scan of the function's tree, and yet
it is quite incomplete.

Also, I don't think fmgr_sql_validator is a great place to drive
this from, especially not where you put the work, because that
doesn't run if check_function_bodies is turned off.

I think the right way to make this work is to look through the
array of ObjectAddresses that ProcedureCreate builds to store
into pg_depend, because that is by definition the authoritative
info about what the function is dependent on. There's some
refactoring pain to be endured to make that happen though.
Most of the interesting-for-this-purpose dependencies are
found by recordDependencyOnExpr, which summarily writes them
out before we'd get a chance to look at them. I think what we
want to do is refactor that so that we have a function along
the lines of "add all the dependencies of this expression to
a caller-supplied ObjectAddresses struct". Then merge the
dependencies found by that function into the list of special
dependencies that ProcedureCreate has hard-wired logic for, then
de-duplicate that list, then (if not a temp function) scan the
list for dependencies on temp objects, and finally (if no error)
write it out to pg_depend using recordMultipleDependencies.
This would provide more effective de-duplication of pg_depend
entries than what ProcedureCreate is doing today, and it would
give us full coverage not just partial.

I realize that you probably cribbed this logic from
isQueryUsingTempRelation, but that is looking pretty sad too.
As a concrete example of what I'm talking about:

regression=# create temp table mytemp (f1 int);
CREATE TABLE
regression=# create view vfoo as select * from pg_class where oid = 'mytemp'::regclass;
CREATE VIEW
regression=# \c -
You are now connected to database "regression" as user "postgres".
regression=# \d vfoo
Did not find any relation named "vfoo".

because recordDependencyOnExpr knows that a regclass constant
creates a dependency, but isQueryUsingTempRelation doesn't.
So we might want to up our game for detecting views that should
be temp in a similar fashion, ie merge the test with collection
of the object's real dependencies.

regards, tom lane

Hi Tom,

Thanks for the review and thorough feedback.

On 10/8/25 22:35, Tom Lane wrote:

I think the right way to make this work is to look through the
array of ObjectAddresses that ProcedureCreate builds to store
into pg_depend, because that is by definition the authoritative
info about what the function is dependent on. There's some
refactoring pain to be endured to make that happen though.
Most of the interesting-for-this-purpose dependencies are
found by recordDependencyOnExpr, which summarily writes them
out before we'd get a chance to look at them. I think what we
want to do is refactor that so that we have a function along
the lines of "add all the dependencies of this expression to
a caller-supplied ObjectAddresses struct". Then merge the
dependencies found by that function into the list of special
dependencies that ProcedureCreate has hard-wired logic for, then
de-duplicate that list, then (if not a temp function) scan the
list for dependencies on temp objects, and finally (if no error)
write it out to pg_depend using recordMultipleDependencies.
This would provide more effective de-duplication of pg_depend
entries than what ProcedureCreate is doing today, and it would
give us full coverage not just partial.

PFA a first attempt to address your points.

0001 introduces collectDependenciesFromExpr(), which collects object
dependencies into a caller-supplied ObjectAddresses structure without
recording them immediately. recordDependencyOnExpr() now uses this
helper internally before performing the actual recording.

0002 builds on this infrastructure to collect dependencies before
applying temporary-object validation. It adopts a
"collect–then–filter–then–record" pattern for SQL function bodies in
ProcedureCreate(). After collecting, it calls filter_temp_objects() to
detect any references to temporary objects and raises an ERROR if found,
unless the function itself is being created in a temporary schema.

I realize that you probably cribbed this logic from
isQueryUsingTempRelation, but that is looking pretty sad too.
As a concrete example of what I'm talking about:

regression=# create temp table mytemp (f1 int);
CREATE TABLE
regression=# create view vfoo as select * from pg_class where oid = 'mytemp'::regclass;
CREATE VIEW
regression=# \c -
You are now connected to database "regression" as user "postgres".
regression=# \d vfoo
Did not find any relation named "vfoo".

Here a few tests:

postgres=# CREATE TEMPORARY TABLE temp_table AS SELECT 1 AS val;
SELECT 1
postgres=# CREATE TEMPORARY VIEW temp_view AS SELECT 42 AS val;
CREATE VIEW

== temp table dependency ==
CREATE FUNCTION functest_temp_dep() RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM temp_table;
END;
ERROR: cannot use temporary object "temp_table" in SQL function with
BEGIN ATOMIC
DETAIL: SQL functions with BEGIN ATOMIC cannot depend on temporary objects.

== regclass cast ==

postgres=# CREATE FUNCTION functest_temp_dep() RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT * FROM pg_class WHERE oid = 'temp_table'::regclass;
END;
ERROR: cannot use temporary object "temp_table" in SQL function with
BEGIN ATOMIC
DETAIL: SQL functions with BEGIN ATOMIC cannot depend on temporary objects.

== subquery ==
postgres=# CREATE FUNCTION functest_temp_dep_subquery() RETURNS int
LANGUAGE sql
BEGIN ATOMIC;
SELECT (SELECT COUNT(*) FROM temp_table);
END;
ERROR: cannot use temporary object "temp_table" in SQL function with
BEGIN ATOMIC
DETAIL: SQL functions with BEGIN ATOMIC cannot depend on temporary objects.

== function created in pg_temp ==
-- this should work: the function is created in a temp schema
postgres=# CREATE FUNCTION pg_temp.functest_temp_dep() RETURNS int
LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM temp_table;
END;
CREATE FUNCTION

== temp view ==
postgres=# CREATE FUNCTION functest_temp_view() RETURNS int LANGUAGE sql
BEGIN ATOMIC;
SELECT val FROM temp_view;
END;
ERROR: cannot use temporary object "temp_view" in SQL function with
BEGIN ATOMIC
DETAIL: SQL functions with BEGIN ATOMIC cannot depend on temporary objects.

Thoughts?

Best regards, Jim

On 10/13/25 17:16, Jim Jones wrote:

PFA a first attempt to address your points.

Oops... wrong files. Sorry.
PFA the correct version.

Jim

Jim Jones <jim.jones@uni-muenster.de> writes:

Oops... wrong files. Sorry.
PFA the correct version.

A few thoughts:

0001 is mostly what I had in mind, except that I do not think
collectDependenciesFromExpr should perform
eliminate_duplicate_dependencies; it should be explicitly documented
that the caller should do that before recording the dependencies.
This approach will avoid duplicate work when collecting dependencies
from multiple sources.

It seems like a lot of the changes in recordDependencyOnSingleRelExpr,
maybe all of them, were unnecessary --- why'd you find it useful to
add the "addrs" variable instead of continuing to use context.addrs?

Nitpick: it looks like the comments in 0001 are mostly written to
fit into a window that's a bit wider than 80 characters. Our standard
is to keep lines to 80 or less characters.

I'm not terribly happy with 0002. In particular, I don't like
filter_temp_objects having an explicit list of which object types
might be temp. But I don't think we need to do that, because
the objectaddress.c infrastructure already knows all about
which objects belong to schemas. I think you can just use
get_object_namespace(), and if it returns something that satisfies
OidIsValid(namespace) && isAnyTempNamespace(namespace),
then complain. (Also, use getObjectDescription() to build a
description of what you're complaining about, rather than
hard-coding that knowledge.)

The bigger issue is that it's not only the prosqlbody that
we ought to be applying this check to. For example, we should
similarly refuse cases where a temporary type is used as an
argument or result type. So I think the way that ProcedureCreate
needs to work is to collect up *all* of the dependencies that
it is creating into an ObjectAddresses list, and then de-dup
that (once), check it for temp references, and finally record it.

regards, tom lane

On 04/11/2025 21:41, Tom Lane wrote:

0001 is mostly what I had in mind, except that I do not think
collectDependenciesFromExpr should perform
eliminate_duplicate_dependencies; it should be explicitly documented
that the caller should do that before recording the dependencies.
This approach will avoid duplicate work when collecting dependencies
from multiple sources.

Done. eliminate_duplicate_dependencies() has been removed from
collectDependenciesFromExpr(). The function's comment now explicitly
documents that callers are responsible for calling
eliminate_duplicate_dependencies() before recording. In
recordDependencyOnExpr(), eliminate_duplicate_dependencies() is now
called right before recordMultipleDependencies().

It seems like a lot of the changes in recordDependencyOnSingleRelExpr,
maybe all of them, were unnecessary --- why'd you find it useful to
add the "addrs" variable instead of continuing to use context.addrs?

Yes, you're right. These changes were unnecessary leftovers from an
earlier draft. I've reverted recordDependencyOnSingleRelExpr() to use
context.addrs.

I'm not terribly happy with 0002. In particular, I don't like
filter_temp_objects having an explicit list of which object types
might be temp. But I don't think we need to do that, because
the objectaddress.c infrastructure already knows all about
which objects belong to schemas. I think you can just use
get_object_namespace(), and if it returns something that satisfies
OidIsValid(namespace) && isAnyTempNamespace(namespace),
then complain. (Also, use getObjectDescription() to build a
description of what you're complaining about, rather than
hard-coding that knowledge.)

Done. filter_temp_objects() now uses get_object_namespace() from the
objectaddress.c infrastructure to identify which objects belong to
schemas, then checks if those schemas are temporary. The error message
now uses getObjectDescription() to provide clear descriptions of the
problematic objects.

The bigger issue is that it's not only the prosqlbody that
we ought to be applying this check to. For example, we should
similarly refuse cases where a temporary type is used as an
argument or result type. So I think the way that ProcedureCreate
needs to work is to collect up *all* of the dependencies that
it is creating into an ObjectAddresses list, and then de-dup
that (once), check it for temp references, and finally record it.

The implementation now collects all function dependencies into a single
ObjectAddresses structure and then checks for temporary objects. If no
temporary object was found, it records the dependencies once. For SQL
functions with BEGIN ATOMIC bodies, filter_temp_objects() is called on
the complete set of dependencies before recording, ensuring that
temporary objects are rejected whether they appear in the function
signature or body. The dependencies are then deduplicated and recorded
once via record_object_address_dependencies().

create_function_sql.sql now contain tests for temporary objects in
function parameters, DEFAULT parameters, and RETURN data types.

PFA v6 with these changes.

Thanks for the thorough review.

Best, Jim

Jim Jones <jim.jones@uni-muenster.de> writes:

PFA v6 with these changes.

I went through this and made one big change and some cosmetic ones.

The big change is that it makes zero sense to me to apply this
restriction only to new-style SQL functions. If it's bad for an
allegedly non-temporary function to disappear at session exit,
surely it's not less bad just because it's old-style SQL or not
SQL-language at all. New-style SQL has a somewhat larger attack
surface because dependencies within the function body matter,
but the problem exists for all function languages when it comes
to argument types, result types, or default-argument expressions.

So I changed the code to make the check all the time, and was
rather depressed by how much that broke:

1. We need a couple more CommandCounterIncrement calls to handle
cases where a function is created on a just-created type.
(Without this, get_object_namespace() falls over when it tries
to look up the type.)

2. There are several more regression tests depending on the old
semantics than what you found, and even one test specifically
checking that the implicitly-temp function will go away.

Point 2 scares me quite a bit; if we've depended on this behavior
in our own tests, I wonder if there aren't plenty of end users
depending on it too. We could be in for a lot of push-back.

Although I've left the patch throwing an error (with new wording)
for now, I wonder if it'd be better to reduce the error to a NOTICE,
perhaps worded like "function f will be effectively temporary due to
its dependence on <object>". This would make the behavior more
similar to what we've done for decades with implicitly-temp views:

regression=# create temp table foo (f1 int);
CREATE TABLE
regression=# create view voo as select * from foo;
NOTICE: view "voo" will be a temporary view
CREATE VIEW

Some people might find such a notice annoying, but it's better than
failing. (I wonder if it'd be sane to make the notice come out
only if check_function_bodies is true?)

I did not touch the test cases you added to create_function_sql.sql,
but I find them quite excessive now that the patch doesn't have
any specific dependencies on object kinds. (Also, if we go with a
NOTICE and undo the changes made here to existing tests, then those
test cases would produce the NOTICE and arguably be providing
nearly enough test coverage already.)

Thoughts?

regards, tom lane

I wrote:

Although I've left the patch throwing an error (with new wording)
for now, I wonder if it'd be better to reduce the error to a NOTICE,
perhaps worded like "function f will be effectively temporary due to
its dependence on <object>".

This is, of course, pretty much what you suggested originally.
So I apologize for leading you down the garden path of
it-should-be-an-error. I'd still argue for raising an error
if we were working in a green field, but we're not.

regards, tom lane