On Mon, Jun 10, 2019 at 2:56 PM Andres Freund <andres@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.
