Hi Milanie,
Thanks for updating the patch set. I review 1-6 and got a few more small
comments. I didn’t review 0007 as it’s marked as WIP.
>
> - Melanie
> <v7-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v7-0002-Split-FlushBuffer-into-two-parts.patch><v7-0003-Eagerly-flush-bulkwrite-strategy-ring.patch><v7-0004-Write-combining-for-BAS_BULKWRITE.patch><v7-0005-Add-database-Oid-to-CkptSortItem.patch><v7-0006-Implement-checkpointer-data-write-combining.patch><v7-0007-WIP-Refactor-SyncOneBuffer-for-bgwriter-only.patch>
1 - 0001
```
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -421,6 +421,12 @@ ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer
buffer)
/*
* Internal buffer management routines
*/
+
+
+/* Note: these two macros only work on shared buffers, not local ones! */
```
Nit: here you added two empty lines, I think we need only 1.
2 - 0002
```
+static void
+CleanVictimBuffer(BufferDesc *bufdesc,
+ bool from_ring, IOContext io_context)
+{
- /* Find smgr relation for buffer */
- if (reln == NULL)
- reln = smgropen(BufTagGetRelFileLocator(&buf->tag),
INVALID_PROC_NUMBER);
+ XLogRecPtr max_lsn = InvalidXLogRecPtr;
+ LWLock *content_lock;
```
Nit: the empty line after “{“ should be removed.
3 - 0003
```
+/*
+ * Return the next buffer in the ring or InvalidBuffer if the current sweep is
+ * over.
+ */
+Buffer
+StrategySweepNextBuffer(BufferAccessStrategy strategy, int *sweep_cursor)
+{
+ if (++(*sweep_cursor) >= strategy->nbuffers)
+ *sweep_cursor = 0;
+
+ return strategy->buffers[*sweep_cursor];
+}
```
Feels the function comment is a bit confusing, because the function code
doesn’t really perform sweep, the function is just a getter. InvalidBuffer just
implies the current sweep is over.
Maybe rephrase to something like: “Return the next buffer in the range. If
InvalidBuffer is returned, that implies the current sweep is done."
4 - 0003
```
static BufferDesc *
NextStratBufToFlush(BufferAccessStrategy strategy,
Buffer sweep_end,
XLogRecPtr *lsn, int *sweep_cursor)
``
“Strat” is confusing. I think it’s the short version of “Strategy”. As this is
a static function, and other function names all have the whole word of
“strategy”, why don’t also use the whole word in this function name as well?
5 - 0004
```
+uint32
+StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
+{
+ uint32 max_possible_buffer_limit;
+ uint32 max_write_batch_size;
+ int strategy_pin_limit;
+
+ max_write_batch_size = io_combine_limit;
+
+ strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
+ max_possible_buffer_limit = GetPinLimit();
+
+ max_write_batch_size = Min(strategy_pin_limit, max_write_batch_size);
+ max_write_batch_size = Min(max_possible_buffer_limit,
max_write_batch_size);
+ max_write_batch_size = Max(1, max_write_batch_size);
+ max_write_batch_size = Min(max_write_batch_size, io_combine_limit);
+ Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
+ return max_write_batch_size;
+}
```
This implementation is hard to understand. I tried to simplify it:
```
uint32
StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
{
int strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
uint32 max_write_batch_size = Min(GetPinLimit(),
(uint32)strategy_pin_limit);
/* Clamp to io_combine_limit and enforce minimum of 1 */
if (max_write_batch_size > io_combine_limit)
max_write_batch_size = io_combine_limit;
if (max_write_batch_size == 0)
max_write_batch_size = 1;
Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
return max_write_batch_size;
}
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
