Re: buildfarm: strange OOM failures on markhor (running CLOBBER_CACHE_RECURSIVELY)

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-18 14:49:10 -0400, Tom Lane wrote:
>> if (RelationHasReferenceCountZero(oldrel))
>> RelationDestroyRelation(oldrel, false);
>> else
>> elog(WARNING, "leaking still-referenced duplicate relation");

> If that happens we'd essentially have a too low reference count on the
> entry remaining in the relcache.

No, we'd have two independent entries, each with its own correct refcount.
When the refcount on the no-longer-linked-in-the-hashtable entry goes to
zero, it'd be leaked, same as it's always been. (The refcount presumably
corresponds to someone holding a direct pointer to the Relation struct,
which is what they'd use to decrement the refcount.)

> I'd consider putting an Assert() in that branch.

I'm a bit afraid to do that for a condition that the system's never tested
for at all up to now; in any case, a WARNING would be visible in production
whereas an Assert would probably do nothing. If we see no reports of this
WARNING for a release cycle or so, maybe asserting would be appropriate.

I did put in an assert to prevent this path from being taken in cases
where we don't expect that a previous entry could possibly exist.

> Perhaps it should also be only allowed for system
> relations?

One would hope those are the only ones that get opened during relcache
load ;-)

>> BTW, it strikes me that we could probably improve the runtime of the
>> CLOBBER tests noticeably if we were to nail AttrDefaultIndexId,
>> IndexIndrelidIndexId, and ConstraintRelidIndexId into cache. I see
>> no very good reason not to do that; it should help performance a bit
>> in normal cases too.

> Sounds like a good plan.

I experimented with that and didn't really see any marked improvement
in the CLOBBER_CACHE_ALWAYS runtime, so I'm less excited about it than
I was yesterday.

Still concerned about RememberToFreeTupleDescAtEOX, but that's an
independent issue really.

regards, tom lane