Re: SQLFunctionCache and generic plans

I spent some time reading and reworking this code, and have
arrived at a patch set that I'm pretty happy with. I'm not
sure it's quite committable but it's close:

0001: Same as in v8, extend plancache.c to allow caching
starting from a Query.

0002: Add a post-rewrite callback hook in plancache.c. There
is no chance of getting check_sql_fn_retval to work correctly
without that in the raw-parsetree case: we can't apply the
transformation on what goes into the plancache, and we have
to be able to re-apply it if plancache regenerates the plan
from the raw parse trees.

0003: As I mentioned yesterday, I think we should use the same
cache management logic that plpgsql does, and the best way for
that to happen is to share code. So this invents a new module
"funccache" that extracts the logic plpgsql was using. I did
have to add one feature that plpgsql doesn't have, to allow
part of the cache key to be the output rowtype. Otherwise
cases like this one from rangefuncs.sql won't work:

select * from array_to_set(array['one', 'two']) as t(f1 int,f2 text);
select * from array_to_set(array['one', 'two']) as t(f1 numeric(4,2),f2 text);

These have to have separate cached plans because check_sql_fn_retval
will modify the plans differently.

0004: Restructure check_sql_fn_retval so that we can use it in the
callback hook envisioned in 0002. There's an edge-case semantics
change as explained in the commit message; perhaps that will be
controversial?

0005: This extracts the RLS test case you had and commits it
with the old non-failing behavior, just so that we can see that
the new code does it differently. (I didn't adopt your test
from rules.sql, because AFAICS it works the same with or without
this patch set. What was the point of that one again?)

0006: The guts of the patch. I couldn't break this down any
further.

One big difference from what you had is that there is only one path
of control: we always use the plan cache. The hack you had to not
use it for triggers was only needed because you didn't include the
right cache key items to distinguish different trigger usages, but
the code coming from plpgsql has that right.

Also, the memory management is done a bit differently. The
"fcontext" memory context holding the SQLFunctionCache struct is
now discarded at the end of each execution of the SQL function,
which considerably alleviates worries about leaking memory there.
I invented a small "SQLFunctionLink" struct that is what fn_extra
points at, and it survives as long as the FmgrInfo does, so that's
what saves us from redoing hash key computations in most cases.

I also moved some code around -- notably, init_execution_state now
builds plans for only one CachedPlanState at a time, and we don't
try to construct the output JunkFilter until we plan the last
CachedPlanState. Because of this change, there's no longer a List
of execution_state sublists, but only a sublist matching the
current CachedPlan. We track where we are in the function using
an integer counter of the CachedPlanStates instead.

There's more stuff that could be done, but I feel that all of this
could be left for later:

* I really wanted to do what I mentioned upthread and change things
so we don't even parse the later queries until we've executed the
ones before that. However that seems to be a bit of a mess to
make happen, and the patch is large/complex enough already.

* The execution_state sublist business seems quite vestigial now:
we could probably drop it in favor of one set of those fields and
a counter. But that would involve a lot of notational churn,
much of it in code that doesn't need changes otherwise, and in
the end it would not buy much except removing a tiny amount of
transient space usage. Maybe some other day.

* There's some duplication of effort between cache key computation
and the callers, particularly that for SQL functions we end up
doing get_call_result_type() twice during the initial call.
This could probably be fixed with more refactoring, but it's not
really expensive enough to get too excited about.

I redid Pavel's tests from [1], and got these results in
non-assert builds:

master v10 patch
fx: 50077.251 ms 21221.104 ms
fx2: 8578.874 ms 8576.935 ms
fx3: 66331.186 ms 21173.215 ms
fx4: 56233.003 ms 22757.320 ms
fx5: 13248.177 ms 12370.693 ms
fx6: 13103.840 ms 12245.266 ms

We get substantial wins on all of fx, fx3, fx4. fx2 is the
case that gets inlined and never reaches functions.c, so the
lack of change there is expected. What I found odd is that
I saw a small speedup (~6%) on fx5 and fx6; those functions
are in plpgsql so they really shouldn't change either.
The only thing I can think of is that I made the hash key
computation a tiny bit faster by omitting unused argtypes[]
entries. That does avoid hashing several hundred bytes
typically, but it's hard to believe it'd amount to any
visible savings overall.

Anyway, PFA v10.

regards, tom lane

[1] https://www.postgresql.org/message-id/CAFj8pRDWDeF2cC%2BpCjLHJno7KnK5kdtjYN-f933RHS7UneArFw%40mail.gmail.com