On Wed, Sep 25, 2019 at 1:52 PM Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
> Sounds OK ... except that Travis points out that Ashwin forgot to patch
> contrib:
>
> make[4]: Entering directory
> '/home/travis/build/postgresql-cfbot/postgresql/contrib/amcheck'
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
> -fexcess-precision=standard -g -O2 -Wall -Werror -fPIC -I. -I.
> -I../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE -c -o
> verify_nbtree.o verify_nbtree.c
> verify_nbtree.c: In function ‘bt_check_every_level’:
> verify_nbtree.c:614:11: error: passing argument 6 of
> ‘table_index_build_scan’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
> bt_tuple_present_callback, (void *) state, scan);
> ^
> In file included from verify_nbtree.c:29:0:
> ../../src/include/access/tableam.h:1499:1: note: expected
> ‘IndexBuildCallback {aka void (*)(struct RelationData *, struct
> ItemPointerData *, long unsigned int *, _Bool *, _Bool, void *)}’ but
> argument is of type ‘void (*)(struct RelationData *, HeapTupleData *, Datum
> *, _Bool *, _Bool, void *) {aka void (*)(struct RelationData *, struct
> HeapTupleData *, long unsigned int *, _Bool *, _Bool, void *)}’
> table_index_build_scan(Relation table_rel,
> ^
> cc1: all warnings being treated as errors
> <builtin>: recipe for target 'verify_nbtree.o' failed
> make[4]: *** [verify_nbtree.o] Error 1
>
Thanks for reporting, I did indeed missed out contrib. Please find attached
the v2 of the patch which includes the change required in contrib as well.
From 873711e0601d5035a302c9f2a4147250e902a28f Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Wed, 25 Sep 2019 22:19:43 -0700
Subject: [PATCH v2] 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.
---
contrib/amcheck/verify_nbtree.c | 6 +++---
contrib/bloom/blinsert.c | 4 ++--
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 +-
10 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 05e7d678ed..3542545de5 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -140,7 +140,7 @@ static BTScanInsert bt_right_page_check_scankey(BtreeCheckState *state);
static void bt_downlink_check(BtreeCheckState *state, BTScanInsert targetkey,
BlockNumber childblock);
static void bt_downlink_missing_check(BtreeCheckState *state);
-static void bt_tuple_present_callback(Relation index, HeapTuple htup,
+static void bt_tuple_present_callback(Relation index, ItemPointer tid,
Datum *values, bool *isnull,
bool tupleIsAlive, void *checkstate);
static IndexTuple bt_normalize_tuple(BtreeCheckState *state,
@@ -1890,7 +1890,7 @@ bt_downlink_missing_check(BtreeCheckState *state)
* also allows us to detect the corruption in many cases.
*/
static void
-bt_tuple_present_callback(Relation index, HeapTuple htup, Datum *values,
+bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
bool *isnull, bool tupleIsAlive, void *checkstate)
{
BtreeCheckState *state = (BtreeCheckState *) checkstate;
@@ -1901,7 +1901,7 @@ bt_tuple_present_callback(Relation index, HeapTuple htup, Datum *values,
/* Generate a normalized index tuple for fingerprinting */
itup = index_form_tuple(RelationGetDescr(index), values, isnull);
- itup->t_tid = htup->t_self;
+ itup->t_tid = *tid;
norm = bt_normalize_tuple(state, itup);
/* Probe Bloom filter -- tuple should be present */
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 4b2186b8dd..66c6c1858d 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -73,7 +73,7 @@ initCachedPage(BloomBuildState *buildstate)
* Per-tuple callback for table_index_build_scan.
*/
static void
-bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
+bloomBuildCallback(Relation index, ItemPointer tid, Datum *values,
bool *isnull, bool tupleIsAlive, void *state)
{
BloomBuildState *buildstate = (BloomBuildState *) state;
@@ -82,7 +82,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
oldCtx = MemoryContextSwitchTo(buildstate->tmpCtx);
- itup = BloomFormTuple(&buildstate->blstate, &htup->t_self, values, isnull);
+ itup = BloomFormTuple(&buildstate->blstate, tid, values, isnull);
/* Try to add next item to cached page */
if (BloomPageAddItem(&buildstate->blstate, buildstate->data.data, itup))
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ae7b729edd..d27d503c7f 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 55eab14617..d19cbcb2f9 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 2f4543dee5..739846a257 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,
@@ -440,7 +440,7 @@ calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep)
*/
static void
gistBuildCallback(Relation index,
- HeapTuple htup,
+ ItemPointer tid,
Datum *values,
bool *isnull,
bool tupleIsAlive,
@@ -454,7 +454,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 5cc30dac42..30f994edb5 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 2dd8821fac..07eb903a4d 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1636,10 +1636,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]))
@@ -1650,17 +1649,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 ab19692006..f47972b362 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 b40bd440cf..dd9088741c 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 7f81703b78..64022917e2 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
