Hi,
Some comments for v5:
0001
====
1) heap_begin_batch()
```
/* Single allocation for HeapBatch header + tupdata array */
alloc_size = sizeof(HeapBatch) + sizeof(HeapTupleData) * maxitems;
hb = palloc(alloc_size);
hb->tupdata = (HeapTupleData *) ((char *) hb + sizeof(HeapBatch));
```
Do we need a MAXALIGN() here to avoid unaligned access? Something like this:
```
/* Single allocation for HeapBatch header + tupdata array */
alloc_size = MAXALIGN(sizeof(HeapBatch)) + sizeof(HeapTupleData) *
maxitems;
hb = palloc(alloc_size);
hb->tupdata = (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 = palloc(offsetof(HeapBatch, tupdata) + sizeof(HeapTupleData) * maxitems);
```
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 += n;
\
} while (0)
```
"+= n" -> "+= (n)", just like pgstat_count_index_tuples().
0002
====
1) TupleBatchCreate()
```
/* Single allocation for TupleBatch + inslots + outslots arrays */
alloc_size = sizeof(TupleBatch) + 2 * sizeof(TupleTableSlot *) *
capacity;
b = palloc(alloc_size);
inslots = (TupleTableSlot **) ((char *) b + sizeof(TupleBatch));
outslots = (TupleTableSlot **) ((char *) b + sizeof(TupleBatch) +
sizeof(TupleTableSlot *) * capacity);
```
Do we need a MAXALIGN() here to avoid unaligned access?
2) TupleBatchReset()
```
for (int i = 0; i < b->maxslots; i++)
{
ExecClearTuple(b->inslots[i]);
if (drop_slots)
ExecDropSingleTupleTableSlot(b->inslots[i]);
}
```
ExecDropSingleTupleTableSlot() will call ExecClearTuple(), so ExecClearTuple()
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 &= ~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, then it
means
* there is nothing more to scan so we just return an empty
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
ExecScanFetch()?
--
Regards,
ChangAo Chen
