Re: Pluggable Storage - Andres's take

On Tue, Aug 28, 2018 at 1:48 PM Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:

> On Fri, Aug 24, 2018 at 12:50 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> Hi,
>>
>> On 2018-08-24 11:55:41 +1000, Haribabu Kommi wrote:
>> > On Tue, Aug 21, 2018 at 6:59 PM Andres Freund <andres(at)anarazel(dot)de>
>> wrote:
>> >
>> > > On 2018-08-21 16:55:47 +1000, Haribabu Kommi wrote:
>> > > > On Sun, Aug 5, 2018 at 7:48 PM Andres Freund <andres(at)anarazel(dot)de>
>> wrote:
>> > > > > I'm currently in the process of rebasing zheap onto the pluggable
>> > > > > storage work. The goal, which seems to work surprisingly well, is
>> to
>> > > > > find issues that the current pluggable storage patch doesn't yet
>> deal
>> > > > > with. I plan to push a tree including a lot of fixes and
>> improvements
>> > > > > soon.
>> > > > >
>> > > > That's good. Did you find any problems in porting zheap into
>> pluggable
>> > > > storage? Does it needs any API changes or new API requirement?
>> > >
>> > > A lot, yes. The big changes are:
>> > > - removal of HeapPageScanDesc
>> > > - introduction of explicit support functions for tablesample & bitmap
>> scans
>> > > - introduction of callbacks for vacuum_rel, cluster
>> > >
>> > > And quite a bit more along those lines.
>> > >
>> >
>> > OK. Those are quite a bit of changes.
>>
>> I've pushed a current version of that to my git tree to the
>> pluggable-storage branch. It's not really a version that I think makese
>> sense to review or such, but it's probably more useful if you work based
>> on that. There's also the pluggable-zheap branch, which I found
>> extremely useful to develop against.
>>
>
> OK. Thanks, will check that also.
>
>
>> - fixing your (Haribabu's) slotification of copy patch to compute memory
>> usage somehow
>>
>
> I will check it.
>

Attached is the copy patch that brings back the size validation.
Compute the tuple size from the first tuple in the batch and use the same
for the
rest of the tuples in that batch. This way the calculation overhead is also
reduced,
but there may be chances that the first tuple size is very low and rest can
be very
long, but I feel those are rare.

>
>
>> - table creation callback, currently the pluggable-zheap patch has a few
>> conditionals outside of access/zheap for that purpose (see
>> RelationTruncate
>>
>
> I will check it.
>

I found couple of places where the zheap is using some extra logic in
verifying
whether it is zheap AM or not, based on that it used to took some extra
decisions.
I am analyzing all the extra code that is done, whether any callbacks can
handle it
or not? and how? I can come back with more details later.

>
>
>> And then:
>> - lotsa cleanups
>> - rebasing onto a newer version of the abstract slot patchset
>> - splitting out smaller patches
>>
>>
>> You'd moved the bulk insert into tableam callbacks - I don't quite get
>> why? There's not really anything AM specific in that code?
>>
>
> The main reason of adding them to AM is just to provide a control to
> the specific AM to decide whether they can support the bulk insert or
> not?
>
> Current framework doesn't support AM specific bulk insert state to be
> passed from one function to another and it's structure is fixed. This needs
> to be enhanced to add AM specific private members also.
>

Do you want me to work on it to make it generic to AM methods to extend
the structure?

>
>
>> > > > Does the new TupleTableSlot abstraction patches has fixed any of
>> these
>> > > > issues in the recent thread [1]? so that I can look into the change
>> of
>> > > > FDW API to return slot instead of tuple.
>> > >
>> > > Yea, that'd be a good thing to start with.
>> > >
>> >
>> > I found out only the RefetchForeignRow API needs the change and done the
>> > same.
>> > Along with that, I fixed all the issues of running make check-world.
>> > Attached patches
>> > for the same.
>>
>> Thanks, that's really helpful! I'll try to merge these soon.
>>
>
> I can share the rebased patches for the fixes, so that it will be easy to
> merge.
>

Rebased FDW and check-world fixes patch is attached.
I will continue working on the rest of the miss items.

Regards,
Haribabu Kommi
Fujitsu Australia