Horton

Re: Pluggable Storage - Andres's take

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
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>
Date: 2018-07-16T13:35:27Z
Lists: pgsql-hackers 
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

Commits

  1. Add default_table_access_method to postgresql.conf.sample.

  2. tableam: Avoid relying on relation size to determine validity of tids.

  3. tableam: Don't assume that every AM uses md.c style storage.

  4. Allow pg_class xid & multixid horizons to not be set.

  5. Fix slot type issue for fuzzy distance index scan over out-of-core table AM.

  6. tableam: comment and formatting fixes.

  7. table: docs: fix typos and grammar.

  8. tableam: basic documentation.

  9. Only allow heap in a number of contrib modules.

  10. tableam: Add table_finish_bulk_insert().

  11. tableam: sample scan.

  12. tableam: bitmap table scan.

  13. tableam: Move heap specific logic from estimate_rel_size below tableam.

  14. tableam: VACUUM and ANALYZE support.

  15. tableam: relation creation, VACUUM FULL/CLUSTER, SET TABLESPACE.

  16. tableam: Support for an index build's initial table scan(s).

  17. tableam: Add table_get_latest_tid, to wrap heap_get_latest_tid.

  18. tableam: Add helper for indexes to check if a corresponding table tuples exist.

  19. tableam: Add and use table_fetch_row_version().

  20. tableam: Use in CREATE TABLE AS and CREATE MATERIALIZED VIEW.

  21. Ensure sufficient alignment for ParallelTableScanDescData in BTShared.

  22. Don't reuse slots between root and partition in ON CONFLICT ... UPDATE.

  23. Use a virtual rather than a heap slot in two places where that suffices.

  24. Store tuples for EvalPlanQual in slots, rather than as HeapTuples.

  25. Use slots in trigger infrastructure, except for the actual invocation.

  26. Store table oid and tuple's tid in tuple slots directly.

  27. Allow to use HeapTupleData embedded in [Buffer]HeapTupleTableSlot.

  28. Add ExecStorePinnedBufferHeapTuple.

  29. Add ArchiveOpts to pass options to ArchiveEntry

  30. Rename RelationData.rd_amroutine to rd_indam.

  31. Rephrase references to "time qualification".

  32. Move remaining code from tqual.[ch] to heapam.h / heapam_visibility.c.

  33. Move generic snapshot related code from tqual.h to snapmgr.h.

  34. Remove superfluous tqual.h includes.

  35. Don't duplicate parallel seqscan shmem sizing logic in nbtree.

  36. Move vacuumlazy.c into access/heap.

  37. Support parallel bitmap heap scans.