Re: Pluggable Storage - Andres's take

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.

> There's a few further changes since last time: - Pluggable handlers are
> now stored in static global variables, and thus do not need to be copied
> anymore - VACUUM FULL / CLUSTER is moved into one callback that does the
> actual copying. The various previous rewrite callbacks imo integrated at
> the wrong level. - there's a GUC that allows to change the default
> table AM - moving COPY to use slots (roughly based on your / Haribabu's
> patch) - removed the AM specific shmem initialization callbacks -
> various AMs are going to need the syncscan APIs, so moving that into AM
> callbacks doesn't make sense.
>

OK.

> Missing:
> - callback for the second scan of CREATE INDEX CONCURRENTLY
> - commands/analyze.c integration (Working on it)
> - fixing your (Haribabu's) slotification of copy patch to compute memory
> usage somehow
>

I will check it.

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

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

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

I'm starting to think that we're getting closer to something that
> looks right from a high level, even though there's a lot of details to
> clean up.
>

That's good.

Regards,
Haribabu Kommi
Fujitsu Australia