Re: Tightening behaviour for non-immutable behaviour in immutable functions

On Mon, 13 Jun 2022 at 16:50, Greg Stark <stark(at)mit(dot)edu> wrote:
>
> For that matter.... the gotchas are a bit .... convoluted....
>
> Perhaps we should automatically fix up the current search patch and
> attach it to functions where necessary for users instead of just
> whingeing at them....

So I reviewed my own patch and.... it was completely broken... I fixed
it to actually check the right variables.

I also implemented the other idea above of actually fixing up
search_path in proconfig for the user by default. I think this is
going to be more practical.

The problem with expecting the user to provide search_path is that
they probably aren't today so the warnings would be firing for
everything...

Providing a fixed up search_path for users would give them a smoother
upgrade process where we can give a warning only if the search_path is
changed substantively which is much less likely.

I'm still quite concerned about the performance impact of having
search_path on so many functions. It causes a cache flush which could
be pretty painful on a frequently called function such as one in an
index expression during a data load....

The other issue is that having proconfig set does prevent these
functions from being inlined which can be seen in the regression test
as seen below. I'm not sure how big a problem this is for users.
Inlining is important for many use cases I think. Maybe we can go
ahead and inline things even if they have a proconfig if it matches
the proconfig on the caller? Or maybe even check if we get the same
objects from both search_paths?

Of course this patch is still very WIP. Only one or the other function
makes sense to keep. And I'm not opposed to having a GUC to
enable/disable the enforcement or warnings. And the code itself needs
to be cleaned up with parts of it moving to guc.c and/or namespace.c.

Example of regression tests noticing that immutable functions with
proconfig set become non-inlineable:

diff -U3 /home/stark/src/postgresql/src/test/regress/expected/rangefuncs.out
/home/stark/src/postgresql/src/test/regress/results/rangefuncs.out
--- /home/stark/src/postgresql/src/test/regress/expected/rangefuncs.out
2022-01-17 12:01:54.958628564 -0500
+++ /home/stark/src/postgresql/src/test/regress/results/rangefuncs.out
2022-06-16 02:16:47.589703966 -0400
@@ -1924,14 +1924,14 @@
select * from array_to_set(array['one', 'two']) as t(f1 point,f2 text);
ERROR: return type mismatch in function declared to return record
DETAIL: Final statement returns integer instead of point at column 1.
-CONTEXT: SQL function "array_to_set" during inlining
+CONTEXT: SQL function "array_to_set" during startup
explain (verbose, costs off)
select * from array_to_set(array['one', 'two']) as t(f1
numeric(4,2),f2 text);
- QUERY PLAN
---------------------------------------------------------------
- Function Scan on pg_catalog.generate_subscripts i
- Output: i.i, ('{one,two}'::text[])[i.i]
- Function Call: generate_subscripts('{one,two}'::text[], 1)
+ QUERY PLAN
+----------------------------------------------------
+ Function Scan on public.array_to_set t
+ Output: f1, f2
+ Function Call: array_to_set('{one,two}'::text[])
(3 rows)

create temp table rngfunc(f1 int8, f2 int8);
@@ -2064,11 +2064,12 @@

explain (verbose, costs off)
select * from testrngfunc();
- QUERY PLAN
---------------------------------------------------------
- Result
- Output: 7.136178::numeric(35,6), 7.14::numeric(35,2)
-(2 rows)
+ QUERY PLAN
+-------------------------------------
+ Function Scan on public.testrngfunc
+ Output: f1, f2
+ Function Call: testrngfunc()
+(3 rows)

select * from testrngfunc();
f1 | f2

--
greg