Date: Mon, 16 Jul 2018 06:35:27 -0700
From: Andres Freund <andres@anarazel.de>
To: Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Pluggable Storage - Andres's take
Message-ID: <20180716133527.csbps4chmr7qsa7e@alap3.anarazel.de>
References: <20180703070645.wchpu5muyto5n647@alap3.anarazel.de>
 <CAJrrPGeSycj+JiTZ2M+yX_z457EY3vEP0PzCdfoguO055f=5Wg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAJrrPGeSycj+JiTZ2M+yX_z457EY3vEP0PzCdfoguO055f=5Wg@mail.gmail.com>
Precedence: bulk

Hi,

I've pushed up a new version to
https://github.com/anarazel/postgres-pluggable-storage which now passes
all the tests.

Besides a lot of bugfixes, I've rebased the tree, moved TriggerData to
be primarily slot based (with a conversion roundtrip when calling
trigger functions), and a lot of other small things.


On 2018-07-04 20:11:21 +1000, Haribabu Kommi wrote:
> On Tue, Jul 3, 2018 at 5:06 PM Andres Freund <andres@anarazel.de> wrote:
> > The current state of my version of the patch is *NOT* ready for proper
> > review (it doesn't even pass all tests, there's FIXME / elog()s).  But I
> > think it's getting close enough to it's eventual shape that more eyes,
> > and potentially more hands on keyboards, can be useful.
> >
> 
> I will try to update it to make sure that it passes all the tests and also
> try to
> reduce the FIXME's.

Cool.

Alexander, Haribabu, if you give me (privately) your github accounts,
I'll give you write access to that repository.


> > The most fundamental issues I had with Haribabu's last version from [2]
> > are the following:
> >
> > - The use of TableTuple, a typedef from void *, is bad from multiple
> >   fronts. For one it reduces just about all type safety. There were
> >   numerous bugs in the patch where things were just cast from HeapTuple
> >   to TableTuple to HeapTuple (and even to TupleTableSlot).  I think it's
> >   a really, really bad idea to introduce a vague type like this for
> >   development purposes alone, it makes it way too hard to refactor -
> >   essentially throwing the biggest benefit of type safe languages out of
> >   the window.
> >
> 
> My earlier intention was to remove the HeapTuple usage entirely and replace
> it with slot everywhere outside the tableam. But it ended up with TableTuple
> before it reach to the stage because of heavy use of HeapTuple.

I don't think that's necessary - a lot of the system catalog accesses
are going to continue to be heap tuple accesses. And the conversions you
did significantly continue to access TableTuples as heap tuples - it was
just that the compiler didn't warn about it anymore.

A prime example of that is the way the rewriteheap / cluster
integreation was done. Cluster continued to sort tuples as heap tuples -
even though that's likely incompatible with other tuple formats which
need different state.


> >   Additionally I think it's also the wrong approach architecturally. We
> >   shouldn't assume that a tuple can efficiently be represented as a
> >   single palloc'ed chunk. In fact, we should move *away* from relying on
> >   that so much.
> >
> >   I've thus removed the TableTuple type entirely.
> >
> 
> Thanks for the changes, I didn't check the code yet, so for now whenever the
> HeapTuple is required it will be generated from slot?

Pretty much.


> > - the heap tableam did a heap_copytuple() nearly everywhere. Leading to
> >   a higher memory usage, because the resulting tuples weren't freed or
> >   anything. There might be a reason for doing such a change - we've
> >   certainly discussed that before - but I'm *vehemently* against doing
> >   that at the same time we introduce pluggable storage. Analyzing the
> >   performance effects will be hard enough without changes like this.
> >
> 
> How about using of slot instead of tuple and reusing of it? I don't know
> yet whether it is possible everywhere.

Not quite sure what you mean?


> > Tasks / Questions:
> >
> > - split up patch
> >
> 
> How about generating refactoring changes as patches first based on
> the code in your repo as discussed here[1]?

Sure - it was just at the moment too much work ;)


> > - Change heap table AM to not allocate handler function for each table,
> >   instead allocate it statically. Avoids a significant amount of data
> >   duplication, and allows for a few more compiler optimizations.
> >
> 
> Some kind of static variable handlers for each tableam, but need to check
> how can we access that static handler from the relation.

I'm not sure what you mean by "how can we access"? We can just return a
pointer from the constant data from the current handler? Except that
adding a bunch of consts would be good, the external interface wouldn't
need to change?


> > - change scan level slot creation to use tableam function for doing so
> > - get rid of slot->tts_tid, tts_tupleOid and potentially tts_tableOid
> >
> 
> so with this there shouldn't be a way from slot to tid mapping or there
> should be some other way.

I'm not sure I follow?


> - bitmap index scans probably need a new tableam.h callback, abstracting
> >   bitgetpage()
> >
> 
> OK.

Any chance you could try to tackle this?  I'm going to be mostly out
this week, so we'd probably not run across each others feet...

Greetings,

Andres Freund