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>
 <CAJDiXgh7vJAu39QkEk4TiJexPVsM6X6RXeNy7dNX9KjhX_5=9A@mail.gmail.com>
 <CA+HiwqED=0D1jQsYoXGuATxKr=Pxhm24Kwh01212KBAJ-bK9Bg@mail.gmail.com>
 <CAJDiXgh6-JS3Z4L+HPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ+w@mail.gmail.com>
In-Reply-To: 
 <CAJDiXgh6-JS3Z4L+HPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ+w@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Sat, 20 Dec 2025 23:36:13 +0900
Message-ID: 
 <CA+HiwqFGo-QHirV8dkYWA+tkRDujnGetMLWzLen9B7+MtsW-Xg@mail.gmail.com>
Subject: Re: Batching in executor
To: Daniil Davydov <3danissimo@gmail.com>
Cc: Tomas Vondra <tomas@vondra.me>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqFGo-QHirV8dkYWA%2BtkRDujnGetMLWzLen9B7%2BMtsW-Xg%40mail.gmail.com>
Precedence: bulk

Hi Daniil,


On Thu, Oct 30, 2025 at 9:12=E2=80=AFPM Daniil Davydov <3danissimo@gmail.co=
m> wrote:
> On Wed, Oct 29, 2025 at 9:23=E2=80=AFAM Amit Langote <amitlangote09@gmail=
.com> wrote:
> >
> > Hi Daniil,
> >
> > On Tue, Oct 28, 2025 at 11:32=E2=80=AFPM Daniil Davydov <3danissimo@gma=
il.com> wrote:
> > >
> > > Hi,
> > >
> > > As far as I understand, this work partially overlaps with what we did=
 in the
> > > thread [1] (in short - we introduce support for batching within the M=
odifyTable
> > > node). Am I correct?
> >
> > There might be some relation, but not much overlap. The thread you
> > mention seems to focus on batching in the write path (for INSERT,
> > etc.), while this work targets batching in the read path via Table AM
> > scan callbacks. I think they can be developed independently, though
> > I'm happy to take a look.
>
> Oh, I got it. Thanks!
>
> I looked at 0001-0003 patches and got some comments :
> 1)
> I noticed that some Nodes may set SO_ALLOW_PAGEMODE flag to 'false'
> during ExecReScan. heap_getnextslot works carefully with it - checks whet=
her
> pagemode is allowed at every call. If not - it just uses tuple-at-a-time =
mode.
> At the same time, heap_getnextbatch always expects that pagemode is enabl=
ed.
> I didn't find any code paths which can lead to an assertion [1] fail.
> If such a code
> path is unreachable under any circumstances, maybe we should add a commen=
t
> why?
>
> 2)
> heapgettup_pagemode_batch : Do we really need to compute lineindex variab=
le
> in this way? :
> ***
>             lineindex =3D scan->rs_cindex + dir;
>             if (ScanDirectionIsForward(dir))
>                 linesleft =3D (lineindex <=3D (uint32) scan->rs_ntuples) =
?
>                     (scan->rs_ntuples - lineindex) : 0;
> ***
>
> As far as I understand, this is enough :
> ***
>         lineindex =3D scan->rs_cindex + dir;
>         if (ScanDirectionIsForward(dir))
>             linesleft =3D scan->rs_ntuples - lineindex;
> ***
>
> 3)
> Is this code inside heapgettup_pagemode_batch necessary? :
> ***
> ScanDirectionIsForward(dir) ? 0 : 0
> ***
>
> 4)
> heapgettup_pagemode has this change :
> HeapTuple    tuple =3D &(scan->rs_ctup) ---> HeapTuple tuple =3D &scan->r=
s_ctup
> I guess it was changed accidentally.
>
> 5)
> I apologize for the tediousness, but these braces are not in the
> postgres style :
> ***
> static const TupleBatchOps TupleBatchHeapOps =3D {
>     .materialize_all =3D heap_materialize_batch_all
> };
> ***
>
> [1] heap_getnextbatch : Assert(sscan->rs_flags & SO_ALLOW_PAGEMODE)

Thanks for the review and apologies for getting to them so late.

I think I've addressed your comments in v4 that I just posted.

--=20
Thanks, Amit Langote