public inbox for [email protected]
help / color / mirror / Atom feedFrom: =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
To: =?utf-8?B?QW1pdCBMYW5nb3Rl?= <[email protected]>
Cc: =?utf-8?B?UG9zdGdyZVNRTC1kZXZlbG9wbWVudA==?= <[email protected]>
Cc: =?utf-8?B?VG9tYXMgVm9uZHJh?= <[email protected]>
Subject: Re: Batching in executor
Date: Mon, 22 Dec 2025 19:45:49 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+HiwqEZja5rJ78p3FBDZNvynWsHwanxyt6h0YaK_r84NemXng@mail.gmail.com>
References: <CA+HiwqFfAY_ZFqN8wcAEMw71T9hM_kA8UtyHaZZEZtuT3UyogA@mail.gmail.com>
<[email protected]>
<CA+HiwqGqeS_94wxiYa8VpymiR_OtFPKDSpX+Me=MYWO45f5yig@mail.gmail.com>
<[email protected]>
<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>
Hi,
Some comments for v4:
0001
====
1) table_scan_getnextbatch()
"Assert(dir == ForwardScanDirection);" -> "Assert(ScanDirectionIsForward(dir));"
2) heapgettup_pagemode_batch()
"TupleDesc tupdesc = key ? RelationGetDescr(rel) : NULL;" -> "TupleDesc tupdesc = RelationGetDescr(rel);"
I think the latter is enough.
3) heapgettup_pagemode_batch()
```
/* Are there more visible tuples left on this page? */
lineindex = scan->rs_cindex + dir;
linesleft = (lineindex <= (uint32) scan->rs_ntuples) ?
(scan->rs_ntuples - lineindex) : 0;
if (linesleft > 0)
break; /* continue on this page */
```
The "scan->rs_ntuples" is already an uint32.
4) heapgettup_pagemode_batch()
```
Assert(lineindex <= (uint32) scan->rs_ntuples);
```
The "scan->rs_ntuples" is already an uint32. And I think this should be "Assert(lineindex < scan->rs_ntuples);", the related
assert in heapgettup_pagemode() is also wrong.
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?
6) heap_begin_batch()
```
hb = palloc(sizeof(HeapBatch));
hb->tupdata = palloc(sizeof(HeapTupleData) * maxitems);
```
Can we just use one palloc() for cache-friendly?
0002
====
1) heap_materialize_batch_all()
```
slot->base.tts_flags &= ~(TTS_FLAG_EMPTY | TTS_FLAG_SHOULDFREE);
slot->base.tts_tid = tuple->t_self;
slot->base.tts_tableOid = tuple->t_tableOid;
slot->base.tts_flags &= ~(TTS_FLAG_SHOULDFREE | TTS_FLAG_EMPTY);
```
Redundant of "slot->base.tts_flags &="?
2) TupleBatchCreate()
```
inslots = palloc(sizeof(TupleTableSlot *) * capacity);
outslots = palloc(sizeof(TupleTableSlot *) * capacity);
for (int i = 0; i < capacity; i++)
inslots[i] = MakeSingleTupleTableSlot(scandesc, &TTSOpsHeapTuple);
b = (TupleBatch *) palloc(sizeof(TupleBatch));
```
Can we just use one palloc() for cache-friendly?
3) TupleBatchCreate()
```
b->outslots = outslots;
b->activeslots = NULL;
b->outslots = outslots;
```
Redundant of "b->outslots = outslots;"?
4) TupleBatchReset()
```
if (b == NULL)
return;
```
This can never happen, convert to a assert or just delete it?
5) SeqNextBatch()
"Assert(direction == ForwardScanDirection);" -> "Assert(ScanDirectionIsForward(direction));"
--
Regards,
ChangAo Chen
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Batching in executor
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox