On 19 January 2016 at 17:14, Robert Haas <robertmh...@gmail.com> wrote:

> On Mon, Jan 18, 2016 at 12:34 PM, Robert Haas <robertmh...@gmail.com>
> wrote:
> > On Mon, Jan 18, 2016 at 12:09 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> Robert Haas <robertmh...@gmail.com> writes:
> >>> Yeah.  The API contract for an expanded object took me quite a while
> >>> to puzzle out, but it seems to be this: if somebody hands you an R/W
> >>> pointer to an expanded object, you're entitled to assume that you can
> >>> "take over" that object and mutate it however you like.  But the
> >>> object might be in some other memory context, so you have to move it
> >>> into your own memory context.
> >>
> >> Only if you intend to keep it --- for example, a function that is
> mutating
> >> and returning an object isn't required to move it somewhere else, if the
> >> input is R/W, and I think it generally shouldn't.
> >>
> >> In the context here, I'd think it is the responsibility of nodeAgg.c
> >> not individual datatype functions to make sure that expanded objects
> >> live where it wants them to.
> >
> > That's how I did it in my prototype, but the problem with that is that
> > spinning up a memory context for every group sucks when there are many
> > groups with only a small number of elements each - hence the 50%
> > regression that David Rowley observed.  If we're going to use expanded
> > objects here, which seems like a good idea in principle, that's going
> > to have to be fixed somehow.  We're flogging the heck out of malloc by
> > repeatedly creating a context, doing one or two allocations in it, and
> > then destroying the context.
> >
> > I think that, in general, the memory context machinery wasn't really
> > designed to manage lots of small contexts.  The overhead of spinning
> > up a new context for just a few allocations is substantial.  That
> > matters in some other situations too, I think - I've commonly seen
> > AllocSetContextCreate taking several percent  of runtime in profiles.
> > But here it's much exacerbated.  I'm not sure whether it's better to
> > attack that problem at the root and try to make AllocSetContextCreate
> > cheaper, or whether we should try to figure out some change to the
> > expanded-object machinery to address the issue.
> Here is a patch that helps a good deal.  I changed things so that when
> we create a context, we always allocate at least 1kB.  If that's more
> than we need for the node itself and the name, then we use the rest of
> the space as a sort of keeper block.  I think there's more that can be
> done to improve this, but I'm wimping out for now because it's late
> here.
> I suspect something like this is a good idea even if we don't end up
> using it for aggregate transition functions.

Thanks for writing this up, but I think the key issue is not addressed,
which is the memory context per aggregate group just being a performance
killer. I ran the test query again with the attached patch and the hashagg
query still took 300 seconds on my VM with 4GB ram.

 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply via email to