On Tue, Mar 8, 2016 at 11:20 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> I have come up with this patch..
>
> If this approach looks fine then I will prepare final patch (more comments,
> indentation, and improve some code) and do some long run testing (current
> results are 5 mins run).
>
> Idea is same what Robert and Amit suggested up thread.
So this seems promising, but I think the code needs some work.
LockWaiterCount() bravely accesses a shared memory data structure that
is mutable with no locking at all. That might actually be safe for
our purposes, but I think it would be better to take the partition
lock in shared mode if it doesn't cost too much performance. If
that's too expensive, then it should at least have a comment
explaining (1) that it is doing this without the lock and (2) why
that's safe (sketch: the LOCK can't go away because we hold it, and
nRequested could change but we don't mind a stale value, and a 32-bit
read won't be torn).
A few of the other functions in here also lack comments, and perhaps
should have them.
The changes to RelationGetBufferForTuple need a visit from the
refactoring police. Look at the way you are calling
RelationAddOneBlock. The logic looks about like this:
if (needLock)
{ if (trylock relation for extension) RelationAddOneBlock(); else { lock relation for extension; if (use fsm) {
complicated; } else RelationAddOneBlock();
}
else RelationAddOneBlock();
So basically you want to do the RelationAddOneBlock() thing if
!needLock || !use_fsm || can't trylock. See if you can rearrange the
code so that there's only one fallthrough call to
RelationAddOneBlock() instead of three separate ones.
Also, consider moving the code that adds multiple blocks at a time to
its own function instead of including it all in line.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
