Pluggable Storage - Andres's take

Hi,

As I've previously mentioned I had planned to spend some time to polish
Haribabu's version of the pluggable storage patch and rebase it on the
vtable based slot approach from [1]. While doing so I found more and
more things that I previously hadn't noticed. I started rewriting things
into something closer to what I think we want architecturally.

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.

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.

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.

- Previous verions of the patchset exposed Buffers in the tableam.h API,
performed buffer locking / pinning / ExecStoreTuple() calls outside of
it. That is wrong in my opinion, as various AMs will deal very
differently with buffer pinning & locking. The relevant logic is
largely moved within the AM. Bringing me to the next point:

- tableam exposed various operations based on HeapTuple/TableTuple's
(and their Buffers). This all need be slot based, as we can't
represent the way each AM will deal with this. I've largely converted
the API to be slot based. That has some fallout, but I think largely
works. Lots of outdated comments.

- I think the move of the indexing from outside the table layer into the
storage layer isn't a good idea. It lead to having to pass EState into
the tableam, a callback API to perform index updates, etc. This seems
to have at least partially been triggered by the speculative insertion
codepaths. I've reverted this part of the changes. The speculative
insertion / confirm codepaths are now exposed to tableam.h - I think
that's the right thing because we'll likely want to have that
functionality across more than a single tuple in the future.

- The visibility functions relied on the *caller* performing buffer
locking. That's not a great idea, because generic code shouldn't know
about the locking scheme a particular AM needs. I've changed the
external visibility functions to instead take a slot, and perform the
necessary locking inside.

- There were numerous tableam callback uses inside heapam.c - that makes
no sense, we know what the storage is therein. The relevant

- The integration between index lookups and heap lookups based on the
results on a index lookup was IMO too tight. The index code dealt
with heap tuples, which isn't great. I've introduced a new concept, a
'IndexFetchTableData' scan. It's initialized when building an index
scan, and provides the necessary state (say current heap buffer), to
do table lookups from within a heap.

- The am of relations required for bootstrapping was set to 0 - I don't
think that's a good idea. I changed it so it's set to the heap AM as
well.

- HOT was encoded in the API in a bunch of places. That doesn't look
right to me. I tried to improve a bit on that, but I'm not yet quite
sure I like it. Needs written explanation & arguments...

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

- I've for now backed out the heap rewrite changes, partially. Mostly
because I didn't like the way the abstraction looks, but haven't quite
figured out how it should look like.

- I did not like that speculative tokens were moved to slots. There's
really no reason for them to live outside parameters to tableam.h
functsions.

- lotsa additional smaller changes.

- lotsa new bugs

My current working state is at [3] (urls to clone repo are at [4]).
This is *HEAVILY WIP*. I plan to continue working on it over the next
days, but I'll temporarily focus onto v11 work. If others want I could
move repo to github and grant others write access.

I think the patchseries should eventually look like:

- move vacuumlazy.c (and other similar files) into access/heap, there's
really nothing generic here. This is a fairly independent task.
- slot'ify FDW RefetchForeignRow_function
- vtable based slot API, based on [1]
- slot'ify trigger API
- redo EPQ based on slots (prototyped in my git tree)
- redo trigger API to be slot based
- tuple traversal API changes
- tableam infrastructure, with error if a non-builtin AM is chosen
- move heap and calling code to be tableam based
- make vacuum callback based (not vacuum.c, just vacuumlazy.c)
- [other patches]
- allow other AMs
- introduce test AM

Tasks / Questions:

- split up patch
- 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.
- Merge tableam.h and tableamapi.h and make most tableam.c functions
small inline functions. Having one-line tableam.c wrappers makes this
more expensive than necessary. We'll have a big enough trouble not
regressing performancewise.
- change scan level slot creation to use tableam function for doing so
- get rid of slot->tts_tid, tts_tupleOid and potentially tts_tableOid
- COPY's multi_insert path should probably deal with a bunch of slots,
rather than forming HeapTuples
- bitmap index scans probably need a new tableam.h callback, abstracting
bitgetpage()
- suspect IndexBuildHeapScan might need to move into the tableam.h API -
it's not clear to me that it's realistically possible to this in a
generic manner.

Greetings,

Andres Freund

[1] http://archives.postgresql.org/message-id/20180220224318.gw4oe5jadhpmcdnm%40alap3.anarazel.de
[2] http://archives.postgresql.org/message-id/CAJrrPGcN5A4jH0PJ-s=6k3+SLA4pozC4HHRdmvU1ZBuA20TE-A@mail.gmail.com
[3] https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/pluggable-storage
[4] https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary