Re: Pluggable storage

Hi,

On 2017-06-13 11:50:27 +1000, Haribabu Kommi wrote:
> Here I attached WIP patches to support pluggable storage. The patch series
> are may not work individually. Still so many things are under development.
> These patches are just to share the approach of the current development.

Making a pass through the patchset to get a feel where this at, and
where this is headed. I previously skimmed the thread to get a rough
sense on what's discused, but not in a very detailed manner.

General:

- I think one important discussion we need to have is what kind of
performance impact we're going to accept introducing this. It seems
very likely that this'll cause some slowdown. We can kind of
alleviate that by doing some optimizations at the same time, but
nevertheless, this abstraction is going to cost.

- I don't think we should introduce this without a user besides
heapam. The likelihood that API will be usable by anything else
without a testcase seems fairly remote. I think some src/test/modules
type implementation of a per-session, in-memory storage - relatively
easy to implement - or such is necessary.

- I think, and detailed some of that, we're going to need some cleanups
that go in before this, to decrease the size / increase the quality of
the new APIs. It's going to get more painful to change APIs
subsequently.

- We'll have to document clearly that these APIs are going to change for
a while, even after the release introducing them.

StorageAm - Scan stuff:

- I think API isn't quite right. There's a lot of granular callback
functionality like scan_begin_function / scan_begin_catalog /
scan_begin_bm - these largely are convenience wrappers around the same
function, and I don't think that would, or rather should, change in
any abstracted storage layer. So I think there needs to be some
unification first (pretty close w/ beginscan_internal already, but
perhaps we should get rid of a few of these wrappers).

- Some of the exposed functionality, e.g. scan_getpage,
scan_update_snapshot, scan_rescan_set_params looks like it should just
be excised, i.e. there's no reason for it to exist.

- Minor: don't think the _function suffix for Storis necessary, just
makes things long, and every member has it. Besides that, it's also
easy to misunderstand - for a second I understood
scan_getnext_function to be about getting the next function...

- Scans are still represented as HeapScanDesc - I don't think that's
going to fly. Either this needs to be an opaque type (i.e. a struct
that's not defined, just forward declared), or it needs to be a base
struct that individual AMs embed in their own structs. Individual AMs
definitely are going to need different pieces of data.

Storage AM - tuple stuff:

- tuple_get_{xmin, updated_xid, cmin, itempointer, ctid, heaponly} are
each individual functions, that seems pretty painful to maintain, and
v/ likely to just grow and grow. Not sure what the solution is, but
this seems like a hard sell.

- The three *speculative* functions don't quite seem right to me, nor do
I understand:
+ *
+ * Setting a tuple's speculative token is a slot-only operation, so no need
+ * for a storage AM method, but after inserting a tuple containing a
+ * speculative token, the insertion must be completed by these routines:
+ */
I don't see anything related to slots, here?

Storage AM - slot stuff:

- I think there's a number of wrapper functions (slot_getattr,
slot_getallattrs, getsomeattrs, attisnull) around the same
functionality - that bloats the API and causes slowdowns. Instead we
need something like slot_virtualize_tuple(int16 upto), and the rest
should just be wrappers.

- I think it's wrong to have the slot functionality defined on the
StorageAm level. That'll cause even more indirect function calls (=>
slowness), and besides that the TupleTableSlot struct will need
members for each and every Am.

I think the solution is to instead have an Am level "create slot"
function, and the returned slot is allocated by the Am, with a base
member of TupleTableSlot with basically just tts_nvalid, tts_values,
tts_isnull as members. Those are the only members that can be
accessed without functions.

Access to the individual functions (say store_tuple) would then be
direct members of the TupleTableSlot interface. While that costs a bit
of memory, it removes one indirection from an already performance
critical path.

- MinimalTuples should be one type of slot for the above, except it's
not created by an StorageAm but by a function returning a
TupleTableSlot.

This should remove the need for the slot_copy_min_tuple,
slot_is_physical_tuple functions.

- Right now TupleTableSlots are an executor datastructure, but
these patches (rightly!) make it much more widely used. So I think it
needs to be moved outside of executor/, and probably renamed to
something like TupleHolder or something.

- The oid integration seems wrong - without an accessor oids won't be
queryable with this unless you break through the API. But from a
higher level view I do wonder if it's not time to remove "special" oid
columns and replace them with a normal column. We should be hesitant
enshrining crusty old concepts in new APIs.

Executor integration:

- I'm quite fearful that this'll cause slowdowns in a few tight paths.
The most likely cases here seem to be a) bitmap indexscans b)
indexscans c) highly selective sequential scans. I do wonder if
that can be partially addressed by switching out the individual
executor routines in the relevant scan nodes by something using or
similar to the infrastructure in cc9f08b6b8

Regards,

Andres