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