Re: Batching in executor

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