Re: Batching in executor - Mailing list pgsql-hackers

Amit Langote <amitlangote09@gmail.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