MIME-Version: 1.0
References: <CA+HiwqFfAY_ZFqN8wcAEMw71T9hM_kA8UtyHaZZEZtuT3UyogA@mail.gmail.com>
 <68f3771f-91f5-4cb7-b1de-74d9abbf0b96@vondra.me> <CA+HiwqGqeS_94wxiYa8VpymiR_OtFPKDSpX+Me=MYWO45f5yig@mail.gmail.com>
 <dbce8def-e817-4e70-a9cb-e62574724d95@vondra.me> <CA+HiwqGM0ZTeVHicSkGnCp-2U-jvU-KBQCkPJ0N7nAj_c2LjZg@mail.gmail.com>
 <CA+HiwqHyE7-oOvtZ+OC-4N7DvKSr8Jbu75erMLQ7O4d6gfkBhg@mail.gmail.com>
 <CA+HiwqEPMwhg6pUE4XML2rG4fRqKMpeGTtMwRPGes90f9iOqtg@mail.gmail.com>
 <CA+HiwqEZja5rJ78p3FBDZNvynWsHwanxyt6h0YaK_r84NemXng@mail.gmail.com>
 <tencent_B45268659AF0400F68EC1643950ABB6A1B09@qq.com> <CAJDiXggP41+-HrRzT+BmtgmS8wkoZM7b4skkQA5NAe+NFEMPSQ@mail.gmail.com>
 <CA+HiwqGufj46_4kT6z5nQrCOxp3rcMDR8fn_t33n2pDKokNRQg@mail.gmail.com>
 <CA+HiwqH-2GmTKLW9kHdnqV4KdFiPfuAdVK2TgqOM2JaaeUYXnw@mail.gmail.com>
 <CAEG8a3JKNRGwBb-C_YV6Px4aFHwd_G=AsB5AbfyU4-8YZT54AQ@mail.gmail.com> <tencent_164C2A5EDF8189DD70287661406D688A8506@qq.com>
In-Reply-To: <tencent_164C2A5EDF8189DD70287661406D688A8506@qq.com>
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Tue, 3 Feb 2026 23:54:48 +0800
Message-ID: <CAEG8a3K22s9LmDZa486suFu3AV=yRuTf09j+Ww+djG63y=R8hg@mail.gmail.com>
Subject: Re: Batching in executor
To: cca5507 <cca5507@qq.com>
Cc: Amit Langote <amitlangote09@gmail.com>, Daniil Davydov <3danissimo@gmail.com>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>, Tomas Vondra <tomas@vondra.me>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAEG8a3K22s9LmDZa486suFu3AV%3DyRuTf09j%2BWw%2BdjG63y%3DR8hg%40mail.gmail.com>
Precedence: bulk

On Tue, Feb 3, 2026 at 9:30=E2=80=AFPM cca5507 <cca5507@qq.com> wrote:
>
> Hi,
>
> Some comments for v5:
>
> 0001
> =3D=3D=3D=3D
>
> 1) heap_begin_batch()
>
> ```
>         /* Single allocation for HeapBatch header + tupdata array */
>         alloc_size =3D sizeof(HeapBatch) + sizeof(HeapTupleData) * maxite=
ms;
>         hb =3D palloc(alloc_size);
>         hb->tupdata =3D (HeapTupleData *) ((char *) hb + sizeof(HeapBatch=
));
> ```
>
> Do we need a MAXALIGN() here to avoid unaligned access? Something like th=
is:

