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

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.

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