Pavel Stehule <pavel.stehule@gmail.com> writes:
> [ plpgsql-using-local-resowner-for-call-plans-20200108.patch ]
I took a quick look through this patch, just reading it without
any testing. A few thoughts:
* Instead of adding an argument to GetCachedPlan and ReleaseCachedPlan,
I think it'd be better to *replace* the useResOwner bool with
a ResourceOwner pointer, with the obvious semantics "do nothing if
it's NULL". Otherwise you have to explain what it means to pass NULL
with useResOwner = true. In any case, changing the APIs of these
functions without updating their header comments is not okay.
* I'm not really happy with adding yet another nonorthogonal variant
of SPI_execute_plan. Maybe better to do something like I did with
SPI_prepare_extended() in commit 844fe9f15, and create a struct with
all the inessential parameters so that we can make future API extensions
without inventing whole new functions. Remember also that new SPI
functions have to be documented in spi.sgml.
* Do we really need a PG_TRY in exec_toplevel_block? Not to mention
creating and destroying a ResOwner? That seems pretty expensive, and it
should be unnecessary for ordinary plpgsql functions. (I'm also unhappy
that you've utterly falsified that function's comment without doing
anything to update it.) This is really the part that needs more
work. I'm not sure that you can sell a speedup of CALL operations
if the penalty is to slow down every other plpgsql function.
* The part of the patch around exec_stmt_call is just about unreadable,
mainly because git diff seems to think that exec_stmt_call is being
changed into make_callstmt_target. Maybe it'd be less messy if you
put make_callstmt_target after exec_stmt_call.
* Looks like an extra exec_prepare_plan() call snuck into
exec_assign_expr()?
regards, tom lane
