Re: Batching in executor

Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> The last version on this thread (v7, the "Rebased" post) used the
> RowBatch design: the AM handed the executor a RowBatch carrying a
> slice of tuples, a single scan slot was re-pointed at the current
> tuple through a repoint_slot AM callback, and an executor_batch_rows
> GUC controlled the batch size. As I described in my pgconf.dev talk,
> I have regrouped around a smaller, incremental foundation and dropped
> that design. This series is the result; it supersedes v7 rather than
> extending it.

Interesting project! I've spent some time looking at the code:

0001:

* Since the BatchMVCCState structure ends up with a single field, the comment
about not increasing the number of arguments is no longer valid. (I'm not
sure though if the structer should be removed now.)

@@ -498,14 +500,13 @@ extern bool HeapTupleIsSurelyDead(HeapTuple htup,
*/
typedef struct BatchMVCCState
{
- HeapTupleData tuples[MaxHeapTuplesPerPage];
bool visible[MaxHeapTuplesPerPage];
} BatchMVCCState;

0003:

* I'm not sure the batch-specific callbacks should be mandatory

diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 5450a27faeb..9ef6b5cca65 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -45,6 +45,8 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->scan_end != NULL);
Assert(routine->scan_rescan != NULL);
Assert(routine->scan_getnextslot != NULL);
+ Assert(routine->scan_getnextbatch != NULL);
+ Assert(routine->batch_slot_callbacks != NULL);

* I think it's clearer to retur NULL here. (The current coding works because
TupIsNull() is called upper in the stack.)

diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index 5bcb0a861d7..943944ededa 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -85,11 +85,22 @@ SeqNext(SeqScanState *node)
}
...
+ ExecClearTuple(slot);
+
+ if (!table_scan_getnextbatch(scandesc, direction, slot))
+ return slot;
...

* I suggest that ExecStoreBatchBufferHeapTuples() checks the kind of slot, so
it cannot be called on batch-unaware slots. Likewise, the existing
ExecStore...Tuple() functions (plus ExecForceStoreHeapTuple()) should not
accept the batch-aware slot. Maybe you need a separate macro for this
(e.g. TTS_IS_BATCHBUFFERTUPLE).

* I wonder if the 'tuples', 'ntuples' and 'cindex' fields should be wrapped in
a structure, as in the future we'll need to add them to other slot types,
when implementing their batch-aware variants. (The 'tuples' member of such a
structure can point to an array of heap tuples now, but would need to be
more generic in the future, to handle other kinds of tuples, e.g. "minimal
tuple".)

@@ -309,6 +311,34 @@ typedef struct BufferHeapTupleTableSlot
Buffer buffer; /* tuple's buffer, or InvalidBuffer */
} BufferHeapTupleTableSlot;

...

+typedef struct BatchBufferHeapTupleTableSlot
+{
+ BufferHeapTupleTableSlot base;
+
+ HeapTupleData *tuples; /* points into HeapScanDesc->rs_vistuples[] */
+ int ntuples; /* number of tuples in this batch */
+ int cindex; /* index of last-returned tuple, or -1 if
+ * none returned from this batch yet */
+} BatchBufferHeapTupleTableSlot;

* In the commit message you say "materialize copies the current tuple and
retains the buffer pin, since the remaining batch entries still reference the
page". Shouldn't it release the pin if after having materialized the _last_
tuple of the batch?

--
Antonin Houska
Web: https://www.cybertec-postgresql.com