On Tue, Jul 9, 2019 at 6:21 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangot...@gmail.com> writes: > > [ parse-plan-memcxt_v2.patch ] > > I got around to looking at this finally.
Thanks for the review.
> I'm not at all happy with
> the fact that it's added a plantree copy step to the only execution
> path through exec_simple_query. That's a very significant overhead,
> in many use-cases, to solve something that nobody had complained
> about for a couple of decades before now. I don't see the need for
> any added copy step anyway. The only reason you're doing it AFAICS
> is so you can release the per-statement context a bit earlier, which
> is a completely unnecessary optimization. Just wait to release it
> till the bottom of the loop.
Ah, that makes sense. I've removed the copying of plan tree and also
moved the temporary context deletion to the bottom of the loop.
> Also, creating/deleting the sub-context is in itself an added overhead
> that accomplishes exactly nothing in the typical case where there's
> not multiple statements. I thought the idea was to do that only if
> there was more than one raw parsetree (or, maybe better, do it for
> all but the last parsetree).
That makes sense too. I've made it (creation/deletion of the child
context) conditional on whether there are more than one queries to
plan.
> To show that this isn't an empty concern, I did a quick pgbench
> test. Using a single-client select-only test ("pgbench -S -T 60"
> in an -s 10 database), I got these numbers in three trials with HEAD:
>
> tps = 9593.818478 (excluding connections establishing)
> tps = 9570.189163 (excluding connections establishing)
> tps = 9596.579038 (excluding connections establishing)
>
> and these numbers after applying the patch:
>
> tps = 9411.918165 (excluding connections establishing)
> tps = 9389.279079 (excluding connections establishing)
> tps = 9409.350175 (excluding connections establishing)
>
> That's about a 2% dropoff.
With the updated patch, here are the numbers on my machine (HEAD vs patch)
HEAD:
tps = 3586.233815 (excluding connections establishing)
tps = 3569.252542 (excluding connections establishing)
tps = 3559.027733 (excluding connections establishing)
Patched:
tps = 3586.988057 (excluding connections establishing)
tps = 3585.169589 (excluding connections establishing)
tps = 3526.437968 (excluding connections establishing)
A bit noisy but not much degradation.
Attached updated patch. Thanks again.
Regards,
Amit
parse-plan-memcxt_v3.patch
Description: Binary data
