agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Chao Li <li.evan.chao@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Checkpointer write combining
Date: Thu, 13 Nov 2025 16:29:50 +0800
Message-ID: <0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com> (raw)
In-Reply-To: <CAAKRu_Z8GtkTPsbScp8QoNF=V1YT3cVT6mPf1sR-NAtSE+jzoQ@mail.gmail.com>
References: <CAAKRu_bcWRvRwZUop_d9vzF9nHAiT+-uPzkJ=S3ShZ1GqeAYOw@mail.gmail.com>
<CAAKRu_bQWv5hJH0ncbGw8BEufAxdwMw4KY0KYuYLaw6KA=fpCA@mail.gmail.com>
<CAN55FZ3FOv+ubp4yyFKt=Rr+ceM0tfXRDVr6nKgqSyWXh8G8Gg@mail.gmail.com>
<CAAKRu_Yurj0sabXZB5EVqO3fKgwN1vELe1H4e0s1zXV3MKbZjQ@mail.gmail.com>
<CAAKRu_bv2=_DU-tYFdTQJj7ohKUyFnrJvETyCes3_NpFzvVt4Q@mail.gmail.com>
<CAAKRu_bz=t1FGXhUytPx9SC648-ozb-UxBps4zWUmS2XMg4Z4A@mail.gmail.com>
<2FA0BAC7-5413-4ABD-94CA-4398FE77750D@gmail.com>
<CAAKRu_atZuU3gYY6hVVuvqDW+mfXM+fCWCyYODbZPesxyr=y6g@mail.gmail.com>
<3FC2442D-1012-4079-A009-A0B8C45E092D@gmail.com>
<CAAKRu_b3k+KbeFhFgDZ26PvJbvL9W5HYwKrn8bBS4CTU13khpw@mail.gmail.com>
<6B520FA8-B805-4C0D-B8B9-202B3AF168A9@gmail.com>
<CAAKRu_Z8GtkTPsbScp8QoNF=V1YT3cVT6mPf1sR-NAtSE+jzoQ@mail.gmail.com>
> On Nov 4, 2025, at 07:34, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> Thanks for continuing to review! I've revised the patches to
> incorporate all of your feedback except for where I mention below.
>
> There were failures in CI due to issues with max batch size, so
> attached v8 also seeks to fix those.
>
> - Melanie
>
> On Thu, Oct 16, 2025 at 12:25 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>> 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."
>
> Yes, actually I think having these helpers mention the sweep is more
> confusing than anything else. I've revised them to be named more
> generically and updated the comments accordingly.
>
>> 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;
>> }
>> ```
>
> I agree that the implementation was hard to understand. I've not quite
> gone with your version but I have rewritten it like this:
>
> uint32
> StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
> {
> uint32 max_write_batch_size = Min(io_combine_limit,
> MAX_IO_COMBINE_LIMIT);
> int strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
> uint32 max_possible_buffer_limit = GetPinLimit();
>
> /* Identify the minimum of the above */
> 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);
>
> /* Must allow at least 1 IO for forward progress */
> max_write_batch_size = Max(1, max_write_batch_size);
>
> return max_write_batch_size;
> }
>
> Is this better?
Yes, I think your version is safer because it enforces the max limit at runtime instead of only asserting it in debug builds.
>
> - Melanie
> <v8-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v8-0002-Split-FlushBuffer-into-two-parts.patch><v8-0003-Eagerly-flush-bulkwrite-strategy-ring.patch><v8-0004-Write-combining-for-BAS_BULKWRITE.patch><v8-0005-Add-database-Oid-to-CkptSortItem.patch><v8-0006-Implement-checkpointer-data-write-combining.patch><v8-0007-WIP-Refactor-SyncOneBuffer-for-bgwriter-only.patch>
I quickly went through 0001-0006, looks good to me now. As 0007 has WIP in the subject, I skipped it.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
view thread (52+ messages) latest in thread
Message-ID: <0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com>
Permalink: ../0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com/
Also on: postgresql.org/message-id/0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: li.evan.chao@gmail.com, melanieplageman@gmail.com, byavuz81@gmail.com, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: Checkpointer write combining
In-Reply-To: <0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox