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: