Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve detection of implicitly-temporary views.

  2. Issue a NOTICE if a created function depends on any temp objects.

  1. Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-09-21T11:49:20Z

    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

  2. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Pavel Stehule <pavel.stehule@gmail.com> — 2025-09-21T12:33:00Z

    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



>
> Best regards, Jim
>

  3. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-09-21T13:10:22Z

    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

  4. Add notification on BEGIN ATOMIC SQL functions using temp relations

    David G. Johnston <david.g.johnston@gmail.com> — 2025-09-21T13:52:45Z

    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.

  5. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Vik Fearing <vik@postgresfriends.org> — 2025-09-21T14:43:51Z

    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

  6. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-21T14:59:38Z

    "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

  7. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Pavel Stehule <pavel.stehule@gmail.com> — 2025-09-21T15:07:50Z

    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


>
>                         regards, tom lane
>

  8. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-09-21T15:37:06Z

    
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

  9. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-09-21T16:42:28Z

    
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

  10. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Pavel Stehule <pavel.stehule@gmail.com> — 2025-09-21T17:14:36Z

    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


>
> Best regards, Jim
>
>
>

  11. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-21T17:34:15Z

    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

  12. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-09-21T22:02:21Z

    
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

  13. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-10-08T19:48:52Z

    rebased

Jim

  14. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-08T20:35:49Z

    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

  15. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-10-13T15:16:33Z

    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

  16. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-10-13T15:49:31Z

    
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

  17. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-04T20:41:25Z

    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

  18. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-05T13:44:44Z

    
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

  19. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-21T23:46:16Z

    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

  20. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-22T00:01:24Z

    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

  21. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-22T13:16:36Z

    
On 22/11/2025 00:46, Tom Lane wrote:
> 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.

Right. In my tests I didn't cover dependencies that are not within the
body, in which case the user could create the temporary object right
before calling the function, e.g.

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 37 AS val;
SELECT 1
postgres=# CREATE FUNCTION f() RETURNS integer
AS 'SELECT val FROM tmp'
LANGUAGE SQL;
CREATE FUNCTION
postgres=# \q

$ psql postgres
psql (19devel)
Type "help" for help.

postgres=# SELECT f();
ERROR:  relation "tmp" does not exist
LINE 1: SELECT val FROM tmp
                        ^
QUERY:  SELECT val FROM tmp
CONTEXT:  SQL function "f" during inlining
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# SELECT f();
 f
----
 42
(1 row)


If the dependency is not within the function body the function also
disappears (PG 18):


$ psql
psql (18.0 (Debian 18.0-1.pgdg13+3))
Type "help" for help.

postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# CREATE FUNCTION f() RETURNS tmp AS
$$ SELECT 42; $$ LANGUAGE sql;
CREATE FUNCTION
postgres=# \df f
                       List of functions
 Schema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------
 public | f    | tmp              |                     | func
(1 row)

postgres=# \q

$ psql
psql (18.0 (Debian 18.0-1.pgdg13+3))
Type "help" for help.

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


So +1 to extend it.


> 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

I briefly considered this option, but since there is no equivalent to
temporary views, I didn't explore it much. I can see the appeal of
reducing it to a NOTICE, so that we 1) don't break existing scripts and
2) still allow users to create “temporary functions.” The argument
against only showing a NOTICE was that it could create invalid output
for a concurrent pg_dump ...


/usr/local/postgres-dev/bin/pg_dump db
--
-- PostgreSQL database dump
--

\restrict 9rqkoF0g7KkNMvOhju9xXgmhVahbjBed6ffU7iBwHjp4dvFNhrDPXhpzIpaRODO

-- Dumped from database version 19devel
-- Dumped by pg_dump version 19devel

SET statement_timeout = 0;
SET lock_timeout = 0;
SET idle_in_transaction_session_timeout = 0;
SET transaction_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = on;
SELECT pg_catalog.set_config('search_path', '', false);
SET check_function_bodies = false;
SET xmloption = content;
SET client_min_messages = warning;
SET row_security = off;

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

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


ALTER FUNCTION public.f() OWNER TO jim;

--
-- PostgreSQL database dump complete
--

\unrestrict 9rqkoF0g7KkNMvOhju9xXgmhVahbjBed6ffU7iBwHjp4dvFNhrDPXhpzIpaRODO


... which would generate an error on restore:


psql:dump.sql:31: ERROR:  relation "pg_temp_5.tmp" does not exist
LINE 5:     FROM pg_temp_5.tmp;
                 ^
psql:dump.sql:34: ERROR:  function public.f() does not exist


