Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Aug 27, 2019 at 1:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1. That fix is also back-patchable, which adding fields to relcache
>> entries would not be.
> There is a fly in the ointment: REL9_6_STABLE's copy of
> RelationHasUnloggedIndex() is hardcoded to return true for hash
> indexes (see commit 2cc41acd8).
True, in 9.6 hash indexes *were* effectively unlogged, so that the code
actually did something in that branch. Given the lack of bug reports
traceable to this, I wouldn't feel too bad about leaving it alone in 9.6.
> However, I now see that there isn't a buffer content lock deadlock
> risk here after all, because we don't reach RelationHasUnloggedIndex()
> if IsCatalogRelation(rel). That reminds me of commit 4fd05bb55b4. It
> still doesn't seem like a great idea to be doing catalog access while
> holding the buffer content lock, though.
Yeah, I'm not convinced that that observation means the problem is
unreachable. Probably does make it harder to hit a deadlock, but
if you mix a few VACUUMs and untimely cache flushes into the
equation, I feel like one could still happen.
> So I think we need to leave 9.6 as is, and discuss how far back to
> back-patch the attached. It could go back to 10, but perhaps we
> should be cautious and push it to master only for now, if you agree
> with my analysis of the deadlock thing.
I'd vote for back-patching to 10. Even if there is in fact no deadlock
hazard, you've clearly demonstrated a significant performance hit that
we're taking for basically no reason.
In the larger picture, the commit this reminds me of is b04aeb0a0.
I'm wondering if we could add some assertions to the effect of
"don't initiate relcache or syscache lookups while holding a buffer
lock". It would be relatively easy to do that if we could make
the rule be "... while holding any LWLock", but I suspect that
that would break some legitimate cases.
regards, tom lane
