Re: SQLFunctionCache and generic plans

Hi

čt 6. 3. 2025 v 9:57 odesílatel Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
napsal:

> Hi.
>
> Tom Lane писал(а) 2025-02-27 23:40:
> > Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
> >> Now sql functions plans are actually saved. The most of it is a
> >> simplified version of plpgsql plan cache. Perhaps, I've missed
> >> something.
> >
> > A couple of thoughts about v6:
> >
> > * I don't think it's okay to just summarily do this:
> >
> > /* It's stale; unlink and delete */
> > fcinfo->flinfo->fn_extra = NULL;
> > MemoryContextDelete(fcache->fcontext);
> > fcache = NULL;
> >
> > when fmgr_sql sees that the cache is stale. If we're
> > doing a nested call of a recursive SQL function, this'd be
> > cutting the legs out from under the outer recursion level.
> > plpgsql goes to some lengths to do reference-counting of
> > function cache entries, and I think you need the same here.
> >
>
> I've looked for original bug report 7881 (
>
> https://www.postgresql.org/message-id/E1U5ytP-0006E3-KB%40wrigleys.postgresql.org
> ).
> It's interesting, but it seems that plan cache is not affected by it as
> when fcinfo xid mismatches we destroy fcinfo, not plan itself (cached
> plan survives and still can be used).
>
> I thought about another case - when recursive function is invalidated
> during its execution. But I haven't found such case. If function is
> modified during function execution in another backend, the original
> backend uses old snapshot during function execution and still sees the
> old function definition. Looked at the following case -
>
> create or replace function f (int) returns setof int language sql as $$
> select i from t where t.j = $1
> union all
> select f(i) from t where t.j = $1
> $$;
>
> and changed function definition to
>
> create or replace function f (int) returns setof int language sql as $$
> select i from t where t.j = $1 and i > 0
> union all
> select f(i) from t where t.j = $1
> $$;
>
> during execution of the function. As expected, backend still sees the
> old definition and uses cached plan.
>
> > * I don't like much of anything about 0004. It's messy and it
> > gives up all the benefit of plan caching in some pretty-common
> > cases (anywhere where the user was sloppy about what data type
> > is being returned). I wonder if we couldn't solve that with
> > more finesse by applying check_sql_fn_retval() to the query tree
> > after parse analysis, but before we hand it to plancache.c?
> > This is different from what happens now because we'd be applying
> > it before not after query rewrite, but I don't think that
> > query rewrite ever changes the targetlist results. Another
> > point is that the resultTargetList output might change subtly,
> > but I don't think we care there either: I believe we only examine
> > that output for its result data types and resjunk markers.
> > (This is nonobvious and inadequately documented, but for sure we
> > couldn't try to execute that tlist --- it's never passed through
> > the planner.)
>
> I'm also not fond of the last patch. So tried to fix it in a way you've
> suggested - we call check_sql_fn_retval() before creating cached plans.
> It fixes issue with revalidation happening after modifying query
> results.
>
> One test now changes behavior. What's happening is that after moving
> extension to another schema, cached plan is invalidated. But
> revalidation
> happens and rebuilds the plan. As we've saved analyzed parse tree, not
> the raw one, we refer to public.dep_req2() not by name, but by oid. Oid
> didn't change, so cached plan is rebuilt and used. Don't know, should we
> fix it and if should, how.
> >
> > * One diff that caught my eye was
> >
> > - if (!ActiveSnapshotSet() &&
> > - plansource->raw_parse_tree &&
> > - analyze_requires_snapshot(plansource->raw_parse_tree))
> > + if (!ActiveSnapshotSet() &&
> StmtPlanRequiresRevalidation(plansource))
> >
> > Because StmtPlanRequiresRevalidation uses
> > stmt_requires_parse_analysis, this is basically throwing away the
> > distinction between stmt_requires_parse_analysis and
> > analyze_requires_snapshot. I do not think that's okay, for the
> > reasons explained in analyze_requires_snapshot. We should expend the
> > additional notational overhead to keep those things separate.
> >
> > * I'm also not thrilled by teaching StmtPlanRequiresRevalidation
> > how to do something equivalent to stmt_requires_parse_analysis for
> > Query trees. The entire point of the current division of labor is
> > for it *not* to know that, but keep the knowledge of the properties
> > of different statement types in parser/analyze.c. So it looks to me
> > like we need to add a function to parser/analyze.c that does this.
> > Not quite sure what to call it though.
> > querytree_requires_parse_analysis() would be a weird name, because
> > if it's a Query tree then it's already been through parse analysis.
> > Maybe querytree_requires_revalidation()?
>
> I've created querytree_requires_revalidation() in analyze.c and used it
> in both
> StmtPlanRequiresRevalidation() and BuildingPlanRequiresSnapshot(). Both
> are essentially the same,
> but this allows to preserve the distinction between
> stmt_requires_parse_analysis and
> analyze_requires_snapshot.
>
> I've checked old performance results:
>
> create or replace function fx2(int) returns int as $$ select 2 * $1; $$
> language sql immutable;
> create or replace function fx3 (int) returns int immutable begin atomic
> select $1 + $1; end;
> create or replace function fx4(int) returns numeric as $$ select $1 +
> $1; $$ language sql immutable;
>
> postgres=# do $$
> begin
> for i in 1..1000000 loop
> perform fx((random()*100)::int);
> end loop;
> end;
> $$;
> DO
> Time: 2896.729 ms (00:02.897)
>
> postgres=# do $$
> begin
> for i in 1..1000000 loop
> perform fx((random()*100)::int);
> end loop;
> end;
> $$;
> DO
> Time: 2943.926 ms (00:02.944)
>
> postgres=# do $$ begin
> for i in 1..1000000 loop
> perform fx3((random()*100)::int);
> end loop;
> end;
> $$;
> DO
> Time: 2941.629 ms (00:02.942)
>
> postgres=# do $$
> begin
> for i in 1..1000000 loop
> perform fx4((random()*100)::int);
> end loop;
> end;
> $$;
> DO
> Time: 2952.383 ms (00:02.952)
>
> Here we see the only distinction - fx4() is now also fast, as we use
> cached plan for it.
>

