Re: Memory Accounting v11

On Tue, Jul 14, 2015 at 9:14 PM, Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> Firstly, do we really have good benchmarks and measurements? I really doubt
> that. We do have some numbers for REINDEX, where we observed 0.5-1%
> regression on noisy results from a Power machine (and we've been unable to
> reproduce that on x86). I don't think that's a representative benchmark, and
> I'd like to see more thorough measurements. And I agreed to do this, once
> Jeff comes up with a new version of the patch.
>
> Secondly, the question is whether the performance is impacted more by the
> additional instructions, or by other things - say, random padding, as was
> explained by Andrew Gierth in [1].
>
> I don't know whether that's happening in this patch, but if it is, it seems
> rather strange to use this against this patch and not the others (because
> there surely will be other patches causing similar issues).

It matters, at least to me, an awful lot where we're adding lines of
code. If you want to add modest amounts of additional code to CREATE
TABLE or CHECKPOINT or something like that, I really don't care,
because that stuff doesn't execute frequently enough to really matter
to performance even if we add a significant chunk of overhead, and
we're doing other expensive stuff at the same time, like fsync. The
same can't be said about functions like LWLockAcquire and
AllocSetAlloc that routinely show up at the top of CPU profiles.

I agree that there is room to question whether the benchmarks I did
are sufficient reason to think that the abstract concern that putting
more code into a function might make it slower translates into a
measurable drop in performance in practice. But I think when it comes
to these very hot code paths, extreme conservatism is warranted. We
can agree to disagree about that.

>> tuplesort.c does its own accounting, and TBH that seems like the right
>> thing to do here, too. The difficulty is, I think, that some
>> transition functions use an internal data type for the transition
>> state, which might not be a single palloc'd chunk. But since we can't
>> spill those aggregates to disk *anyway*, that doesn't really matter.
>> If the transition is a varlena or a fixed-length type, we can know how
>> much space it's consuming without hooking into the memory context
>> framework.
>
> I respectfully disagree. Our current inability to dump/load the state has
> little to do with how we measure consumed memory, IMHO.
>
> It's true that we do have two kinds of aggregates, depending on the nature
> of the aggregate state:
>
> (a) fixed-size state (data types passed by value, variable length types
> that do not grow once allocated, ...)
>
> (b) continuously growing state (as in string_agg/array_agg)
>
> Jeff's HashAgg patch already fixes (a) and can fix (b) once we get a
> solution for dump/load of the aggregate stats - which we need to implement
> anyway for parallel aggregate.
>
> I know there was a proposal to force all aggregates to use regular data
> types as aggregate stats, but I can't see how that could work without a
> significant performance penalty. For example array_agg() is using internal
> to pass ArrayBuildState - how do you turn that to a regular data type
> without effectively serializing/deserializing the whole array on every
> transition?

That is a good point. Tom suggested that his new expanded-format
stuff might be able to be adapted to the purpose, but I have no idea
how possible that really is.

> And even if we come up with a solution for array_agg, do we really believe
> it's possible to do for all custom aggregates? Maybe I'm missing something
> but I doubt that. ISTM designing ephemeral data structure allows tweaks that
> are impossible otherwise.
>
> What might be possible is extending the aggregate API with another custom
> function returning size of the aggregate state. So when defining an
> aggregate using 'internal' for aggregate state, you'd specify transfunc,
> finalfunc and sizefunc. That seems doable, I guess.

And infunc and outfunc. If we don't use the expanded-format stuff for
aggregates with a type-internal transition state, we won't be able to
use input and output functions to serialize and deserialize them,
either.

> I find the memory accounting as a way more elegant solution, though.

I think we're just going to have to agree to disagree on that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company