Re: Checkpointer write combining - Mailing list pgsql-hackers

From Chao Li
Subject Re: Checkpointer write combining
Date
Msg-id 6B520FA8-B805-4C0D-B8B9-202B3AF168A9@gmail.com
Whole thread Raw
In response to Re: Checkpointer write combining  (Melanie Plageman <melanieplageman@gmail.com>)
List pgsql-hackers
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
asWIP. 

>
> - 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
justa 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
currentsweep 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
namesall 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/







pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Adding error messages to a few slash commands
Next
From: Michael Paquier
Date:
Subject: Re: Preserve index stats during ALTER TABLE ... TYPE ...