Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-02-02 15:16:45 -0500, Tom Lane wrote:
>> I've been thinking about this for the past little while, and I believe
>> that it's probably okay to have RelationClearRelation leave the relcache
>> entry un-rebuilt, but with rd_isvalid = false so it will be rebuilt when
>> next opened. The rationale is explained in the comments in the attached
>> patch. I've checked that this fixes Noah's test case and still passes
>> the existing regression tests.
> Hm, a bit scary, but I don't see an immediate problem.
> The following comment now is violated for nailed relations
> * We assume that at the time we are called, we have at least AccessShareLock
> * on the target index. (Note: in the calls from RelationClearRelation,
> * this is legitimate because we know the rel has positive refcount.)
> but that should be easy to fix.
Ah, good point. So we should keep the refcnt test in the nailed-rel case.
> I wonder though, if we couldn't just stop doing the
> RelationReloadIndexInfo() for nailed indexes.
No; you're confusing nailed indexes with mapped indexes. There are nailed
indexes that aren't on mapped catalogs, see the load_critical_index calls
in RelationCacheInitializePhase3.
> The corresponding comment says:
> * If it's a nailed index, then we need to re-read the pg_class row to see
> * if its relfilenode changed.
This comment could use some work I guess; needs to say "nailed but not
mapped index".
> Do you plan to backpatch this? If so, even to 8.4?
I'm of two minds about that. I think this is definitely a bug, but we
do not currently have any evidence that there's an observable problem
in practice. On the other hand, we certainly get reports of
irreproducible issues from time to time, so I don't care to rule out
the theory that some of them might be caused by faulty cache reloads.
That possibility has to be balanced against the risk of introducing
new issues with this patch.
Thoughts?
(BTW, I see that the CLOBBER_CACHE_ALWAYS members of the buildfarm are
unhappy with the IsTransactionState check in RelationIdGetRelation.
Will look at that ... but it seems to be in initdb which breaks a lot
of these rules anyway, so I think it's probably not significant.)
regards, tom lane