But since it's been like that for a while and nobody has complained,
perhaps it's not worth the trouble? I can live with either option, as
long as the user doesn't get surprised by a silently dropped function.

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


If we decide to go with NOTICE, I can adjust the regression tests
accordingly later today.

Thanks for review!

Best, Jim

  22. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Bernice Southey <bernice.southey@gmail.com> — 2025-11-22T17:05:56Z

    On Sat, Nov 22, 2025 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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....
> So I changed the code to make the check all the time, and was
> rather depressed by how much that broke:
.....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.

Yikes. Can I get in early on the push-back? I think what you're saying
is you want to stop old-style SQL functions from using temp tables
that exist outside of them? IIUC this patch is to raise an error if
one tries to create a new-style function that references a temp table,
because it then disappears. I had this happen to me, and it's how I
learned about this patch. Sincere apologies for the intrusion if I'm
misunderstanding and please ignore what follows if so.

My whole system is completely dependent on functions being able to use
temp tables defined outside of them. This is how I encountered the
disappearing function when I tried making one of them new-style. I
copy batched incoming requests into a reusable multi-use session temp
table. I process these batches in functions that reference my temp
table. Temp tables are also an incredibly useful way to pass state
between SQL functions, and I'm sure I'm not the only person who does
this. I would be gutted to lose this. (Unless I'm being dumb and
there's another better way to do all this.) I could live with losing
it in old-style SQL, if it stays in PL/PgSQL. This was also how I
snuck temp tables into old-style SQL functions without syntax errors -
before this became possible in 18. I see this is back to being a
notice, and I really could be completely misunderstanding, but I'm
alarmed enough to jump in.

Thanks,
Bernice

  23. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-22T18:04:21Z

    Bernice Southey <bernice.southey@gmail.com> writes:
> Yikes. Can I get in early on the push-back? I think what you're saying
> is you want to stop old-style SQL functions from using temp tables
> that exist outside of them?

No, that's not part of the proposal.  The one case in which we'd
complain (at an error level TBD) is if a temp table is used to
define an old-style function's argument or result type, eg

	create temp table mytable (id int, data text);

	create function get_mytable() returns setof mytable as ...

This is problematic because the function will go away when mytable
does, no matter how its body is expressed.  That's always been so,
at least since we invented dependencies.

			regards, tom lane

  24. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Bernice Southey <bernice.southey@gmail.com> — 2025-11-22T18:20:27Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
> No, that's not part of the proposal.  The one case in which we'd
> complain (at an error level TBD) is if a temp table is used to
> define an old-style function's argument or result type
Thank you, I see I was being an idiot.

  25. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-22T18:37:21Z

    Jim Jones <jim.jones@uni-muenster.de> writes:
> On 22/11/2025 00:46, Tom Lane 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>".

> I briefly considered this option, but since there is no equivalent to
> temporary views, I didn't explore it much. I can see the appeal of
> reducing it to a NOTICE, so that we 1) don't break existing scripts and
> 2) still allow users to create “temporary functions.” The argument
> against only showing a NOTICE was that it could create invalid output
> for a concurrent pg_dump ...

Yeah.  That does not bother me much: the restore will still succeed
(unless you try to use --single-transaction) with just the function
missing, which is the same state of affairs as if you'd dumped after
the creating session exited.  There are plenty of other ways to
create dumps that will cause a similar level of annoyance.

After sleeping on it, I'm inclined to word the notice like

NOTICE:  function "f" will be effectively temporary
DETAIL:  It depends on temporary <object descriptor>.

This is pretty close to the way that the corresponding notice
for views is worded.  And I'm thinking seriously about a follow-up
patch that adds similar DETAIL for the view case, after jacking up
the very hoary code that produces that notice for views and driving
this infrastructure underneath it.  (That's why I changed the code
to separate the error text from the infrastructure.)

			regards, tom lane

  26. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Bernice Southey <bernice.southey@gmail.com> — 2025-11-22T19:44:19Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
>if a temp table is used to
> define an old-style function's argument or result type, eg
>         create function get_mytable() returns setof mytable as ...
> This is problematic because the function will go away when mytable
> does, no matter how its body is expressed.
I briefly did try using my temp table as the result type and was
delighted I could, but my client library wasn't. I had no idea I'd
accidentally made my functions weirdly temporary, so your notice would
have been very useful.

Thanks, Bernice

  27. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-22T23:09:03Z

    
On 22/11/2025 19:37, Tom Lane wrote:
> After sleeping on it, I'm inclined to word the notice like
> 
> NOTICE:  function "f" will be effectively temporary
> DETAIL:  It depends on temporary <object descriptor>.


