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

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