Hi,
At Wed, 10 Jul 2019 16:35:18 +0900, Amit Langote <[email protected]>
wrote in <ca+hiwqfco4c8tdqmxcdnzyad43a81caapylj6ceh8h3p0tp...@mail.gmail.com>
> On Tue, Jul 9, 2019 at 6:21 AM Tom Lane <[email protected]> wrote:
> >
> > Amit Langote <[email protected]> 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.
- * Switch to appropriate context for constructing querytrees (again,
- * these must outlive the execution context).
+ * Switch to appropriate context for constructing query and plan trees
+ * (again, these must outlive the execution context). Normally, it's
+ * MessageContext, but if there are more queries to plan, we use a
+ * temporary child context that will be reset after executing this
+ * query. We avoid that overhead of setting up a separate context
+ * for the common case of having just a single query.
Might be stupid, but I feel uneasy that "usually it must live in
MessageContxt, but not necessarily if there is succeeding
query".. *I* need more explanation why it is safe to use that
short-lived context.
> > 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
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