I changed the error level to NOTICE:

postgres=# CREATE TEMPORARY TABLE temp_table AS SELECT 1 AS val;
SELECT 1
postgres=# CREATE FUNCTION temp_func() RETURNS int LANGUAGE sql
BEGIN ATOMIC;
  SELECT val FROM temp_table;
END;
NOTICE:  function "temp_func" will be effectively temporary
DETAIL:  It depends on temporary table temp_table.
CREATE FUNCTION

I reverted the changes in the other test cases, with the exception of
this change in returning.sql (although unrelated to this patch):


diff --git a/src/test/regress/sql/returning.sql
b/src/test/regress/sql/returning.sql
index cc99cb53f6..f1c85a9731 100644
--- a/src/test/regress/sql/returning.sql
+++ b/src/test/regress/sql/returning.sql
@@ -2,6 +2,11 @@
 -- Test INSERT/UPDATE/DELETE RETURNING
 --

+-- This script is full of poorly-chosen object names.
+-- Put them in a separate schema to avoid collisions with concurrent
scripts.
+CREATE SCHEMA returning_test;
+SET search_path = returning_test, public;
+
 -- Simple cases

 CREATE TEMP TABLE foo (f1 serial, f2 text, f3 int default 42);
@@ -407,4 +412,7 @@ BEGIN ATOMIC
 END;

 \sf foo_update
-DROP FUNCTION foo_update;
+
+-- Clean up
+RESET search_path;
+DROP SCHEMA returning_test CASCADE;
-- 


I also significantly reduced the tests I previously added to
create_function_sql.sql, leaving only tests for temporary views,
sequences, temporary functions (in pg_temp schema), and domains.

v8 attached.

Thanks!

Best, Jim

  28. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-23T20:20:20Z

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

Pushed with some more editing; for instance there were a bunch
of comments still oriented toward throwing an error.  I also
still thought the tests were pretty duplicative.

One note is that I took out

+                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),

and just let it default to ERRCODE_SUCCESSFUL_COMPLETION,
which is what happens for the longstanding message about
temp views too.  FEATURE_NOT_SUPPORTED seemed less than
apropos, and besides this is an error SQLSTATE code not
a notice/warning code.  I don't see any existing SQLSTATE
that seems on-point here; is it worth inventing one?

Before we leave the topic, here's a quick draft of a patch
to make temp-view detection use the dependency infrastructure
instead of isQueryUsingTempRelation().  That function is a
really old, crufty hack that fails to catch all dependencies.
So this can be sold on the basis of being a functional
improvement as well as adding detail to the notice messages.

It's slightly annoying that this patch means we're going to do
collectDependenciesOfExpr twice on the view body, but the place
where we'll do that again to update pg_depend is so far removed
from DefineView() that it seems unduly invasive to try to pass
down the dependency data.  I think it's not that expensive anyway;
collectDependenciesOfExpr just walks the query tree, I don't believe
there's any catalog access inside it.

I'm also not super satisfied with the wording of the message for
the matview case:

        if (query_uses_temp_object(query, &temp_object))
            ereport(ERROR,
                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                     errmsg("materialized views must not use temporary objects"),
                     errdetail("This view depends on temporary %s.",
                               getObjectDescription(&temp_object, false))));

The "It" antecedent doesn't work here because the errmsg doesn't
state the matview's name, which is also true of a bunch of nearby
errors.  I feel like not a lot of work went into those messages;
as evidenced by the fact that neither this one nor the other ones
get tested at all.  But I'm not sure I want to do the work to make
that situation better.

			regards, tom lane

  29. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-24T15:51:25Z

    
On 23/11/2025 21:20, Tom Lane wrote:
> Pushed with some more editing; for instance there were a bunch
> of comments still oriented toward throwing an error.  I also
> still thought the tests were pretty duplicative.

Thanks!

> One note is that I took out
> 
> +                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> 
> and just let it default to ERRCODE_SUCCESSFUL_COMPLETION,
> which is what happens for the longstanding message about
> temp views too.  FEATURE_NOT_SUPPORTED seemed less than
> apropos, and besides this is an error SQLSTATE code not
> a notice/warning code.  I don't see any existing SQLSTATE
> that seems on-point here; is it worth inventing one?


Something like ERRCODE_DEPENDENT_TEMP_OBJECT perhaps?

> Before we leave the topic, here's a quick draft of a patch
> to make temp-view detection use the dependency infrastructure
> instead of isQueryUsingTempRelation().  That function is a
> really old, crufty hack that fails to catch all dependencies.
> So this can be sold on the basis of being a functional
> improvement as well as adding detail to the notice messages.

