Re: TupleTableSlot abstraction

On Thu, Jul 5, 2018 at 4:07 PM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

>>> I haven't done that, but I think we should split ExecStoreTuple() into
>>> multiple versions, that only work on specific types of tuple
>>> slots. E.g. seqscan et al would call ExecStoreBufferHeapTuple(), other
>>> code dealing with tuples would call ExecStoreHeapTuple(). The relevant
>>> callers already need to know what kind of tuple they're dealing with,
>>> therefore that's not a huge burden.
>>
>> I thought so too, but haven't done that change right now. Will work on
>> that. That's in my TODO list.

Done. Added that as a separate patch 0001 since that change adds value
by itself.

0001 in the earlier patch set got committed, 0002 in that patch set is
not required anymore.

0002 - 0004 in this patch set are same as 0003-0005 in the previous patch set.

0005 in this patch set is 0006 in the previous one with a bunch of
TODO's addressed. An important change is virtual tuple slot contents
are never required to be freed when slot is cleared.

0006-0009 are same as 0007 - 0010 in the previous patch set.

>>
>> Next steps
>> 1. Address TODO in the code. I have listed some of those above.
>
> There are still a handful of TODOs in the patches. I will work on those next.

The number of TODOs has reduced, but there are still some that I am working on.

>
>>
>> 2. Right now we are using TupleTableSlotType, an enum, to create slots
>> of required type. But extensions which want to add their own slot
>> types won't be able to add a type in that enum. So, they will need to
>> write their own MakeTupleTableSlot. That function uses the
>> TupleTableSlotType to set TupleTableSlotOps and calculate the minimum
>> size of slot. We could change the function to accept TupleTableSlotOps
>> and the minimum size and it just works for all the extensions. Or it
>> could just accept TupleTableSlotOps and there's a callback to
>> calculate minimum memory required for the slot (say based on the tuple
>> descriptor available).

This is still TODO.

>
>>
>> 3. compile with LLVM and fix any compilation and regression errors.
>
> When I compiled server with just 0003 applied with LLVM, the
> compilation went well, but there was a server crash. That patch
> changes type of tts_nvalid from int32 to AttrNumber. I tried debugging
> the crash with a debug LLVM build, but couldn't complete the work.
> Attached patch attrnumber_llvm_type.patch is my incomplete attempt to
> fix that crash. I think, we should make it easy to change the data
> types of the members in structures shared by JIT and non-JIT code, may
> be automatically create both copies of the code somehow. I will get
> back to this after addressing other TODOs.
>

This is still a TODO

>>
>> 4. We need to do something with the name ExecStoreVirtualSlot(), which
>> is being (and will be) used for all kinds of TupleTableSlot type.
>> Right now I don't have any bright ideas.
>

Done and added as a separate patch 0010. Separate patch so that we can
discard this change, if we don't agree on it.

>>
>> 5. ExecFetch* functions are now one liners, so we could make those
>> inline and keep those in header file like, say slot_getattr.
>

Done.

>
>>
>> 6. ExecCopySlot can be a thin wrapper if we add a callback copyslot()
>> and invoked on the destination slot type.

This is still a TODO

>
>>
>> 7. slot_attisnull() deforms a heap/minimal tuple if that status for
>> given attribute is not available tts_isnull. Should we instead add a
>> callback attisnull() which can use something like heap_isnull()?
>

This is still a TODO.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company