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.
