Hi čt 6. 3. 2025 v 9:57 odesílatel Alexander Pyhalov <a.pyha...@postgrespro.ru> napsal:
> Hi. > > Tom Lane писал(а) 2025-02-27 23:40: > > Alexander Pyhalov <a.pyha...@postgrespro.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
