Re: BUG #14344: string_agg(DISTINCT ..) crash

On Thu, Oct 13, 2016 at 12:59 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Hmm. That also adds a copy to the sorted-in-mem case. That's safe, but
> should we be worried about performance. Or is the extra copy so cheap that
> it doesn't matter?

I think that simply reading random locations in memory is the dominant
cost, but the exact overhead should be investigated before proceeding.

> We could easily also check for TSS_SORTEDINMEM, if that matters.

You're asking about this because TSS_SORTEDINMEM is a case that
changes (gets caller tuple copied) from 9.5 to a patched 9.6 for what
might seem like no particular reason. That's fair, but if you consider
the code and not the history, then TSS_SORTEDINMEM isn't really a
special case (plus, you can say the same thing about the equivalent
code within tuplestore.c). That is, unnecessary copying will occur
for perhaps over 99% of calls here -- this waste occurs with all
TSS_SORTEDINMEM-state calls, but also with most TSS_FINALMERGE-state
calls.

The point I'm making is that we might be better off worrying about the
general problem, by adding a tuplestore_gettupleslot()-style "copy"
boolean argument at the same time, and having some callers pass
"false" to avoid copying (when they determine no risk of
use-after-free, by not keeping the contents of a slot active across
calls to tuplesort_gettupleslot()). You indicated that you don't
really want to go there for 9.6, but maybe it's worth reconsidering
that. For example, maybe ABI breakage is avoided by making
tuplesort_gettupleslot() into a shim. Or, maybe it's okay to put it in
the release notes of 9.6.1 -- I'm not sure how manageable that is.

--
Peter Geoghegan