Re: SIREAD lock versus ACCESS EXCLUSIVE lock

On 06.06.2011 05:13, Kevin Grittner wrote:
> "Kevin Grittner" wrote:
>
>> Maybe I should submit a patch without added complexity of the
>> scheduled cleanup and we can discuss that as a separate patch?
>
> Here's a patch which adds the missing support for DDL.

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. 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. 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.

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.

> Cleanup of
> predicate locks at commit time for transactions which ran DROP TABLE
> or TRUNCATE TABLE can be added as a separate patch. I consider those
> to be optimizations which are of dubious benefit, especially compared
> to the complexity of the extra code required.

Ok.

> In making sure that the new code for this patch was in pgindent
> format, I noticed that the ASCII art and bullet lists recently added
> to OnConflict_CheckForSerializationFailure() were mangled badly by
> pgindent, so I added the dashes to protect those and included a
> pgindent form of that function.  That should save someone some
> trouble sorting things out after the next global pgindent run.

Thanks, committed that part.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com