+1

> It's slightly annoying that this patch means we're going to do
> collectDependenciesOfExpr twice on the view body, but the place
> where we'll do that again to update pg_depend is so far removed
> from DefineView() that it seems unduly invasive to try to pass
> down the dependency data.  I think it's not that expensive anyway;
> collectDependenciesOfExpr just walks the query tree, I don't believe
> there's any catalog access inside it.
> 
> I'm also not super satisfied with the wording of the message for
> the matview case:
> 
>         if (query_uses_temp_object(query, &temp_object))
>             ereport(ERROR,
>                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                      errmsg("materialized views must not use temporary objects"),
>                      errdetail("This view depends on temporary %s.",
>                                getObjectDescription(&temp_object, false))));
> 
> The "It" antecedent doesn't work here because the errmsg doesn't
> state the matview's name, which is also true of a bunch of nearby
> errors.  I feel like not a lot of work went into those messages;
> as evidenced by the fact that neither this one nor the other ones
> get tested at all.  But I'm not sure I want to do the work to make
> that situation better.

Since there is no test for matviews and temp objects, I guess we could
add this to create_view.sql

-- tests on materialized views depending on temporary objects
CREATE TEMPORARY SEQUENCE temp_seq;
CREATE TEMPORARY VIEW tempmv AS SELECT 1;
CREATE DOMAIN pg_temp.temp_domain AS int CHECK (VALUE > 0);
CREATE TYPE pg_temp.temp_type AS (x int);
-- should fail: temp objects not allowed with MATERIALIZED VIEW
CREATE MATERIALIZED VIEW mv_dep_temptable AS
  SELECT * FROM temp_table;
CREATE MATERIALIZED VIEW mv_dep_tempseq AS
  SELECT currval('temp_seq');
CREATE MATERIALIZED VIEW mv_dep_tempview AS
  SELECT * FROM tempmv;
CREATE MATERIALIZED VIEW mv_dep_tempdomain AS
  SELECT 1::pg_temp.temp_domain;
CREATE MATERIALIZED VIEW mv_dep_temptype AS
  SELECT (NULL::pg_temp.temp_type).x;


While playing with v9 I realised that this issue also affects
non-temporary tables when they use temporary types (in pg_temp)::

$ psql postgres
psql (19devel)
Type "help" for help.

postgres=# CREATE TYPE pg_temp.temp_type AS (x int);
CREATE TYPE
postgres=# CREATE TABLE tb_temp_type (c pg_temp.temp_type, s serial);
CREATE TABLE
postgres=# INSERT INTO tb_temp_type (c) VALUES
  (ROW(42)::pg_temp.temp_type),
  (ROW(37)::pg_temp.temp_type);
INSERT 0 2
postgres=# SELECT * FROM tb_temp_type;
  c   | s
------+---
 (42) | 1
 (37) | 2
(2 rows)

postgres=# \q

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

postgres=# SELECT * FROM tb_temp_type;
 s
---
 1
 2
(2 rows)


The column was dropped because it depended on a temporary type. IMO this
is a bit more serious than the previous case, since it silently leads to
data loss. I believe that at least a NOTICE would add some value here.
If so, I can work on a patch for that too.

Best, Jim

  30. Re: Add notification on BEGIN ATOMIC SQL functions using temp relations

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-24T21:15:10Z

    Jim Jones <jim.jones@uni-muenster.de> writes:
> While playing with v9 I realised that this issue also affects
> non-temporary tables when they use temporary types (in pg_temp)::

Right.  Really the dependency-on-temp-type scenario affects a ton of
object types: aggregates, casts, domains, operators, you name it.

> The column was dropped because it depended on a temporary type. IMO this
> is a bit more serious than the previous case, since it silently leads to
> data loss. I believe that at least a NOTICE would add some value here.

Meh.  It's been like that for a very long time, yet I can recall
approximately zero field complaints about it.  There must have
been one about the view case, sometime in the paleolithic era,
and we had one about functions which prompted the present thread.
But not other cases.

I'm disinclined to add cycles for such tests without actual field
complaints.  While I argued upthread that collectDependenciesOfExpr()
is pretty cheap, it's harder to make that case for find_temp_object(),
since that necessarily involves at least two syscache lookups per
dependency.  So if we add more of those, I'd like to be confident
that we're solving a problem that real users have.

I'll go ahead and add some test cases to the v9 patch and push it.
We already have nearly-good-enough test coverage for the view case,
but I think it'd be nice to have a test for a dependency that the
old code wouldn't have found, perhaps nextval-on-a-temp-sequence.

			regards, tom lane