Re: hyrax vs. RelationBuildPartitionDesc

On Wed, Mar 13, 2019 at 2:21 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2019-Mar-13, Robert Haas wrote:
> > On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > A bit, yes, but not overly so, and it's less fragile that not having
> > > such a protection.  Anything that allocates in CacheMemoryContext needs
> > > to be very careful anyway.
> >
> > True, but I think it's more fragile than either of the options I proposed.
>
> You do?  Unless I misunderstood, your options are:
>
> 1. (the patch you attached) create a temporary memory context that is
> used for everything, then at the end copy the good stuff to CacheMemCxt
> (or a sub-context thereof).  This still needs to copy.
>
> 2. create a temp memory context, do everything there, do retail freeing
> of everything we don't want, reparenting the context to CacheMemCxt.
> Hope we didn't forget to pfree anything.
>
> How is any of those superior to what I propose?

Well, *I* thought of it, so obviously it must be superior.  :-)

More seriously, your idea does seem better in some ways.  My #1
doesn't avoid the copy, but does kill the leaks.  My #2 avoids the
copy, but risks a different flavor of leakage.  Your idea also avoids
the copy and leaks in fewer cases than my #2.  In that sense, it is
the technically superior option.  However, it also requires more
memory context switches than either of my options, and I guess that
seems fragile to me in the sense that I think future code changes are
more likely to go wrong due to the complexity involved.  I might be
mistaken about that, though.

One other note is that, although the extra copy looks irksome, it's
probably not very significant from a performance point of view.  In a
non-CLOBBER_CACHE_ALWAYS build it doesn't happen frequently enough to
matter, and in a CLOBBER_CACHE_ALWAYS build everything is already so
slow that it still doesn't matter.  That's not the only consideration,
though.  Code which copies data structures might be buggy, or might
develop bugs in the future, and avoiding that copy would avoid
exposure to such bugs.  On the other hand, it's not really possible to
remove the copying without increasing the risk of leaking into the
long-lived context.

In some ways, I think this is all a natural outgrowth of the fact that
we rely on palloc() in so many places instead of forcing code to be
explicit about which memory context it intends to target.  Global
variables are considered harmful, and it's not that difficult to see
the connection between that general principle and present
difficulties.  However, I will not have time to write and debug a
patch reversing that choice between now and feature freeze.  :-)

I'm kinda inclined to go for the brute-force approach of slamming the
temporary context in there as in the patch I proposed upthread, which
should solve the immediate problem, and we can implement one of these
other ideas later if we want.  What do you think about that?

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