Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uwuXa-008eXV-BN for pgsql-hackers@arkaria.postgresql.org; Fri, 12 Sep 2025 03:33:38 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uwuXY-00Be0W-LO for pgsql-hackers@arkaria.postgresql.org; Fri, 12 Sep 2025 03:33:37 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uwuXY-00Be0A-7t for pgsql-hackers@lists.postgresql.org; Fri, 12 Sep 2025 03:33:36 +0000 Received: from mail-pj1-x1036.google.com ([2607:f8b0:4864:20::1036]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uwuXW-001xHp-2n for pgsql-hackers@lists.postgresql.org; Fri, 12 Sep 2025 03:33:35 +0000 Received: by mail-pj1-x1036.google.com with SMTP id 98e67ed59e1d1-32de2f8562aso737586a91.1 for ; Thu, 11 Sep 2025 20:33:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1757648014; x=1758252814; darn=lists.postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=p2w3VILO9Fzo1C2SCe+0TV6gXrGEUOlKLEZKtnnIyQE=; b=cUQi8PXWuyHg3cdg69iMGhZJ88K9MyAIfsRwoRvvUq4w6G8QIPydt15Lvggd081lgV s8MsvMwPt0oJplsnS272DQIXTHcbEW5tF1POymWmN1g+qlOujupoqGwUg7dEChs7wmGL iJaCCCYxBkmZw+ZyZz8qrVCJMzCAuI/MvDD03/JtJtwpkXu84aaVEhnVXSydfDJednkX f6xZ5w4CjnpD/tGtmwB6z9GQEzcVchGAcYjktfgi+NEg510KJuDLX7Ogyd5aviDxacgv WKF3qIJ4glpMdF0DmlVXpQ89W1ojz6HIFoOrrtJxIvJ5Mxd5IsQsOvknc+K0yziswLJ1 r55g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757648014; x=1758252814; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=p2w3VILO9Fzo1C2SCe+0TV6gXrGEUOlKLEZKtnnIyQE=; b=uIzs/4GIdn3VJIjQrBy/geDzx9iJoMUfNA83PEMKhSwLvC9NIk6PBuCVMNKMCF6D0W Ch/u0ug3Bxi4UD5s9Ymp2zfJwAlfe7nFl7kHlnbCuBtTVYuroI8G17XZHKPWiItPxExf cK8TgRr+DDpN+AQ7Qw3OZaMz62UAvTy7vuY0naF2akD+yDNzbeT5Ww0zloLqPBJGtDTZ rUoQGB5NqDgHV/vNdlHlPQbZLmeOA1Pxs17sa/g60qSqqq1RcDiHfrzQnlY6qieJ1xSr 2p/119AjIvRKuVosB7ibAL7ZXYwexivHjk8E+KZlmBCEgOq4XeqjK67z2BPn24D0xlaF Z7rg== X-Forwarded-Encrypted: i=1; AJvYcCX3Lf5HnZgBMvt/RPVdXCqc7hrL1NNz0g10whYvMqou1WFb7mpiPg61FthKUlQjU5MDnv9Gc+i4AAlJIPWF@lists.postgresql.org X-Gm-Message-State: AOJu0YyszBY7ahRtEkA+JmmZs8Ae0Od/0CdJJBQC9Zjspd8XSiI2Tt47 hsEq6ZbpyX8tfPJk039HNhPtKpVmIr+YPLgGZ6FX+Esj/69jWuQEzNrH1dYoxy5oRjf4Ow== X-Gm-Gg: ASbGncsCH+QmBlBJx+ax2Oa4qEzqcrxjiEnmUF7YA8shI/CO5W5XLQaU33gsO8dIH6b ZqH2VVutgNkE9doQVCqLQ9ggxatEJr7yJXQtzY8mJwB1t7FKianJ2XWXltGZ6bdXeV9XHb2OE78 NMFZ9w8WxXRtrkG6ntCrtnpjUMqH1qisBpYN9UGoY3WwHyXNg7sOLE4ylFEQU68guXMYaXpJrMv YssLMgIdHfNBDD2MKYPsNI4xRjFw9DguubIrMaxGiRGzcg4DcyLmhJwPs4vUoitlXxpmYr4wmdu oj9sMrXQw2F+wdJFrX+uKgNJG80g+7std0zs+QIsfztN/r/Q+U+ZbMbHY+CK4wAFl9iAfbR2rD7 oGN/+PFuNhry/zkEKhzhaxgNfx2P+vYJf X-Google-Smtp-Source: AGHT+IGV/h1xkucU8GdaIJr6nPTxt2ra6e2owD9K+t/CqjiviZvxDmmhig3dFhRtMIIRhIcF1mLizw== X-Received: by 2002:a17:90b:5403:b0:32b:c610:6ef6 with SMTP id 98e67ed59e1d1-32de4f7df23mr1463347a91.21.1757648013741; Thu, 11 Sep 2025 20:33:33 -0700 (PDT) Received: from smtpclient.apple ([142.171.105.12]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-32dd63268dcsm4421748a91.26.2025.09.11.20.33.31 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 11 Sep 2025 20:33:32 -0700 (PDT) From: Chao Li Message-Id: <3FC2442D-1012-4079-A009-A0B8C45E092D@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_28774CA7-73E8-4217-9C7C-7DFE428FC12F" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: Checkpointer write combining Date: Fri, 12 Sep 2025 11:33:17 +0800 In-Reply-To: Cc: Nazir Bilal Yavuz , PostgreSQL Hackers , Andres Freund To: Melanie Plageman References: <2FA0BAC7-5413-4ABD-94CA-4398FE77750D@gmail.com> X-Mailer: Apple Mail (2.3826.700.81) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_28774CA7-73E8-4217-9C7C-7DFE428FC12F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Sep 12, 2025, at 07:11, Melanie Plageman = wrote: >=20 > On Wed, Sep 10, 2025 at 4:24=E2=80=AFAM Chao Li = wrote: >>=20 >=20 > Thanks for the review! >=20 > For any of your feedback that I simply implemented, I omitted an > inline comment about it. Those changes are included in the attached > v6. My inline replies below are only for feedback requiring more > discussion. >=20 >> On Sep 10, 2025, at 01:55, Melanie Plageman = wrote: >>=20 >> 2 - 0001 >> ``` >> --- a/src/backend/storage/buffer/freelist.c >> +++ b/src/backend/storage/buffer/freelist.c >>=20 >> + if (XLogNeedsFlush(lsn)) >> + { >> + /* >> + * Remove the dirty buffer from the ring; necessary to prevent an >> + * infinite loop if all ring members are dirty. >> + */ >> + strategy->buffers[strategy->current] =3D InvalidBuffer; >> + return true; >> + } >>=20 >> - return true; >> + return false; >> } >> ``` >>=20 >> We can do: >> ``` >> If (!XLogNeedsFlush(lan)) >> Return false >>=20 >> /* Remove the dirty buffer =E2=80=A6. >> */ >> Return true; >> } >> ``` >=20 > This would make the order of evaluation the same as master but I > actually prefer it this way because then we only take the buffer > header spinlock if there is a chance we will reject the buffer (e.g. > we don't need to examine it for strategies except BAS_BULKREAD) >=20 >=20 I don=E2=80=99t understand why the two versions are different: if (XLogNeedsFlush(lsn)) { /* * Remove the dirty buffer from the ring; necessary to prevent = an * infinite loop if all ring members are dirty. */ strategy->buffers[strategy->current] =3D InvalidBuffer; return true; } return false; VS if (XLogNeedsFlush(lsn)) return false; /* * Remove the dirty buffer from the ring; necessary to prevent an * infinite loop if all ring members are dirty. */ strategy->buffers[strategy->current] =3D InvalidBuffer; return true; >=20 >> 10 - 0004 >> ``` >> --- a/src/backend/storage/buffer/bufmgr.c >> +++ b/src/backend/storage/buffer/bufmgr.c >>=20 >> + limit =3D Min(max_batch_size, limit); >> ``` >>=20 >> Do we need to check max_batch_size should be less than = (MAX_IO_COMBINE_LIMIT-1)? Because BufWriteBatch.bufdescs is defined with = length of MAX_IO_COMBINE_LIMIT, and the first place has been used to = store =E2=80=9Cstart=E2=80=9D. >=20 > I assert that in StrategyMaxWriteBatchSize(). io_combine_limit is not > allowed to exceed MAX_IO_COMBINE_LIMIT, so it shouldn't happen anyway, > since we are capping ourselves at io_combine_limit. Or is that your > point? >=20 Please ignore comment 10. I think I cross-line it in my original email. = I added the comment, then lately I found you have checked = MAX_IO_COMBINE_LIMIT in the other function, so tried to delete it by = cross-lining the comment. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ --Apple-Mail=_28774CA7-73E8-4217-9C7C-7DFE428FC12F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Sep 12, 2025, at 07:11, Melanie Plageman = <melanieplageman@gmail.com> wrote:

On Wed, Sep 10, 2025 at = 4:24=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> = wrote:


Thanks for the = review!

For any of your feedback that I simply implemented, I = omitted an
inline comment about it. Those changes are included in the = attached
v6. My inline replies below are only for feedback requiring = more
discussion.

On Sep 10, 2025, at = 01:55, Melanie Plageman <melanieplageman@gmail.com> = wrote:

2 - 0001
```
--- = a/src/backend/storage/buffer/freelist.c
+++ = b/src/backend/storage/buffer/freelist.c

+ if = (XLogNeedsFlush(lsn))
+ {
+ /*
+ * Remove the dirty buffer from = the ring; necessary to prevent an
+ * infinite loop if all ring = members are dirty.
+ */
+ = strategy->buffers[strategy->current] =3D InvalidBuffer;
+ = return true;
+ }

- return true;
+ return false;
= }
```

We can do:
```
=         If = (!XLogNeedsFlush(lan))
=            Return = false

       /* Remove the = dirty buffer =E2=80=A6.
=         */
=       Return = true;
}
```

This would make the order of = evaluation the same as master but I
actually prefer it this way = because then we only take the buffer
header spinlock if there is a = chance we will reject the buffer (e.g.
we don't need to examine it = for strategies except = BAS_BULKREAD)



I = don=E2=80=99t understand why the two versions are = different:

if (XLogNeedsFlush(lsn))
{
= /*
= * Remove the dirty buffer from the ring; necessary to prevent = an
* = infinite loop if all ring members are dirty.
*/
= strategy->buffers[strategy->current] = =3D InvalidBuffer;
return true;
}

return false;

VS

= if (XLogNeedsFlush(lsn))
return false;

= /*
* = Remove the dirty buffer from the ring; necessary to prevent = an
* = infinite loop if all ring members are dirty.
*/
strategy->buffers[strategy->current] =3D = InvalidBuffer;
return true;


10 - = 0004
```
--- a/src/backend/storage/buffer/bufmgr.c
+++ = b/src/backend/storage/buffer/bufmgr.c

+ limit =3D = Min(max_batch_size, limit);
```

Do we need to check = max_batch_size should be less than (MAX_IO_COMBINE_LIMIT-1)? Because = BufWriteBatch.bufdescs is defined with length of MAX_IO_COMBINE_LIMIT, = and the first place has been used to store = =E2=80=9Cstart=E2=80=9D.

I assert that in = StrategyMaxWriteBatchSize(). io_combine_limit is not
allowed to = exceed MAX_IO_COMBINE_LIMIT, so it shouldn't happen anyway,
since we = are capping ourselves at io_combine_limit. Or is that = your
point?


Please = ignore comment 10. I think I cross-line it in my original email. I added = the comment, then lately I found you have checked MAX_IO_COMBINE_LIMIT = in the other function, so tried to delete it by cross-lining the = comment.

Best regards,
--
Chao Li (Evan)
HighGo Software = Co., Ltd.
https://www.highgo.com/




= --Apple-Mail=_28774CA7-73E8-4217-9C7C-7DFE428FC12F--