Re: patch: reduce overhead of execution of CALL statement in no atomic mode from PL/pgSQL

Hi

pá 15. 1. 2021 v 22:46 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> 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.
>

done


> * 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.
>

done


> * 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.
>

I rewrote this part - now there is no new PG_TRY. local_resowner is created
only when routine is executed in non atomic mode


> * 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.
>

done


> * Looks like an extra exec_prepare_plan() call snuck into
> exec_assign_expr()?
>

fixed

I did performance tests and not the slowdown in the worst case is lower
(3-5%) only for execution in non-atomic mode. The performance of atomic
mode is the same.

Regards

Pavel




>                         regards, tom lane
>