I checked these tests with

vendor_id : GenuineIntel
cpu family : 6
model : 154
model name : 12th Gen Intel(R) Core(TM) i7-12700H
stepping : 3
microcode : 0x436
cpu MHz : 400.000
cache size : 24576 KB

CREATE OR REPLACE FUNCTION fx(int)
RETURNS int AS $$
SELECT $1 + $1
$$ LANGUAGE SQL IMMUTABLE;

CREATE OR REPLACE FUNCTION fx2(int)
RETURNS int AS $$
SELECT $1 * 2
$$ LANGUAGE SQL IMMUTABLE;

create or replace function fx3 (int) returns int immutable begin atomic
select $1 + $1; end;

create or replace function fx4(int) returns numeric as $$ select $1 +
$1; $$ language sql immutable;

create or replace function fx5(int) returns int
as $$
begin
return $1 + $1;
end
$$ language plpgsql immutable;

create or replace function fx6(int) returns int
as $$
begin
return $1 + $1;
end
$$ language plpgsql volatile;

postgres=# do $$
begin
for i in 1..10000000 loop
perform fx6((random()*100)::int); -- or fx2
end loop;
end;
$$;

My results are

master f1, fx2, fx3, fx4, fx5, fx6
36233, 7297,45693,40794, 11020,10897
19446, 7315,19777,20547, 11144,10954

Still I see a small slowdown in today's fast cases, but probably it will
not be extra important - on 10M operations it is about 50ms
so in real world there will be other factors more stronger. The speedup in
the slow cases is about 50%.

Regards

Pavel

> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional