From: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Subject: | Re: TupleTableSlot abstraction |
Date: | 2018-07-13 10:15:56 |
Message-ID: | CAFjFpRdjbrq-cmxBcs7D+08-Wf1Bs5YhosJ=oKrv_zaHOEtdbA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
pg_abstract_tts_patches_v3.tar.zip | application/zip | 61.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pierre Ducroquet | 2018-07-13 10:48:35 | Re: function lca('{}'::ltree[]) caused DB Instance crash |
Previous Message | 李海龙 | 2018-07-13 10:09:20 | function lca('{}'::ltree[]) caused DB Instance crash |