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(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>