Alexander Pyhalov писал(а) 2025-03-28 15:22:
Tom Lane писал(а) 2025-03-14 23:52:
I spent some time today going through the actual code in this patch.
I realized that there's no longer any point in 0001: the later patches
don't move or repeatedly-call that bit of code, so it can be left
as-is.
What I think we could stand to split out, though, is the changes in
the plancache support. The new 0001 attached is just the plancache
and analyze.c changes. That could be committed separately, although
of course there's little point in pushing it till we're happy with
the rest.
Hi.
Sorry that didn't reply immediately, was busy with another tasks.
In general, this patch series is paying far too little attention to
updating existing comments that it obsoletes or adding new ones
explaining what's going on. For example, the introductory comment
for struct SQLFunctionCache still says
* Note that currently this has only the lifespan of the calling
query.
* Someday we should rewrite this code to use plancache.c to save
parse/plan
* results for longer than that.
and I wonder how much of the para after that is still accurate either.
The new structs aren't adequately documented either IMO. We now have
about three different structs that have something to do with caches
by their names, but the reader is left to guess how they fit together.
Another example is that the header comment for init_execution_state
still describes an argument list it hasn't got anymore.
I tried to clean up the comment situation in the plancache in 0001,
but I've not done much of anything to functions.c.
I've added some comments to functions.c. Modified comments you've
spotted out.
I'm fairly confused why 0002 and 0003 are separate patches, and the
commit messages for them do nothing to clarify that. It seems like
you're expecting reviewers to review a very transitory state of
affairs in 0002, and it's not clear why. Maybe the code is fine
and you just need to explain the change sequence a bit more
in the commit messages. 0002 could stand to explain the point
of the new test cases, too, especially since one of them seems to
be demonstrating the fixing of a pre-existing bug.
Also merged introducing plan cache to sql functions and session-level
plan cache support. Mostly they were separate for historic reasons.
Something is very wrong in 0004: it should not be breaking that
test case in test_extensions. It seems to me we should already
have the necessary infrastructure for that, in that the plan
ought to have a PlanInvalItem referencing public.dep_req2(),
and the ALTER SET SCHEMA that gets done on that function should
result in an invalidation. So it looks to me like that patch
has somehow rearranged things so we miss an invalidation.
I've not tried to figure out why.
Plan is invalidated in both cases (before and after the patch).
What happens here is that earlier when revalidation happened, we
couldn't find renamed function.
Now function in Query is identified by its oid, and it didn't change.
So, we still can find function by oid and rebuild cached plan.
I'm also sad that 0004
doesn't appear to include any test cases showing it doing
something right: without that, why do it at all?
I've added sample, which is fixed by this patch. It can happen that
plan is adjusted and saved. Later it's invalidated and when
revalidation happens,
we miss modifications, added by check_sql_fn_retval(). Another
interesting issue
is that cached plan is checked for being valid before function starts
executing
(like earlier planning happened before function started executing). So,
once we
discard cached plans, plan for second query in function is not
invalidated immediately,
just on the second execution. And after rebuilding plan, it becomes
wrong.
After writing some comments, looking at it once again, I've found that
one assumption is wrong - function can be discarded from cache during
its execution.
For example,
create or replace function recurse(anyelement) returns record as
$$
begin
if ($1 > 0) then
if (mod($1, 2) = 0) then
execute format($query$
create or replace function sql_recurse(anyelement) returns
record as
$q$ select recurse($1); select ($1,2); $q$ language sql;
$query$);
end if;
return sql_recurse($1 - 1);
else
return row($1, 1::int);
end if;
end;
$$ language plpgsql;
create or replace function sql_recurse(anyelement) returns record as
$$ select recurse($1); select ($1,2); $$ language sql;
create table t1 (i int);
insert into t1 values(2),(3),(4);
select sql_recurse(i) from t1;
leads to dropping cached plans while they are needed. Will look how
better to handle it.
Also one interesting note is as we don't use raw_parse_tree, it seems we
don't need plansource->parserSetup and plansource->parserSetupArg. It
seems we can avoid caching complete parse info.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
