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 1uwG93-008Wht-R2 for pgsql-hackers@arkaria.postgresql.org; Wed, 10 Sep 2025 08:25:39 +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 1uwG83-00HZkA-9j for pgsql-hackers@arkaria.postgresql.org; Wed, 10 Sep 2025 08:24:35 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uwG82-00HZk1-Tn for pgsql-hackers@lists.postgresql.org; Wed, 10 Sep 2025 08:24:35 +0000 Received: from mail-pf1-x435.google.com ([2607:f8b0:4864:20::435]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uwG7z-001fjG-1u for pgsql-hackers@lists.postgresql.org; Wed, 10 Sep 2025 08:24:35 +0000 Received: by mail-pf1-x435.google.com with SMTP id d2e1a72fcca58-7725147ec88so228705b3a.0 for ; Wed, 10 Sep 2025 01:24:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1757492670; x=1758097470; 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=QRMIuhqvPxZInp0Ri4MeSYpaKgnB+UBOLoXDYc8J5DI=; b=MIFFhiH6mCVPcK42iByAi8lgtvz5p2hylYflrWX2w/OFXt3LgOdhi60jEk83C/7db5 LkI5c4IBAx09t1Z7Vy3HOSliVmmPfjwSzTBCJ7c5bKwDsVoNnOx7Bch1OQ0gG/RJRpp0 KMQBMhkJZli7Rnsp5j5ckov/ep+yAY3JkL4hrraqVrlEaJcR5sC9gr8gRo3epoPPtyLa M/SuXKq1kPrXLjh7k83IlJisqPP4CG10lKYIAYubaxbbadU7WqTQXdvQa+fY4EAkQ1kb MoTIHX2iYpkdgLMRfIHArXo356T6qHOvlY47QhtK3PAaeOdl8gGwZLHqI7t++xAwGaM1 FQag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757492670; x=1758097470; 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=QRMIuhqvPxZInp0Ri4MeSYpaKgnB+UBOLoXDYc8J5DI=; b=PwMqxkIyADSFwRxlpN/pZkuifqpL/Cy96wBjzQ/qXMAVoZ3jrPKkbXNgxaAq6/Sd6k EgzwsipdAAgL3CAg2To+AffEJm+FnEhO8IlAcu1HexXxauapdbuczxDd5D82t5tj/9wQ PPqfm3IvLIbN4/JwjZIj8meZB4XuzTyjUtUZGvrMq2ooRKA9ifzlemiwCox4IKQIvVlK 39la06jckrH4wQGx6XX8QfWWTrOAkgOjN3Li0aMGUNaYSLtYW0YnBkOQrP4w4L/cEQSn OXnMN4lepSg/Wcq2vgx8nobWNLcJN4nx9K1O3q/luG9ytBKeOMgR2R0owquQU/9SXz3H Zy3w== X-Forwarded-Encrypted: i=1; AJvYcCWI8w86ikqjkHJJRFIDQRlFWxxD8m0mu04lgLdiMc7U3O/bCMfCVFXCKcPdZCIk8Vb2BnGeKNf1jcJK89dw@lists.postgresql.org X-Gm-Message-State: AOJu0YzdClwhH7q7ppmHhltHE8GgSHvQskxXv0YKXOydZs06KcBELonl df8raQ5fDo7EbGex6PAioDGVJQz0wIkHdUb93HTbfqtVR4nErCGFg0d4 X-Gm-Gg: ASbGncv1kWByS2hzy8Kkji1oXOthLw9lYPpyUrqw6KpM2gLlKeqZqE2DkYrQXf5B+q+ oHbinfRWiFmTzYaOhUWrDkTXFlQGtD+1z/XGoVAHqZePgIU26I8iTmxwHqp3bg4L8utL3Ghedj6 DuNbNWKZg+413JNgbjNxT15CiW/RjvvGTbNCirT2CR+Pmnb+kLan2Bf1UApgF8pDYET+b/XVKZ6 3rgW6k283gaYj+T2RefQC5g3QZGGVHrwo+ZUTq9blN9IfOArBhafoTeFkuDn0DUO39q4tjjZNEQ T/VjOD+3J21R/9gx/4GH3l+WQA8SIiHn0DwdwyhgT1lWQKZDqKnWtDsGwYszZKUbZ4q4JcWPegL 2gw0qsoBkmh/L9mpHBfBhTtr9qRU8eeFz X-Google-Smtp-Source: AGHT+IHGWwuEbVOUSVYUysxewN4NPUfpTbfvcc5duosnSiVxRzPDgKvseuQ+f1AFKmK3fjnEVdEJBA== X-Received: by 2002:a05:6a20:7347:b0:240:d6be:ddc with SMTP id adf61e73a8af0-24e8564f308mr29615038637.21.1757492670114; Wed, 10 Sep 2025 01:24:30 -0700 (PDT) Received: from smtpclient.apple ([142.171.105.12]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-b548a3f0b5esm1867149a12.5.2025.09.10.01.24.27 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 10 Sep 2025 01:24:29 -0700 (PDT) From: Chao Li Message-Id: <2FA0BAC7-5413-4ABD-94CA-4398FE77750D@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_20F67A10-B5A2-417A-BADB-97CB5F2DD8E5" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: Checkpointer write combining Date: Wed, 10 Sep 2025 16:24:13 +0800 In-Reply-To: Cc: Nazir Bilal Yavuz , PostgreSQL Hackers , Andres Freund To: Melanie Plageman References: 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=_20F67A10-B5A2-417A-BADB-97CB5F2DD8E5 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Sep 10, 2025, at 01:55, Melanie Plageman = wrote: >=20 >=20 > [1] = https://www.postgresql.org/message-id/flat/CAAKRu_Yjn4mvN9NBxtmsCQSGwup45C= oA4e05nhR7ADP-v0WCig%40mail.gmail.com > = = 1 - 0001 ``` --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c + * The buffer must pinned and content locked and the buffer header = spinlock ``` =E2=80=9CMust pinned=E2=80=9D -> =E2=80=9Cmust be pinned" 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; + } =20 - return true; + return false; } ``` We can do: ``` If (!XLogNeedsFlush(lan)) Return false /* Remove the dirty buffer =E2=80=A6. */ Return true; } ``` This way makes less diff. 3 - 0002 ``` + * Prepare to write and write a dirty victim buffer. ``` Prepare to write a dirty victim buffer. 4 - 0002 ``` - /* OK, do the I/O */ - FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, = io_context); - LWLockRelease(content_lock); - - = ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, - = &buf_hdr->tag); + CleanVictimBuffer(buf_hdr, &buf_state, = from_ring, io_context); ``` I saw CleanVictimBuffer() will get content_lock from bufdesc and release = it, but it makes the code hard to understand. Readers might be confused = that why content_lock is not released after CleanVictimBuffer() without = further reading CleanVictimBuffer(). I=E2=80=99d suggest pass content_lock to CleanVictimBuffer() as a = parameter, which gives a clear hint that CleanVictimBuffer() will = release the lock. 5 - 0002 ``` * disastrous system-wide consequences. To make sure that can't = happen, * skip the flush if the buffer isn't permanent. */ - if (buf_state & BM_PERMANENT) - XLogFlush(recptr); + if (!XLogRecPtrIsInvalid(buffer_lsn)) + XLogFlush(buffer_lsn); ``` Why this check is changed? Should the comment be updated accordingly as = it says =E2=80=9Cif the buffer isn=E2=80=99t permanent=E2=80=9D, which = reflects to the old code. 6 - 0003 ``` --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c + * patterns than lazily flushing buffers directly before reusing them. + */ ``` Here =E2=80=9Cdirectly=E2=80=9D is kind ambiguous. It could mean = =E2=80=9Cimmediately before=E2=80=9D or =E2=80=9Cwithout going through = something else=E2=80=9D. My understanding is =E2=80=9Cimmediately=E2=80=9D= , If that is true, please change =E2=80=9Cdirectly=E2=80=9D to = =E2=80=9Cimmediately=E2=80=9D or just remove it. 7 - 0003 ``` +void +StartStrategySweep(BufferAccessStrategy strategy) +{ + if (!strategy) + return; ``` I doubt if this =E2=80=9Cstrategy=E2=80=9D null check is needed. Because = it is only called when strategy_supports_eager_flush() is true, and = strategy_supports_eager_flush() has asserted =E2=80=9Cstrategy=E2=80=9D. And as a pair function, StrategySweepNextBuffer() doesn=E2=80=99t do = null check nor assert strategy. 8 - 0003 ``` bool +strategy_supports_eager_flush(BufferAccessStrategy strategy) ``` This function is only used in bufmgr.c, can we move it there and make it = static? 9 - 0004 ``` --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c + batch->start =3D batch->bufdescs[0]->tag.blockNum; + batch->forkno =3D BufTagGetForkNum(&batch->bufdescs[0]->tag); + batch->rlocator =3D = BufTagGetRelFileLocator(&batch->bufdescs[0]->tag); + batch->reln =3D smgropen(batch->rlocator, INVALID_PROC_NUMBER); + + Assert(BlockNumberIsValid(batch->start)); ``` Why don=E2=80=99t assert immediately after batch->start is assigned? So = upon error, smgropen() will not be called. 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. 11 - 0004 ``` --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c + for (batch->n =3D 1; batch->n < limit; batch->n++) + { + Buffer bufnum; + + if ((bufnum =3D StrategySweepNextBuffer(strategy)) =3D=3D = InvalidBuffer) + break; ``` Is sweep next buffer right next to start? If yes, can we assert that? = But my guess is no, if my guess is true, then is it possible that bufnum = meets start? If that=E2=80=99s true, then we should check next buffer = doesn=E2=80=99t equal to start. 12 - 0004 ``` @@ -4306,19 +4370,22 @@ CleanVictimBuffer(BufferAccessStrategy strategy, =20 if (from_ring && strategy_supports_eager_flush(strategy)) { + uint32 max_batch_size =3D = max_write_batch_size_for_strategy(strategy); ``` I think max_batch_size can be attribute of strategy and set it when = creating a strategy, so that we don=E2=80=99t need to calculate in every = round of clean. 13 - 0004 ``` +void +CompleteWriteBatchIO(BufWriteBatch *batch, IOContext io_context, + WritebackContext *wb_context) +{ + ErrorContextCallback errcallback =3D + { + .callback =3D shared_buffer_write_error_callback, + .previous =3D error_context_stack, + }; + + error_context_stack =3D &errcallback; + pgBufferUsage.shared_blks_written +=3D batch->n; ``` Should we only increase shared_blks_written only after the loop of = write-back is done? 14 - 0004 ``` --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c +uint32 +max_write_batch_size_for_strategy(BufferAccessStrategy strategy) ``` I think this function can be moved to bufmgr.c and make it static. 15 - 0004 ``` --- a/src/backend/storage/page/bufpage.c +++ b/src/backend/storage/page/bufpage.c @@ -1546,3 +1546,23 @@ PageSetChecksumInplace(Page page, BlockNumber = blkno) =20 ((PageHeader) page)->pd_checksum =3D pg_checksum_page(page, = blkno); } + +/* + * A helper to set multiple block's checksums. + */ +void +PageSetBatchChecksumInplace(Page *pages, BlockNumber *blknos, uint32 = length) ``` We should mark blknos as const to indicate it is readonly: const = BlockNumber *blknos, which will also prevent from incidentally change on = blknos in within the function.=20 16 - 0005 ``` * instead. So "needs flush" is taken to mean whether = minRecoveryPoint * would need to be updated. */ - if (RecoveryInProgress()) + if (RecoveryInProgress() && !XLogInsertAllowed()) ``` As a new check is added, the comment should be updated accordingly. 17 - 0006 ``` --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -382,6 +382,7 @@ UnlockBufHdr(BufferDesc *desc, uint32 buf_state) typedef struct CkptSortItem { Oid tsId; + Oid db_id; ``` I think =E2=80=9Cdb_id=E2=80=9D should be named =E2=80=9CdbId=E2=80=9D = or =E2=80=9CdbOid=E2=80=9D. Let=E2=80=99s keep the name conversation = consistent. 18 - 0007 ``` --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c + max_batch_size =3D checkpointer_max_batch_size(); ``` Look like we don=E2=80=99t need to calculate max_batch_size in the for = loop. 19 - 0007 ``` + * Each batch will have exactly one start and one max = lsn and one + * length. */ ``` I don=E2=80=99t get what you want to explain with this comment. It = sounds quite unnecessary. Best regards, =E2=80=94 Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ --Apple-Mail=_20F67A10-B5A2-417A-BADB-97CB5F2DD8E5 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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


[1] = https://www.postgresql.org/message-id/flat/CAAKRu_Yjn4mvN9NBxtmsCQSGwup45C= oA4e05nhR7ADP-v0WCig%40mail.gmail.com
<v5-0004-Write-combining-for-BAS_BULKWRITE.patch= ><v5-0001-Refactor-goto-into-for-loop-in-GetVicti= mBuffer.patch><v5-0005-Fix-XLogNeedsFlush-for-checkpointer.pat= ch><v5-0002-Split-FlushBuffer-into-two-parts.patch&= gt;<v5-0003-Eagerly-flush-bulkwrite-strategy-ring.p= atch><v5-0006-Add-database-Oid-to-CkptSortItem.patch&= gt;<v5-0007-Implement-checkpointer-data-write-combi= ning.patch>

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

+ = * The buffer must pinned and content locked and the buffer header = spinlock
```

=E2=80=9CMust pinned=E2=80= =9D -> =E2=80=9Cmust be pinned"

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 way makes = less diff.

3 - 0002
```
+ * = Prepare to write and write a dirty victim = buffer.
```

Prepare to write a dirty = victim buffer.

4 - = 0002
```
- /* OK, do the I/O = */
- = FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, = io_context);
- = LWLockRelease(content_lock);
-
- = ScheduleBufferTagForWriteback(&BackendWritebackContext, = io_context,
- = =  &buf_hdr->tag);
+ = CleanVictimBuffer(buf_hdr, &buf_state, from_ring, = io_context);
```
I saw CleanVictimBuffer() = will get content_lock from bufdesc and release it, but it makes the code = hard to understand. Readers might be confused that why content_lock is = not released after CleanVictimBuffer() without further reading = CleanVictimBuffer().

I=E2=80=99d suggest pass = content_lock to CleanVictimBuffer() as a parameter, which gives a clear = hint that CleanVictimBuffer() will release the = lock.

5 - = 0002
```
  * disastrous system-wide = consequences.  To make sure that can't = happen,
  * skip the flush if the buffer = isn't permanent.
  */
- if = (buf_state & BM_PERMANENT)
- = XLogFlush(recptr);
+ if = (!XLogRecPtrIsInvalid(buffer_lsn))
+ = XLogFlush(buffer_lsn);
```

Why this check is changed? Should the comment be updated accordingly as = it says =E2=80=9Cif the buffer isn=E2=80=99t permanent=E2=80=9D, which = reflects to the old code.

6 - = 0003
```
--- = a/src/backend/storage/buffer/freelist.c
+++ = b/src/backend/storage/buffer/freelist.c

+ * patterns than lazily flushing buffers directly before reusing = them.
+ */
```

Here = =E2=80=9Cdirectly=E2=80=9D is kind ambiguous. It could mean = =E2=80=9Cimmediately before=E2=80=9D or =E2=80=9Cwithout going through = something else=E2=80=9D. My understanding is =E2=80=9Cimmediately=E2=80=9D= , If that is true, please change =E2=80=9Cdirectly=E2=80=9D to = =E2=80=9Cimmediately=E2=80=9D or just remove = it.

7 - = 0003
```
+void
+StartStrategySweep(Buff= erAccessStrategy strategy)
+{
+ if = (!strategy)
+ = return;
```

I doubt if = this =E2=80=9Cstrategy=E2=80=9D null check is needed. Because it is only = called when strategy_supports_eager_flush() is true, and = strategy_supports_eager_flush() has asserted = =E2=80=9Cstrategy=E2=80=9D.

And as a pair = function, StrategySweepNextBuffer() doesn=E2=80=99t do null check nor = assert strategy.

8 - = 0003
```
bool
+strategy_supports_eager_= flush(BufferAccessStrategy = strategy)
```

This function is = only used in bufmgr.c, can we move it there and make it = static?

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

= + = batch->start =3D = batch->bufdescs[0]->tag.blockNum;
+ = batch->forkno =3D = BufTagGetForkNum(&batch->bufdescs[0]->tag);
+ = batch->rlocator =3D = BufTagGetRelFileLocator(&batch->bufdescs[0]->tag);
+ = batch->reln =3D smgropen(batch->rlocator, = INVALID_PROC_NUMBER);
+
+ = Assert(BlockNumberIsValid(batch->start));
```

Why don=E2=80=99t assert immediately after = batch->start is assigned? So upon error, smgropen() will not be = called.

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.

<= div>11 - 0004
```
--- = a/src/backend/storage/buffer/bufmgr.c
+++ = b/src/backend/storage/buffer/bufmgr.c

= + = for (batch->n =3D 1; batch->n < limit; = batch->n++)
+ {
+ = Buffer = bufnum;
+
+ if ((bufnum =3D = StrategySweepNextBuffer(strategy)) =3D=3D = InvalidBuffer)
+ = break;
```

Is sweep next = buffer right next to start? If yes, can we assert that? But my guess is = no, if my guess is true, then is it possible that bufnum meets start? If = that=E2=80=99s true, then we should check next buffer doesn=E2=80=99t = equal to start.

12 - = 0004
```
@@ -4306,19 +4370,22 @@ = CleanVictimBuffer(BufferAccessStrategy = strategy,
 
  if (from_ring && = strategy_supports_eager_flush(strategy))
  = {
+ uint32 = max_batch_size =3D = max_write_batch_size_for_strategy(strategy);
```

I think max_batch_size can be attribute of strategy and = set it when creating a strategy, so that we don=E2=80=99t need to = calculate in every round of clean.

13 - = 0004
```
+void
+CompleteWriteBatchIO(Bu= fWriteBatch *batch, IOContext io_context,
+ = WritebackContext = *wb_context)
+{
+ ErrorContextCallback errcallback = =3D
+ = {
+ .callback =3D = shared_buffer_write_error_callback,
+ = .previous =3D error_context_stack,
+ = };
+
+ error_context_stack =3D = &errcallback;
+ pgBufferUsage.shared_blks_written = +=3D batch->n;
```

Should we = only increase shared_blks_written only after the loop of write-back is = done?

14 - = 0004
```
--- = a/src/backend/storage/buffer/freelist.c
+++ = b/src/backend/storage/buffer/freelist.c

+uint32
+max_write_batch_size_for_strategy(BufferAccessStrateg= y strategy)
```

I think this = function can be moved to bufmgr.c and make it = static.

15 - = 0004
```
--- = a/src/backend/storage/page/bufpage.c
+++ = b/src/backend/storage/page/bufpage.c
@@ -1546,3 +1546,23 @@ = PageSetChecksumInplace(Page page, BlockNumber = blkno)
 
  ((PageHeader) = page)->pd_checksum =3D pg_checksum_page(page, = blkno);
 }
+
+/*
+ * A = helper to set multiple block's checksums.
+ = */
+void
+PageSetBatchChecksumInplace(Page *pages, = BlockNumber *blknos, uint32 = length)
```

We should mark = blknos as const to indicate it is readonly: const BlockNumber *blknos, = which will also prevent from incidentally change on blknos in within the = function. 

16 - = 0005
```
  * instead. So "needs flush" is = taken to mean whether minRecoveryPoint
  * would = need to be updated.
  */
- if = (RecoveryInProgress())
+ if (RecoveryInProgress() = && = !XLogInsertAllowed())
```

As a = new check is added, the comment should be updated = accordingly.

17 - = 0006
```
--- = a/src/include/storage/buf_internals.h
+++ = b/src/include/storage/buf_internals.h
@@ -382,6 +382,7 @@ = UnlockBufHdr(BufferDesc *desc, uint32 buf_state)
 typedef = struct CkptSortItem
 {
  Oid = tsId;
+ Oid = db_id;
```

I think = =E2=80=9Cdb_id=E2=80=9D should be named =E2=80=9CdbId=E2=80=9D or = =E2=80=9CdbOid=E2=80=9D. Let=E2=80=99s keep the name conversation = consistent.

18 - = 0007
```
--- = a/src/backend/storage/buffer/bufmgr.c
+++ = b/src/backend/storage/buffer/bufmgr.c

+ = max_batch_size =3D = checkpointer_max_batch_size();
```

Look= like we don=E2=80=99t need to calculate max_batch_size in the for = loop.

19 - = 0007
```
+ * Each batch will have = exactly one start and one max lsn and one
+ = * length.
  = */
```

I don=E2=80=99t get what = you want to explain with this comment. It sounds quite = unnecessary.

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




= --Apple-Mail=_20F67A10-B5A2-417A-BADB-97CB5F2DD8E5--