Re: heapam_index_build_range_scan's anyvisible

On Mon, Jun 10, 2019 at 5:38 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:

>
> 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.