David Rowley <dgrowleyml@gmail.com> writes:
> On Wed, 5 Oct 2022 at 04:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> After studying the existing usages of MemoryContextContains, I think
>> there is a better answer, which is to just nuke them.
> I was under the impression you wanted to keep that function around in
> cassert builds for some of the guc.c changes you were making.
I would've liked to have it, but for that purpose an unreliable version
of MemoryContextContains is probably little help. In any case, as you
mention, I can do something with GetMemoryChunkContext() instead.
>> As far as I can tell, the datumCopy steps associated with aggregate
>> finalfns are basically useless. They only serve to prevent
>> returning a pointer directly to the aggregate's transition value
>> (or, perhaps, to a portion thereof). But what's wrong with that?
>> It'll last as long as we need it to. Maybe there was a reason
>> back before we required finalfns to not scribble on the transition
>> values, but those days are gone.
> Yeah, I wondered the same thing. I couldn't see a situation where the
> aggregate context would disappear.
I have a feeling that we might once have aggressively reset the
aggregate context ... but we don't anymore.
>> The one place where we actually need the conditional datumCopy is
>> with window functions, and even there I don't think we need it
>> in simple cases with only one window function. The case that is
>> hazardous is where multiple window functions are sharing a
>> WindowObject. So I'm content to optimize the single-window-function
>> case and just always copy if there's more than one. (Sadly, there
>> is no existing regression test that catches this, so I added one.)
> I was unsure what window functions might exist out in the wild, so I'd
> added some code to pass along the return type information so that any
> extensions which need to make a copy can do so. However, maybe it's
> better just to wait to see if anyone complains about that before we go
> to the trouble.
I'd originally feared that a window function might return a pointer
into the WindowObject's tuplestore, which we manipulate immediately
after the window function returns. However, AFAICS the APIs we
provide don't have any such hazard. The actual hazard is that we
might get a pointer into one of the temp slots, which are independent
storage because we tell them to copy the source tuple. (Maybe a comment
about that would be appropriate.)
> I've looked at your patches and don't see any problems. Our findings
> seem to be roughly the same. i.e the datumCopy is mostly useless.
Cool, I'll push in a little bit.
> Maybe it's worth doing;
> #define MemoryContextContains(c, p) (GetMemoryChunkContext(p) == (c))
> in memutils.h? or are we better to force extension authors to
> re-evaluate their code in case anyone is passing memory that's not
> pointing directly to a palloc'd chunk?
I think the latter. The fact that MemoryContextContains was (mostly)
safe on arbitrary pointers was an important part of its API IMO.
I'm okay with giving up that property to reduce chunk overhead,
but we'll do nobody any service by pretending we still have it.
regards, tom lane