TBH I don't think this single allocation helps too much, it's not on
the hot path,
but makes the code harder to read ;(

>
> ```
>         /* Single allocation for HeapBatch header + tupdata array */
>         alloc_size =3D MAXALIGN(sizeof(HeapBatch)) + sizeof(HeapTupleData=
) * maxitems;
>         hb =3D palloc(alloc_size);
>         hb->tupdata =3D (HeapTupleData *) ((char *) hb + MAXALIGN(sizeof(=
HeapBatch)));
> ```
>
> Or how about just using zero-length array:
>
> ```
> typedef struct HeapBatch
> {
>         Buffer                  buf;
>         int                             maxitems;
>         int                             nitems;
>         HeapTupleData   tupdata[FLEXIBLE_ARRAY_MEMBER];
> } HeapBatch;
>
> // and
> hb =3D palloc(offsetof(HeapBatch, tupdata) + sizeof(HeapTupleData) * maxi=
tems);
> ```
>
> 2) pgstat_count_heap_getnext_batch()
>
> ```
> #define pgstat_count_heap_getnext_batch(rel, n)                          =
               \
>         do {                                                             =
                                                       \
>                 if (pgstat_should_count_relation(rel))                   =
                       \
>                         (rel)->pgstat_info->counts.tuples_returned +=3D n=
;                \
>         } while (0)
> ```
>
> "+=3D n" -> "+=3D (n)", just like pgstat_count_index_tuples().
>
> 0002
> =3D=3D=3D=3D
>
> 1) TupleBatchCreate()
>
> ```
>         /* Single allocation for TupleBatch + inslots + outslots arrays *=
/
>         alloc_size =3D sizeof(TupleBatch) + 2 * sizeof(TupleTableSlot *) =
* capacity;
>         b =3D palloc(alloc_size);
>         inslots =3D (TupleTableSlot **) ((char *) b + sizeof(TupleBatch))=
;
>         outslots =3D (TupleTableSlot **) ((char *) b + sizeof(TupleBatch)=
 +
>                 sizeof(TupleTableSlot *) * capacity);
> ```
>
> Do we need a MAXALIGN() here to avoid unaligned access?
>
> 2) TupleBatchReset()
>
> ```
>         for (int i =3D 0; i < b->maxslots; i++)
>         {
>                 ExecClearTuple(b->inslots[i]);
>                 if (drop_slots)
>                         ExecDropSingleTupleTableSlot(b->inslots[i]);
>         }
> ```
>
> ExecDropSingleTupleTableSlot() will call ExecClearTuple(), so ExecClearTu=
ple() will be
> called twice if drop_slots is true, I think we can avoid this.
>
> 3) ScanCanUseBatching()
>
> In heap_beginscan(), we may disable page-at-a-time mode:
>
> ```
>         /*
>          * Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
>          */
>         if (!(snapshot && IsMVCCSnapshot(snapshot)))
>                 scan->rs_base.rs_flags &=3D ~SO_ALLOW_PAGEMODE;
> ```
>
> It seems that ScanCanUseBatching() didn't consider this.
>
> 4) struct TupleBatch
>
> ```
>         struct TupleTableSlot **inslots; /* slots for tuples read "into" =
batch */
>         struct TupleTableSlot **outslots; /* slots for tuples going "out =
of"
>                                                                          =
  * batch */
>         struct TupleTableSlot **activeslots;
> ```
>
> I think we can remove the word "struct".
>
> 5) ExecScanExtendedBatchSlot()
>
> ```
>                 /* Get next input slot from current batch, or refill */
>                 if (!TupleBatchHasMore(b))
>                 {
>                         if (!accessBatchMtd(node))
>                                 return NULL;
>                 }
> ```
>
> I think we cannot just return NULL here, see comments in ExecScanExtended=
():
>
> ```
>                 /*
>                  * if the slot returned by the accessMtd contains NULL, t=
hen it means
>                  * there is nothing more to scan so we just return an emp=
ty slot,
>                  * being careful to use the projection result slot so it =
has correct
>                  * tupleDesc.
>                  */
>                 if (TupIsNull(slot))
>                 {
>                         if (projInfo)
>                                 return ExecClearTuple(projInfo->pi_state.=
resultslot);
>                         else
>                                 return slot;
>                 }
> ```
>
> And why not just write this function like ExecScanExtended() and ExecScan=
Fetch()?
>
> --
> Regards,
> ChangAo Chen


--=20
Regards
Junwang Zhao