Re: Relation extension scalability - Mailing list pgsql-hackers

1.

 /*

+ * RelationAddOneBlock

+ *

+ * Extend relation by one block and lock the buffer

+ */

+static Buffer

+RelationAddOneBlock(Relation relation, Buffer otherBuffer, BulkInsertState bistate)

Shall we mention in comments that this API returns locked buffer and it's the responsibility of caller to unlock it.

2.

+ /* To avoid the cases when there are huge number of lock waiters, and

+ * extend file size by big amount at a 

time, put some limit on the

first line in multi-line comments should not contain anything.

3.

+ extraBlocks = Min(512, lockWaiters * 20);

I think we should explain in comments about the reason of choosing 20 in above calculation.

4. Sometime back [1], you agreed on doing some analysis for the overhead that XLogFlush can cause during buffer eviction,  but I don't see the results of same, are you planing to complete the same?

5.

+ if (LOCKACQUIRE_OK

+ != RelationExtensionLockConditional(relation, 

ExclusiveLock))

I think the coding style is to keep constant on right side of condition, did you see any other place in code which uses the check in a similar way?

6.

- /*

- * Now acquire lock on the new page.

+ /* For all case we need to add at least one block to satisfy 

our

+ * own request.

  */

Same problem as in point 2.

7.

@@ -472,6 +581,8 @@ RelationGetBufferForTuple(Relation relation, Size len,

  if (needLock)

 

UnlockRelationForExtension(relation, ExclusiveLock);

 

+

+

Spurious newline addition.

8.

+int

+RelationExtensionLockWaiter(Relation relation)

How about naming this function as RelationExtensionLockWaiterCount?

9.

+ /* Other waiter has extended the block for us*/

Provide an extra space before ending the comment.

10.

+ if (use_fsm)

+ {

+ if (lastValidBlock != 

InvalidBlockNumber)

+ {

+ targetBlock = 

RecordAndGetPageWithFreeSpace(relation,

+

lastValidBlock,

+

pageFreeSpace,

+

len + saveFreeSpace);

+ }

Are you using RecordAndGetPageWithFreeSpace() instead of GetPageWithFreeSpace() to get the page close to the previous target page?  If yes, then do you see enough benefit of the same that it can compensate the additional write operation which Record* API might cause due to additional dirtying of buffer?

11.

+ {

+ /*

+ * First try to get the lock in no-wait mode, if succeed extend one

+

 * block, else get the lock in normal mode and after we get the lock

+ * extend some extra blocks, extra 

blocks will be added to satisfy

+ * request of other waiters and avoid future contention. Here instead

+

* of directly taking lock we try no-wait mode, this is to handle the

+ * case, when there is no 

contention - it should not find the lock

+ * waiter and execute extra instructions.

+ */

+

if (LOCKACQUIRE_OK

+ != RelationExtensionLockConditional(relation, ExclusiveLock))

+

{

+ LockRelationForExtension(relation, ExclusiveLock);

+

+ if (use_fsm)

+

{

+ if (lastValidBlock != InvalidBlockNumber)

+

{

+ targetBlock = RecordAndGetPageWithFreeSpace(relation,

+

lastValidBlock,

+

pageFreeSpace,

+

len + saveFreeSpace);

+

}

+

+ /* Other waiter has extended the block for us*/

+

if (targetBlock != InvalidBlockNumber)

+ {

+

UnlockRelationForExtension(relation, ExclusiveLock);

+ goto loop;

+

}

+

+ RelationAddExtraBlocks(relation, bistate);

+ }

+

}

+ }

 

- /*

- * Now acquire lock on the new page.

+ /* For all case we need to add at least one block to 

satisfy our

+ * own request.

  */

- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);

+ buffer = 

RelationAddOneBlock(relation, otherBuffer, bistate);

Won't this cause one extra block addition after backend extends the relation for multiple blocks, what is the need of same?

12. I think it is good to once test pgbench read-write tests to ensure that this doesn't introduce any new regression.