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

On 22/11/2025 00:46, Tom Lane wrote:
> Jim Jones <jim(dot)jones(at)uni-muenster(dot)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