Re: Reducing the chunk header sizes on all memory context types

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.

> 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.

> The same goes for aggregate serialfns --- although there, I can't
> avoid the feeling that the datumCopy step was just cargo-culted in.
> I don't think there can exist a serialfn that doesn't return a
> freshly-palloced bytea.

Most likely. I probably copied that as I wouldn't have understood why
we did any copying when calling the finalfn. I still don't understand
why. Seems there's no good reason if we're both in favour of removing
it.

> 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'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.
However, you've noticed the requirement to datumCopy when there are
multiple window functions using the same window along with yours
containing the call to MakeExpandedObjectReadOnly() where I missed
that.

This should also slightly improve the performance of LEAD and LAG with
byref types, which seems like a good side-effect.

I guess the commit message for 0002 should mention that for pointers
to allocated chunks that GetMemoryChunkContext() can be used in place
of MemoryContextContains(). I did see that PostGIS does use
MemoryContextContains(), though I didn't look at their code to figure
out if they're always passing it a pointer to an allocated chunk.
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?

David