Обсуждение: Add notification on BEGIN ATOMIC SQL functions using temp relations

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?

Best regards, Jim

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

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?

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

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

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

https://www.postgresql.org/message-id/2736425.1758475979%40sss.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
> 
> https://www.postgresql.org/message-id/2736425.1758475979%40sss.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