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>
In-Reply-To: <tencent_B45268659AF0400F68EC1643950ABB6A1B09@qq.com>
From: Daniil Davydov <3danissimo@gmail.com>
Date: Mon, 26 Jan 2026 16:34:44 +0700
Message-ID: <CAJDiXggP41+-HrRzT+BmtgmS8wkoZM7b4skkQA5NAe+NFEMPSQ@mail.gmail.com>
Subject: Re: Batching in executor
To: cca5507 <2624345507@qq.com>
Cc: Amit Langote <amitlangote09@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/CAJDiXggP41%2B-HrRzT%2BBmtgmS8wkoZM7b4skkQA5NAe%2BNFEMPSQ%40mail.gmail.com>
Precedence: bulk

Hi,

On Mon, Dec 22, 2025 at 6:46=E2=80=AFPM cca5507 <2624345507@qq.com> wrote:
>
> Some comments for v4:
>

Agree with your (1)-(4) comments.

> 5) heapgettup_pagemode_batch()
> If the scan key filters out all tuples on a page, we may return 0 before =
reaching the end of scan, right?
>

Yes. I think that we should advance to the next page if "nout =3D=3D 0"
at the end of walking through the rs_vistuples.

> 6) heap_begin_batch()
> ```
>         hb =3D palloc(sizeof(HeapBatch));
>         hb->tupdata =3D palloc(sizeof(HeapTupleData) * maxitems);
> ```
> Can we just use one palloc() for cache-friendly?
>

Actually, we are using memory context when calling the palloc function.
I.e. in the general case it will not cause memory allocation. But of course
there is no guarantee for it. I saw a lot of places in the code where we
are calling the palloc function several times in a row, so I guess that
this is OK.

If you will decide to leave these palloc calls, I suggest using the
palloc_object/palloc_array functions.

A few other comments on 0001 patch:

1)
+    void       *(*scan_begin_batch)(TableScanDesc sscan, int maxitems);
Is it syntactically correct?

2)
    /* Initialize static fields of HeapTupleData. Row bodies remain on page=
. */
    relid =3D RelationGetRelid(sscan->rs_rd);
    for (int i =3D 0; i < maxitems; i++)
        hb->tupdata[i].t_tableOid =3D relid;

Is it really necessary? I see that we are setting this field inside the
heapgettup_pagemode_batch function.

A few comment on 0002 patch:

1)
I guess that you should rebase your patches on the current master, because
the second patch doesn't apply.

2)
Maybe we can use tuplestore for tuples stored in TupleBatch? It is just a
proposal - I didn't check this idea carefully.

--
Best regards,
Daniil Davydov