Re: SIREAD lock versus ACCESS EXCLUSIVE lock - Mailing list pgsql-hackers

Heikki Linnakangas  wrote:
> It makes me a bit uncomfortable to do catalog cache lookups while
> holding all the lwlocks. We've also already removed the reserved
> entry for scratch space while we do that - if a cache lookup errors
> out, we'll leave behind quite a mess. I guess it shouldn't fail,
> but it seems a bit fragile.
> 
> When TransferPredicateLocksToHeapRelation() is called for a heap,
> do we really need to transfer all the locks on its indexes too?
> When a heap is dropped or rewritten or truncated, surely all its
> indexes are also dropped or reindexed or truncated, so you'll get
> separate Transfer calls for each index anyway.
Probably.  Will confirm and simplify based on that.
> I think the logic is actually wrong at the moment: When you reindex
> a single index, DropAllPredicateLocksFromTableImpl() will transfer
> all locks belonging to any index on the same table, and any
> finer-granularity locks on the heap. It would be enough to transfer
> only locks on the index being reindexed, so you risk getting some
> unnecessary false positives.
It seemed like a good idea at the time -- a relation lock on the heap
makes any other locks on the heap or any of its indexes redundant. 
So it was an attempt at "cleaning house". Since we don't do anything
for an index request unless there is a lock on that index, it
couldn't cause false positives.  But this probably fits into the
category of premature optimizations, since the locks can't cause any
difference in when you get a serialization failure -- it's only a
matter of "taking up space".  I could revert that.
> As a bonus, if you dumb down DropAllPredicateLocksFromTableImpl()
> to only transfer locks on the given heap or index, and not any
> other indexes on the same table, you won't need
> IfIndexGetRelation() anymore, making the issue of catalog cache
> lookups moot.
Which really makes it look like simplifying here to avoid the attempt
to clean house is a good idea.  If there's a benefit to be had from
it, it should be demonstrated before attempting (in some later
release), the same as any other optimization.
> Seems weird to call SkipSplitTracking() for heaps. I guess it's
> doing the right thing, but all the comments and the name of that
> refer to indexes.
Yeah, I think it's the right thing, but the macro name should
probably be changed.  It was originally created to do the right thing
during index split operations and became useful in other cases where
a transaction was doing things to predicate locks for all
transactions.
Most of this is simplifying, plus one search-and-replace in a single
file.  I'll try to post a new patch this evening.
Thanks for the review.
-Kevin