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-09-04T00:33:08Z
Lists: pgsql-hackers 
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.


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


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


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

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.