Horton

Re: Pluggable Storage - Andres's take

Haribabu Kommi <kommi.haribabu@gmail.com>

View thread View on postgr.es
From: Haribabu Kommi <kommi.haribabu@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2018-09-05T04:04:30Z
Lists: pgsql-hackers 
On Tue, Sep 4, 2018 at 10:33 AM Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> Thanks for the patches!
>
> On 2018-09-03 19:06:27 +1000, Haribabu Kommi wrote:
> > I found couple of places where the zheap is using some extra logic in
> > verifying
> > whether it is zheap AM or not, based on that it used to took some extra
> > decisions.
> > I am analyzing all the extra code that is done, whether any callbacks can
> > handle it
> > or not? and how? I can come back with more details later.
>
> Yea, I think some of them will need to stay (particularly around
> integrating undo) and som other ones we'll need to abstract.
>

OK. I will list all the areas that I found with my observation of how to
abstract or leaving it and then implement around it.


> > >> And then:
> > >> - lotsa cleanups
> > >> - rebasing onto a newer version of the abstract slot patchset
> > >> - splitting out smaller patches
> > >>
> > >>
> > >> You'd moved the bulk insert into tableam callbacks - I don't quite get
> > >> why? There's not really anything AM specific in that code?
> > >>
> > >
> > > The main reason of adding them to AM is just to provide a control to
> > > the specific AM to decide whether they can support the bulk insert or
> > > not?
> > >
> > > Current framework doesn't support AM specific bulk insert state to be
> > > passed from one function to another and it's structure is fixed. This
> needs
> > > to be enhanced to add AM specific private members also.
> > >
> >
> > Do you want me to work on it to make it generic to AM methods to extend
> > the structure?
>
> I think the best thing here would be to *remove* all AM abstraction for
> bulk insert, until it's actuallly needed. The likelihood of us getting
> the interface right and useful without an actual user seems low. Also,
> this already is a huge patch...
>

OK. Will remove them and share the patch.


>
> > @@ -308,7 +308,7 @@ static void CopyFromInsertBatch(CopyState cstate,
> EState *estate,
> >                                       CommandId mycid, int hi_options,
> >                                       ResultRelInfo *resultRelInfo,
> >                                       BulkInsertState bistate,
> > -                                     int nBufferedTuples,
> TupleTableSlot **bufferedSlots,
> > +                                     int nBufferedSlots, TupleTableSlot
> **bufferedSlots,
> >                                       uint64 firstBufferedLineNo);
> >  static bool CopyReadLine(CopyState cstate);
> >  static bool CopyReadLineText(CopyState cstate);
> > @@ -2309,11 +2309,12 @@ CopyFrom(CopyState cstate)
> >       void       *bistate;
> >       uint64          processed = 0;
> >       bool            useHeapMultiInsert;
> > -     int                     nBufferedTuples = 0;
> > +     int                     nBufferedSlots = 0;
> >       int                     prev_leaf_part_index = -1;
>
> > -#define MAX_BUFFERED_TUPLES 1000
> > +#define MAX_BUFFERED_SLOTS 1000
>
> What's the point of these renames? We're still dealing in tuples. Just
> seems to make the patch larger.
>

OK. I will correct it.


>
> >                               if (useHeapMultiInsert)
> >                               {
> > +                                     int tup_size;
> > +
> >                                       /* Add this tuple to the tuple
> buffer */
> > -                                     if (nBufferedTuples == 0)
> > +                                     if (nBufferedSlots == 0)
> > +                                     {
> >                                               firstBufferedLineNo =
> cstate->cur_lineno;
> > -
>  Assert(bufferedSlots[nBufferedTuples] == myslot);
> > -                                     nBufferedTuples++;
> > +
> > +                                             /*
> > +                                              * Find out the Tuple size
> of the first tuple in a batch and
> > +                                              * use it for the rest
> tuples in a batch. There may be scenarios
> > +                                              * where the first tuple
> is very small and rest can be large, but
> > +                                              * that's rare and this
> should work for majority of the scenarios.
> > +                                              */
> > +                                             tup_size =
> heap_compute_data_size(myslot->tts_tupleDescriptor,
> > +
>                                        myslot->tts_values,
> > +
>                                        myslot->tts_isnull);
> > +                                     }
>
> This seems too exensive to me.  I think it'd be better if we instead
> used the amount of input data consumed for the tuple as a proxy. Does that
> sound reasonable?
>

Yes, the cstate structure contains the line_buf member that holds the
information of
the line length of the row, this can be used as a tuple length to limit the
size usage.
comments?

Regards,
Haribabu Kommi
Fujitsu Australia

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.