Re: Pluggable Storage - Andres's take

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(at)anarazel(dot)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