On Mon, Jun 10, 2019 at 5:38 PM Ashwin Agrawal <aagra...@pivotal.io> wrote:
>
> On Mon, Jun 10, 2019 at 2:56 PM Andres Freund <and...@anarazel.de> wrote:
>
>> > Currently, all AM needs to build HeapTuple in
>> > index_build_range_scan function. I looked into all the callback
>> functions
>> > and only htup->t_self is used from heaptuple in all the functions
>> (unless I
>> > missed something). So, if seems fine will be happy to write patch to
>> make
>> > that argument ItemPointer instead of HeapTuple?
>>
>> I wonder if it should better be the slot? It's not inconceivable that
>> some AMs could benefit from that. Although it'd add some complication
>> to the heap HeapTupleIsHeapOnly case.
>>
>
> I like the slot suggestion, only if can push FormIndexDatum() out of AM
> code as a result and pass slot to the callback. Not sure what else can it
> help. HeapTupleIsHeapOnly possibly can be made to work with slot similar to
> current hack of updating the t_self and slot's tid field, maybe.
>
> Index callback using the slot can form the index datum. Though that would
> mean every Index AM callback function needs to do it, not sure which way is
> better. Plus, need to create ExecutorState for the same. With current setup
> every AM needs to do. Feels good if belongs to indexing code though instead
> of AM.
>
> Currently, index build needing to create ExecutorState and all at AM layer
> seems not nice either. Maybe there is quite generic logic here and possible
> can be extracted into common code which either most of AM leverage. Or
> possibly the API itself can be simplified to get minimum input from AM and
> have rest of flow/machinery at upper layer. As Robert pointed at start of
> thread at heart its pretty simple flow and possibly will remain same for
> any AM.
>
>
Please find attached the patch to remove IndexBuildCallback's dependency on
HeapTuple, as discussed. Changed to have the argument as ItemPointer
instead of HeapTuple. Other larger refactoring if feasible for
index_build_range_scan API itself can be performed as follow-up changes.
From f73b0061795f0c320f96ecfed0c0602ae318d73e Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Thu, 11 Jul 2019 16:58:50 -0700
Subject: [PATCH v1] Remove IndexBuildCallback's dependency on HeapTuple.
With IndexBuildCallback taking input as HeapTuple, all table AMs
irrespective of storing the tuples in HeapTuple form or not, are
forced to construct HeapTuple, to insert the tuple in Index. Since,
only thing required by the index callbacks is TID and not really the
full tuple, modify callback to only take ItemPointer.
---
src/backend/access/brin/brin.c | 4 ++--
src/backend/access/gin/gininsert.c | 4 ++--
src/backend/access/gist/gistbuild.c | 6 +++---
src/backend/access/hash/hash.c | 8 ++++----
src/backend/access/heap/heapam_handler.c | 11 +++++------
src/backend/access/nbtree/nbtsort.c | 8 ++++----
src/backend/access/spgist/spginsert.c | 4 ++--
src/include/access/tableam.h | 2 +-
8 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ae7b729edd9..d27d503c7f1 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -597,7 +597,7 @@ brinendscan(IndexScanDesc scan)
*/
static void
brinbuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -607,7 +607,7 @@ brinbuildCallback(Relation index,
BlockNumber thisblock;
int i;
- thisblock = ItemPointerGetBlockNumber(&htup->t_self);
+ thisblock = ItemPointerGetBlockNumber(tid);
/*
* If we're in a block that belongs to a future range, summarize what
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index 55eab146173..d19cbcb2f90 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -273,7 +273,7 @@ ginHeapTupleBulkInsert(GinBuildState *buildstate, OffsetNumber attnum,
}
static void
-ginBuildCallback(Relation index, HeapTuple htup, Datum *values,
+ginBuildCallback(Relation index, ItemPointer tid, Datum *values,
bool *isnull, bool tupleIsAlive, void *state)
{
GinBuildState *buildstate = (GinBuildState *) state;
@@ -285,7 +285,7 @@ ginBuildCallback(Relation index, HeapTuple htup, Datum *values,
for (i = 0; i < buildstate->ginstate.origTupdesc->natts; i++)
ginHeapTupleBulkInsert(buildstate, (OffsetNumber) (i + 1),
values[i], isnull[i],
- &htup->t_self);
+ tid);
/* If we've maxed out our available memory, dump everything to the index */
if (buildstate->accum.allocatedMemory >= (Size) maintenance_work_mem * 1024L)
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index ecef0ff0724..2a004937078 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -80,7 +80,7 @@ typedef struct
static void gistInitBuffering(GISTBuildState *buildstate);
static int calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep);
static void gistBuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -459,7 +459,7 @@ calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep)
*/
static void
gistBuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -473,7 +473,7 @@ gistBuildCallback(Relation index,
/* form an index tuple and point it at the heap tuple */
itup = gistFormTuple(buildstate->giststate, index, values, isnull, true);
- itup->t_tid = htup->t_self;
+ itup->t_tid = *tid;
if (buildstate->bufferingMode == GIST_BUFFERING_ACTIVE)
{
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 5cc30dac429..30f994edb50 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -43,7 +43,7 @@ typedef struct
} HashBuildState;
static void hashbuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -201,7 +201,7 @@ hashbuildempty(Relation index)
*/
static void
hashbuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -220,14 +220,14 @@ hashbuildCallback(Relation index,
/* Either spool the tuple for sorting, or just put it into the index */
if (buildstate->spool)
- _h_spool(buildstate->spool, &htup->t_self,
+ _h_spool(buildstate->spool, tid,
index_values, index_isnull);
else
{
/* form an index tuple and point it at the heap tuple */
itup = index_form_tuple(RelationGetDescr(index),
index_values, index_isnull);
- itup->t_tid = htup->t_self;
+ itup->t_tid = *tid;
_hash_doinsert(index, itup, buildstate->heapRel);
pfree(itup);
}
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 09bc6fe98a7..7e18538455b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1634,10 +1634,9 @@ heapam_index_build_range_scan(Relation heapRelation,
* For a heap-only tuple, pretend its TID is that of the root. See
* src/backend/access/heap/README.HOT for discussion.
*/
- HeapTupleData rootTuple;
+ ItemPointerData tid;
OffsetNumber offnum;
- rootTuple = *heapTuple;
offnum = ItemPointerGetOffsetNumber(&heapTuple->t_self);
if (!OffsetNumberIsValid(root_offsets[offnum - 1]))
@@ -1648,17 +1647,17 @@ heapam_index_build_range_scan(Relation heapRelation,
offnum,
RelationGetRelationName(heapRelation))));
- ItemPointerSetOffsetNumber(&rootTuple.t_self,
- root_offsets[offnum - 1]);
+ ItemPointerSet(&tid, ItemPointerGetBlockNumber(&heapTuple->t_self),
+ root_offsets[offnum - 1]);
/* Call the AM's callback routine to process the tuple */
- callback(indexRelation, &rootTuple, values, isnull, tupleIsAlive,
+ callback(indexRelation, &tid, values, isnull, tupleIsAlive,
callback_state);
}
else
{
/* Call the AM's callback routine to process the tuple */
- callback(indexRelation, heapTuple, values, isnull, tupleIsAlive,
+ callback(indexRelation, &heapTuple->t_self, values, isnull, tupleIsAlive,
callback_state);
}
}
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index d0b9013caf4..7943d471c1f 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -278,7 +278,7 @@ static void _bt_spooldestroy(BTSpool *btspool);
static void _bt_spool(BTSpool *btspool, ItemPointer self,
Datum *values, bool *isnull);
static void _bt_leafbuild(BTSpool *btspool, BTSpool *btspool2);
-static void _bt_build_callback(Relation index, HeapTuple htup, Datum *values,
+static void _bt_build_callback(Relation index, ItemPointer tid, Datum *values,
bool *isnull, bool tupleIsAlive, void *state);
static Page _bt_blnewpage(uint32 level);
static BTPageState *_bt_pagestate(BTWriteState *wstate, uint32 level);
@@ -594,7 +594,7 @@ _bt_leafbuild(BTSpool *btspool, BTSpool *btspool2)
*/
static void
_bt_build_callback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -607,12 +607,12 @@ _bt_build_callback(Relation index,
* processing
*/
if (tupleIsAlive || buildstate->spool2 == NULL)
- _bt_spool(buildstate->spool, &htup->t_self, values, isnull);
+ _bt_spool(buildstate->spool, tid, values, isnull);
else
{
/* dead tuples are put into spool2 */
buildstate->havedead = true;
- _bt_spool(buildstate->spool2, &htup->t_self, values, isnull);
+ _bt_spool(buildstate->spool2, tid, values, isnull);
}
buildstate->indtuples += 1;
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index b40bd440cf0..dd9088741cf 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -40,7 +40,7 @@ typedef struct
/* Callback to process one heap tuple during table_index_build_scan */
static void
-spgistBuildCallback(Relation index, HeapTuple htup, Datum *values,
+spgistBuildCallback(Relation index, ItemPointer tid, Datum *values,
bool *isnull, bool tupleIsAlive, void *state)
{
SpGistBuildState *buildstate = (SpGistBuildState *) state;
@@ -55,7 +55,7 @@ spgistBuildCallback(Relation index, HeapTuple htup, Datum *values,
* lock on some buffer. So we need to be willing to retry. We can flush
* any temp data when retrying.
*/
- while (!spgdoinsert(index, &buildstate->spgstate, &htup->t_self,
+ while (!spgdoinsert(index, &buildstate->spgstate, tid,
*values, *isnull))
{
MemoryContextReset(buildstate->tmpCtx);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c2b0481e7e5..14f23362357 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -141,7 +141,7 @@ typedef struct TM_FailureData
/* Typedef for callback function for table_index_build_scan */
typedef void (*IndexBuildCallback) (Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
--
2.19.1
