On Thu, Dec 10, 2015 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Well, sorta. To be honest, I think this patch is really ugly. If we
>> were going to do this then, yes, I would want to split the patch into
>> two parts along those lines. But actually I don't really want to do
>> it this way at all. It's not that I don't want the performance
>> benefits: I do. But the current code is really easy to read and
>> extremely simple, and this changes it into something that is a heck of
>> a lot harder to read and understand. I'm not sure exactly what to do
>> about that, but it seems like a problem.
>
> I haven't read the patch in any detail, but keep in mind that part of the
> API contract for ResourceOwners is that ResourceOwnerRememberFoo() cannot
> fail. Period. So any patch that makes those functions less than trivial
> is going to be really suspect from a reliability standpoint. It would be
> advisable for example that hash_any not suddenly become covered by the
> "must not fail" requirement.
Hmm. I hadn't thought about that.
I wonder if there's a simpler approach to this whole problem. What
the patch aims to do is allow resources to be freed in arbitrary order
without slowdown. But do we really need that? Resource owners are
currently optimized for freeing resources in the order opposite to the
order of allocation. I bet in this case the order in which they are
freed isn't random, but is exactly in order of allocation. If so, we
might be able to either (a) jigger things so that freeing in order of
allocation is efficient or (b) jigger things so that the resources are
released in reverse order of allocation as the resource owner code
expects. That might be simpler and less invasive than this fix.
Then again, it's somehow appealing for higher-level code not to have
to care about this...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company